From f97764c670f877bf293635901c8dc16a5420f801 Mon Sep 17 00:00:00 2001 From: PJ Date: Thu, 23 Nov 2023 14:28:18 +0100 Subject: [PATCH 1/8] api: update S3AuthenticationSettings validation --- api/setting.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/api/setting.go b/api/setting.go index 8d12f12d6..02e369350 100644 --- a/api/setting.go +++ b/api/setting.go @@ -18,7 +18,7 @@ const ( const ( S3MinAccessKeyLen = 16 - S3MaxAccessKeyLen = 40 + S3MaxAccessKeyLen = 128 ) var ( @@ -137,11 +137,13 @@ func (rs RedundancySettings) Validate() error { // Validate returns an error if the authentication settings are not considered // valid. func (s3as S3AuthenticationSettings) Validate() error { - for id, key := range s3as.V4Keypairs { - if len(id) == 0 { + for accessKeyID, secretAccessKey := range s3as.V4Keypairs { + if len(accessKeyID) == 0 { return fmt.Errorf("AccessKeyID cannot be empty") - } else if len(key) < S3MinAccessKeyLen || len(key) > S3MaxAccessKeyLen { - return fmt.Errorf("AccessKeyID must be between %d and %d characters long but was %d", S3MinAccessKeyLen, S3MaxAccessKeyLen, len(key)) + } else if len(accessKeyID) < S3MinAccessKeyLen || len(accessKeyID) > S3MaxAccessKeyLen { + return fmt.Errorf("AccessKeyID must be between %d and %d characters long but was %d", S3MinAccessKeyLen, S3MaxAccessKeyLen, len(accessKeyID)) + } else if len(secretAccessKey) == 0 { + return fmt.Errorf("SecretAccessKey cannot be empty") } } return nil From f78bdccc4e64f0546bf910857679c5396f2b9c57 Mon Sep 17 00:00:00 2001 From: PJ Date: Thu, 23 Nov 2023 14:39:07 +0100 Subject: [PATCH 2/8] s3: assert secret key length --- api/setting.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/api/setting.go b/api/setting.go index 02e369350..07bb605a5 100644 --- a/api/setting.go +++ b/api/setting.go @@ -19,6 +19,7 @@ const ( const ( S3MinAccessKeyLen = 16 S3MaxAccessKeyLen = 128 + S3SecretKeyLen = 40 ) var ( @@ -144,6 +145,8 @@ func (s3as S3AuthenticationSettings) Validate() error { return fmt.Errorf("AccessKeyID must be between %d and %d characters long but was %d", S3MinAccessKeyLen, S3MaxAccessKeyLen, len(accessKeyID)) } else if len(secretAccessKey) == 0 { return fmt.Errorf("SecretAccessKey cannot be empty") + } else if len(secretAccessKey) != S3SecretKeyLen { + return fmt.Errorf("SecretAccessKey must be %d characters long but was %d", S3SecretKeyLen, len(secretAccessKey)) } } return nil From f1b916c8c55ecb43f3c4e74b28eee7d732e280df Mon Sep 17 00:00:00 2001 From: PJ Date: Thu, 23 Nov 2023 15:40:09 +0100 Subject: [PATCH 3/8] testing: ensure test keys pass validation --- internal/testing/cluster.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/testing/cluster.go b/internal/testing/cluster.go index 00aea17dc..e94fa0bd0 100644 --- a/internal/testing/cluster.go +++ b/internal/testing/cluster.go @@ -92,8 +92,8 @@ var ( TotalShards: 3, } - testS3Credentials = credentials.NewStaticV4("myaccesskeyidentifier", "mysupersecretkey", "") - testS3AuthPairs = map[string]string{"myaccesskeyidentifier": "mysupersecretkey"} + testS3Credentials = credentials.NewStaticV4("TESTINGYNHUWCPKOPSYQ", "Rh30BNyj+qNI4ftYRteoZbHJ3X4Ln71QtZkRXzJ9", "") + testS3AuthPairs = map[string]string{"TESTING4HRG3CPA63XEQ": "pARhvm1GmHyvLydUtFNCCMIIu4VEyaZNo9MbR3IJ"} ) type TT struct { From 75999aee39762808dfd001ef08524ca253a8cdf9 Mon Sep 17 00:00:00 2001 From: PJ Date: Thu, 23 Nov 2023 16:03:49 +0100 Subject: [PATCH 4/8] testing: fix S3 credentials --- internal/testing/cluster.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/testing/cluster.go b/internal/testing/cluster.go index e94fa0bd0..4edccd18f 100644 --- a/internal/testing/cluster.go +++ b/internal/testing/cluster.go @@ -92,8 +92,9 @@ var ( TotalShards: 3, } - testS3Credentials = credentials.NewStaticV4("TESTINGYNHUWCPKOPSYQ", "Rh30BNyj+qNI4ftYRteoZbHJ3X4Ln71QtZkRXzJ9", "") - testS3AuthPairs = map[string]string{"TESTING4HRG3CPA63XEQ": "pARhvm1GmHyvLydUtFNCCMIIu4VEyaZNo9MbR3IJ"} + testS3AccessKeyID = "TESTINGYNHUWCPKOPSYQ" + testS3SecretAccessKey = "Rh30BNyj+qNI4ftYRteoZbHJ3X4Ln71QtZkRXzJ9" + testS3Credentials = credentials.NewStaticV4(testS3AccessKeyID, testS3SecretAccessKey, "") ) type TT struct { @@ -521,7 +522,7 @@ func newTestCluster(t *testing.T, opts testClusterOptions) *TestCluster { tt.OK(busClient.UpdateSetting(context.Background(), api.SettingRedundancy, testRedundancySettings)) tt.OK(busClient.UpdateSetting(context.Background(), api.SettingContractSet, testContractSetSettings)) tt.OK(busClient.UpdateSetting(context.Background(), api.SettingS3Authentication, api.S3AuthenticationSettings{ - V4Keypairs: testS3AuthPairs, + V4Keypairs: map[string]string{testS3AccessKeyID: testS3SecretAccessKey}, })) tt.OK(busClient.UpdateSetting(context.Background(), api.SettingUploadPacking, api.UploadPackingSettings{Enabled: enableUploadPacking})) From 694f7ebcdee60a2a8cd0969a076289579d8dffef Mon Sep 17 00:00:00 2001 From: PJ Date: Mon, 27 Nov 2023 09:53:34 +0100 Subject: [PATCH 5/8] stores: avoid NDF in TestSlabHealthInvalidation --- stores/metadata_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/stores/metadata_test.go b/stores/metadata_test.go index 7a9e32a33..580113b4b 100644 --- a/stores/metadata_test.go +++ b/stores/metadata_test.go @@ -3769,8 +3769,8 @@ func TestSlabHealthInvalidation(t *testing.T) { } // assert it's validity is within expected bounds - min := now.Add(refreshHealthMinHealthValidity) - max := now.Add(refreshHealthMaxHealthValidity) + min := now.Add(refreshHealthMinHealthValidity).Add(-time.Second) // avoid NDF + max := now.Add(refreshHealthMaxHealthValidity).Add(time.Second) // avoid NDF validUntil := time.Unix(slab.HealthValidUntil, 0) if !(min.Before(validUntil) && max.After(validUntil)) { t.Fatal("valid until not in boundaries", min, max, validUntil, now) From 1a2a8c6dc9829537b77f4cf4b3cddf623a4856bd Mon Sep 17 00:00:00 2001 From: PJ Date: Mon, 27 Nov 2023 11:08:50 +0100 Subject: [PATCH 6/8] testing: fix TestS3SettingsValidate --- internal/testing/s3_test.go | 42 ++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/internal/testing/s3_test.go b/internal/testing/s3_test.go index 1ac815ab1..34a8c184f 100644 --- a/internal/testing/s3_test.go +++ b/internal/testing/s3_test.go @@ -603,6 +603,15 @@ func TestS3SettingsValidate(t *testing.T) { t.SkipNow() } + charset := "doesntreallymatter" + stringOfLength := func(n int) string { + b := make([]byte, n) + for i := range b { + b[i] = charset[frand.Intn(len(charset))] + } + return string(b) + } + cluster := newTestCluster(t, clusterOptsDefault) defer cluster.Shutdown() @@ -612,33 +621,38 @@ func TestS3SettingsValidate(t *testing.T) { shouldFail bool }{ { - // Min length - id: "id", - key: "aaaaaaaaaaaaaaaa", + id: stringOfLength(api.S3MinAccessKeyLen), + key: stringOfLength(api.S3SecretKeyLen), shouldFail: false, }, { - // Max length - id: "id", - key: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + id: stringOfLength(api.S3MaxAccessKeyLen), + key: stringOfLength(api.S3SecretKeyLen), shouldFail: false, }, { - // Min length - 1 - id: "id", - key: "aaaaaaaaaaaaaaa", + id: stringOfLength(api.S3MinAccessKeyLen - 1), + key: stringOfLength(api.S3SecretKeyLen), shouldFail: true, }, { - // Max length + 1 - id: "id", - key: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + id: stringOfLength(api.S3MaxAccessKeyLen + 1), + key: stringOfLength(api.S3SecretKeyLen), shouldFail: true, }, { - // No ID id: "", - key: "aaaaaaaaaaaaaaaa", + key: stringOfLength(api.S3SecretKeyLen), + shouldFail: true, + }, + { + id: stringOfLength(api.S3MinAccessKeyLen), + key: "", + shouldFail: true, + }, + { + id: stringOfLength(api.S3MinAccessKeyLen), + key: stringOfLength(api.S3SecretKeyLen + 1), shouldFail: true, }, } From 074786643d5184dd71c394331324f22874a494bc Mon Sep 17 00:00:00 2001 From: PJ Date: Mon, 27 Nov 2023 17:38:25 +0100 Subject: [PATCH 7/8] main: remove invalid key pairs --- cmd/renterd/main.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/cmd/renterd/main.go b/cmd/renterd/main.go index 90ce3eaa2..956ecca92 100644 --- a/cmd/renterd/main.go +++ b/cmd/renterd/main.go @@ -591,6 +591,17 @@ func main() { } else if as.V4Keypairs == nil { as.V4Keypairs = make(map[string]string) } + + // S3 key pair validation was broken at one point, we need to remove the + // invalid key pairs here to ensure we don't fail when we update the + // setting below. + for k, v := range as.V4Keypairs { + if err := (api.S3AuthenticationSettings{V4Keypairs: map[string]string{k: v}}).Validate(); err != nil { + logger.Sugar().Infof("removing invalid S3 keypair for AccessKeyID %s, reason: %v", k, err) + delete(as.V4Keypairs, k) + } + } + // merge keys for k, v := range cfg.S3.KeypairsV4 { as.V4Keypairs[k] = v From 7aa8b0196d42b5bed4a42e62c39f266df506c694 Mon Sep 17 00:00:00 2001 From: PJ Date: Mon, 27 Nov 2023 17:58:10 +0100 Subject: [PATCH 8/8] testing: use strings.Repeat --- internal/testing/s3_test.go | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/internal/testing/s3_test.go b/internal/testing/s3_test.go index 34a8c184f..f5cb2fba8 100644 --- a/internal/testing/s3_test.go +++ b/internal/testing/s3_test.go @@ -603,15 +603,6 @@ func TestS3SettingsValidate(t *testing.T) { t.SkipNow() } - charset := "doesntreallymatter" - stringOfLength := func(n int) string { - b := make([]byte, n) - for i := range b { - b[i] = charset[frand.Intn(len(charset))] - } - return string(b) - } - cluster := newTestCluster(t, clusterOptsDefault) defer cluster.Shutdown() @@ -621,38 +612,39 @@ func TestS3SettingsValidate(t *testing.T) { shouldFail bool }{ { - id: stringOfLength(api.S3MinAccessKeyLen), - key: stringOfLength(api.S3SecretKeyLen), + + id: strings.Repeat("a", api.S3MinAccessKeyLen), + key: strings.Repeat("a", api.S3SecretKeyLen), shouldFail: false, }, { - id: stringOfLength(api.S3MaxAccessKeyLen), - key: stringOfLength(api.S3SecretKeyLen), + id: strings.Repeat("a", api.S3MaxAccessKeyLen), + key: strings.Repeat("a", api.S3SecretKeyLen), shouldFail: false, }, { - id: stringOfLength(api.S3MinAccessKeyLen - 1), - key: stringOfLength(api.S3SecretKeyLen), + id: strings.Repeat("a", api.S3MinAccessKeyLen-1), + key: strings.Repeat("a", api.S3SecretKeyLen), shouldFail: true, }, { - id: stringOfLength(api.S3MaxAccessKeyLen + 1), - key: stringOfLength(api.S3SecretKeyLen), + id: strings.Repeat("a", api.S3MaxAccessKeyLen+1), + key: strings.Repeat("a", api.S3SecretKeyLen), shouldFail: true, }, { id: "", - key: stringOfLength(api.S3SecretKeyLen), + key: strings.Repeat("a", api.S3SecretKeyLen), shouldFail: true, }, { - id: stringOfLength(api.S3MinAccessKeyLen), + id: strings.Repeat("a", api.S3MinAccessKeyLen), key: "", shouldFail: true, }, { - id: stringOfLength(api.S3MinAccessKeyLen), - key: stringOfLength(api.S3SecretKeyLen + 1), + id: strings.Repeat("a", api.S3MinAccessKeyLen), + key: strings.Repeat("a", api.S3SecretKeyLen+1), shouldFail: true, }, }