Skip to content

feat: use scoped signing keys for users #104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
BZValoche wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat: use scoped signing keys for users #104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions api/v1alpha1/account_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ type AccountClaims struct {
type AccountStatus struct {
// +optional
Claims AccountClaims `json:"claims,omitempty"`
// +listType=set
// +optional
SigningKeys []string `json:"signingKeys,omitempty"`
Copy link
Collaborator

@choufraise choufraise Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Correct me if I'm wrong. If a user secret is exposed, and we want to revoke that signing key to deny the credentials by either deleting it, or re-creating it (along with a new user secret). How would the process look like? What resource do I (manually) delete/modify/trigger in Kubernetes to achieve this?

Copy link
Collaborator

@choufraise choufraise Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: My guess is that this is a []string of Signing Key public part(?). If you have an account with e.g. 20+ users, then this will be a very non-intuitive list of IDs that is hard for any human to verify upon e.g. troubleshooting permission issues. Suggestion would be to use an array of struct instead and provide more information, for instance pub key + user name/user resource reference/user DisplayName instead.

// +listType=map
// +listMapKey=type
// +patchStrategy=merge
Expand Down
28 changes: 28 additions & 0 deletions api/v1alpha1/user_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ limitations under the License.
package v1alpha1

import (
"crypto/md5"
"encoding/hex"
"fmt"
"io"
"reflect"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -30,6 +33,9 @@ import (
type UserSpec struct {
// AccountName references the account used to create the user.
AccountName string `json:"accountName"`
// UseSigningKey generates a scopping signing key for the user.
// +Optional
UseSigningKey bool `json:"useSigningKey,omitempty"`
Copy link
Collaborator

@choufraise choufraise Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: The NATS naming of this concept is "Scoped Signing Keys" according to the Signing Keys docs. Re-use this wording when referring to the concept, e.g. UseScopedSigningKey.

// DisplayName is an optional name for the NATS resource representing the user. May be derived if absent.
// +optional
DisplayName string `json:"displayName,omitempty"`
Expand Down Expand Up @@ -96,6 +102,28 @@ func (u *User) GetUserSecretName() string {
return fmt.Sprintf("%s-nats-user-creds", u.GetName())
}

const (
SecretNameUserSignTemplate = "%s-u-sign-%s" // #nosec G101
Copy link
Collaborator

@choufraise choufraise Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: This constant is also defined in internal/user/constants.go. Suggestion is to keep only one of them.

)

func mustGenerateShortHashFromID(ID string) string {
hasher := md5.New()
_, err := io.WriteString(hasher, ID)
if err != nil {
panic(fmt.Sprintf("failed to generate hash from ID: %v", err))
}

hash := hex.EncodeToString(hasher.Sum(nil))
if len(hash) > 6 {
return hash[:6]
}
return hash
}

func (u *User) GetUserSigningKeySecretName() string {
Copy link
Collaborator

@choufraise choufraise Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: The signing key secret name is rather an implementation detail than related to the API model. Suggestion is to move this logic into the manager/domain instead.

return fmt.Sprintf(SecretNameUserSignTemplate, u.GetName(), mustGenerateShortHashFromID(u.GetName()))
}

// +kubebuilder:object:root=true

// UserList contains a list of User.
Expand Down
5 changes: 5 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions charts/nauth-crds/crds/nauth.io_accounts.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,11 @@ spec:
name:
type: string
type: object
signingKeys:
items:
type: string
type: array
x-kubernetes-list-type: set
type: object
type: object
served: true
Expand Down
4 changes: 4 additions & 0 deletions charts/nauth-crds/crds/nauth.io_users.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,10 @@ spec:
type: array
type: object
type: object
useSigningKey:
description: UseSigningKey generates a scopping signing key for the
user.
type: boolean
userLimits:
properties:
src:
Expand Down
5 changes: 5 additions & 0 deletions charts/nauth/resources/crds/nauth.io_accounts.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,11 @@ spec:
name:
type: string
type: object
signingKeys:
items:
type: string
type: array
x-kubernetes-list-type: set
type: object
type: object
served: true
Expand Down
4 changes: 4 additions & 0 deletions charts/nauth/resources/crds/nauth.io_users.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,10 @@ spec:
type: array
type: object
type: object
useSigningKey:
description: UseSigningKey generates a scopping signing key for the
user.
type: boolean
userLimits:
properties:
src:
Expand Down
17 changes: 15 additions & 2 deletions cmd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package main
import (
"crypto/tls"
"flag"
"fmt"
"os"
"path/filepath"

Expand Down Expand Up @@ -71,6 +72,7 @@ func main() {
var probeAddr string
var secureMetrics bool
var enableHTTP2 bool
var devMode bool
var tlsOpts []func(*tls.Config)
flag.StringVar(&namespace, "namespace", "", "Limits the scope of nauth to a single namespace. "+
"If not specified, all namespaces will be watched.")
Expand All @@ -91,9 +93,16 @@ func main() {
flag.StringVar(&metricsCertKey, "metrics-cert-key", "tls.key", "The name of the metrics server key file.")
flag.BoolVar(&enableHTTP2, "enable-http2", false,
"If set, HTTP/2 will be enabled for the metrics and webhook servers")
flag.BoolVar(&devMode, "dev", false, "Run in development mode")

// Detect if we run inside the cluster
_, err := os.Stat("/var/run/secrets/kubernetes.io/serviceaccount/token")
inCluster := err == nil
opts := zap.Options{
Development: true,
Development: devMode || !inCluster,
}

fmt.Println(opts.Development)
opts.BindFlags(flag.CommandLine)
flag.Parse()

Expand Down Expand Up @@ -244,7 +253,11 @@ func main() {
os.Exit(1)
}

userManager := user.NewManager(accountClient, secretClient)
userManager := user.NewManager(
accountClient,
natsClient,
secretClient,
)
userReconciler := controller.NewUserReconciler(
mgr.GetClient(),
mgr.GetScheme(),
Expand Down
25 changes: 10 additions & 15 deletions internal/account/account.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,14 @@ import (
"io"
"log"
"os"
"strings"
"sync"

natsv1alpha1 "github.com/WirelessCar/nauth/api/v1alpha1"
"github.com/WirelessCar/nauth/internal/controller"
"github.com/WirelessCar/nauth/internal/k8s"
"github.com/WirelessCar/nauth/internal/k8s/secret"
"github.com/WirelessCar/nauth/internal/nats"
"github.com/nats-io/jwt/v2"
"github.com/nats-io/nkeys"
v1 "k8s.io/api/core/v1"
Expand All @@ -24,34 +26,26 @@ import (

type SecretClient interface {
// TODO: Keys created should be immutable
Apply(ctx context.Context, owner *secret.Owner, meta metav1.ObjectMeta, valueMap map[string]string) error
Apply(ctx context.Context, owner *secret.Owner, meta metav1.ObjectMeta, valueMap map[string]string, update bool) error
Get(ctx context.Context, namespace string, name string) (map[string]string, error)
GetByLabels(ctx context.Context, namespace string, labels map[string]string) (*v1.SecretList, error)
Delete(ctx context.Context, namespace string, name string) error
DeleteByLabels(ctx context.Context, namespace string, labels map[string]string) error
Label(ctx context.Context, namespace, name string, labels map[string]string) error
}

type NatsClient interface {
EnsureConnected(namespace string) error
Disconnect()
LookupAccountJWT(string) (string, error)
UploadAccountJWT(jwt string) error
DeleteAccountJWT(jwt string) error
}

type AccountGetter interface {
Get(ctx context.Context, accountRefName string, namespace string) (account *natsv1alpha1.Account, err error)
}

type Manager struct {
accounts AccountGetter
natsClient NatsClient
natsClient nats.NatsClient
secretClient SecretClient
nauthNamespace string
}

func NewManager(accounts AccountGetter, natsClient NatsClient, secretClient SecretClient, opts ...func(*Manager)) *Manager {
func NewManager(accounts AccountGetter, natsClient nats.NatsClient, secretClient SecretClient, opts ...func(*Manager)) *Manager {
manager := &Manager{
accounts: accounts,
natsClient: natsClient,
Expand All @@ -67,11 +61,11 @@ func NewManager(accounts AccountGetter, natsClient NatsClient, secretClient Secr
if err != nil {
log.Fatalf("Failed create account manager. Failed to read namespace: %v", err)
}
manager.nauthNamespace = string(controllerNamespace)
manager.nauthNamespace = strings.TrimSpace(string(controllerNamespace))
}

if !manager.valid() {
log.Fatalf("Failed to crate Account manager. Missing required fields.")
log.Fatalf("Failed to create Account manager. Missing required fields.")
return nil
}

Expand Down Expand Up @@ -124,7 +118,7 @@ func (a *Manager) Create(ctx context.Context, state *natsv1alpha1.Account) (*con
accountSeed, _ := accountKeyPair.Seed()
accountSecretValue := map[string]string{k8s.DefaultSecretKeyName: string(accountSeed)}

if err := a.secretClient.Apply(ctx, nil, accountSecretMeta, accountSecretValue); err != nil {
if err := a.secretClient.Apply(ctx, nil, accountSecretMeta, accountSecretValue, true); err != nil {
return nil, err
}

Expand All @@ -142,7 +136,7 @@ func (a *Manager) Create(ctx context.Context, state *natsv1alpha1.Account) (*con
accountSigningSeed, _ := accountSigningKeyPair.Seed()
accountSignSeedSecretValue := map[string]string{k8s.DefaultSecretKeyName: string(accountSigningSeed)}

if err := a.secretClient.Apply(ctx, nil, accountSigningSecretMeta, accountSignSeedSecretValue); err != nil {
if err := a.secretClient.Apply(ctx, nil, accountSigningSecretMeta, accountSignSeedSecretValue, true); err != nil {
return nil, err
}

Expand All @@ -151,6 +145,7 @@ func (a *Manager) Create(ctx context.Context, state *natsv1alpha1.Account) (*con
natsClaims, err := newClaimsBuilder(ctx, getDisplayName(state), state.Spec, accountPublicKey, a.accounts).
signingKey(accountSigningPublicKey).
build()

if err != nil {
accountName := fmt.Sprintf("%s-%s", state.GetNamespace(), state.GetName())
return nil, fmt.Errorf("failed to build NATS account claims for %s: %w", accountName, err)
Expand Down
2 changes: 1 addition & 1 deletion internal/account/mocks_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ type SecretStorerMock struct {
}

// ApplySecret implements ports.SecretStorer.
func (s *SecretStorerMock) Apply(ctx context.Context, secretOwner *secret.Owner, meta metav1.ObjectMeta, valueMap map[string]string) error {
func (s *SecretStorerMock) Apply(ctx context.Context, secretOwner *secret.Owner, meta metav1.ObjectMeta, valueMap map[string]string, update bool) error {
args := s.Called(ctx, secretOwner, meta, valueMap)
return args.Error(0)
}
Expand Down
12 changes: 6 additions & 6 deletions internal/controller/account.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"fmt"
"os"
"time"

"github.com/WirelessCar/nauth/internal/k8s"
apierrors "k8s.io/apimachinery/pkg/api/errors"
Expand Down Expand Up @@ -85,7 +86,7 @@ func (r *AccountReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
err := r.Get(ctx, req.NamespacedName, natsAccount)
if err != nil {
if apierrors.IsNotFound(err) {
log.Info("resource not found. Ignoring since object must be deleted")
// log.Info("resource not found. Ignoring since object must be deleted")
return ctrl.Result{}, nil
}
// Error reading the object - requeue the request.
Expand Down Expand Up @@ -127,11 +128,10 @@ func (r *AccountReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
}

if len(userList.Items) > 0 {
return r.reporter.error(
ctx,
natsAccount,
fmt.Errorf("cannot delete an account with associated users, found %d users", len(userList.Items)),
)
log.Info("Cannot delete an account with associated users, found", "account", natsAccount, "users", len(userList.Items))
return ctrl.Result{
RequeueAfter: 2 * time.Second,
}, nil
}

if controllerutil.ContainsFinalizer(natsAccount, controllerAccountFinalizer) {
Expand Down
8 changes: 8 additions & 0 deletions internal/controller/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@ package controller

import (
"context"
"errors"
"fmt"
"os"
"time"

"k8s.io/client-go/tools/record"

Expand All @@ -36,6 +38,7 @@ import (
logf "sigs.k8s.io/controller-runtime/pkg/log"

natsv1alpha1 "github.com/WirelessCar/nauth/api/v1alpha1"
"github.com/WirelessCar/nauth/internal/k8s"
)

type UserManager interface {
Expand Down Expand Up @@ -147,6 +150,10 @@ func (r *UserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
}

if err := r.userManager.CreateOrUpdate(ctx, user); err != nil {
if errors.Is(err, k8s.NoErrRetryLater) {
log.Info("User creation was not successful", "reason", err)
return ctrl.Result{RequeueAfter: 2 * time.Second}, nil
}
return r.reporter.error(ctx, user, err)
}

Expand All @@ -169,6 +176,7 @@ func (r *UserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
return ctrl.Result{}, err
}

log.Info("User created or updated")
return r.reporter.status(ctx, user)
}

Expand Down
2 changes: 1 addition & 1 deletion internal/k8s/account.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func (a *AccountClient) Get(ctx context.Context, accountRefName string, namespac
}

if !isReady(account) {
log.Error(err, "account is not ready", "namespace", namespace, "accountName", accountRefName)
log.Info("account is not ready", "namespace", namespace, "accountName", accountRefName)
return nil, errors.Join(ErrAccountNotReady, err)
}

Expand Down
1 change: 1 addition & 0 deletions internal/k8s/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@
ErrNoAccountFound = errors.New("no account found")
ErrAccountNotReady = errors.New("account is not ready")
ErrNotFound = errors.New("not found")
NoErrRetryLater = errors.New("try again later")

Check failure on line 9 in internal/k8s/errors.go

View workflow job for this annotation

GitHub Actions / Run on Ubuntu 

ST1012: error var NoErrRetryLater should have name of the form ErrFoo (staticcheck)
)
2 changes: 2 additions & 0 deletions internal/k8s/labels.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ const (
LabelAccountSignedBy = "account.nauth.io/signed-by"
LabelUserID = "user.nauth.io/id"
LabelUserAccountID = "user.nauth.io/account-id"
LabelUserSigningKeyID = "user.nauth.io/signing-key-id"
LabelUserName = "user.nauth.io/username"
LabelUserSignedBy = "user.nauth.io/signed-by"
LabelSecretType = "nauth.io/secret-type"
LabelManaged = "nauth.io/managed"
Expand Down
9 changes: 7 additions & 2 deletions internal/k8s/secret/secret.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"log"
"maps"
"os"
"strings"

"github.com/WirelessCar/nauth/internal/k8s"
v1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -48,13 +49,13 @@ func NewClient(client client.Client, opts ...Option) *Client {
if err != nil {
log.Fatalf("Failed to read namespace: %v", err)
}
secretClient.controllerNamespace = string(namespacePath)
secretClient.controllerNamespace = strings.TrimSpace(string(namespacePath))
}

return secretClient
}

func (k *Client) Apply(ctx context.Context, owner *Owner, meta metav1.ObjectMeta, valueMap map[string]string) error {
func (k *Client) Apply(ctx context.Context, owner *Owner, meta metav1.ObjectMeta, valueMap map[string]string, update bool) error {
if !isManagedSecret(&meta) {
return fmt.Errorf("label %s not supplied by secret %s/%s", k8s.LabelManaged, meta.Namespace, meta.Name)
}
Expand All @@ -77,6 +78,10 @@ func (k *Client) Apply(ctx context.Context, owner *Owner, meta metav1.ObjectMeta
return fmt.Errorf("failed to create secret: %w", err)
}
} else {
if !update {
Copy link
Collaborator

@choufraise choufraise Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: If Signing Keys are being used, hence update is true, then we will never update the secret according to this code. This also means that if meta or valueMap changes between reconciliations we won't update the secret with e.g. new labels etc. Suggestion is to remove the update bool parameter and simply always apply/update the secret. I consider this parameter to be sub-optimizing while adding unnecessary complexity. Please correct me if I'm missing something crucial here.

// Do not update the secret, we just needed to know it's here
return nil
}
if !isManagedSecret(&currentSecret.ObjectMeta) {
return fmt.Errorf("existing secret %s/%s not managed by nauth", meta.Namespace, meta.Name)
}
Expand Down
Loading
Loading