From 6518246f63dd15d670cb39ea13a042cca17133a7 Mon Sep 17 00:00:00 2001 From: Tung Lam <53996158+tunglambk@users.noreply.github.com> Date: Wed, 16 Sep 2026 22:16:55 +0700 Subject: [PATCH] fix: omit ObjectLockConfiguration Rule when there is no default retention * fix: omit ObjectLockConfiguration Rule when there is no default retention ParseBucketLockConfigurationOutput always set Rule, so a bucket with object lock enabled and no default retention answered GET ?object-lock with an empty . AWS S3 omits the element, and the AWS SDK v2 clients that read the rule from it follow the empty element with a malformed request. Fixes #2397 * test: cover GetObjectLockConfiguration with no default retention in the integration suite and drop the stale non-nil Rule expectation from the controller unit test --------- Co-authored-by: Tung Lam --- auth/object_lock.go | 8 +- auth/object_lock_test.go | 43 +++++++++++ s3api/controllers/bucket-get_test.go | 3 - .../integration/GetBucketLockConfiguration.go | 76 +++++++++++++++++++ tests/integration/group-tests.go | 2 + 5 files changed, 126 insertions(+), 6 deletions(-) diff --git a/auth/object_lock.go b/auth/object_lock.go index ca8dcb25..77083cb2 100644 --- a/auth/object_lock.go +++ b/auth/object_lock.go @@ -134,10 +134,12 @@ func ParseBucketLockConfigurationOutput(input []byte) (*types.ObjectLockConfigur return nil, fmt.Errorf("parse object lock config: %w", err) } - result := &types.ObjectLockConfiguration{ - Rule: &types.ObjectLockRule{ + result := &types.ObjectLockConfiguration{} + + if config.DefaultRetention != nil { + result.Rule = &types.ObjectLockRule{ DefaultRetention: config.DefaultRetention, - }, + } } if config.Enabled { diff --git a/auth/object_lock_test.go b/auth/object_lock_test.go index 1e7b59db..411e14e9 100644 --- a/auth/object_lock_test.go +++ b/auth/object_lock_test.go @@ -17,6 +17,7 @@ package auth import ( "context" "encoding/json" + "encoding/xml" "testing" "time" @@ -379,3 +380,45 @@ func TestVerifyBypassGovernancePermission_ArnPrincipals(t *testing.T) { assert.Contains(t, apiErr.Description, "with an explicit deny in a resource-based policy") }) } + +// TestParseBucketLockConfigurationOutput covers the object lock configuration +// reported to clients. A bucket with object lock enabled and no default +// retention must omit the element, the way AWS S3 does: an empty +// is accepted by the XML parser but leaves AWS SDK v2 clients +// (the Java SDK among them) with nothing to read the rule from. +func TestParseBucketLockConfigurationOutput(t *testing.T) { + days := int32(30) + + tests := []struct { + name string + config string + wantRule bool + wantMarker string + }{ + {name: "no default retention", config: `{"Enabled":true}`, wantMarker: "Enabled"}, + {name: "default retention", config: `{"Enabled":true,"DefaultRetention":{"Mode":"GOVERNANCE","Days":30}}`, wantRule: true, wantMarker: "30"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + out, err := ParseBucketLockConfigurationOutput([]byte(tt.config)) + assert.NoError(t, err) + + if !tt.wantRule { + assert.Nil(t, out.Rule) + } else { + assert.NotNil(t, out.Rule) + assert.Equal(t, &days, out.Rule.DefaultRetention.Days) + } + + data, err := xml.Marshal(out) + assert.NoError(t, err) + assert.Contains(t, string(data), tt.wantMarker) + if tt.wantRule { + assert.Contains(t, string(data), "") + } else { + assert.NotContains(t, string(data), "") + } + }) + } +} diff --git a/s3api/controllers/bucket-get_test.go b/s3api/controllers/bucket-get_test.go index 92ded301..3c65f72b 100644 --- a/s3api/controllers/bucket-get_test.go +++ b/s3api/controllers/bucket-get_test.go @@ -799,9 +799,6 @@ func TestS3ApiController_GetObjectLockConfiguration(t *testing.T) { response: &Response{ Data: &types.ObjectLockConfiguration{ ObjectLockEnabled: types.ObjectLockEnabledEnabled, - Rule: &types.ObjectLockRule{ - DefaultRetention: nil, - }, }, MetaOpts: &MetaOptions{ BucketOwner: "root", diff --git a/tests/integration/GetBucketLockConfiguration.go b/tests/integration/GetBucketLockConfiguration.go index 24a04dcf..6b304cbc 100644 --- a/tests/integration/GetBucketLockConfiguration.go +++ b/tests/integration/GetBucketLockConfiguration.go @@ -117,3 +117,79 @@ func GetObjectLockConfiguration_success(s *S3Conf) error { return nil }, withLock()) } + +func GetObjectLockConfiguration_no_default_retention(s *S3Conf) error { + testName := "GetObjectLockConfiguration_no_default_retention" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + resp, err := s3client.GetObjectLockConfiguration(ctx, &s3.GetObjectLockConfigurationInput{ + Bucket: &bucket, + }) + cancel() + if err != nil { + return err + } + + if resp.ObjectLockConfiguration == nil { + return fmt.Errorf("got nil object lock configuration") + } + if resp.ObjectLockConfiguration.ObjectLockEnabled != types.ObjectLockEnabledEnabled { + return fmt.Errorf("expected lock status to be %v, instead got %v", + types.ObjectLockEnabledEnabled, resp.ObjectLockConfiguration.ObjectLockEnabled) + } + if resp.ObjectLockConfiguration.Rule != nil { + return fmt.Errorf("expected nil object lock rule, instead got %+v", + *resp.ObjectLockConfiguration.Rule) + } + + // A default retention must still be reported as a rule, so a response + // that omits the rule unconditionally cannot pass this case. + var days int32 = 20 + config := types.ObjectLockConfiguration{ + ObjectLockEnabled: types.ObjectLockEnabledEnabled, + Rule: &types.ObjectLockRule{ + DefaultRetention: &types.DefaultRetention{ + Mode: types.ObjectLockRetentionModeCompliance, + Days: &days, + }, + }, + } + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.PutObjectLockConfiguration(ctx, &s3.PutObjectLockConfigurationInput{ + Bucket: &bucket, + ObjectLockConfiguration: &config, + }) + cancel() + if err != nil { + return err + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + resp, err = s3client.GetObjectLockConfiguration(ctx, &s3.GetObjectLockConfigurationInput{ + Bucket: &bucket, + }) + cancel() + if err != nil { + return err + } + + if resp.ObjectLockConfiguration == nil { + return fmt.Errorf("got nil object lock configuration") + } + if resp.ObjectLockConfiguration.Rule == nil { + return fmt.Errorf("got nil object lock rule for a configured default retention") + } + if resp.ObjectLockConfiguration.Rule.DefaultRetention == nil { + return fmt.Errorf("got nil object lock default retention") + } + if resp.ObjectLockConfiguration.Rule.DefaultRetention.Days == nil { + return fmt.Errorf("expected lock config days to be not nil") + } + if *resp.ObjectLockConfiguration.Rule.DefaultRetention.Days != days { + return fmt.Errorf("expected lock config days to be %v, instead got %v", + days, *resp.ObjectLockConfiguration.Rule.DefaultRetention.Days) + } + + return nil + }, withLock()) +} diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 9c9f0522..90825d68 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -770,6 +770,7 @@ func TestPutObjectLockConfiguration(ts *TestState) { func TestGetObjectLockConfiguration(ts *TestState) { ts.Run(GetObjectLockConfiguration_non_existing_bucket) ts.Run(GetObjectLockConfiguration_unset_config) + ts.Run(GetObjectLockConfiguration_no_default_retention) ts.Run(GetObjectLockConfiguration_success) } @@ -3293,6 +3294,7 @@ func GetIntTests() IntTests { "PutObjectLockConfiguration_success": PutObjectLockConfiguration_success, "GetObjectLockConfiguration_non_existing_bucket": GetObjectLockConfiguration_non_existing_bucket, "GetObjectLockConfiguration_unset_config": GetObjectLockConfiguration_unset_config, + "GetObjectLockConfiguration_no_default_retention": GetObjectLockConfiguration_no_default_retention, "GetObjectLockConfiguration_success": GetObjectLockConfiguration_success, "PutObjectRetention_non_existing_bucket": PutObjectRetention_non_existing_bucket, "PutObjectRetention_non_existing_object": PutObjectRetention_non_existing_object,