From 2864bc0fe83f40ac02a78c6aae7d547e82cfff10 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sun, 27 Sep 2026 07:01:51 +0800 Subject: [PATCH] s3: honor configured session bounds on AssumeRole and LDAP identity (#11478) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * sts: export CalculateSessionDuration Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * s3: honor configured session bounds on AssumeRole and LDAP identity prepareSTSCredentials hardcoded a one-hour session when the caller omitted DurationSeconds, so sts.tokenDuration was ignored and sts.maxSessionLength only clamped explicit requests: asking for 3600s against a 20m ceiling was rejected while omitting the parameter was granted a full hour (#11473). The two affected handlers now use the same default-then-cap calculation as AssumeRoleWithWebIdentity. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * iam: keep MaxSessionDuration through role store copies copyRoleDefinition rebuilt RoleDefinition field by field and dropped MaxSessionDuration, so memory-backed role stores silently discarded the per-role session bound on every write and read (devin on #11478). Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * sts: apply per-role MaxSessionDuration to resolved session durations Review follow-up on #11478 (devin): the role bound only ever applied to explicit DurationSeconds values — an omitted duration resolved to the configured default and sailed past a shorter role max on every assume path. - capDurationByRole now resolves min(requested||tokenDuration, roleMax), so AssumeRoleWithWebIdentity and AssumeRoleWithCredentials cap defaults the same way they cap explicit values. - prepareSTSCredentials caps the calculated duration at the named role's MaxSessionDuration, covering the AssumeRole and LDAP handlers; self-assumption has no role definition to consult. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * iam: keep MaxSessionDuration through the cached role store genericCopyRoleDefinition drops MaxSessionDuration the same way copyRoleDefinition did, so the cached filer role store reads back a zero maximum and every downstream duration cap is skipped (greptile on #11478). Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * sts: only materialize defaults that pass session duration validation Review follow-up on #11478 (greptile): materializing an omitted DurationSeconds into an explicit value could exceed the service's own input bound (a configured tokenDuration above maxSessionLength) and turn a previously working request into a validation error. capDurationByRole now leaves nil anything the service can resolve better itself, clamps a tightened default at maxSessionLengthSeconds, and floors a role bound below 900s to the tightest issuable value. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .../integration/cached_role_store_generic.go | 7 +- weed/iam/integration/iam_manager.go | 64 ++++++++++++++----- weed/iam/integration/role_max_session_test.go | 28 ++++---- weed/iam/integration/role_store.go | 7 +- weed/iam/sts/sts_service.go | 8 +-- weed/iam/sts/sts_service_test.go | 4 +- weed/s3api/s3api_sts.go | 24 ++++++- weed/s3api/s3api_sts_assume_role_test.go | 61 ++++++++++++++++++ 8 files changed, 161 insertions(+), 42 deletions(-) diff --git a/weed/iam/integration/cached_role_store_generic.go b/weed/iam/integration/cached_role_store_generic.go index 510fc147f..4165958d4 100644 --- a/weed/iam/integration/cached_role_store_generic.go +++ b/weed/iam/integration/cached_role_store_generic.go @@ -123,9 +123,10 @@ func genericCopyRoleDefinition(role *RoleDefinition) *RoleDefinition { } result := &RoleDefinition{ - RoleName: role.RoleName, - RoleArn: role.RoleArn, - Description: role.Description, + RoleName: role.RoleName, + RoleArn: role.RoleArn, + Description: role.Description, + MaxSessionDuration: role.MaxSessionDuration, } // Deep copy trust policy if it exists diff --git a/weed/iam/integration/iam_manager.go b/weed/iam/integration/iam_manager.go index ef4aae516..bff5a9c1c 100644 --- a/weed/iam/integration/iam_manager.go +++ b/weed/iam/integration/iam_manager.go @@ -942,7 +942,7 @@ func (m *IAMManager) AssumeRoleWithWebIdentity(ctx context.Context, request *sts // Apply role-level MaxSessionDuration cap. The STS service still applies // the global MaxSessionLength and the source-token-expiry cap on top of // this; per-role takes precedence whenever it is the tightest bound. - request.DurationSeconds = capDurationByRole(request.DurationSeconds, roleDef.MaxSessionDuration) + request.DurationSeconds = capDurationByRole(request.DurationSeconds, roleDef.MaxSessionDuration, m.defaultTokenDurationSeconds(), m.maxSessionLengthSeconds()) // Use STS service to assume the role return m.stsService.AssumeRoleWithWebIdentity(ctx, request) @@ -1005,22 +1005,54 @@ func extractIssuerFromJWT(token string) (string, error) { return iss, nil } -// capDurationByRole returns the requested duration clamped to the role's -// MaxSessionDuration. A nil requested duration is left nil so the STS -// service's calculateSessionDuration applies the global default (typically -// 1 hour) — substituting the role's max here would silently mint a 12h -// session for any caller who omitted DurationSeconds, which AWS does not -// do. The role-max upper bound still applies in the downstream cap chain -// once the request has a concrete duration. -func capDurationByRole(requested *int64, roleMax int64) *int64 { - if roleMax <= 0 || requested == nil { - return requested +// capDurationByRole returns the session duration clamped to the role's +// MaxSessionDuration. An omitted DurationSeconds resolves to the configured +// default first, so the role bound caps defaults and explicit values alike. +// A nil request is only materialized when something tightened the default +// and the explicit value still passes the service's own input validation — +// everything else is left nil so the service resolves the default and its +// MaxSessionLength cap itself. +func capDurationByRole(requested *int64, roleMax, defaultSec, serviceMaxSec int64) *int64 { + if requested != nil { + d := *requested + if roleMax > 0 && d > roleMax { + d = roleMax + } + return &d } - if *requested > roleMax { - v := roleMax - return &v + d := defaultSec + if roleMax > 0 && d > roleMax { + d = roleMax } - return requested + if d > serviceMaxSec { + d = serviceMaxSec + } + if d < 900 && roleMax > 0 { + d = 900 + } + if d >= defaultSec || d < 900 { + return nil + } + return &d +} + +func (m *IAMManager) defaultTokenDurationSeconds() int64 { + if m.stsService == nil || m.stsService.Config == nil { + return sts.DefaultTokenDuration + } + return int64(m.stsService.Config.TokenDuration.Duration / time.Second) +} + +// maxSessionLengthSeconds mirrors validateSessionDurationSeconds so a +// materialized default stays inside the bound the service will enforce. +func (m *IAMManager) maxSessionLengthSeconds() int64 { + maxSec := int64(sts.DefaultMaxSessionLength) + if m.stsService != nil && m.stsService.Config != nil && m.stsService.Config.MaxSessionLength.Duration > 0 { + if configured := int64(m.stsService.Config.MaxSessionLength.Duration / time.Second); configured >= 900 { + maxSec = configured + } + } + return maxSec } // AssumeRoleWithCredentials assumes a role using credentials (LDAP) @@ -1044,7 +1076,7 @@ func (m *IAMManager) AssumeRoleWithCredentials(ctx context.Context, request *sts } // Apply role-level MaxSessionDuration cap. - request.DurationSeconds = capDurationByRole(request.DurationSeconds, roleDef.MaxSessionDuration) + request.DurationSeconds = capDurationByRole(request.DurationSeconds, roleDef.MaxSessionDuration, m.defaultTokenDurationSeconds(), m.maxSessionLengthSeconds()) // Use STS service to assume the role return m.stsService.AssumeRoleWithCredentials(ctx, request) diff --git a/weed/iam/integration/role_max_session_test.go b/weed/iam/integration/role_max_session_test.go index 9f011c71f..c25d86f22 100644 --- a/weed/iam/integration/role_max_session_test.go +++ b/weed/iam/integration/role_max_session_test.go @@ -6,21 +6,27 @@ func intPtr(v int64) *int64 { return &v } func TestCapDurationByRole(t *testing.T) { cases := []struct { - name string - requested *int64 - roleMax int64 - want *int64 + name string + requested *int64 + roleMax int64 + defaultSec int64 + serviceMaxSec int64 + want *int64 }{ - {"no cap, no request", nil, 0, nil}, - {"no cap, with request", intPtr(7200), 0, intPtr(7200)}, - {"cap only, no request -> nil so STS default applies", nil, 3600, nil}, - {"request below cap -> request", intPtr(1800), 3600, intPtr(1800)}, - {"request equal cap -> request", intPtr(3600), 3600, intPtr(3600)}, - {"request above cap -> cap", intPtr(43200), 3600, intPtr(3600)}, + {"no cap, no request -> nil keeps service default", nil, 0, 3600, 43200, nil}, + {"no cap, with request", intPtr(7200), 0, 3600, 43200, intPtr(7200)}, + {"cap below default, no request -> cap", nil, 1800, 3600, 43200, intPtr(1800)}, + {"cap above default, no request -> nil", nil, 43200, 3600, 43200, nil}, + {"request below cap -> request", intPtr(1800), 3600, 900, 43200, intPtr(1800)}, + {"request equal cap -> request", intPtr(3600), 3600, 900, 43200, intPtr(3600)}, + {"request above cap -> cap", intPtr(43200), 3600, 900, 43200, intPtr(3600)}, + {"default above service max -> service cap materialized", nil, 0, 7200, 3600, intPtr(3600)}, + {"role bound above service max -> service cap still applies", nil, 40000, 43200, 3600, intPtr(3600)}, + {"role bound below service floor -> tightest issuable", nil, 500, 3600, 43200, intPtr(900)}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - got := capDurationByRole(tc.requested, tc.roleMax) + got := capDurationByRole(tc.requested, tc.roleMax, tc.defaultSec, tc.serviceMaxSec) switch { case got == nil && tc.want == nil: return diff --git a/weed/iam/integration/role_store.go b/weed/iam/integration/role_store.go index 11fbbb44e..c814293f0 100644 --- a/weed/iam/integration/role_store.go +++ b/weed/iam/integration/role_store.go @@ -112,9 +112,10 @@ func copyRoleDefinition(original *RoleDefinition) *RoleDefinition { } copied := &RoleDefinition{ - RoleName: original.RoleName, - RoleArn: original.RoleArn, - Description: original.Description, + RoleName: original.RoleName, + RoleArn: original.RoleArn, + Description: original.Description, + MaxSessionDuration: original.MaxSessionDuration, } // Deep copy trust policy if it exists diff --git a/weed/iam/sts/sts_service.go b/weed/iam/sts/sts_service.go index cb41d4d16..ff9cc53dd 100644 --- a/weed/iam/sts/sts_service.go +++ b/weed/iam/sts/sts_service.go @@ -604,7 +604,7 @@ func (s *STSService) AssumeRoleWithWebIdentity(ctx context.Context, request *Ass } // 4. Calculate session duration - sessionDuration := s.calculateSessionDuration(request.DurationSeconds) + sessionDuration := s.CalculateSessionDuration(request.DurationSeconds) expiresAt := time.Now().Add(sessionDuration) // 5. Generate session ID and credentials @@ -769,7 +769,7 @@ func (s *STSService) AssumeRoleWithCredentials(ctx context.Context, request *Ass func (s *STSService) issueSession(roleArn, roleSessionName, sessionPolicy string, durationSeconds *int64, providerName, subject string) (*AssumeRoleResponse, error) { - sessionDuration := s.calculateSessionDuration(durationSeconds) + sessionDuration := s.CalculateSessionDuration(durationSeconds) expiresAt := time.Now().Add(sessionDuration) sessionId, err := GenerateSessionId() @@ -1120,11 +1120,11 @@ func (s *STSService) validateRoleAssumptionForCredentials(ctx context.Context, r return nil } -// calculateSessionDuration returns the requested DurationSeconds, or the +// CalculateSessionDuration returns the requested DurationSeconds, or the // configured TokenDuration default, capped at MaxSessionLength. The source // token's exp deliberately plays no part: per AWS semantics the session // outlives the (already verified) web identity token. -func (s *STSService) calculateSessionDuration(durationSeconds *int64) time.Duration { +func (s *STSService) CalculateSessionDuration(durationSeconds *int64) time.Duration { var duration time.Duration if durationSeconds != nil { duration = time.Duration(*durationSeconds) * time.Second diff --git a/weed/iam/sts/sts_service_test.go b/weed/iam/sts/sts_service_test.go index 83bb46c89..1fbc1c32d 100644 --- a/weed/iam/sts/sts_service_test.go +++ b/weed/iam/sts/sts_service_test.go @@ -30,8 +30,8 @@ func TestCalculateSessionDuration(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - if got := svc.calculateSessionDuration(tc.durationSeconds); got != tc.want { - t.Errorf("calculateSessionDuration(%v) = %v, want %v", tc.durationSeconds, got, tc.want) + if got := svc.CalculateSessionDuration(tc.durationSeconds); got != tc.want { + t.Errorf("CalculateSessionDuration(%v) = %v, want %v", tc.durationSeconds, got, tc.want) } }) } diff --git a/weed/s3api/s3api_sts.go b/weed/s3api/s3api_sts.go index 66f0ecaa3..106a32e93 100644 --- a/weed/s3api/s3api_sts.go +++ b/weed/s3api/s3api_sts.go @@ -834,12 +834,30 @@ func (h *STSHandlers) handleGetFederationToken(w http.ResponseWriter, r *http.Re func (h *STSHandlers) prepareSTSCredentials(ctx context.Context, roleArn, roleSessionName string, durationSeconds *int64, sessionPolicy string, modifyClaims func(*sts.STSSessionClaims)) (STSCredentials, *AssumedRoleUser, error) { - // Calculate duration - duration := time.Hour // Default 1 hour - if durationSeconds != nil { + duration := time.Hour + if h.stsService != nil && h.stsService.Config != nil { + duration = h.stsService.CalculateSessionDuration(durationSeconds) + } else if durationSeconds != nil { duration = time.Duration(*durationSeconds) * time.Second } + // A named role's MaxSessionDuration bounds the resolved duration the same + // way capDurationByRole does on the SDK paths; self-assumption has no role + // definition to consult. + if h.iam != nil && h.iam.iamIntegration != nil { + if roleName := utils.ExtractRoleNameFromArn(roleArn); roleName != "" { + if provider, ok := h.iam.iamIntegration.(IAMManagerProvider); ok { + if mgr := provider.GetIAMManager(); mgr != nil { + if roleDef, roleErr := mgr.GetRole(ctx, roleName); roleErr == nil && roleDef.MaxSessionDuration > 0 { + if roleMax := time.Duration(roleDef.MaxSessionDuration) * time.Second; duration > roleMax { + duration = roleMax + } + } + } + } + } + } + // Generate session ID sessionId, err := sts.GenerateSessionId() if err != nil { diff --git a/weed/s3api/s3api_sts_assume_role_test.go b/weed/s3api/s3api_sts_assume_role_test.go index 27e74a79d..d4329ec0f 100644 --- a/weed/s3api/s3api_sts_assume_role_test.go +++ b/weed/s3api/s3api_sts_assume_role_test.go @@ -241,3 +241,64 @@ func newTestSTSIntegrationManager(t *testing.T) *integration.IAMManager { require.NoError(t, manager.Initialize(config, func() string { return "" })) return manager } + +// AssumeRole derives its session length from the same default-then-cap rule as +// the service layer (#11473): an omitted DurationSeconds yields TokenDuration, +// and either path is capped at MaxSessionLength. +func TestPrepareSTSCredentialsHonorsConfiguredDurations(t *testing.T) { + stsService := sts.NewSTSService() + require.NoError(t, stsService.Initialize(&sts.STSConfig{ + TokenDuration: sts.FlexibleDuration{Duration: 15 * time.Minute}, + MaxSessionLength: sts.FlexibleDuration{Duration: 20 * time.Minute}, + Issuer: "test-issuer", + SigningKey: []byte("test-signing-key-at-least-32-bytes-long-for-security"), + })) + stsHandlers := NewSTSHandlers(stsService, nil) + roleArn := fmt.Sprintf("arn:aws:iam::%s:role/test-role", defaultAccountID) + + expiresIn := func(durationSeconds *int64) time.Duration { + stsCreds, _, err := stsHandlers.prepareSTSCredentials(context.Background(), roleArn, "test-session", durationSeconds, "", nil) + require.NoError(t, err) + exp, err := time.Parse(time.RFC3339, stsCreds.Expiration) + require.NoError(t, err) + return time.Until(exp) + } + + oneHour := int64(3600) + assert.InDelta(t, (15 * time.Minute).Seconds(), expiresIn(nil).Seconds(), 60) + assert.InDelta(t, (20 * time.Minute).Seconds(), expiresIn(&oneHour).Seconds(), 60) +} + +// A named role's MaxSessionDuration bounds the session however DurationSeconds +// was resolved, matching the SDK paths' capDurationByRole. +func TestPrepareSTSCredentialsCapsAtRoleMaxDuration(t *testing.T) { + ctx := context.Background() + manager := newTestSTSIntegrationManager(t) + require.NoError(t, manager.CreateRole(ctx, "", "ShortLivedRole", &integration.RoleDefinition{ + RoleName: "ShortLivedRole", + MaxSessionDuration: 3600, + })) + + stsService := sts.NewSTSService() + require.NoError(t, stsService.Initialize(&sts.STSConfig{ + TokenDuration: sts.FlexibleDuration{Duration: 2 * time.Hour}, + MaxSessionLength: sts.FlexibleDuration{Duration: 12 * time.Hour}, + Issuer: "test-issuer", + SigningKey: []byte("test-signing-key-at-least-32-bytes-long-for-security"), + })) + iam := &IdentityAccessManagement{iamIntegration: NewS3IAMIntegration(manager, "")} + stsHandlers := NewSTSHandlers(stsService, iam) + roleArn := fmt.Sprintf("arn:aws:iam::%s:role/ShortLivedRole", defaultAccountID) + + expiresIn := func(durationSeconds *int64) time.Duration { + stsCreds, _, err := stsHandlers.prepareSTSCredentials(ctx, roleArn, "test-session", durationSeconds, "", nil) + require.NoError(t, err) + exp, err := time.Parse(time.RFC3339, stsCreds.Expiration) + require.NoError(t, err) + return time.Until(exp) + } + + twoHours := int64(7200) + assert.InDelta(t, float64(3600), expiresIn(nil).Seconds(), 60, "omitted duration resolves to the 2h default but the role caps it at 1h") + assert.InDelta(t, float64(3600), expiresIn(&twoHours).Seconds(), 60, "explicit duration above the role max is capped") +}