diff --git a/internal/autoconfig/stream_manager.go b/internal/autoconfig/stream_manager.go index 1c77fe87..5c15b845 100644 --- a/internal/autoconfig/stream_manager.go +++ b/internal/autoconfig/stream_manager.go @@ -498,16 +498,11 @@ func (s *StreamManager) dispatchEnvAction(id config.EnvironmentID, rep envfactor } // validateCredentialPayload checks that an environment rep carries a structurally valid credential -// set. It is run at the stream parse boundary — before the rep's version is recorded via Upsert — -// mirroring how an unparseable event is handled by gotMalformedEvent. -// -// A malformed credential payload must preserve the previous accepted set and force a -// stream reconnect (RAC is one-way push with no NAK channel, so the reconnect is what makes the -// backend resend a fresh put). Validating here rather than after Upsert is essential: the version is -// not advanced, so the fresh put — which carries the same version — is not deduplicated away by the -// MessageReceiver. Any error from BuildAcceptedSet is a *MalformedCredentialSetError. +// set. It runs at the stream parse boundary, before the rep's version is recorded via Upsert: a +// malformed payload must not advance the version, or the backend's fresh put — which carries the same +// version — would be deduplicated away by the MessageReceiver. func (s *StreamManager) validateCredentialPayload(rep envfactory.EnvironmentRep) error { - _, err := envfactory.BuildAcceptedSet(rep.ToParams()) + _, _, err := envfactory.BuildAcceptedSet(rep.ToParams()) return err } diff --git a/internal/credential/accepted_set.go b/internal/credential/accepted_set.go index 24dee4f3..c3011929 100644 --- a/internal/credential/accepted_set.go +++ b/internal/credential/accepted_set.go @@ -1,7 +1,6 @@ package credential import ( - "errors" "fmt" "github.com/launchdarkly/ld-relay/v8/config" @@ -48,11 +47,6 @@ func (s AcceptedSet) hasMobileKey(key config.MobileKey) bool { return ok } -// errAcceptedSetMissingSDKKey is returned by AcceptedSetBuilder.Build when no SDK key was added. An -// environment must always have at least one SDK key (its anchor), so an empty set indicates a caller -// mistake rather than a benign edge case — surfacing it avoids a silent misconfiguration. -var errAcceptedSetMissingSDKKey = errors.New("accepted credential set must contain at least one SDK key") - // MalformedCredentialSetError is returned when a credential payload cannot produce a valid // AcceptedSet. This covers: // @@ -65,6 +59,9 @@ var errAcceptedSetMissingSDKKey = errors.New("accepted credential set must conta // would keep using the previous (possibly revoked) primary. (No mobile keys at all is valid.) // 4. An entry in sdkKeys[] or mobileKeys[] has an empty value — a credential that would be // accepted by relay but can never authenticate any SDK. +// 5. No SDK key survived at all, so the environment would have nothing to authenticate with. A +// payload reaches this by combining an undefined anchor with an sdkKeys[] array that is either +// empty or entirely made up of keys relay excludes, such as keys scoped to a view. // // Validation happens before Reconcile is called; Rotator.Reconcile trusts the set it is handed. // Because the error is raised before any state mutation, the environment's previous accepted set is @@ -86,6 +83,13 @@ func newMissingAnchorError() *MalformedCredentialSetError { return &MalformedCredentialSetError{msg: "malformed credential set: anchor SDK key is missing"} } +// newNoSDKKeysError returns a MalformedCredentialSetError for a set that ended up with no SDK key at +// all. The message describes the payload rather than the builder, because that is what an operator +// reading the log can act on. +func newNoSDKKeysError() *MalformedCredentialSetError { + return &MalformedCredentialSetError{msg: "malformed credential set: no usable SDK key in sdkKeys[]"} +} + // NewAnchorNotInSetError returns a MalformedCredentialSetError for a payload whose designated anchor // (sdkKey.value) is defined but not present in the sdkKeys[] array — a structural inconsistency. The // anchor value is a secret, so it is deliberately not included in the message. diff --git a/internal/credential/accepted_set_builder.go b/internal/credential/accepted_set_builder.go index 47136a8b..d6332749 100644 --- a/internal/credential/accepted_set_builder.go +++ b/internal/credential/accepted_set_builder.go @@ -89,13 +89,13 @@ func (b *AcceptedSetBuilder) WithEnvironmentID(id config.EnvironmentID) *Accepte return b } -// Build validates and returns the accumulated AcceptedSet. It returns errAcceptedSetMissingSDKKey if -// no SDK key was added, or a *MalformedCredentialSetError if no anchor was designated (via +// Build validates and returns the accumulated AcceptedSet. It returns a +// *MalformedCredentialSetError if no SDK key was added, or if no anchor was designated (via // WithAnchor). Because WithAnchor also adds the key, a designated anchor is always among the // accepted SDK keys. func (b *AcceptedSetBuilder) Build() (AcceptedSet, error) { if len(b.set.sdkKeys) == 0 { - return AcceptedSet{}, errAcceptedSetMissingSDKKey + return AcceptedSet{}, newNoSDKKeysError() } if !b.set.anchor.Defined() { return AcceptedSet{}, newMissingAnchorError() diff --git a/internal/credential/accepted_set_builder_test.go b/internal/credential/accepted_set_builder_test.go index 3f89f9c9..3b7b6be4 100644 --- a/internal/credential/accepted_set_builder_test.go +++ b/internal/credential/accepted_set_builder_test.go @@ -10,15 +10,15 @@ import ( ) func TestAcceptedSetBuilderValidation(t *testing.T) { - // No SDK key at all is a caller error. + // No SDK key at all is malformed: the environment would have nothing to authenticate with. + var malformed *MalformedCredentialSetError _, err := NewAcceptedSetBuilder(). WithMobileKey(MobileKeyParams{Value: "mob"}). WithEnvironmentID(config.EnvironmentID("env")). Build() - require.ErrorIs(t, err, errAcceptedSetMissingSDKKey) + require.ErrorAs(t, err, &malformed) // An SDK key with no designated anchor is malformed. - var malformed *MalformedCredentialSetError _, err = NewAcceptedSetBuilder().WithSDKKey(SDKKeyParams{Value: "sdk"}).Build() require.ErrorAs(t, err, &malformed) diff --git a/internal/envfactory/env_params.go b/internal/envfactory/env_params.go index 35acffcd..20d275b2 100644 --- a/internal/envfactory/env_params.go +++ b/internal/envfactory/env_params.go @@ -46,18 +46,22 @@ type EnvironmentParams struct { // AcceptedSDKKey is one entry in the accepted SDK key set for an environment. // Expiry is zero if the key is permanent. +// HasViews is true if the SDK key is associated with a view. type AcceptedSDKKey struct { - Key string - Value config.SDKKey - Expiry time.Time + Key string + Value config.SDKKey + Expiry time.Time + HasViews bool } // AcceptedMobileKey is one entry in the accepted mobile key set for an environment. // Expiry is zero if the key is permanent. +// HasViews is true if the mobile key is associated with a view. type AcceptedMobileKey struct { - Key string - Value config.MobileKey - Expiry time.Time + Key string + Value config.MobileKey + Expiry time.Time + HasViews bool } func (e EnvironmentParams) WithFilter(key config.FilterKey) EnvironmentParams { diff --git a/internal/envfactory/env_rep.go b/internal/envfactory/env_rep.go index d83f7ec0..0b150f72 100644 --- a/internal/envfactory/env_rep.go +++ b/internal/envfactory/env_rep.go @@ -105,9 +105,10 @@ type ExpiringKeyRep struct { // Key is the human-readable identifier (non-secret, e.g. "default-sdk"); Value is // the credential secret (e.g. "sdk-xxxx-..."). See the EnvironmentRep TERMINOLOGY comment. type ConcurrentKeyRep struct { - Key string `json:"key"` - Value string `json:"value"` - Expiry *int64 `json:"expiry,omitempty"` // Unix-ms; nil = permanent + Key string `json:"key"` + Value string `json:"value"` + Expiry *int64 `json:"expiry,omitempty"` // Unix-ms; nil = permanent + HasViews bool `json:"hasViews"` } func ToTime(millisecondTime ldtime.UnixMillisecondTime) time.Time { @@ -135,8 +136,9 @@ func (r EnvironmentRep) ToParams() EnvironmentParams { params.AcceptedSDKKeys = make([]AcceptedSDKKey, 0, len(r.SDKKeys)) for _, k := range r.SDKKeys { entry := AcceptedSDKKey{ - Key: k.Key, - Value: config.SDKKey(k.Value), + Key: k.Key, + Value: config.SDKKey(k.Value), + HasViews: k.HasViews, } if k.Expiry != nil { entry.Expiry = time.UnixMilli(*k.Expiry) @@ -162,8 +164,9 @@ func (r EnvironmentRep) ToParams() EnvironmentParams { params.AcceptedMobileKeys = make([]AcceptedMobileKey, 0, len(r.MobileKeys)) for _, k := range r.MobileKeys { entry := AcceptedMobileKey{ - Key: k.Key, - Value: config.MobileKey(k.Value), + Key: k.Key, + Value: config.MobileKey(k.Value), + HasViews: k.HasViews, } if k.Expiry != nil { entry.Expiry = time.UnixMilli(*k.Expiry) diff --git a/internal/envfactory/env_rep_test.go b/internal/envfactory/env_rep_test.go index 2499b045..6baaea75 100644 --- a/internal/envfactory/env_rep_test.go +++ b/internal/envfactory/env_rep_test.go @@ -159,6 +159,67 @@ func TestEnvironmentRepNewFormatWithArrays(t *testing.T) { assert.Equal(t, AcceptedMobileKey{Key: "mob-key-1", Value: config.MobileKey("mob-f41c")}, params.AcceptedMobileKeys[0]) } +// TestEnvironmentRepViewScopedKeys pins the hasViews wire contract on both arrays: it decodes onto +// ConcurrentKeyRep and is carried through ToParams onto the accepted entries, where BuildAcceptedSet +// consumes it. +// +// The absent-field case is the important one. hasViews is a plain bool, so an entry that omits it — +// every entry a backend that predates the field emits — decodes to false and is treated as not +// view-scoped. An explicit false is indistinguishable from absent, which is the intent: there is no +// third state. +func TestEnvironmentRepViewScopedKeys(t *testing.T) { + jsonStr := `{ + "envID": "68e5179e8307e4099c277e2a", + "envKey": "production", + "envName": "Production", + "mobKey": "mob-primary", + "projKey": "my-project", + "projName": "My Project", + "sdkKey": { "value": "sdk-anchor" }, + "sdkKeys": [ + { "key": "default-sdk", "value": "sdk-anchor" }, + { "key": "service-a", "value": "sdk-service-a", "hasViews": false }, + { "key": "view-scoped", "value": "sdk-viewy", "hasViews": true } + ], + "mobileKeys": [ + { "key": "default-mob", "value": "mob-primary" }, + { "key": "view-scoped-mob", "value": "mob-viewy", "hasViews": true } + ], + "version": 26 + }` + + var rep EnvironmentRep + require.NoError(t, json.Unmarshal([]byte(jsonStr), &rep)) + + require.Len(t, rep.SDKKeys, 3) + assert.False(t, rep.SDKKeys[0].HasViews, "an absent hasViews must decode to false") + assert.False(t, rep.SDKKeys[1].HasViews) + assert.True(t, rep.SDKKeys[2].HasViews) + + require.Len(t, rep.MobileKeys, 2) + assert.False(t, rep.MobileKeys[0].HasViews) + assert.True(t, rep.MobileKeys[1].HasViews) + + params := rep.ToParams() + + require.Len(t, params.AcceptedSDKKeys, 3) + assert.Equal(t, AcceptedSDKKey{Key: "default-sdk", Value: config.SDKKey("sdk-anchor")}, params.AcceptedSDKKeys[0]) + assert.Equal(t, AcceptedSDKKey{Key: "service-a", Value: config.SDKKey("sdk-service-a")}, params.AcceptedSDKKeys[1]) + assert.Equal(t, AcceptedSDKKey{ + Key: "view-scoped", + Value: config.SDKKey("sdk-viewy"), + HasViews: true, + }, params.AcceptedSDKKeys[2]) + + require.Len(t, params.AcceptedMobileKeys, 2) + assert.Equal(t, AcceptedMobileKey{Key: "default-mob", Value: config.MobileKey("mob-primary")}, params.AcceptedMobileKeys[0]) + assert.Equal(t, AcceptedMobileKey{ + Key: "view-scoped-mob", + Value: config.MobileKey("mob-viewy"), + HasViews: true, + }, params.AcceptedMobileKeys[1]) +} + // TestEnvironmentRepOldFormatNoArrays verifies that an old-format payload (singular sdkKey/mobKey // only, no sdkKeys/mobileKeys arrays) is normalized by ToParams() into a consistent accepted set. // The wire rep's SDKKeys/MobileKeys remain nil, but params.AcceptedSDKKeys/AcceptedMobileKeys are diff --git a/internal/envfactory/reconcile_helper.go b/internal/envfactory/reconcile_helper.go index 4a5f418e..e71eebf2 100644 --- a/internal/envfactory/reconcile_helper.go +++ b/internal/envfactory/reconcile_helper.go @@ -1,10 +1,31 @@ package envfactory import ( + "github.com/launchdarkly/ld-relay/v8/config" "github.com/launchdarkly/ld-relay/v8/internal/credential" "github.com/launchdarkly/ld-relay/v8/internal/util" ) +// collectViewScopedValues returns the set of credential values that any entry marks as view-scoped, +// excluding the designated key (the anchor, or the primary mobile key) which is never filtered. +// +// Two entries can carry the same value with only one of them marked. Keying on the value means one +// marked entry rejects it, whichever position it holds. +func collectViewScopedValues[E any, V comparable](entries []E, designated V, get func(E) (V, bool)) map[V]bool { + var viewScoped map[V]bool + for _, e := range entries { + value, hasViews := get(e) + if !hasViews || value == designated { + continue + } + if viewScoped == nil { + viewScoped = make(map[V]bool) + } + viewScoped[value] = true + } + return viewScoped // nil is a valid empty set to read from +} + // BuildAcceptedSet converts an EnvironmentParams into the AcceptedSet needed by // EnvContext.ReconcileCredentials. // @@ -20,33 +41,47 @@ import ( // The builder de-duplicates by value, so an anchor or primary mobile key that also appears in its // array is added only once. // -// A *credential.MalformedCredentialSetError is returned (with an empty AcceptedSet) for a -// structurally malformed payload: an undefined anchor (params.SDKKey not set), a defined anchor that -// is absent from params.AcceptedSDKKeys, a defined primary mobile key (params.MobileKey) that is -// absent from params.AcceptedMobileKeys, a non-empty params.AcceptedMobileKeys with no designated -// primary (params.MobileKey undefined), or an array entry with an empty value. The caller must -// preserve the previous accepted state and, for RAC handlers, reconnect the stream with jitter to -// force a fresh put. This is the single home for the anchor invariant. -func BuildAcceptedSet(params EnvironmentParams) (credential.AcceptedSet, error) { +// An error is returned (with an empty AcceptedSet) for a structurally malformed payload: an undefined +// anchor (params.SDKKey not set), a defined anchor that is absent from params.AcceptedSDKKeys, a +// defined primary mobile key (params.MobileKey) that is absent from params.AcceptedMobileKeys, a +// non-empty params.AcceptedMobileKeys with no designated primary (params.MobileKey undefined), an +// array entry with an empty value, or no usable SDK key at all. The caller must preserve the previous +// accepted state and, for RAC handlers, reconnect the stream with jitter to force a fresh put. This is +// the single home for the anchor invariant. +// +// Keys scoped to a view are filtered out here rather than at authentication time, making this the +// single funnel for both RAC and the offline archive. The second return value names the keys that were +// dropped, so callers can log what they lost. An SDK presenting one of them gets a 401: the key is +// simply absent from the lookup map. +func BuildAcceptedSet(params EnvironmentParams) (credential.AcceptedSet, []string, error) { anchor := params.SDKKey b := credential.NewAcceptedSetBuilder().WithEnvironmentID(params.EnvID) + var rejected []string // Add every accepted SDK key, designating the anchor as we encounter it. WithAnchor both adds and // designates, and forces the anchor permanent — so a payload that (wrongly) carries an expiry on // the anchor's own entry cannot demote it. An undefined anchor never matches a (defined) array - // value, so it is never designated and Build returns a *MalformedCredentialSetError. + // value, so it is never designated and Build rejects the payload. // // Entries with an empty value are structurally malformed: relay would silently accept them but // they can never authenticate any SDK. Reject loudly rather than produce a credential-short env. + viewScopedSDKValues := collectViewScopedValues(params.AcceptedSDKKeys, anchor, + func(k AcceptedSDKKey) (config.SDKKey, bool) { return k.Value, k.HasViews }) + anchorInArray := false for _, k := range params.AcceptedSDKKeys { if !k.Value.Defined() { - return credential.AcceptedSet{}, credential.NewEmptyCredentialError("sdkKeys", k.Key) + return credential.AcceptedSet{}, nil, credential.NewEmptyCredentialError("sdkKeys", k.Key) } - if k.Value == anchor { + switch { + // A marker on the anchor's own entry is disregarded: dropping the designated key would take the + // whole environment down, and the backend forbids views on a default key in the first place. + case k.Value == anchor: anchorInArray = true b.WithAnchor(credential.SDKKeyParams{Value: k.Value, Key: util.PtrOrNil(k.Key)}) - } else { + case viewScopedSDKValues[k.Value]: + rejected = append(rejected, k.Key) + default: b.WithSDKKey(credential.SDKKeyParams{Value: k.Value, Key: util.PtrOrNil(k.Key), Expiry: util.PtrOrNil(k.Expiry)}) } } @@ -55,21 +90,28 @@ func BuildAcceptedSet(params EnvironmentParams) (credential.AcceptedSet, error) // synthesizes it into the array for old-format payloads). A defined anchor absent from the array is // a structurally malformed payload — reject it. if anchor.Defined() && !anchorInArray { - return credential.AcceptedSet{}, credential.NewAnchorNotInSetError() + return credential.AcceptedSet{}, nil, credential.NewAnchorNotInSetError() } // Add every accepted mobile key, designating the primary as we encounter it. Like the anchor, // WithPrimaryMobileKey forces the primary permanent, so an expiry the payload may carry on the // primary's own entry cannot demote it. + viewScopedMobileValues := collectViewScopedValues(params.AcceptedMobileKeys, params.MobileKey, + func(k AcceptedMobileKey) (config.MobileKey, bool) { return k.Value, k.HasViews }) + primaryMobileInArray := false for _, k := range params.AcceptedMobileKeys { if !k.Value.Defined() { - return credential.AcceptedSet{}, credential.NewEmptyCredentialError("mobileKeys", k.Key) + return credential.AcceptedSet{}, nil, credential.NewEmptyCredentialError("mobileKeys", k.Key) } - if k.Value == params.MobileKey { + switch { + // Like the anchor, a marker on the primary's own entry is disregarded rather than honored. + case k.Value == params.MobileKey: primaryMobileInArray = true b.WithPrimaryMobileKey(credential.MobileKeyParams{Value: k.Value, Key: util.PtrOrNil(k.Key)}) - } else { + case viewScopedMobileValues[k.Value]: + rejected = append(rejected, k.Key) + default: b.WithMobileKey(credential.MobileKeyParams{Value: k.Value, Key: util.PtrOrNil(k.Key), Expiry: util.PtrOrNil(k.Expiry)}) } } @@ -79,7 +121,7 @@ func BuildAcceptedSet(params EnvironmentParams) (credential.AcceptedSet, error) // without this guard the primary would be silently left undesignated, clearing it on reconcile and // breaking event forwarding. (An undefined mobKey is valid — a server-side-only environment.) if params.MobileKey.Defined() && !primaryMobileInArray { - return credential.AcceptedSet{}, credential.NewPrimaryMobileKeyNotInSetError() + return credential.AcceptedSet{}, nil, credential.NewPrimaryMobileKeyNotInSetError() } // A non-empty mobileKeys[] with no designated primary (undefined mobKey) is malformed: the reconcile @@ -89,12 +131,12 @@ func BuildAcceptedSet(params EnvironmentParams) (credential.AcceptedSet, error) // server-side-only environment. Old-format payloads synthesize the array from mobKey only, so an // undefined mobKey yields an empty array and is unaffected.) if len(params.AcceptedMobileKeys) > 0 && !params.MobileKey.Defined() { - return credential.AcceptedSet{}, credential.NewPrimaryMobileKeyMissingError() + return credential.AcceptedSet{}, nil, credential.NewPrimaryMobileKeyMissingError() } set, err := b.Build() if err != nil { - return credential.AcceptedSet{}, err + return credential.AcceptedSet{}, nil, err } - return set, nil + return set, rejected, nil } diff --git a/internal/envfactory/reconcile_helper_test.go b/internal/envfactory/reconcile_helper_test.go index df87d178..240cbaa9 100644 --- a/internal/envfactory/reconcile_helper_test.go +++ b/internal/envfactory/reconcile_helper_test.go @@ -50,7 +50,7 @@ func TestBuildAcceptedSet_HappyPath(t *testing.T) { []AcceptedSDKKey{{Key: "default", Value: "sdk-anchor"}}, "mob-primary", ) - set, err := BuildAcceptedSet(params) + set, _, err := BuildAcceptedSet(params) require.NoError(t, err) expected := mustBuild(t, credential.NewAcceptedSetBuilder(). @@ -72,7 +72,7 @@ func TestBuildAcceptedSet_MultipleKeys(t *testing.T) { }, "mob-primary", ) - set, err := BuildAcceptedSet(params) + set, _, err := BuildAcceptedSet(params) require.NoError(t, err) expected := mustBuild(t, credential.NewAcceptedSetBuilder(). @@ -99,8 +99,8 @@ func TestBuildAcceptedSet_Rename(t *testing.T) { "mob-primary", ) - setOld, errOld := BuildAcceptedSet(paramsOldName) - setNew, errNew := BuildAcceptedSet(paramsNewName) + setOld, _, errOld := BuildAcceptedSet(paramsOldName) + setNew, _, errNew := BuildAcceptedSet(paramsNewName) require.NoError(t, errOld) require.NoError(t, errNew) @@ -143,8 +143,8 @@ func TestBuildAcceptedSet_Deexpiry(t *testing.T) { "mob-primary", ) - setWithExpiry, errWithExpiry := BuildAcceptedSet(paramsWithExpiry) - setNoExpiry, errNoExpiry := BuildAcceptedSet(paramsNoExpiry) + setWithExpiry, _, errWithExpiry := BuildAcceptedSet(paramsWithExpiry) + setNoExpiry, _, errNoExpiry := BuildAcceptedSet(paramsNoExpiry) require.NoError(t, errWithExpiry) require.NoError(t, errNoExpiry) @@ -173,7 +173,7 @@ func TestBuildAcceptedSet_AnchorNotInArray(t *testing.T) { }, "mob-primary", ) - _, err := BuildAcceptedSet(params) + _, _, err := BuildAcceptedSet(params) require.Error(t, err) var malformed *credential.MalformedCredentialSetError @@ -194,7 +194,7 @@ func TestBuildAcceptedSet_PrimaryMobileNotInArray(t *testing.T) { {Key: "other", Value: "mob-other"}, // ...but NOT in the array }, } - _, err := BuildAcceptedSet(params) + _, _, err := BuildAcceptedSet(params) require.Error(t, err) var malformed *credential.MalformedCredentialSetError @@ -211,7 +211,7 @@ func TestBuildAcceptedSet_NoMobileKey(t *testing.T) { SDKKey: SDKKeyRep{Value: config.SDKKey("sdk-anchor")}, // no MobKey, no MobileKeys } - set, err := BuildAcceptedSet(rep.ToParams()) + set, _, err := BuildAcceptedSet(rep.ToParams()) require.NoError(t, err) expected := mustBuild(t, credential.NewAcceptedSetBuilder(). @@ -235,7 +235,7 @@ func TestBuildAcceptedSet_MobileKeysWithoutPrimary(t *testing.T) { {Key: "mob-1", Value: "mob-primary"}, // ...but the array is non-empty }, } - _, err := BuildAcceptedSet(params) + _, _, err := BuildAcceptedSet(params) require.Error(t, err) var malformed *credential.MalformedCredentialSetError @@ -254,7 +254,7 @@ func TestBuildAcceptedSet_EmptyMobileArrayValid(t *testing.T) { AcceptedSDKKeys: []AcceptedSDKKey{{Key: "default", Value: "sdk-anchor"}}, AcceptedMobileKeys: []AcceptedMobileKey{}, // empty } - set, err := BuildAcceptedSet(params) + set, _, err := BuildAcceptedSet(params) require.NoError(t, err) expected := mustBuild(t, credential.NewAcceptedSetBuilder(). @@ -273,7 +273,7 @@ func TestBuildAcceptedSet_AnchorUndefined(t *testing.T) { }, "mob-primary", ) - _, err := BuildAcceptedSet(params) + _, _, err := BuildAcceptedSet(params) require.Error(t, err) var malformed *credential.MalformedCredentialSetError @@ -281,16 +281,32 @@ func TestBuildAcceptedSet_AnchorUndefined(t *testing.T) { assert.Contains(t, malformed.Error(), "anchor SDK key is missing") } -// TestBuildAcceptedSet_NoSDKKeys verifies that when neither an anchor nor any array SDK keys are -// present, Build returns an error (the set has no SDK key at all). +// TestBuildAcceptedSet_NoSDKKeys verifies that when no SDK key survives, the payload is rejected as +// malformed. Two shapes reach this, both requiring an undefined anchor: an empty array, and an array +// whose every entry is filtered out for being scoped to a view. The second is why the case is worth +// pinning by error type — a filtered-to-empty array is a payload problem, not a caller mistake. func TestBuildAcceptedSet_NoSDKKeys(t *testing.T) { - params := EnvironmentParams{ - SDKKey: "", // undefined anchor - AcceptedSDKKeys: []AcceptedSDKKey{}, - AcceptedMobileKeys: []AcceptedMobileKey{}, + tests := []struct { + name string + sdkKeys []AcceptedSDKKey + }{ + {"empty array", []AcceptedSDKKey{}}, + {"every entry view-scoped", []AcceptedSDKKey{{Key: "view-sdk", Value: "sdk-viewy", HasViews: true}}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + params := EnvironmentParams{ + SDKKey: "", // undefined anchor + AcceptedSDKKeys: tt.sdkKeys, + AcceptedMobileKeys: []AcceptedMobileKey{}, + } + _, _, err := BuildAcceptedSet(params) + + require.Error(t, err, "a set with no SDK key at all must be rejected") + var malformed *credential.MalformedCredentialSetError + require.ErrorAs(t, err, &malformed, "every rejection from BuildAcceptedSet is a malformed-payload error") + }) } - _, err := BuildAcceptedSet(params) - require.Error(t, err, "a set with no SDK key at all must be rejected") } // TestBuildAcceptedSet_MixedUpdate verifies add + re-anchor + remove in a single params update @@ -312,7 +328,7 @@ func TestBuildAcceptedSet_MixedUpdate(t *testing.T) { }, "mob-primary", ) - set, err := BuildAcceptedSet(params) + set, _, err := BuildAcceptedSet(params) require.NoError(t, err) expected := mustBuild(t, credential.NewAcceptedSetBuilder(). @@ -336,7 +352,7 @@ func TestBuildAcceptedSet_AnchorNeverExpiring(t *testing.T) { }, "mob-primary", ) - set, err := BuildAcceptedSet(params) + set, _, err := BuildAcceptedSet(params) require.NoError(t, err) // Anchor is permanent (WithAnchor), not expiring — identical to a payload with no anchor expiry. @@ -362,7 +378,7 @@ func TestBuildAcceptedSet_MultipleMobileKeys(t *testing.T) { {Key: "mob-2", Value: "mob-secondary"}, }, } - set, err := BuildAcceptedSet(params) + set, _, err := BuildAcceptedSet(params) require.NoError(t, err) expected := mustBuild(t, credential.NewAcceptedSetBuilder(). @@ -388,7 +404,7 @@ func TestBuildAcceptedSet_ExpiringMobileKey(t *testing.T) { {Key: "mob-old", Value: "mob-old", Expiry: expiry1}, // expiring }, } - set, err := BuildAcceptedSet(params) + set, _, err := BuildAcceptedSet(params) require.NoError(t, err) expected := mustBuild(t, credential.NewAcceptedSetBuilder(). @@ -398,3 +414,160 @@ func TestBuildAcceptedSet_ExpiringMobileKey(t *testing.T) { WithPrimaryMobileKey(credential.MobileKeyParams{Value: "mob-primary", Key: util.PtrOrNil("mob-1")})) assert.Equal(t, expected, set, "expiring mobile key must land as an expiring key in the set") } + +// TestBuildAcceptedSet_ViewScopedKeys covers the ingestion filter across both arrays. A key scoped to a +// view may only see a subset of the environment's flags; relay serves the whole environment payload, so +// admitting one would silently over-deliver. Such a key is therefore never added to the set, and an SDK +// presenting it is rejected because the credential is simply absent from the lookup map. The dropped +// keys are returned to the caller so it can WARN. +func TestBuildAcceptedSet_ViewScopedKeys(t *testing.T) { + const ( + anchor = config.SDKKey("sdk-anchor") + primary = config.MobileKey("mob-primary") + ) + + // The four entries every case starts from; individual cases add view-scoped entries alongside them. + anchorEntry := AcceptedSDKKey{Key: "default-sdk", Value: anchor} + extraSDK := AcceptedSDKKey{Key: "service-a", Value: "sdk-service-a"} + primaryEntry := AcceptedMobileKey{Key: "default-mob", Value: primary} + extraMob := AcceptedMobileKey{Key: "mob-extra", Value: "mob-extra"} + + // base is the set with only the two designated keys; cases add whatever survived the filter. + base := func() *credential.AcceptedSetBuilder { + return credential.NewAcceptedSetBuilder(). + WithEnvironmentID("env-abc"). + WithAnchor(credential.SDKKeyParams{Value: anchor, Key: util.PtrOrNil("default-sdk")}). + WithPrimaryMobileKey(credential.MobileKeyParams{Value: primary, Key: util.PtrOrNil("default-mob")}) + } + acceptedExtraSDK := credential.SDKKeyParams{Value: extraSDK.Value, Key: util.PtrOrNil(extraSDK.Key)} + acceptedExtraMob := credential.MobileKeyParams{Value: extraMob.Value, Key: util.PtrOrNil(extraMob.Key)} + + // baseWithExtras is base plus both non-designated keys — the expectation for every case where the + // filter drops nothing that was going to be accepted anyway. + baseWithExtras := func() *credential.AcceptedSetBuilder { + return base().WithSDKKey(acceptedExtraSDK).WithMobileKey(acceptedExtraMob) + } + + tests := []struct { + name string + sdkKeys []AcceptedSDKKey + mobileKeys []AcceptedMobileKey + wantSet func() *credential.AcceptedSetBuilder + wantRejected []string + }{ + { + // Baseline: nothing view-scoped behaves exactly as it did before the field existed. + name: "no view-scoped keys", + sdkKeys: []AcceptedSDKKey{anchorEntry, extraSDK}, + mobileKeys: []AcceptedMobileKey{primaryEntry, extraMob}, + wantSet: baseWithExtras, + }, + { + name: "view-scoped non-anchor SDK key is excluded", + sdkKeys: []AcceptedSDKKey{anchorEntry, extraSDK, {Key: "view-sdk", Value: "sdk-viewy", HasViews: true}}, + mobileKeys: []AcceptedMobileKey{primaryEntry, extraMob}, + wantSet: baseWithExtras, + wantRejected: []string{"view-sdk"}, + }, + { + name: "view-scoped non-primary mobile key is excluded", + sdkKeys: []AcceptedSDKKey{anchorEntry, extraSDK}, + mobileKeys: []AcceptedMobileKey{primaryEntry, extraMob, {Key: "view-mob", Value: "mob-viewy", HasViews: true}}, + wantSet: baseWithExtras, + wantRejected: []string{"view-mob"}, + }, + { + // Both arrays filter in one pass; SDK keys are walked first, hence the order. + name: "view-scoped keys in both arrays are excluded", + sdkKeys: []AcceptedSDKKey{anchorEntry, {Key: "view-sdk", Value: "sdk-viewy", HasViews: true}}, + mobileKeys: []AcceptedMobileKey{primaryEntry, {Key: "view-mob", Value: "mob-viewy", HasViews: true}}, + wantSet: base, + // Every non-designated key is view-scoped, so only the anchor and primary survive. + wantRejected: []string{"view-sdk", "view-mob"}, + }, + { + // A view-scoped key is dropped outright rather than being admitted as an expiring key — + // the marker takes precedence over expiry handling. + name: "view-scoped key carrying an expiry is still excluded", + sdkKeys: []AcceptedSDKKey{anchorEntry, {Key: "view-sdk", Value: "sdk-viewy", Expiry: expiry1, HasViews: true}}, + mobileKeys: []AcceptedMobileKey{primaryEntry}, + wantSet: base, + wantRejected: []string{"view-sdk"}, + }, + { + // Two entries, one value, only the later one marked. The accepted-set builder is + // first-wins, so filtering per-entry would admit the credential via the unmarked entry + // while still reporting it rejected. One marked entry must reject the value outright. + name: "value marked by any duplicate entry is excluded", + sdkKeys: []AcceptedSDKKey{ + anchorEntry, + {Key: "clean-alias", Value: "sdk-dup"}, + {Key: "view-alias", Value: "sdk-dup", HasViews: true}, + }, + mobileKeys: []AcceptedMobileKey{primaryEntry}, + wantSet: base, + wantRejected: []string{"clean-alias", "view-alias"}, + }, + { + // The same, with the marked entry first — the outcome must not depend on array order. + name: "value marked by any duplicate entry is excluded regardless of order", + sdkKeys: []AcceptedSDKKey{ + anchorEntry, + {Key: "view-alias", Value: "sdk-dup", HasViews: true}, + {Key: "clean-alias", Value: "sdk-dup"}, + }, + mobileKeys: []AcceptedMobileKey{primaryEntry}, + wantSet: base, + wantRejected: []string{"view-alias", "clean-alias"}, + }, + { + // The mobile analogue, so both loops are pinned against per-entry filtering. + name: "mobile value marked by any duplicate entry is excluded", + sdkKeys: []AcceptedSDKKey{anchorEntry}, + mobileKeys: []AcceptedMobileKey{ + primaryEntry, + {Key: "clean-mob-alias", Value: "mob-dup"}, + {Key: "view-mob-alias", Value: "mob-dup", HasViews: true}, + }, + wantSet: base, + wantRejected: []string{"clean-mob-alias", "view-mob-alias"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + params := EnvironmentParams{ + EnvID: "env-abc", + SDKKey: anchor, + MobileKey: primary, + AcceptedSDKKeys: tt.sdkKeys, + AcceptedMobileKeys: tt.mobileKeys, + } + + set, rejected, err := BuildAcceptedSet(params) + + // A view-scoped key is filtered, never fatal — the environment always keeps operating. + require.NoError(t, err) + assert.Equal(t, mustBuild(t, tt.wantSet()), set) + assert.Equal(t, tt.wantRejected, rejected) + }) + } +} + +// TestBuildAcceptedSet_ViewScopedKeysEmptyOnError verifies that the rejected list is empty whenever an +// error is returned. The caller discards the whole payload and preserves its previous credentials in +// that case, so reporting keys it did not act on would produce a misleading WARN. +func TestBuildAcceptedSet_ViewScopedKeysEmptyOnError(t *testing.T) { + // The anchor is absent from sdkKeys[] — malformed — and a view-scoped entry is present alongside. + params := EnvironmentParams{ + EnvID: "env-abc", + SDKKey: "sdk-anchor", + AcceptedSDKKeys: []AcceptedSDKKey{{Key: "view-sdk", Value: "sdk-viewy", HasViews: true}}, + AcceptedMobileKeys: []AcceptedMobileKey{}, + } + + _, rejected, err := BuildAcceptedSet(params) + + require.Error(t, err) + assert.Empty(t, rejected) +} diff --git a/relay/autoconfig_actions.go b/relay/autoconfig_actions.go index c59c4a1a..2f4b1697 100644 --- a/relay/autoconfig_actions.go +++ b/relay/autoconfig_actions.go @@ -1,6 +1,10 @@ package relay import ( + "strings" + + "github.com/launchdarkly/go-sdk-common/v3/ldlog" + "github.com/launchdarkly/ld-relay/v8/config" "github.com/launchdarkly/ld-relay/v8/internal/envfactory" "github.com/launchdarkly/ld-relay/v8/internal/sdkauth" @@ -11,8 +15,34 @@ const ( logMsgAutoConfUpdateUnknownEnv = "Got auto-configuration update for environment %q but did not have previous configuration - will add" logMsgAutoConfDeleteUnknownEnv = "Got auto-configuration delete message for environment %s but did not have previous configuration - ignoring" logMsgAutoConfReceivedAllEnvironments = "Finished processing auto-configuration data" + + logMsgViewScopedKeysRejected = "Environment %q: rejecting credentials scoped to a view: %s." + + " The Relay Proxy serves the entire environment payload and cannot filter it to a view," + + " so SDKs presenting these credentials will be denied." ) +// logViewScopedKeys reports the view-scoped credentials that BuildAcceptedSet filtered out of a +// payload. This logs once per payload that actually reaches a handler. +func logViewScopedKeys(loggers ldlog.Loggers, envName string, rejected []string) { + if len(rejected) > 0 { + loggers.Warnf(logMsgViewScopedKeysRejected, envName, joinKeyIdentifiers(rejected)) + } +} + +// joinKeyIdentifiers renders wire key identifiers for a log message. The backend requires a non-empty +// identifier on every array entry, but the offline archive is an operator-supplied file, so substitute +// a placeholder rather than emitting a message that names nothing. +func joinKeyIdentifiers(identifiers []string) string { + named := make([]string, 0, len(identifiers)) + for _, id := range identifiers { + if id == "" { + id = "" + } + named = append(named, id) + } + return strings.Join(named, ", ") +} + // relayAutoConfigActions is an implementation of the autoconfig.MessageHandler interface. The low-level // autoconfig.StreamManager component, which manages the configuration stream protocol, will call the // interface methods on this object to let us know when environments have been added or changed. @@ -32,11 +62,12 @@ func (a *relayAutoConfigActions) AddEnvironment(params envfactory.EnvironmentPar return } - set, buildErr := envfactory.BuildAcceptedSet(params) + set, rejected, buildErr := envfactory.BuildAcceptedSet(params) if buildErr != nil { a.r.loggers.Errorf(logMsgAutoConfEnvInitError, params.Identifiers.GetDisplayName(), buildErr) return } + logViewScopedKeys(a.r.loggers, params.Identifiers.GetDisplayName(), rejected) env.ReconcileCredentials(set) } @@ -51,7 +82,7 @@ func (a *relayAutoConfigActions) UpdateEnvironment(params envfactory.Environment env.SetTTL(params.TTL) env.SetSecureMode(params.SecureMode) - set, buildErr := envfactory.BuildAcceptedSet(params) + set, rejected, buildErr := envfactory.BuildAcceptedSet(params) if buildErr != nil { // Credential payloads are validated at the stream parse boundary (see StreamManager) before // being dispatched here, so a malformed set should not reach this point. Log defensively and @@ -59,6 +90,7 @@ func (a *relayAutoConfigActions) UpdateEnvironment(params envfactory.Environment a.r.loggers.Errorf(logMsgAutoConfEnvInitError, params.Identifiers.GetDisplayName(), buildErr) return } + logViewScopedKeys(a.r.loggers, params.Identifiers.GetDisplayName(), rejected) env.ReconcileCredentials(set) } diff --git a/relay/filedata_actions.go b/relay/filedata_actions.go index 0ce77763..27008d73 100644 --- a/relay/filedata_actions.go +++ b/relay/filedata_actions.go @@ -1,11 +1,8 @@ package relay import ( - "errors" "time" - "github.com/launchdarkly/ld-relay/v8/internal/credential" - "github.com/launchdarkly/ld-relay/v8/internal/sdkauth" "github.com/launchdarkly/ld-relay/v8/internal/envfactory" @@ -23,6 +20,8 @@ const ( logMsgOfflineEnvTimeoutError = "Unable to initialize offline environment %q: timed out waiting for client creation" logMsgInternalErrorUpdatedEnvNotFound = "Unexpected error in file data processing: environment ID %s not found when updating" logMsgInternalErrorNoUpdatesForEnv = "Unexpected error in file data processing: environment ID %s not found in envUpdates" + + logMsgOfflineMalformedPayload = "Malformed credential payload for offline environment %q — preserving previous credentials: %s" ) // relayFileDataActions is an implementation of the filedata.UpdateHandler interface. The low-level @@ -58,17 +57,13 @@ func (a *relayFileDataActions) AddEnvironment(ae filedata.ArchiveEnvironment) { return } - set, buildErr := envfactory.BuildAcceptedSet(ae.Params) + set, rejected, buildErr := envfactory.BuildAcceptedSet(ae.Params) if buildErr != nil { - var malformed *credential.MalformedCredentialSetError - if errors.As(buildErr, &malformed) { - a.r.loggers.Errorf("Malformed credential payload for offline environment %q — preserving previous credentials: %s", ae.Params.Identifiers.GetDisplayName(), buildErr) - } else { - a.r.loggers.Errorf(logMsgAutoConfEnvInitError, ae.Params.Identifiers.GetDisplayName(), buildErr) - } + a.r.loggers.Errorf(logMsgOfflineMalformedPayload, ae.Params.Identifiers.GetDisplayName(), buildErr) // No reconnect for offline mode: preserve previous state (env was just created with // the singular sdkKey from envConfig) and wait for the next archive reload. } else { + logViewScopedKeys(a.r.loggers, ae.Params.Identifiers.GetDisplayName(), rejected) env.ReconcileCredentials(set) } @@ -101,17 +96,12 @@ func (a *relayFileDataActions) UpdateEnvironment(ae filedata.ArchiveEnvironment) env.SetTTL(ae.Params.TTL) env.SetSecureMode(ae.Params.SecureMode) - set, buildErr := envfactory.BuildAcceptedSet(ae.Params) + set, rejected, buildErr := envfactory.BuildAcceptedSet(ae.Params) if buildErr != nil { - var malformed *credential.MalformedCredentialSetError - if errors.As(buildErr, &malformed) { - a.r.loggers.Errorf("Malformed credential payload for offline environment %q — preserving previous credentials: %s", ae.Params.Identifiers.GetDisplayName(), buildErr) - } else { - // The environment was found above; this is a credential-build failure, not a missing env. - a.r.loggers.Errorf(logMsgAutoConfEnvInitError, ae.Params.Identifiers.GetDisplayName(), buildErr) - } + a.r.loggers.Errorf(logMsgOfflineMalformedPayload, ae.Params.Identifiers.GetDisplayName(), buildErr) // Preserve previous credentials; no reconnect (offline path has no live stream). } else { + logViewScopedKeys(a.r.loggers, ae.Params.Identifiers.GetDisplayName(), rejected) env.ReconcileCredentials(set) }