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 <Rule></Rule>. 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 <lamphamabtung96@gmail.com>

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 {

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 <Rule> element, the way AWS S3 does: an empty
+// <Rule></Rule> 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: "<ObjectLockEnabled>Enabled</ObjectLockEnabled>"},
+		{name: "default retention", config: `{"Enabled":true,"DefaultRetention":{"Mode":"GOVERNANCE","Days":30}}`, wantRule: true, wantMarker: "<Days>30</Days>"},
+	}
+
+	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), "<Rule>")
+			} else {
+				assert.NotContains(t, string(data), "<Rule>")
+			}
+		})
+	}
+}

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",

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())
+}

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,