mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-11-04 02:57:44 +01:00
iam/oidc: drop single-use web-identity tokens and the jti replay cache (#12065)
* iam/oidc: drop single-use web-identity tokens and the jti replay cache Reverts the single-use semantics added in #11936 (and the #12017 refactor): AssumeRoleWithWebIdentity consumed each web-identity token's jti into a bounded replay cache, so a second exchange of the same token was rejected until exp + skew. AWS STS accepts a web-identity token repeatedly until it expires, and Kubernetes workload identity depends on that: the kubelet keeps a projected ServiceAccount token until ~80% of its lifetime, so pod restarts, co-located containers sharing the token file, repeated CLI invocations, and pre-rotation SDK refreshes all re-present the same jti and were refused on 4.50. The replay machinery also carried a real maintenance cost — a bounded concurrent cache, fingerprint-keyed entries for jti-less tokens, adopt/merge handoffs so provider rebuilds could not reopen consumed tokens, and provider-reuse bookkeeping in the refresh path that existed only to preserve that state. Removing it restores AWS semantics and deletes the whole apparatus. Refs #12063. Alternative to the per-provider opt-out in #12064. * iam: reuse unchanged OIDC providers on refresh refreshOIDCProvidersFrom rebuilt every managed provider on any store change, so an unrelated write discarded each provider's JWKS cache and an unreachable issuer could then fail exchanges the cached keys would have allowed. Keep a per-record provider cache keyed on the record content fingerprint and only rebuild when the record changes. * iam/oidc: require exp in ValidateToken Dropping the single-use check also removed the implicit bound on expiry-less tokens: a signed web-identity token without exp could mint STS sessions indefinitely. Require the exp claim, matching AWS AssumeRoleWithWebIdentity which rejects tokens without one. * iam/oidc: bound idle connections on the provider transport A provider replaced on refresh drops its http.Transport without a close; give it an IdleConnTimeout so pooled connections and their goroutines cannot linger. * s3api: cover web-identity token reuse over HTTP Sign one token and exchange that exact string twice, directly covering the Kubernetes projected-token reuse this change restores. * test(iam): refresh reuses an unchanged record's provider Covers the provider-reuse cache: an unrelated record's refresh keeps a provider's JWKS state, a changed record rebuilds it.
This commit is contained in:
1 parent
515380c349
commit
6dd6ad512c
13 files changed
+110
-1944
No files matched your search
@@ -53,12 +53,9 @@ type IAMManager struct {
|
||||
oidcRetryAgain bool
|
||||
// oidcRefreshMu serializes refreshes from reading the store to handing
|
||||
// STS the result, so an older snapshot cannot replace a newer one.
|
||||
oidcRefreshMu sync.Mutex
|
||||
// oidcManagedProviders caches providers refreshOIDCProvidersFrom built,
|
||||
// keyed by record identity, so an unchanged record reuses its provider:
|
||||
// rebuilding would drop the provider's consumed-token (jti) replay cache
|
||||
// and let an already-used token be presented again. Guarded by
|
||||
// oidcRefreshMu.
|
||||
// oidcManagedProviders caches the provider built per stored record so a
|
||||
// refresh reuses unchanged providers and keeps their JWKS cache.
|
||||
oidcRefreshMu sync.Mutex
|
||||
oidcManagedProviders map[string]*oidcManagedProvider
|
||||
oidcAuditSink OIDCProviderAuditSink
|
||||
revocationStore SessionRevocationStore
|
||||
@@ -953,10 +950,6 @@ func (m *IAMManager) refreshOIDCProvidersFrom(ctx context.Context, store OIDCPro
|
||||
m.oidcManagedProviders = make(map[string]*oidcManagedProvider)
|
||||
}
|
||||
seen := make(map[string]struct{}, len(records))
|
||||
// Records for one issuer with identical validation settings share a single
|
||||
// provider — and with it the consumed-token (jti) replay store — so a token
|
||||
// spent through one account's record stays spent through the other's too.
|
||||
sharedProviders := make(map[string]*oidc.OIDCProvider)
|
||||
for _, rec := range records {
|
||||
if rec == nil || rec.URL == "" {
|
||||
continue
|
||||
@@ -967,25 +960,10 @@ func (m *IAMManager) refreshOIDCProvidersFrom(ctx context.Context, store OIDCPro
|
||||
}
|
||||
seen[key] = struct{}{}
|
||||
fingerprint := oidcProviderFingerprint(rec)
|
||||
configFingerprint := oidcProviderConfigFingerprint(rec)
|
||||
var provider *oidc.OIDCProvider
|
||||
cached := m.oidcManagedProviders[key]
|
||||
if shared, ok := sharedProviders[configFingerprint]; ok {
|
||||
provider = shared
|
||||
if cached != nil && cached.provider != nil && cached.provider != shared {
|
||||
// The record's own provider is superseded; its consumed
|
||||
// tokens move to the shared store so none reopen for replay.
|
||||
shared.MergeReplayStateFrom(cached.provider)
|
||||
}
|
||||
m.oidcManagedProviders[key] = &oidcManagedProvider{fingerprint: fingerprint, provider: provider}
|
||||
if cached != nil && cached.provider != nil && cached.provider != shared && !m.oidcProviderStillReferenced(cached.provider) {
|
||||
if err := cached.provider.Shutdown(ctx); err != nil {
|
||||
glog.Warningf("shutdown superseded OIDC provider %s: %v", rec.ARN, err)
|
||||
}
|
||||
}
|
||||
} else if cached != nil && cached.fingerprint == fingerprint {
|
||||
var provider *oidc.OIDCProvider
|
||||
if cached != nil && cached.fingerprint == fingerprint {
|
||||
provider = cached.provider
|
||||
sharedProviders[configFingerprint] = provider
|
||||
} else {
|
||||
var err error
|
||||
provider, err = buildOIDCProviderFromRecord(rec)
|
||||
@@ -994,20 +972,7 @@ func (m *IAMManager) refreshOIDCProvidersFrom(ctx context.Context, store OIDCPro
|
||||
glog.Warningf("skip refreshing OIDC provider %s: %v", rec.ARN, err)
|
||||
continue
|
||||
}
|
||||
if cached != nil && cached.provider != nil {
|
||||
// The rebuild must keep rejecting tokens the replaced
|
||||
// provider consumed; carry its replay store over, then stop
|
||||
// its cleanup worker so only the new one sweeps the store —
|
||||
// unless another record still references that provider.
|
||||
provider.AdoptReplayStateFrom(cached.provider)
|
||||
}
|
||||
sharedProviders[configFingerprint] = provider
|
||||
m.oidcManagedProviders[key] = &oidcManagedProvider{fingerprint: fingerprint, provider: provider}
|
||||
if cached != nil && cached.provider != nil && !m.oidcProviderStillReferenced(cached.provider) {
|
||||
if err := cached.provider.Shutdown(ctx); err != nil {
|
||||
glog.Warningf("shutdown replaced OIDC provider %s: %v", rec.ARN, err)
|
||||
}
|
||||
}
|
||||
}
|
||||
// Multiple records may share an issuer when each is scoped to a
|
||||
// different account; STS picks the right one at validation time
|
||||
@@ -1017,16 +982,9 @@ func (m *IAMManager) refreshOIDCProvidersFrom(ctx context.Context, store OIDCPro
|
||||
Provider: provider,
|
||||
})
|
||||
}
|
||||
for key, managed := range m.oidcManagedProviders {
|
||||
for key := range m.oidcManagedProviders {
|
||||
if _, ok := seen[key]; !ok {
|
||||
delete(m.oidcManagedProviders, key)
|
||||
// A removed record's provider may be shared by records that are
|
||||
// still present; shut it down only when nothing references it.
|
||||
if managed.provider != nil && !m.oidcProviderStillReferenced(managed.provider) {
|
||||
if err := managed.provider.Shutdown(ctx); err != nil {
|
||||
glog.Warningf("shutdown removed OIDC provider %s: %v", key, err)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
m.stsService.SetIAMManagedOIDCProviders(byIssuer)
|
||||
@@ -1040,17 +998,6 @@ type oidcManagedProvider struct {
|
||||
provider *oidc.OIDCProvider
|
||||
}
|
||||
|
||||
// oidcProviderStillReferenced reports whether any managed record still points
|
||||
// at the provider, so a shared provider is not shut down with a removed record.
|
||||
func (m *IAMManager) oidcProviderStillReferenced(provider *oidc.OIDCProvider) bool {
|
||||
for _, managed := range m.oidcManagedProviders {
|
||||
if managed.provider == provider {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
// oidcProviderFingerprint identifies the record content a provider was built
|
||||
// from, so a refresh rebuilds the provider only when that content changed.
|
||||
func oidcProviderFingerprint(rec *OIDCProviderRecord) string {
|
||||
@@ -1067,21 +1014,6 @@ func oidcProviderFingerprint(rec *OIDCProviderRecord) string {
|
||||
return hex.EncodeToString(sum[:])
|
||||
}
|
||||
|
||||
// oidcProviderConfigFingerprint identifies the validation settings a provider
|
||||
// is built from — everything buildOIDCProviderFromRecord consumes except the
|
||||
// record's name — so records that differ only in identity reuse one provider.
|
||||
func oidcProviderConfigFingerprint(rec *OIDCProviderRecord) string {
|
||||
key, _ := json.Marshal(&oidc.OIDCConfig{
|
||||
Issuer: rec.URL,
|
||||
ClientIDs: rec.ClientIDs,
|
||||
Thumbprints: rec.Thumbprints,
|
||||
AllowedPrincipalTagKeys: rec.AllowedPrincipalTagKeys,
|
||||
PolicyClaim: rec.PolicyClaim,
|
||||
})
|
||||
sum := sha256.Sum256(key)
|
||||
return hex.EncodeToString(sum[:])
|
||||
}
|
||||
|
||||
// buildOIDCProviderFromRecord turns a stored record into a runtime
|
||||
// OIDCProvider. The provider name is the ARN so re-registration is
|
||||
// idempotent and never collides with static-config entries.
|
||||
|
||||
@@ -560,92 +560,47 @@ func TestAFailureDuringARetryIsNotDropped(t *testing.T) {
|
||||
assert.GreaterOrEqual(t, store.successes(), 2, "the retry ended on its first success, dropping the refresh that failed while it ran")
|
||||
}
|
||||
|
||||
// A refresh must not rebuild a provider whose record is unchanged: rebuilding
|
||||
// would drop the provider's consumed-token (jti) replay cache and let an
|
||||
// already-used token be presented again.
|
||||
func TestRefreshReusesProviderWhileRecordIsUnchanged(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
store := NewMemoryOIDCProviderStore()
|
||||
// A refresh caused by an unrelated record must not rebuild a provider whose
|
||||
// own record is unchanged: rebuilding drops its JWKS cache, so a briefly
|
||||
// unreachable issuer would fail exchanges its cached keys still allow.
|
||||
func TestAnUnrelatedRefreshKeepsAProvidersRuntimeState(t *testing.T) {
|
||||
store := &persistentTestStore{NewMemoryOIDCProviderStore()}
|
||||
mgr := startServer(t, store)
|
||||
|
||||
arn := arnOf(t, persistTestAPIIssuer)
|
||||
require.NoError(t, store.StoreProvider(ctx, "", &OIDCProviderRecord{
|
||||
ARN: arn, URL: persistTestAPIIssuer, ClientIDs: []string{"aud"},
|
||||
}))
|
||||
require.NoError(t, mgr.refreshOIDCProvidersFrom(ctx, store))
|
||||
first := mgr.oidcManagedProviders[arn].provider
|
||||
require.NotNil(t, first)
|
||||
createAPIProvider(t, mgr, persistTestAPIIssuer)
|
||||
|
||||
require.NoError(t, mgr.refreshOIDCProvidersFrom(ctx, store))
|
||||
assert.Same(t, first, mgr.oidcManagedProviders[arn].provider,
|
||||
"an unchanged record was rebuilt, dropping its replay cache")
|
||||
mgr.oidcRefreshMu.Lock()
|
||||
before := mgr.oidcManagedProviders[arn]
|
||||
mgr.oidcRefreshMu.Unlock()
|
||||
require.NotNil(t, before)
|
||||
|
||||
// A changed record must rebuild so the new configuration applies.
|
||||
require.NoError(t, store.StoreProvider(ctx, "", &OIDCProviderRecord{
|
||||
ARN: arn, URL: persistTestAPIIssuer, ClientIDs: []string{"aud", "aud2"},
|
||||
}))
|
||||
require.NoError(t, mgr.refreshOIDCProvidersFrom(ctx, store))
|
||||
rebuilt := mgr.oidcManagedProviders[arn].provider
|
||||
require.NotNil(t, rebuilt)
|
||||
assert.NotSame(t, first, rebuilt, "a changed record kept the old provider")
|
||||
createAPIProvider(t, mgr, persistTestStaticIssuer) // refreshes over an unrelated record
|
||||
|
||||
// Deleting the record drops the provider from the cache and the STS map.
|
||||
require.NoError(t, store.DeleteProvider(ctx, "", arn))
|
||||
require.NoError(t, mgr.refreshOIDCProvidersFrom(ctx, store))
|
||||
assert.NotContains(t, mgr.oidcManagedProviders, arn)
|
||||
assert.False(t, stsKnowsIssuer(t, mgr, persistTestAPIIssuer))
|
||||
mgr.oidcRefreshMu.Lock()
|
||||
after := mgr.oidcManagedProviders[arn]
|
||||
mgr.oidcRefreshMu.Unlock()
|
||||
require.NotNil(t, after)
|
||||
assert.Same(t, before.provider, after.provider, "an unchanged record's provider was rebuilt")
|
||||
}
|
||||
|
||||
// stsKnowsIssuerForAccount is stsKnowsIssuer through the account-scoped
|
||||
// lookup used for providers that have no global record.
|
||||
func stsKnowsIssuerForAccount(t *testing.T, mgr *IAMManager, issuer, accountID string) bool {
|
||||
t.Helper()
|
||||
tok, err := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{
|
||||
"iss": issuer, "sub": "probe", "aud": "aud", "exp": time.Now().Add(time.Hour).Unix(),
|
||||
}).SignedString([]byte("not-the-providers-key"))
|
||||
require.NoError(t, err)
|
||||
_, _, err = mgr.GetSTSService().ValidateWebIdentityTokenForAccount(context.Background(), tok, accountID)
|
||||
require.Error(t, err, "an unsigned-by-provider token for %s was accepted", issuer)
|
||||
return !strings.Contains(err.Error(), "no identity provider registered")
|
||||
}
|
||||
|
||||
// Two records for one issuer with identical validation settings share a
|
||||
// single provider — and with it the consumed-token (jti) replay store — so a
|
||||
// token spent through one account's record stays spent through the other's.
|
||||
// Removing one record of the pair must not shut the shared provider down.
|
||||
func TestSameIssuerRecordsShareOneProvider(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
store := NewMemoryOIDCProviderStore()
|
||||
// A record whose validation settings change must get a new provider: the
|
||||
// cached runtime cannot keep serving settings the store no longer holds.
|
||||
func TestAChangedRecordRebuildsItsProvider(t *testing.T) {
|
||||
store := &persistentTestStore{NewMemoryOIDCProviderStore()}
|
||||
mgr := startServer(t, store)
|
||||
arn := arnOf(t, persistTestAPIIssuer)
|
||||
createAPIProvider(t, mgr, persistTestAPIIssuer)
|
||||
|
||||
arnA := arnOf(t, persistTestAPIIssuer) + "-account-a"
|
||||
arnB := arnOf(t, persistTestAPIIssuer) + "-account-b"
|
||||
for _, rec := range []*OIDCProviderRecord{
|
||||
{ARN: arnA, URL: persistTestAPIIssuer, AccountID: "111111111111", ClientIDs: []string{"aud"}},
|
||||
{ARN: arnB, URL: persistTestAPIIssuer, AccountID: "222222222222", ClientIDs: []string{"aud"}},
|
||||
} {
|
||||
require.NoError(t, store.StoreProvider(ctx, "", rec))
|
||||
}
|
||||
require.NoError(t, mgr.refreshOIDCProvidersFrom(ctx, store))
|
||||
mgr.oidcRefreshMu.Lock()
|
||||
before := mgr.oidcManagedProviders[arn].provider
|
||||
mgr.oidcRefreshMu.Unlock()
|
||||
|
||||
shared := mgr.oidcManagedProviders[arnA].provider
|
||||
require.NotNil(t, shared)
|
||||
assert.Same(t, shared, mgr.oidcManagedProviders[arnB].provider,
|
||||
"identical records for one issuer built separate replay stores")
|
||||
require.NoError(t, mgr.UpdateOIDCProviderThumbprints(context.Background(), arn,
|
||||
[]string{"0123456789abcdef0123456789abcdef01234567"}))
|
||||
|
||||
// Removing one record of the pair keeps the shared provider alive.
|
||||
require.NoError(t, store.DeleteProvider(ctx, "", arnA))
|
||||
require.NoError(t, mgr.refreshOIDCProvidersFrom(ctx, store))
|
||||
assert.Same(t, shared, mgr.oidcManagedProviders[arnB].provider,
|
||||
"a removed sibling record shut down the shared provider")
|
||||
assert.True(t, stsKnowsIssuerForAccount(t, mgr, persistTestAPIIssuer, "222222222222"))
|
||||
|
||||
// The remaining record diverging rebuilds its own provider.
|
||||
require.NoError(t, store.StoreProvider(ctx, "", &OIDCProviderRecord{
|
||||
ARN: arnB, URL: persistTestAPIIssuer, AccountID: "222222222222", ClientIDs: []string{"b-aud"},
|
||||
}))
|
||||
require.NoError(t, mgr.refreshOIDCProvidersFrom(ctx, store))
|
||||
assert.NotSame(t, shared, mgr.oidcManagedProviders[arnB].provider,
|
||||
"a changed record kept the shared provider")
|
||||
assert.True(t, stsKnowsIssuerForAccount(t, mgr, persistTestAPIIssuer, "222222222222"))
|
||||
mgr.oidcRefreshMu.Lock()
|
||||
after := mgr.oidcManagedProviders[arn]
|
||||
mgr.oidcRefreshMu.Unlock()
|
||||
require.NotNil(t, after)
|
||||
assert.NotSame(t, before, after.provider, "a changed record kept its provider")
|
||||
}
|
||||
Reference in new issue
Block a user