From 0de9c1f2310a2a03d7b7ffc9cb7da4a3f80d2c67 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Thu, 10 Sep 2026 23:03:32 -0700 Subject: [PATCH] fix(s3api/sts): respect MaxSessionLength config in DurationSeconds validation (#11267) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Refactor parseDurationSeconds into a STSHandlers method Convert the parseDurationSeconds wrapper from a package-level function into a method on STSHandlers so it can reach the configured STS service. No behavior change; the three AssumeRole* handlers now invoke it via their receiver. * Respect MaxSessionLength config in STS DurationSeconds validation parseDurationSeconds validated DurationSeconds against a hardcoded 43200s (12h) ceiling, so raising maxSessionLength in iam.json above 12h had no effect on AssumeRole, AssumeRoleWithWebIdentity, or AssumeRoleWithLDAPIdentity — requests were rejected at the handler before reaching the service layer. Derive the upper bound from the configured STS MaxSessionLength, falling back to maxDurationSeconds (43200s) when unset. The service layer (calculateSessionDuration) already caps the issued duration at MaxSessionLength, so this only relaxes the input-validation gate. * Add tests for STS DurationSeconds MaxSessionLength bound Cover the configured MaxSessionLength upper bound, rejection above it, fallback to the 43200s default when STS config is unset, the 900s minimum, and the empty-parameter nil path. * Refactor validateSessionDurationSeconds into a STSService method Convert validateSessionDurationSeconds from a package-level function into a method on STSService so it can reach the configured STS config. No behavior change; the three assume-role entry points in the service (AssumeRoleForPrincipal, validateAssumeRoleWithWebIdentityRequest, validateAssumeRoleWithCredentialsRequest) now invoke it via their receiver. * Respect MaxSessionLength config in STS service DurationSeconds validation The STS service validateSessionDurationSeconds rejected DurationSeconds above a hardcoded 43200s (12h) ceiling, so even after the handler accepted a longer duration it was rejected again in the service layer for AssumeRoleForPrincipal, AssumeRoleWithWebIdentity, and AssumeRoleWithCredentials. Derive the upper bound from the configured MaxSessionLength, falling back to DefaultMaxSessionLength (43200s) when unset. The issued duration is still capped at MaxSessionLength by calculateSessionDuration. * Add tests for STS service DurationSeconds MaxSessionLength bound Cover the configured MaxSessionLength upper bound, rejection above it, fallback to the 43200s default when STS config is unset, the 900s minimum, and the nil DurationSeconds path. * Preserve capping when MaxSessionLength is below the API minimum Deriving the DurationSeconds upper bound directly from MaxSessionLength created an empty valid range when MaxSessionLength is configured below the 900s API minimum, rejecting every explicit DurationSeconds that the old code silently capped via calculateSessionDuration. Only apply the configured MaxSessionLength as the upper bound when it is at least minDurationSeconds; otherwise keep the default bound and let calculateSessionDuration enforce the shorter configured limit. * Add tests for sub-minimum MaxSessionLength capping behavior Verify that a MaxSessionLength below the 900s API minimum keeps the default upper bound so explicit DurationSeconds within the default range are still accepted (and later capped by calculateSessionDuration). --- weed/iam/sts/sts_service.go | 19 ++-- weed/iam/sts/sts_service_duration_test.go | 57 ++++++++++++ weed/s3api/s3api_sts.go | 19 ++-- weed/s3api/s3api_sts_duration_test.go | 105 ++++++++++++++++++++++ 4 files changed, 188 insertions(+), 12 deletions(-) create mode 100644 weed/iam/sts/sts_service_duration_test.go create mode 100644 weed/s3api/s3api_sts_duration_test.go diff --git a/weed/iam/sts/sts_service.go b/weed/iam/sts/sts_service.go index 6905f12fa..320665da8 100644 --- a/weed/iam/sts/sts_service.go +++ b/weed/iam/sts/sts_service.go @@ -807,12 +807,19 @@ func (s *STSService) issueSession(roleArn, roleSessionName, sessionPolicy string // validateSessionDurationSeconds bounds a requested session lifetime the way // AWS STS does. Every assume-role entry point runs it, so a duration that came // from configuration is checked the same as one from a request. -func validateSessionDurationSeconds(durationSeconds *int64) error { +func (s *STSService) validateSessionDurationSeconds(durationSeconds *int64) error { if durationSeconds == nil { return nil } - if *durationSeconds < 900 || *durationSeconds > 43200 { // 15min to 12 hours - return fmt.Errorf("DurationSeconds must be between 900 and 43200 seconds") + maxSec := int64(DefaultMaxSessionLength) + if s.Config != nil && s.Config.MaxSessionLength.Duration > 0 { + configuredMax := int64(s.Config.MaxSessionLength.Duration / time.Second) + if configuredMax >= 900 { + maxSec = configuredMax + } + } + if *durationSeconds < 900 || *durationSeconds > maxSec { + return fmt.Errorf("DurationSeconds must be between 900 and %d seconds", maxSec) } return nil } @@ -857,7 +864,7 @@ func (s *STSService) AssumeRoleForPrincipal(ctx context.Context, request *Assume if request.Principal == "" { return nil, fmt.Errorf("principal cannot be empty") } - if err := validateSessionDurationSeconds(request.DurationSeconds); err != nil { + if err := s.validateSessionDurationSeconds(request.DurationSeconds); err != nil { return nil, fmt.Errorf("invalid request: %w", err) } @@ -935,7 +942,7 @@ func (s *STSService) validateAssumeRoleWithWebIdentityRequest(request *AssumeRol return fmt.Errorf("RoleSessionName is required") } - return validateSessionDurationSeconds(request.DurationSeconds) + return s.validateSessionDurationSeconds(request.DurationSeconds) } // validateWebIdentityToken validates the web identity token with strict issuer-to-provider mapping @@ -1152,7 +1159,7 @@ func (s *STSService) validateAssumeRoleWithCredentialsRequest(request *AssumeRol return fmt.Errorf("ProviderName is required") } - return validateSessionDurationSeconds(request.DurationSeconds) + return s.validateSessionDurationSeconds(request.DurationSeconds) } // ExpireSessionForTesting manually expires a session for testing purposes diff --git a/weed/iam/sts/sts_service_duration_test.go b/weed/iam/sts/sts_service_duration_test.go new file mode 100644 index 000000000..30b5e5298 --- /dev/null +++ b/weed/iam/sts/sts_service_duration_test.go @@ -0,0 +1,57 @@ +package sts + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +func newSTSServiceWithMaxSession(t *testing.T, maxSession time.Duration) *STSService { + t.Helper() + s := NewSTSService() + assert.NoError(t, s.Initialize(&STSConfig{ + Issuer: "test-issuer", + SigningKey: []byte("test-signing-key-at-least-32-bytes-long"), + TokenDuration: FlexibleDuration{Duration: time.Hour}, + MaxSessionLength: FlexibleDuration{Duration: maxSession}, + })) + return s +} + +func secondsPtr(v int64) *int64 { return &v } + +func TestValidateSessionDurationSeconds_RespectsConfiguredMaxSessionLength(t *testing.T) { + s := newSTSServiceWithMaxSession(t, 168*time.Hour) + assert.NoError(t, s.validateSessionDurationSeconds(secondsPtr(604800))) +} + +func TestValidateSessionDurationSeconds_RejectsAboveConfiguredMaxSessionLength(t *testing.T) { + s := newSTSServiceWithMaxSession(t, 24*time.Hour) + err := s.validateSessionDurationSeconds(secondsPtr(90000)) + assert.Error(t, err) +} + +func TestValidateSessionDurationSeconds_FallsBackToDefaultWhenUnset(t *testing.T) { + s := &STSService{} + assert.NoError(t, s.validateSessionDurationSeconds(secondsPtr(43200))) + err := s.validateSessionDurationSeconds(secondsPtr(43201)) + assert.Error(t, err) +} + +func TestValidateSessionDurationSeconds_EnforcesMinimum(t *testing.T) { + s := newSTSServiceWithMaxSession(t, 168*time.Hour) + assert.Error(t, s.validateSessionDurationSeconds(secondsPtr(899))) +} + +func TestValidateSessionDurationSeconds_NilReturnsNil(t *testing.T) { + s := newSTSServiceWithMaxSession(t, 168*time.Hour) + assert.NoError(t, s.validateSessionDurationSeconds(nil)) +} + +func TestValidateSessionDurationSeconds_SubMinimumMaxSessionLengthKeepsCapping(t *testing.T) { + s := newSTSServiceWithMaxSession(t, 5*time.Minute) + assert.NoError(t, s.validateSessionDurationSeconds(secondsPtr(900))) + assert.NoError(t, s.validateSessionDurationSeconds(secondsPtr(43200))) + assert.Error(t, s.validateSessionDurationSeconds(secondsPtr(43201))) +} diff --git a/weed/s3api/s3api_sts.go b/weed/s3api/s3api_sts.go index e142d5f84..1baddb3b0 100644 --- a/weed/s3api/s3api_sts.go +++ b/weed/s3api/s3api_sts.go @@ -131,9 +131,16 @@ func parseDurationSecondsWithBounds(r *http.Request, minSec, maxSec int64) (*int return &ds, "", nil } -// parseDurationSeconds parses DurationSeconds for AssumeRole (15 min to 12 hours) -func parseDurationSeconds(r *http.Request) (*int64, STSErrorCode, error) { - return parseDurationSecondsWithBounds(r, minDurationSeconds, maxDurationSeconds) +// parseDurationSeconds parses DurationSeconds for AssumeRole (15 min to MaxSessionLength) +func (h *STSHandlers) parseDurationSeconds(r *http.Request) (*int64, STSErrorCode, error) { + maxSec := maxDurationSeconds + if h.stsService != nil && h.stsService.Config != nil && h.stsService.Config.MaxSessionLength.Duration > 0 { + configuredMax := int64(h.stsService.Config.MaxSessionLength.Duration / time.Second) + if configuredMax >= minDurationSeconds { + maxSec = configuredMax + } + } + return parseDurationSecondsWithBounds(r, minDurationSeconds, maxSec) } // Removed generateSecureCredentials - now using STS service's JWT token generation @@ -253,7 +260,7 @@ func (h *STSHandlers) handleAssumeRoleWithWebIdentity(w http.ResponseWriter, r * } // Parse and validate DurationSeconds using helper - durationSeconds, errCode, err := parseDurationSeconds(r) + durationSeconds, errCode, err := h.parseDurationSeconds(r) if err != nil { h.writeSTSErrorResponse(w, r, errCode, err) return @@ -350,7 +357,7 @@ func (h *STSHandlers) handleAssumeRole(w http.ResponseWriter, r *http.Request) { } // Parse and validate DurationSeconds using helper - durationSeconds, errCode, err := parseDurationSeconds(r) + durationSeconds, errCode, err := h.parseDurationSeconds(r) if err != nil { h.writeSTSErrorResponse(w, r, errCode, err) return @@ -506,7 +513,7 @@ func (h *STSHandlers) handleAssumeRoleWithLDAPIdentity(w http.ResponseWriter, r } // Parse and validate DurationSeconds using helper - durationSeconds, errCode, err := parseDurationSeconds(r) + durationSeconds, errCode, err := h.parseDurationSeconds(r) if err != nil { h.writeSTSErrorResponse(w, r, errCode, err) return diff --git a/weed/s3api/s3api_sts_duration_test.go b/weed/s3api/s3api_sts_duration_test.go new file mode 100644 index 000000000..e19f24cc1 --- /dev/null +++ b/weed/s3api/s3api_sts_duration_test.go @@ -0,0 +1,105 @@ +package s3api + +import ( + "net/http" + "net/url" + "strings" + "testing" + "time" + + "github.com/seaweedfs/seaweedfs/weed/iam/sts" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newDurationSecondsRequest(t *testing.T, seconds string) *http.Request { + t.Helper() + form := url.Values{} + if seconds != "" { + form.Set("DurationSeconds", seconds) + } + req, err := http.NewRequest("POST", "/", strings.NewReader(form.Encode())) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + require.NoError(t, req.ParseForm()) + return req +} + +func newSTSHandlersWithMaxSession(t *testing.T, maxSession time.Duration) *STSHandlers { + t.Helper() + s := sts.NewSTSService() + require.NoError(t, s.Initialize(&sts.STSConfig{ + Issuer: "test-issuer", + SigningKey: []byte("test-signing-key-at-least-32-bytes-long-for-security"), + TokenDuration: sts.FlexibleDuration{Duration: time.Hour}, + MaxSessionLength: sts.FlexibleDuration{Duration: maxSession}, + })) + return NewSTSHandlers(s, nil) +} + +func TestParseDurationSeconds_RespectsConfiguredMaxSessionLength(t *testing.T) { + h := newSTSHandlersWithMaxSession(t, 168*time.Hour) + + ds, errCode, err := h.parseDurationSeconds(newDurationSecondsRequest(t, "604800")) + require.NoError(t, err) + assert.Equal(t, STSErrorCode(""), errCode) + if assert.NotNil(t, ds) { + assert.Equal(t, int64(604800), *ds) + } +} + +func TestParseDurationSeconds_RejectsAboveConfiguredMaxSessionLength(t *testing.T) { + h := newSTSHandlersWithMaxSession(t, 24*time.Hour) + + ds, errCode, err := h.parseDurationSeconds(newDurationSecondsRequest(t, "90000")) + assert.Error(t, err) + assert.Equal(t, STSErrInvalidParameterValue, errCode) + assert.Nil(t, ds) +} + +func TestParseDurationSeconds_FallsBackToDefaultWhenUnset(t *testing.T) { + h := &STSHandlers{stsService: nil} + + ds, errCode, err := h.parseDurationSeconds(newDurationSecondsRequest(t, "43200")) + require.NoError(t, err) + assert.Equal(t, STSErrorCode(""), errCode) + if assert.NotNil(t, ds) { + assert.Equal(t, int64(43200), *ds) + } + + _, errCode, err = h.parseDurationSeconds(newDurationSecondsRequest(t, "43201")) + assert.Error(t, err) + assert.Equal(t, STSErrInvalidParameterValue, errCode) +} + +func TestParseDurationSeconds_EnforcesMinimum(t *testing.T) { + h := newSTSHandlersWithMaxSession(t, 168*time.Hour) + + _, errCode, err := h.parseDurationSeconds(newDurationSecondsRequest(t, "899")) + assert.Error(t, err) + assert.Equal(t, STSErrInvalidParameterValue, errCode) +} + +func TestParseDurationSeconds_EmptyReturnsNil(t *testing.T) { + h := newSTSHandlersWithMaxSession(t, 168*time.Hour) + + ds, errCode, err := h.parseDurationSeconds(newDurationSecondsRequest(t, "")) + require.NoError(t, err) + assert.Equal(t, STSErrorCode(""), errCode) + assert.Nil(t, ds) +} + +func TestParseDurationSeconds_SubMinimumMaxSessionLengthKeepsCapping(t *testing.T) { + h := newSTSHandlersWithMaxSession(t, 5*time.Minute) + + ds, errCode, err := h.parseDurationSeconds(newDurationSecondsRequest(t, "900")) + require.NoError(t, err) + assert.Equal(t, STSErrorCode(""), errCode) + if assert.NotNil(t, ds) { + assert.Equal(t, int64(900), *ds) + } + + _, errCode, err = h.parseDurationSeconds(newDurationSecondsRequest(t, "43201")) + assert.Error(t, err) + assert.Equal(t, STSErrInvalidParameterValue, errCode) +}