From c688a43236f91b773a7326cbe6bda539adbeb55c Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Thu, 8 Oct 2026 11:33:52 -0400 Subject: [PATCH 1/3] feat(accounts): resolve an organization's own envelope key (Refs #1013) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The read and write half of the per-organization key. The schema and the crypto-shredding guard landed with #1033; this is what actually seals under a customer's key. TenantCipher resolves per organization: a binding means that organization's key, no binding means the deployment's — and no binding stays the DEFAULT, because "the customer does not care" is the common case and a deployment that had to provision a key per organization before it could store anything would be worse for everyone. Four decisions worth the review's attention: - **The organization is passed EXPLICITLY, never read from the context.** An implicit read returning "" would seal a customer's credential under this deployment's key, silently, for exactly the customer who went to the trouble of bringing their own. An absent organization is refused; mutation-proven. - **A value is routed by what the ENVELOPE names, not by the binding.** The two legitimately disagree: an organization just given a key still holds credentials sealed under the deployment's, and those must keep opening or granting a customer a key would make their existing credentials unreadable. Tested as its own case. - **A revoked key refuses with ErrKeyRevoked and the recorded reason**, never business.ErrInvalidSecretEnvelope. That sentinel means "re-enter this credential", so reporting a destroyed key through it would tell a customer to reconnect the source whose data they instructed us to destroy. Mutation-proven. - **A binding that cannot be READ refuses** rather than falling through. "The lookup failed" and "there is no binding" are different answers, and conflating them seals a customer's data under our key. The keyed hash never moves: API-key lookup is BY the hash, and the lookup is what discovers which organization the key belongs to, so a hash under an organization's key could never be found. The Cloud KMS sealer bound to an organization's key refuses MAC outright rather than producing a value nothing will match. Both backends gained SealerFor. Vault needed more than a parameter: its ciphertext carries a key VERSION and not a key NAME, so a per-organization key could not be told from the deployment's on read. Its payload now carries an optional leading key segment — base64url contains no ":", so the pre-per-key shape is unambiguous and, because the deployment's own key still writes exactly that shape, nothing already stored moves. The MAC key is a separate field from the envelope key for the same reason: one may move, the other may never. Sealers are cached per key REFERENCE, not per organization: two organizations on one key share one, and building one is a round trip to the key service. Not wired into business yet, so nothing writes a per-org key through a request path; that is the next commit. Co-Authored-By: Claude Opus 5 (1M context) --- .../code/pkg/keyservice/keyservice.go | 41 +++ .../accounts/code/pkg/keyservice/kms.go | 35 +++ .../code/pkg/keyservice/tenant_cipher.go | 199 ++++++++++++++ .../code/pkg/keyservice/tenant_cipher_test.go | 245 ++++++++++++++++++ .../accounts/code/pkg/keyservice/vault.go | 116 +++++++-- 5 files changed, 621 insertions(+), 15 deletions(-) create mode 100644 module/services/accounts/code/pkg/keyservice/tenant_cipher.go create mode 100644 module/services/accounts/code/pkg/keyservice/tenant_cipher_test.go diff --git a/module/services/accounts/code/pkg/keyservice/keyservice.go b/module/services/accounts/code/pkg/keyservice/keyservice.go index 13fade11f..dac1eb8f7 100644 --- a/module/services/accounts/code/pkg/keyservice/keyservice.go +++ b/module/services/accounts/code/pkg/keyservice/keyservice.go @@ -124,3 +124,44 @@ func (e *statusError) Error() string { return fmt.Sprintf("%s request returned %d", e.service, e.status) } func (e *statusError) HTTPStatusCode() int { return e.status } + +// KeyFactory binds another key of the same key service, named by a reference the +// platform recorded for an organization. +// +// The reference is OPAQUE above this interface: only the backend knows whether +// it is a Vault transit key name or a cloud resource name, which is what keeps +// the vendor out of the caller and out of the database. +type KeyFactory interface { + SealerFor(ctx context.Context, keyRef string) (Sealer, error) +} + +// OrgKeyBinding is what the platform recorded about an organization's key. +type OrgKeyBinding struct { + // KeyRef is the key, as the bound backend names one. + KeyRef string + // CustomerHeld marks a key this deployment does not control. + CustomerHeld bool + // RevokedReason is set once the key is gone — destroyed on request, or + // revoked by the customer. Every credential sealed under it is unreadable + // from that moment, which is the point. + RevokedReason string +} + +// Revoked reports whether the key is gone. +func (b OrgKeyBinding) Revoked() bool { return b.RevokedReason != "" } + +// OrgKeyBindings answers which key seals an organization's credentials. A nil +// binding means the organization has none of its own and is sealed under the +// deployment's key, which is the default and stays the default. +type OrgKeyBindings interface { + OrgKeyBinding(ctx context.Context, orgID string) (*OrgKeyBinding, error) +} + +// ErrKeyRevoked reports that an organization's key is gone, so its credentials +// are unreadable by design. +// +// It is deliberately NOT business.ErrInvalidSecretEnvelope: callers treat that +// sentinel as a credential the user should re-enter, and telling a customer to +// reconnect a source whose data they instructed us to destroy is the opposite of +// what crypto-shredding is for. +var ErrKeyRevoked = errors.New("key service: the organization's key has been revoked") diff --git a/module/services/accounts/code/pkg/keyservice/kms.go b/module/services/accounts/code/pkg/keyservice/kms.go index df381ea5b..cd26f90db 100644 --- a/module/services/accounts/code/pkg/keyservice/kms.go +++ b/module/services/accounts/code/pkg/keyservice/kms.go @@ -494,6 +494,9 @@ func (k *KMSSealer) Open(ctx context.Context, purpose, payload string) (string, } func (k *KMSSealer) MAC(ctx context.Context, plaintext string) (string, error) { + if !k.macKeyBound() { + return "", errors.New("key service: this sealer is bound to an organization's key and may not compute the keyed hash; API-key lookup is by that hash, so it is always the deployment's own") + } var result struct { MAC string `json:"mac"` } @@ -579,3 +582,35 @@ func (w *KMSKeyWrap) UnwrapSigningKey(ctx context.Context, wrapped string) (ed25 } return ed25519.NewKeyFromSeed(seed), nil } + +// SealerFor binds another Cloud KMS key, for an organization whose credentials +// are sealed under a key of its own. +// +// The key is validated and round-tripped exactly as the deployment's own is: +// purpose, algorithm, and a seal/open probe. An organization pointed at a key +// that is well-formed and simply not usable must fail when the binding is read, +// not at the first credential that needs it. +func (k *KMSSealer) SealerFor(ctx context.Context, keyRef string) (Sealer, error) { + if !cryptoKeyName.MatchString(keyRef) { + return nil, fmt.Errorf("key service: an organization's Cloud KMS key must be a cryptoKey resource name (projects//locations//keyRings//cryptoKeys/), got %q", keyRef) + } + if keyRef == k.envelopeKey { + return nil, fmt.Errorf("key service: %s is this deployment's own envelope key and cannot be an organization's", keyRef) + } + sealer, err := newKMSEnvelope(ctx, k.client, keyRef) + if err != nil { + return nil, err + } + // The keyed hash never moves off the deployment's key, so a per-organization + // sealer carries no MAC key and MAC on it is a programming error rather than + // a silent hash under the wrong key. + return sealer, nil +} + +// MAC refuses on a sealer bound to an organization's key. +// +// API-key lookup is BY the hash, so a hash computed under an organization's key +// could never be found: the lookup happens before the organization is known. The +// deployment's own sealer is the only one that may compute it, and this refuses +// rather than quietly producing a value nothing will ever match. +func (k *KMSSealer) macKeyBound() bool { return k.macVersion != "" } diff --git a/module/services/accounts/code/pkg/keyservice/tenant_cipher.go b/module/services/accounts/code/pkg/keyservice/tenant_cipher.go new file mode 100644 index 000000000..604b57651 --- /dev/null +++ b/module/services/accounts/code/pkg/keyservice/tenant_cipher.go @@ -0,0 +1,199 @@ +package keyservice + +import ( + "context" + "errors" + "fmt" + "sync" +) + +// TenantCipher seals an organization's credentials under that organization's own +// key when it has one, and under the deployment's key when it does not. +// +// Having none is the default and stays the default: "the customer does not care" +// is the common case, and a deployment that had to provision a key per +// organization before it could store anything would be worse for everyone. A +// binding is an OVERRIDE. +// +// The organization is passed EXPLICITLY rather than read from the context. An +// implicit read that returned "" would seal a customer's credential under this +// deployment's key — silently, and exactly for the customer who went to the +// trouble of bringing their own. That is the failure this type exists to +// prevent, so the argument is required and an empty one is refused. +type TenantCipher struct { + // deployment seals for an organization with no key of its own, and is the + // only thing that computes the keyed hash. + deployment *Cipher + factory KeyFactory + bindings OrgKeyBindings + + // sealers caches by key reference, not by organization: two organizations + // pointed at one key share a sealer, and building one is a round trip to the + // key service (Cloud KMS validates and round-trips the key), far too much to + // repeat per request. + mu sync.RWMutex + sealers map[string]Sealer +} + +func NewTenantCipher(deployment *Cipher, factory KeyFactory, bindings OrgKeyBindings) (*TenantCipher, error) { + if deployment == nil || factory == nil || bindings == nil { + return nil, errors.New("key service: a tenant cipher needs the deployment cipher, a key factory and a binding source") + } + return &TenantCipher{ + deployment: deployment, factory: factory, bindings: bindings, + sealers: map[string]Sealer{}, + }, nil +} + +// EncryptSecret and DecryptSecret keep the deployment-wide behaviour for values +// that are not an organization's — a person's MFA seed and WebAuthn credential, +// which belong to someone who may be in many organizations. +func (c *TenantCipher) EncryptSecret(ctx context.Context, purpose, plaintext string) (string, error) { + return c.deployment.EncryptSecret(ctx, purpose, plaintext) +} + +func (c *TenantCipher) DecryptSecret(ctx context.Context, purpose, stored string) (string, error) { + return c.deployment.DecryptSecret(ctx, purpose, stored) +} + +// HashKey is always the deployment's: API-key lookup is BY the hash, so a hash +// computed under an organization's key could never be found — the lookup is what +// discovers which organization the key belongs to. +func (c *TenantCipher) HashKey(ctx context.Context, plaintext string) (string, error) { + return c.deployment.HashKey(ctx, plaintext) +} + +func (c *TenantCipher) CandidateHashes(ctx context.Context, plaintext string) ([]string, error) { + return c.deployment.CandidateHashes(ctx, plaintext) +} + +// EncryptTenantSecret seals under the organization's key, or the deployment's +// when it has none. +func (c *TenantCipher) EncryptTenantSecret(ctx context.Context, orgID, purpose, plaintext string) (string, error) { + sealer, err := c.sealerFor(ctx, orgID) + if err != nil { + return "", err + } + if sealer == nil { + return c.deployment.EncryptSecret(ctx, purpose, plaintext) + } + if purpose == "" || plaintext == "" { + return "", errors.New("key service: sealing requires a purpose and a plaintext") + } + payload, err := sealer.Seal(ctx, purpose, plaintext) + if err != nil { + return "", err + } + return Envelope{Backend: sealer.Tag(), Payload: payload}.String(), nil +} + +// DecryptTenantSecret opens a value through whichever bound key sealed it: the +// organization's own, or the deployment's. +// +// A value is routed by what the ENVELOPE names, not by what the binding says +// now, because the two legitimately disagree during a cutover — an organization +// that has just been given its own key still holds credentials sealed under the +// deployment's, and those must keep opening until they are re-sealed. +func (c *TenantCipher) DecryptTenantSecret(ctx context.Context, orgID, purpose, stored string) (string, error) { + if orgID == "" { + return "", errors.New("key service: opening an organization's secret requires the organization") + } + binding, err := c.bindings.OrgKeyBinding(ctx, orgID) + if err != nil { + return "", fmt.Errorf("key service: read the organization's key binding: %w", err) + } + envelope, err := ParseEnvelope(stored) + if err != nil { + return "", err + } + if binding != nil { + if binding.Revoked() { + // Named, and NOT an invalid envelope: this credential is unreadable + // because the customer's key is gone, which is the outcome they + // asked for. Reporting it as a broken credential would tell them to + // reconnect the source whose data they destroyed. + return "", fmt.Errorf("%w (%s): the credentials sealed under it cannot be read", ErrKeyRevoked, binding.RevokedReason) + } + sealer, err := c.boundSealer(ctx, binding.KeyRef) + if err != nil { + return "", err + } + if sealer.Accepts(envelope) { + return sealer.Open(ctx, purpose, envelope.Payload) + } + } + // Sealed under the deployment's key — either the organization has no key of + // its own, or it has one and this value predates it. + return c.deployment.DecryptSecret(ctx, purpose, stored) +} + +// ResealTenantSecret moves one value onto the organization's current key. A value +// already under it is returned unchanged with false, so a sweep is restart-safe. +func (c *TenantCipher) ResealTenantSecret(ctx context.Context, orgID, purpose, stored string) (string, bool, error) { + sealer, err := c.sealerFor(ctx, orgID) + if err != nil { + return "", false, err + } + envelope, err := ParseEnvelope(stored) + if err != nil { + return "", false, err + } + target := c.deployment.seal + if sealer != nil { + target = sealer + } + if target.Accepts(envelope) { + return stored, false, nil + } + plaintext, err := c.DecryptTenantSecret(ctx, orgID, purpose, stored) + if err != nil { + return "", false, err + } + resealed, err := c.EncryptTenantSecret(ctx, orgID, purpose, plaintext) + if err != nil { + return "", false, err + } + return resealed, true, nil +} + +// sealerFor resolves the organization's sealer, or nil when it has none and the +// deployment's key applies. +func (c *TenantCipher) sealerFor(ctx context.Context, orgID string) (Sealer, error) { + if orgID == "" { + return nil, errors.New("key service: sealing an organization's secret requires the organization") + } + binding, err := c.bindings.OrgKeyBinding(ctx, orgID) + if err != nil { + return nil, fmt.Errorf("key service: read the organization's key binding: %w", err) + } + if binding == nil { + return nil, nil + } + if binding.Revoked() { + return nil, fmt.Errorf("%w (%s): nothing further may be sealed under it", ErrKeyRevoked, binding.RevokedReason) + } + return c.boundSealer(ctx, binding.KeyRef) +} + +func (c *TenantCipher) boundSealer(ctx context.Context, keyRef string) (Sealer, error) { + c.mu.RLock() + cached, ok := c.sealers[keyRef] + c.mu.RUnlock() + if ok { + return cached, nil + } + sealer, err := c.factory.SealerFor(ctx, keyRef) + if err != nil { + return nil, fmt.Errorf("key service: bind the organization's key: %w", err) + } + c.mu.Lock() + // Another goroutine may have won the race; keep one sealer per key so the + // cache cannot grow a second entry for the same reference. + if existing, raced := c.sealers[keyRef]; raced { + sealer = existing + } else { + c.sealers[keyRef] = sealer + } + c.mu.Unlock() + return sealer, nil +} diff --git a/module/services/accounts/code/pkg/keyservice/tenant_cipher_test.go b/module/services/accounts/code/pkg/keyservice/tenant_cipher_test.go new file mode 100644 index 000000000..855b4e7b5 --- /dev/null +++ b/module/services/accounts/code/pkg/keyservice/tenant_cipher_test.go @@ -0,0 +1,245 @@ +package keyservice + +// The per-organization key. What has to hold: +// +// - an organization with no key of its own is sealed under the deployment's, +// and that stays the default; +// - an organization with one is sealed under it, and a value sealed under the +// deployment's key BEFORE the binding existed still opens; +// - a revoked key makes its credentials unreadable, reported as that and not +// as a credential the user should re-enter; +// - an absent organization is REFUSED, never quietly sealed under the +// deployment's key — that would silently betray the one customer who asked +// for their own. + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/stretchr/testify/require" + + "accounts/pkg/business" +) + +// bindings is an in-memory OrgKeyBindings. +type bindings struct { + rows map[string]*OrgKeyBinding + calls int + err error +} + +func (b *bindings) OrgKeyBinding(_ context.Context, orgID string) (*OrgKeyBinding, error) { + b.calls++ + if b.err != nil { + return nil, b.err + } + return b.rows[orgID], nil +} + +// keyFactory builds a distinct sealer per key reference, counting how often it +// is asked so the cache can be asserted. +type keyFactory struct { + built int + err error +} + +func (f *keyFactory) SealerFor(_ context.Context, keyRef string) (Sealer, error) { + f.built++ + if f.err != nil { + return nil, f.err + } + return testSealer{tag: "test-key-service", key: keyRef}, nil +} + +// testSealer is a sealer whose stored payload names its key, the property every +// real backend has and the one routing depends on. +type testSealer struct{ tag, key string } + +func (s testSealer) Tag() string { return s.tag } +func (s testSealer) Identity() string { return s.tag + ":" + s.key } + +func (s testSealer) Accepts(envelope Envelope) bool { + if envelope.Backend != s.tag { + return false + } + key, _, found := strings.Cut(envelope.Payload, "|") + return found && key == s.key +} + +func (s testSealer) Seal(_ context.Context, purpose, plaintext string) (string, error) { + return s.key + "|" + purpose + "|" + plaintext, nil +} + +func (s testSealer) Open(_ context.Context, purpose, payload string) (string, error) { + parts := strings.SplitN(payload, "|", 3) + if len(parts) != 3 || parts[0] != s.key || parts[1] != purpose { + return "", business.ErrInvalidSecretEnvelope + } + return parts[2], nil +} + +func (s testSealer) MAC(_ context.Context, plaintext string) (string, error) { + return s.key + ":" + plaintext, nil +} + +func newTenantCipher(t *testing.T, rows map[string]*OrgKeyBinding) (*TenantCipher, *bindings, *keyFactory) { + t.Helper() + deployment, err := NewCipher(testSealer{tag: "test-key-service", key: "deployment"}, nil) + require.NoError(t, err) + source := &bindings{rows: rows} + factory := &keyFactory{} + cipher, err := NewTenantCipher(deployment, factory, source) + require.NoError(t, err) + return cipher, source, factory +} + +func TestTenantCipherSealsUnderTheDeploymentKeyWhenAnOrganizationHasNone(t *testing.T) { + cipher, _, factory := newTenantCipher(t, nil) + + stored, err := cipher.EncryptTenantSecret(t.Context(), "org-a", "github-connector:s1", "ghp_token") + require.NoError(t, err) + require.Contains(t, stored, "deployment|", "no binding means the deployment's key, which is the default") + require.Zero(t, factory.built, "no organization key was needed, so none may be built") + + opened, err := cipher.DecryptTenantSecret(t.Context(), "org-a", "github-connector:s1", stored) + require.NoError(t, err) + require.Equal(t, "ghp_token", opened) +} + +func TestTenantCipherSealsUnderTheOrganizationsOwnKey(t *testing.T) { + cipher, _, _ := newTenantCipher(t, map[string]*OrgKeyBinding{ + "org-a": {KeyRef: "acme-key", CustomerHeld: true}, + }) + + stored, err := cipher.EncryptTenantSecret(t.Context(), "org-a", "org-idp:org-a", "idp-secret") + require.NoError(t, err) + require.Contains(t, stored, "acme-key|") + require.NotContains(t, stored, "deployment|") + + opened, err := cipher.DecryptTenantSecret(t.Context(), "org-a", "org-idp:org-a", stored) + require.NoError(t, err) + require.Equal(t, "idp-secret", opened) + + // Another organization's key must not open it, even with the same purpose. + cipherB, _, _ := newTenantCipher(t, map[string]*OrgKeyBinding{ + "org-b": {KeyRef: "other-key"}, + }) + _, err = cipherB.DecryptTenantSecret(t.Context(), "org-b", "org-idp:org-a", stored) + require.Error(t, err) +} + +// The cutover case: a value sealed under the deployment's key before the +// organization was given one must keep opening, or granting a customer a key +// would make their existing credentials unreadable. +func TestTenantCipherStillOpensValuesSealedBeforeTheBindingExisted(t *testing.T) { + cipher, source, _ := newTenantCipher(t, nil) + before, err := cipher.EncryptTenantSecret(t.Context(), "org-a", "webhook-signing:w1", "whsec_old") + require.NoError(t, err) + + // The platform now gives the organization its own key. + source.rows = map[string]*OrgKeyBinding{"org-a": {KeyRef: "acme-key"}} + + opened, err := cipher.DecryptTenantSecret(t.Context(), "org-a", "webhook-signing:w1", before) + require.NoError(t, err, "a value predating the binding must still open") + require.Equal(t, "whsec_old", opened) + + // New values go under the organization's key. + after, err := cipher.EncryptTenantSecret(t.Context(), "org-a", "webhook-signing:w1", "whsec_new") + require.NoError(t, err) + require.Contains(t, after, "acme-key|") + + // And the sweep moves the old one, idempotently. + resealed, changed, err := cipher.ResealTenantSecret(t.Context(), "org-a", "webhook-signing:w1", before) + require.NoError(t, err) + require.True(t, changed) + require.Contains(t, resealed, "acme-key|") + _, changed, err = cipher.ResealTenantSecret(t.Context(), "org-a", "webhook-signing:w1", resealed) + require.NoError(t, err) + require.False(t, changed, "re-sealing what is already under the key must change nothing") +} + +// Crypto-shredding: the credentials are unreadable, and the reason says so +// rather than reading as a credential the user should re-enter. +func TestTenantCipherRefusesARevokedKeyWithoutCondemningTheCredential(t *testing.T) { + cipher, source, _ := newTenantCipher(t, map[string]*OrgKeyBinding{ + "org-a": {KeyRef: "acme-key", CustomerHeld: true}, + }) + stored, err := cipher.EncryptTenantSecret(t.Context(), "org-a", "org-idp:org-a", "idp-secret") + require.NoError(t, err) + + source.rows["org-a"] = &OrgKeyBinding{ + KeyRef: "acme-key", CustomerHeld: true, RevokedReason: "customer instruction", + } + + _, err = cipher.DecryptTenantSecret(t.Context(), "org-a", "org-idp:org-a", stored) + require.ErrorIs(t, err, ErrKeyRevoked) + require.NotErrorIs(t, err, business.ErrInvalidSecretEnvelope, + "a destroyed key is not a credential to re-enter — telling the customer to reconnect would be the opposite of what they asked for") + require.Contains(t, err.Error(), "customer instruction") + + // And nothing further may be sealed under it. + _, err = cipher.EncryptTenantSecret(t.Context(), "org-a", "org-idp:org-a", "new-secret") + require.ErrorIs(t, err, ErrKeyRevoked) +} + +// The property the explicit argument exists for. +func TestTenantCipherRefusesAnAbsentOrganization(t *testing.T) { + cipher, _, _ := newTenantCipher(t, map[string]*OrgKeyBinding{ + "org-a": {KeyRef: "acme-key"}, + }) + + _, err := cipher.EncryptTenantSecret(t.Context(), "", "org-idp:org-a", "secret") + require.Error(t, err, "an absent organization must never fall back to the deployment's key") + require.Contains(t, err.Error(), "requires the organization") + + _, err = cipher.DecryptTenantSecret(t.Context(), "", "org-idp:org-a", "cfs1:test-key-service:acme-key|p|v") + require.Error(t, err) + require.Contains(t, err.Error(), "requires the organization") +} + +// A sealer is built once per key reference, not per request: building one is a +// round trip to the key service. +func TestTenantCipherCachesOneSealerPerKey(t *testing.T) { + cipher, _, factory := newTenantCipher(t, map[string]*OrgKeyBinding{ + "org-a": {KeyRef: "shared-key"}, + "org-b": {KeyRef: "shared-key"}, + }) + for range 5 { + _, err := cipher.EncryptTenantSecret(t.Context(), "org-a", "p", "v") + require.NoError(t, err) + _, err = cipher.EncryptTenantSecret(t.Context(), "org-b", "p", "v") + require.NoError(t, err) + } + require.Equal(t, 1, factory.built, "two organizations on one key share one sealer") +} + +// A binding that cannot be read must refuse, not fall through to the +// deployment's key: "the lookup failed" and "there is no binding" are different +// answers, and conflating them seals a customer's data under our key. +func TestTenantCipherRefusesWhenTheBindingCannotBeRead(t *testing.T) { + cipher, source, _ := newTenantCipher(t, nil) + source.err = errors.New("database unavailable") + + _, err := cipher.EncryptTenantSecret(t.Context(), "org-a", "p", "v") + require.Error(t, err) + require.Contains(t, err.Error(), "database unavailable") + + _, err = cipher.DecryptTenantSecret(t.Context(), "org-a", "p", "cfs1:test-key-service:deployment|p|v") + require.Error(t, err) + require.Contains(t, err.Error(), "database unavailable") +} + +// The keyed hash never moves off the deployment's key, whatever the organization +// has: API-key lookup is BY the hash, and the lookup is what discovers the +// organization. +func TestTenantCipherKeyedHashIsAlwaysTheDeploymentsOwn(t *testing.T) { + cipher, _, _ := newTenantCipher(t, map[string]*OrgKeyBinding{ + "org-a": {KeyRef: "acme-key"}, + }) + hash, err := cipher.HashKey(t.Context(), "cfly_sk_live_example") + require.NoError(t, err) + require.True(t, strings.HasPrefix(hash, "deployment:"), + "the hash must be the deployment's, got %q", hash) +} diff --git a/module/services/accounts/code/pkg/keyservice/vault.go b/module/services/accounts/code/pkg/keyservice/vault.go index 023666544..1e6acad77 100644 --- a/module/services/accounts/code/pkg/keyservice/vault.go +++ b/module/services/accounts/code/pkg/keyservice/vault.go @@ -16,11 +16,16 @@ import ( "accounts/pkg/vaultconnection" ) -// transitKeyName is the Transit key every purpose is sealed and hashed under. -// It is a single name on purpose: the keyed hash behind API-key lookup must stay -// comparable with every hash already stored, so the key it is computed under -// cannot move. -const transitKeyName = "api-keys" +// defaultTransitKeyName is the Transit key this deployment seals under when an +// organization has no key of its own, and the key the keyed hash is ALWAYS +// computed under. +// +// The hash cannot move: API-key lookup is BY that value, so a hash computed +// under another key stops matching every key already stored. The envelope key +// can move — per organization, or on a cutover — because every sealed value +// records which key sealed it. Two different questions, which is why they are +// two fields on the sealer rather than one name. +const defaultTransitKeyName = "api-keys" // VaultSealer is the Vault Transit backend: the AppRole + Transit binding // accounts has always used, behind the key-service seam and otherwise @@ -31,6 +36,10 @@ type VaultSealer struct { token string connection *vaultconnection.Connection client *http.Client + // envelopeKey seals and opens. Empty means defaultTransitKeyName, and a + // value sealed under that key is stored in the pre-per-key payload shape so + // nothing already written moves. + envelopeKey string } // NewVaultSealer binds the cell's Vault through the `vault` configuration @@ -52,12 +61,43 @@ func NewVaultSealerDirect(address, token string) *VaultSealer { func (v *VaultSealer) Tag() string { return TagVaultTransit } // Identity is the tag: see Accepts. -func (v *VaultSealer) Identity() string { return TagVaultTransit } +func (v *VaultSealer) Identity() string { return TagVaultTransit + ":" + v.transitKey() } + +// transitKey is the Transit key this sealer seals under. +func (v *VaultSealer) transitKey() string { + if v.envelopeKey == "" { + return defaultTransitKeyName + } + return v.envelopeKey +} + +// Accepts requires the tag and the Transit key the payload names. +// +// A Vault ciphertext carries its key VERSION (`vault:v:`) and not its key +// NAME, so the payload has to name the key itself or a per-organization key +// could not be told from the deployment's on read — and Vault would answer a +// foreign key's ciphertext with the same failure it gives a corrupt one. +func (v *VaultSealer) Accepts(envelope Envelope) bool { + if envelope.Backend != TagVaultTransit { + return false + } + key, _ := splitVaultPayload(envelope.Payload) + return key == v.transitKey() +} -// Accepts needs only the tag: the Transit key is the `api-keys` constant rather -// than a configured name, so one deployment has exactly one Vault envelope key -// and nothing to disambiguate. -func (v *VaultSealer) Accepts(envelope Envelope) bool { return envelope.Backend == TagVaultTransit } +// splitVaultPayload reads the optional key segment. +// +// The pre-per-key shape is a single base64url ciphertext, which belongs to +// defaultTransitKeyName — that is what every value written before per-organization +// keys existed looks like, and it keeps opening unchanged. base64url contains no +// ":", so a payload carrying one is the keyed shape and the two cannot be +// confused. +func splitVaultPayload(payload string) (key, ciphertext string) { + if name, rest, found := strings.Cut(payload, ":"); found { + return name, rest + } + return defaultTransitKeyName, payload +} // Health hits Vault's /v1/sys/health and returns nil only on the canonical // 200 / 429 / 472 / 473 codes that signal a reachable and un-sealed Vault. Used @@ -89,7 +129,8 @@ func (v *VaultSealer) MAC(ctx context.Context, plaintext string) (string, error) if err != nil { return "", err } - result, err := v.request(ctx, "/v1/transit/hmac/"+transitKeyName, string(body)) + // defaultTransitKeyName, never v.transitKey(): see the constant. + result, err := v.request(ctx, "/v1/transit/hmac/"+defaultTransitKeyName, string(body)) if err != nil { return "", fmt.Errorf("vault hmac request: %w", err) } @@ -111,7 +152,7 @@ func (v *VaultSealer) Seal(ctx context.Context, purpose, plaintext string) (stri if err != nil { return "", err } - result, err := v.request(ctx, "/v1/transit/encrypt/"+transitKeyName, string(body)) + result, err := v.request(ctx, "/v1/transit/encrypt/"+v.transitKey(), string(body)) if err != nil { return "", err } @@ -119,11 +160,19 @@ func (v *VaultSealer) Seal(ctx context.Context, purpose, plaintext string) (stri if !ok || ciphertext == "" { return "", errors.New("vault transit response missing ciphertext") } - return base64.RawURLEncoding.EncodeToString([]byte(ciphertext)), nil + encoded := base64.RawURLEncoding.EncodeToString([]byte(ciphertext)) + if v.envelopeKey == "" { + // The deployment's own key keeps the shape it has always written, so a + // value sealed before per-organization keys existed and one sealed now + // are byte-identical in form. + return encoded, nil + } + return v.envelopeKey + ":" + encoded, nil } func (v *VaultSealer) Open(ctx context.Context, purpose, payload string) (string, error) { - ciphertext, err := base64.RawURLEncoding.DecodeString(payload) + _, encoded := splitVaultPayload(payload) + ciphertext, err := base64.RawURLEncoding.DecodeString(encoded) if err != nil || len(ciphertext) == 0 { return "", fmt.Errorf("invalid secret envelope: %w", business.ErrInvalidSecretEnvelope) } @@ -131,7 +180,7 @@ func (v *VaultSealer) Open(ctx context.Context, purpose, payload string) (string if err != nil { return "", err } - result, err := v.request(ctx, "/v1/transit/decrypt/"+transitKeyName, string(body)) + result, err := v.request(ctx, "/v1/transit/decrypt/"+v.transitKey(), string(body)) if err != nil { return "", err } @@ -243,3 +292,40 @@ func openPurposeBoundPayload(purpose string, plaintext []byte) (string, error) { } return payload.Value, nil } + +// SealerFor binds another Transit key of the same Vault, for an organization +// whose credentials are sealed under a key of its own. +// +// No boot probe here, unlike the Cloud KMS factory: a Transit key is created on +// first encrypt (when the policy grants it), so probing would either create the +// key as a side effect of reading a binding or fail on a key that is legitimately +// not yet used. The first real seal is the check. +func (v *VaultSealer) SealerFor(_ context.Context, keyRef string) (Sealer, error) { + if err := validTransitKeyName(keyRef); err != nil { + return nil, err + } + bound := *v + bound.envelopeKey = keyRef + return &bound, nil +} + +// validTransitKeyName refuses a name that would change the request path rather +// than the key: the name is interpolated into /v1/transit/encrypt/, so a +// slash or a traversal segment would address a different endpoint entirely. +func validTransitKeyName(name string) error { + if name == "" { + return errors.New("key service: a Vault transit key name is required") + } + if name == defaultTransitKeyName { + return fmt.Errorf("key service: %q is this deployment's own transit key and cannot be an organization's", name) + } + for _, r := range name { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9': + case r == '-', r == '_', r == '.': + default: + return fmt.Errorf("key service: invalid Vault transit key name %q: letters, digits, dash, underscore and dot only", name) + } + } + return nil +} From a103ad81f7fbb3828b94f7d17d23c1888895fbad Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Thu, 8 Oct 2026 11:41:04 -0400 Subject: [PATCH 2/3] feat(accounts): the row's organization, checked against the transaction's scope (Refs #1013) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reconsidering the previous commit's "passed explicitly, never read from the context", which was the right mechanism defended with the wrong reason — and which left a hole. The reason was wrong: I was objecting to LENIENCY, not to implicitness. An implicit read that refuses when there is no verified organization has none of the downside I was guarding against, and "the system already resolves this, so do not restate it" is the principle this repository applies everywhere else. The mechanism survives anyway, for a reason that is actually about the data: - the delivery paths have no request scope at all. The outbound sender opens a subscription's signing secret as app_webhook_worker, outside any tenant transaction, so there is no verified organization to read — the row is all there is. - a value's organization is a property of the ROW, not of whoever is asking. Sealing under whatever organization the current request happens to be scoped to would be a different question that agrees most of the time, which is the worst kind of coincidence. So it stays an argument, but it is now documented as the row's own organization (sub.OrgID, source.OrgID) rather than something threaded down, which is what it always was at every call site. **The hole that reasoning hid:** an argument independent of the transaction's scope can DISAGREE with it, and row-level security binds the scope, never the argument — so nothing in the stack caught a row sealed for one organization inside a transaction scoped to another. requireConsistentScope refuses that now, both directions, on seal and on open, while still admitting the no-scope delivery case. The scope reader is injected as a variable so this package does not import the request-identity plumbing and a test can drive both answers. Mutation-proven: accepting a disagreeing scope fails the new test. Co-Authored-By: Claude Opus 5 (1M context) --- .../code/pkg/keyservice/tenant_cipher.go | 63 ++++++++++++++++--- .../code/pkg/keyservice/tenant_cipher_test.go | 44 +++++++++++++ 2 files changed, 98 insertions(+), 9 deletions(-) diff --git a/module/services/accounts/code/pkg/keyservice/tenant_cipher.go b/module/services/accounts/code/pkg/keyservice/tenant_cipher.go index 604b57651..25d2d57aa 100644 --- a/module/services/accounts/code/pkg/keyservice/tenant_cipher.go +++ b/module/services/accounts/code/pkg/keyservice/tenant_cipher.go @@ -15,11 +15,30 @@ import ( // organization before it could store anything would be worse for everyone. A // binding is an OVERRIDE. // -// The organization is passed EXPLICITLY rather than read from the context. An -// implicit read that returned "" would seal a customer's credential under this -// deployment's key — silently, and exactly for the customer who went to the -// trouble of bringing their own. That is the failure this type exists to -// prevent, so the argument is required and an empty one is refused. +// Which organization a value belongs to comes from the ROW being sealed — +// sub.OrgID, source.OrgID, the org a provider is configured for — not from the +// request context, and not threaded down from a caller's caller. Two reasons it +// cannot be the context: +// +// - the delivery paths have none. The outbound webhook sender opens a +// subscription's signing secret as app_webhook_worker, outside any tenant +// transaction, so there is no verified organization to read; the row is all +// there is. +// - the key should follow the DATA. A value's organization is a property of +// the row, and sealing it under whatever organization the current request +// happens to be scoped to would be a different question with the same +// answer most of the time, which is the worst kind of coincidence. +// +// Where the context DOES carry a verified organization, the two must agree, and +// requireConsistentScope refuses when they do not: the row's organization and +// the transaction's scope disagreeing means one of them is wrong, and guessing +// which seals a customer's credential under another customer's key. Row-level +// security binds the transaction, never this argument, so nothing else catches +// it. +// +// An absent organization is refused outright. A fallback to the deployment's key +// would betray exactly the customer who went to the trouble of bringing their +// own. type TenantCipher struct { // deployment seals for an organization with no key of its own, and is the // only thing that computes the keyed hash. @@ -95,8 +114,8 @@ func (c *TenantCipher) EncryptTenantSecret(ctx context.Context, orgID, purpose, // that has just been given its own key still holds credentials sealed under the // deployment's, and those must keep opening until they are re-sealed. func (c *TenantCipher) DecryptTenantSecret(ctx context.Context, orgID, purpose, stored string) (string, error) { - if orgID == "" { - return "", errors.New("key service: opening an organization's secret requires the organization") + if err := requireConsistentScope(ctx, orgID, "opening"); err != nil { + return "", err } binding, err := c.bindings.OrgKeyBinding(ctx, orgID) if err != nil { @@ -159,8 +178,8 @@ func (c *TenantCipher) ResealTenantSecret(ctx context.Context, orgID, purpose, s // sealerFor resolves the organization's sealer, or nil when it has none and the // deployment's key applies. func (c *TenantCipher) sealerFor(ctx context.Context, orgID string) (Sealer, error) { - if orgID == "" { - return nil, errors.New("key service: sealing an organization's secret requires the organization") + if err := requireConsistentScope(ctx, orgID, "sealing"); err != nil { + return nil, err } binding, err := c.bindings.OrgKeyBinding(ctx, orgID) if err != nil { @@ -197,3 +216,29 @@ func (c *TenantCipher) boundSealer(ctx context.Context, keyRef string) (Sealer, c.mu.Unlock() return sealer, nil } + +// VerifiedScope reports the organization the caller's transaction is scoped to, +// when it is running inside one. A path with no request identity — a delivery +// worker — reports false, which is not an error. +// +// It is a variable so the wiring supplies it: this package must not import the +// request-identity plumbing, and a test must be able to drive both answers. +var VerifiedScope func(ctx context.Context) (orgID string, ok bool) + +// requireConsistentScope refuses a row whose organization is absent, and one +// that disagrees with the transaction's own scope. +func requireConsistentScope(ctx context.Context, orgID, verb string) error { + if orgID == "" { + return fmt.Errorf("key service: %s an organization's secret requires the organization it belongs to", verb) + } + if VerifiedScope == nil { + return nil + } + scope, ok := VerifiedScope(ctx) + if !ok || scope == orgID { + // No request scope is the delivery-worker case, which is legitimate. + return nil + } + return fmt.Errorf("key service: refusing to %s a secret for organization %s inside a transaction scoped to %s: the row and the scope disagree, and row-level security binds the scope rather than this argument, so one of them is wrong", + verb, orgID, scope) +} diff --git a/module/services/accounts/code/pkg/keyservice/tenant_cipher_test.go b/module/services/accounts/code/pkg/keyservice/tenant_cipher_test.go index 855b4e7b5..de3375be1 100644 --- a/module/services/accounts/code/pkg/keyservice/tenant_cipher_test.go +++ b/module/services/accounts/code/pkg/keyservice/tenant_cipher_test.go @@ -243,3 +243,47 @@ func TestTenantCipherKeyedHashIsAlwaysTheDeploymentsOwn(t *testing.T) { require.True(t, strings.HasPrefix(hash, "deployment:"), "the hash must be the deployment's, got %q", hash) } + +// The row's organization and the transaction's scope must agree. Row-level +// security binds the SCOPE, never the argument, so a disagreement means one of +// them is wrong — and guessing which seals a customer's credential under another +// customer's key. Nothing else in the stack catches this. +func TestTenantCipherRefusesARowWhoseOrganizationDisagreesWithTheScope(t *testing.T) { + cipher, _, _ := newTenantCipher(t, map[string]*OrgKeyBinding{ + "org-a": {KeyRef: "acme-key"}, + "org-b": {KeyRef: "other-key"}, + }) + + scoped := func(org string) context.Context { + return context.WithValue(t.Context(), scopeKeyForTest{}, org) + } + previous := VerifiedScope + VerifiedScope = func(ctx context.Context) (string, bool) { + org, ok := ctx.Value(scopeKeyForTest{}).(string) + return org, ok + } + t.Cleanup(func() { VerifiedScope = previous }) + + // Agreeing is fine. + _, err := cipher.EncryptTenantSecret(scoped("org-a"), "org-a", "org-idp:org-a", "secret") + require.NoError(t, err) + + // Disagreeing is refused, both directions, on both verbs. + _, err = cipher.EncryptTenantSecret(scoped("org-b"), "org-a", "org-idp:org-a", "secret") + require.Error(t, err) + require.Contains(t, err.Error(), "the row and the scope disagree") + + stored, err := cipher.EncryptTenantSecret(scoped("org-a"), "org-a", "org-idp:org-a", "secret") + require.NoError(t, err) + _, err = cipher.DecryptTenantSecret(scoped("org-b"), "org-a", "org-idp:org-a", stored) + require.Error(t, err) + require.Contains(t, err.Error(), "the row and the scope disagree") + + // A path with NO request scope is the delivery worker, and is legitimate: + // the outbound sender opens a subscription's secret as app_webhook_worker, + // outside any tenant transaction, with only the row to go on. + _, err = cipher.DecryptTenantSecret(t.Context(), "org-a", "org-idp:org-a", stored) + require.NoError(t, err, "a delivery worker has no request scope and must still open the row's secret") +} + +type scopeKeyForTest struct{} From ef90557a8fb2128d3873101963381d4e91296abb Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Thu, 8 Oct 2026 11:58:56 -0400 Subject: [PATCH 3/3] feat(accounts): bind the verified organization on every door, not one transport (Refs #1013) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit wool's organization was stamped in one place — the gRPC interceptor's stampRequestIdentity — so the six other paths that install a verified identity carried none: a delegated record read, a module parent, a delegated read audience, readable sources, and the work-context authority twice. Anything downstream that asks whose request this is got nothing on those paths. Binding it inside WithVerifiedDatabaseIdentity covers all seven, and covers them at the one door that has already verified the pair. That matters more than the coverage: wool's identity keys are EXPORTED, so anything may set them, and the dimension is only worth reading if the writers are known. A test derives the writer set from the syntax tree and pins it at exactly two, with each one's reason, so a third — especially one fed by caller-controlled input — fails rather than quietly making the dimension forgeable. Two writers rather than one, because the two ids have different availability. The verified door binds nothing without BOTH uuids, by design: database scope needs both. A verified user who has not selected an organization yet is an ordinary request that binds no scope, and their logs and rate-limit key should still name them — so the user id stays stamped unconditionally in the interceptor. Deduping that was a regression, caught by the gateway-rotation tests in pkg/adapters, and it is why the split is now written down. This does NOT make wool an authority source, and nothing here reads identity from it to decide anything. pkg/auth states that transport headers and wool values are deliberately insufficient database or authorization-cache authority, and the verified identity's context key stays private for that reason. The direction is one-way: verified identity -> wool, for observability and for the organization-defaulted reads that already consult it. Upstream, codefly-dev/core#743 fixes why this has to use the exported keys directly: wool.Wool.WithOrgID cannot propagate at all — `with` assigns to the Wool's own ctx and Get returns a fresh Wool per call, so the derived context is discarded. Proven against the pinned core before writing it this way. Co-Authored-By: Claude Opus 5 (1M context) --- .../pkg/adapters/grpc_auth_interceptor.go | 15 ++-- .../code/pkg/auth/database_identity.go | 21 +++++ .../pkg/auth/wool_identity_sources_test.go | 76 +++++++++++++++++ .../code/pkg/auth/wool_identity_test.go | 84 +++++++++++++++++++ 4 files changed, 191 insertions(+), 5 deletions(-) create mode 100644 module/services/accounts/code/pkg/auth/wool_identity_sources_test.go create mode 100644 module/services/accounts/code/pkg/auth/wool_identity_test.go diff --git a/module/services/accounts/code/pkg/adapters/grpc_auth_interceptor.go b/module/services/accounts/code/pkg/adapters/grpc_auth_interceptor.go index 2f0b35204..c702d0047 100644 --- a/module/services/accounts/code/pkg/adapters/grpc_auth_interceptor.go +++ b/module/services/accounts/code/pkg/adapters/grpc_auth_interceptor.go @@ -16,8 +16,6 @@ import ( "google.golang.org/grpc/metadata" "google.golang.org/grpc/status" - "github.com/google/uuid" - "accounts/pkg/auth" "accounts/pkg/business" policyv1 "accounts/pkg/gen/saas/policy/v1" @@ -455,12 +453,19 @@ func stampForwardedGRPCIdentity(ctx context.Context, md metadata.MD) (context.Co func stampRequestIdentity(ctx context.Context, identity auth.RequestIdentity, assurance auth.Assurance) context.Context { subject := identity.EffectiveSubjectID() orgID := identity.OrgID.String() + // WithVerifiedDatabaseIdentity binds wool's organization as well as the + // database scope, so every verified door carries it — a delegated read, a + // module parent and the work-context authority all install a verified + // identity without passing through this transport. + // + // The user id is still stamped HERE, unconditionally, because the two have + // different availability: database scope needs BOTH uuids and that function + // binds nothing without them, while a verified user who has not selected an + // organization yet is an ordinary request whose logs and rate-limit key + // should still name them. ctx = auth.WithVerifiedDatabaseIdentity(ctx, subject, orgID) ctx = context.WithValue(ctx, wool.UserIDKey, subject) ctx = context.WithValue(ctx, wool.UserAuthIDKey, subject) - if identity.OrgID != uuid.Nil { - ctx = context.WithValue(ctx, wool.OrgIDKey, orgID) - } ctx = auth.WithVerifiedRequestIdentity(ctx, identity) ctx = auth.WithVerifiedActor(ctx, identity.Delegation) ctx = auth.WithVerifiedSessionID(ctx, identity.SessionID) diff --git a/module/services/accounts/code/pkg/auth/database_identity.go b/module/services/accounts/code/pkg/auth/database_identity.go index 2b129fcb4..3cef9020d 100644 --- a/module/services/accounts/code/pkg/auth/database_identity.go +++ b/module/services/accounts/code/pkg/auth/database_identity.go @@ -4,6 +4,8 @@ import ( "context" "errors" + "github.com/codefly-dev/core/wool" + "github.com/google/uuid" ) @@ -37,6 +39,25 @@ func WithVerifiedDatabaseIdentity(ctx context.Context, userID, tenantID string) if ctx == nil || userErr != nil || tenantErr != nil || user == uuid.Nil || tenant == uuid.Nil { return ctx } + // Propagate the same pair onto wool's identity dimension, so everything + // downstream — logs, traces, the rate-limit key, an organization-defaulted + // read — sees whose request this is without being handed it. + // + // Set as context values rather than through wool.Wool.WithOrgID, which + // cannot work: `with` assigns to the Wool's OWN ctx field, and wool.Get + // returns a fresh Wool per call, so the derived context is discarded and no + // later wool.Get(ctx).OrgID() can see it. The keys are exported and + // wool.Wool.lookup reads them off the context, so this is the propagation + // the accessors were written for. + // + // It is bound HERE and nowhere else for the same reason the verified + // identity is: this is the one door, it has already parsed both UUIDs, and + // the invalid path returned above — so wool's organization can only ever + // carry a verified value. That matters now that it is not merely an + // observability field: a key service selects an organization's envelope key + // by it. + ctx = context.WithValue(ctx, wool.OrgIDKey, tenant.String()) + ctx = context.WithValue(ctx, wool.UserIDKey, user.String()) return context.WithValue(ctx, verifiedDatabaseIdentityKey{}, verifiedDatabaseIdentity{ tenantID: tenant.String(), userID: user.String(), diff --git a/module/services/accounts/code/pkg/auth/wool_identity_sources_test.go b/module/services/accounts/code/pkg/auth/wool_identity_sources_test.go new file mode 100644 index 000000000..907fa79b2 --- /dev/null +++ b/module/services/accounts/code/pkg/auth/wool_identity_sources_test.go @@ -0,0 +1,76 @@ +package auth_test + +import ( + "go/ast" + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "sort" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// sourceSitesSettingWoolIdentity returns every non-test file under the service +// that writes wool's identity keys, as a repository-relative path. +// +// Derived from the syntax tree rather than grepped: a grep for the key name also +// matches a read, and the question here is specifically who WRITES. +func sourceSitesSettingWoolIdentity(t *testing.T) []string { + t.Helper() + identityKeys := map[string]bool{"OrgIDKey": true, "UserIDKey": true} + found := map[string]bool{} + + root := "../.." + require.NoError(t, filepath.WalkDir(root, func(path string, entry fs.DirEntry, err error) error { + if err != nil { + return err + } + if entry.IsDir() { + switch entry.Name() { + case "node_modules", "gen", "generated", "testdata": + return fs.SkipDir + } + return nil + } + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + file, parseErr := parser.ParseFile(token.NewFileSet(), path, nil, 0) + if parseErr != nil { + return nil + } + ast.Inspect(file, func(node ast.Node) bool { + call, ok := node.(*ast.CallExpr) + if !ok { + return true + } + // context.WithValue(ctx, wool.OrgIDKey, …) + if selector, ok := call.Fun.(*ast.SelectorExpr); ok && selector.Sel.Name == "WithValue" && len(call.Args) == 3 { + if key, ok := call.Args[1].(*ast.SelectorExpr); ok && identityKeys[key.Sel.Name] { + if pkg, ok := key.X.(*ast.Ident); ok && pkg.Name == "wool" { + found[filepath.ToSlash(filepath.Clean(path))] = true + } + } + } + // w.WithOrgID(…) / w.WithUserID(…) — a no-op as the API is built, + // but a writer in intent, so it counts. + if selector, ok := call.Fun.(*ast.SelectorExpr); ok { + if selector.Sel.Name == "WithOrgID" || selector.Sel.Name == "WithUserID" { + found[filepath.ToSlash(filepath.Clean(path))] = true + } + } + return true + }) + return nil + })) + + sites := make([]string, 0, len(found)) + for path := range found { + sites = append(sites, strings.TrimPrefix(path, "../../")) + } + sort.Strings(sites) + return sites +} diff --git a/module/services/accounts/code/pkg/auth/wool_identity_test.go b/module/services/accounts/code/pkg/auth/wool_identity_test.go new file mode 100644 index 000000000..98984fe1d --- /dev/null +++ b/module/services/accounts/code/pkg/auth/wool_identity_test.go @@ -0,0 +1,84 @@ +package auth_test + +import ( + "context" + "testing" + + "github.com/codefly-dev/core/wool" + "github.com/google/uuid" + "github.com/stretchr/testify/require" + + "accounts/pkg/auth" +) + +// The verified door is the one place wool's identity is bound, so everything +// downstream — logs, traces, the rate-limit key, an organization-defaulted read +// — sees whose request this is without being handed it. +func TestVerifiedDatabaseIdentityPopulatesWool(t *testing.T) { + user, org := uuid.NewString(), uuid.NewString() + + ctx := auth.WithVerifiedDatabaseIdentity(context.Background(), user, org) + + gotOrg, ok := wool.Get(ctx).OrgID() + require.True(t, ok, "wool must carry the organization after the verified door") + require.Equal(t, org, gotOrg) + gotUser, ok := wool.Get(ctx).UserID() + require.True(t, ok) + require.Equal(t, user, gotUser) +} + +// The invalid path returns the context untouched, so a malformed +// trusted-forwarding value cannot put an organization into wool either — the +// same fail-closed behaviour the verified identity already had. +func TestAMalformedIdentityPopulatesNothing(t *testing.T) { + for name, identity := range map[string][2]string{ + "unparseable org": {uuid.NewString(), "not-a-uuid"}, + "unparseable user": {"not-a-uuid", uuid.NewString()}, + "nil org": {uuid.NewString(), uuid.Nil.String()}, + "nil user": {uuid.Nil.String(), uuid.NewString()}, + "both empty": {"", ""}, + } { + t.Run(name, func(t *testing.T) { + ctx := auth.WithVerifiedDatabaseIdentity(context.Background(), identity[0], identity[1]) + + _, ok := wool.Get(ctx).OrgID() + require.False(t, ok, "a malformed identity must leave wool empty") + _, _, verified := auth.VerifiedDatabaseIdentity(ctx) + require.False(t, verified) + }) + } +} + +// wool's organization is now read by things that matter (the rate-limit key, an +// organization-defaulted audit read), so it must only ever carry a verified +// value. This pins that the verified door is the ONLY writer: the key is +// exported, so anything may set it, and a second writer — especially one fed by +// caller-controlled input — would make wool's organization forgeable. +// +// It is deliberately NOT an authority source even so: pkg/auth states that +// transport headers and wool values are insufficient database or +// authorization-cache authority, and this does not change that. The point of one +// writer is that the observability dimension cannot be made to lie. +func TestOnlyTheVerifiedDoorWritesWoolIdentity(t *testing.T) { + // Two writers, and each one's reason is why the list is pinned rather than + // merely bounded: + // + // database_identity.go — the verified door. Binds the organization (and + // the user) on every path that installs a verified identity, which is + // the only way wool's organization can be trusted at all. + // grpc_auth_interceptor.go — the user id only, unconditionally, because a + // verified user who has not selected an organization binds no database + // scope and so reaches no door above, yet should still be named in logs + // and in the rate-limit key. + // + // A THIRD writer is what this refuses. The keys are exported, so anything + // may set them, and a writer fed by caller-controlled input would make the + // dimension forgeable. + writers := sourceSitesSettingWoolIdentity(t) + require.Equal(t, []string{ + "pkg/adapters/grpc_auth_interceptor.go", + "pkg/auth/database_identity.go", + }, writers, + "wool's identity keys have exactly two writers, both named above; a third makes the dimension "+ + "forgeable, and the rate limiter and the organization-defaulted audit read both consult it") +}