From 9ca2c2e9e34ecf7a342c82b2bbe7db865f3b4621 Mon Sep 17 00:00:00 2001 From: Peter Van Bouwel Date: Wed, 27 Nov 2024 16:40:53 +0100 Subject: [PATCH] feature: multi-backend backward compatibility; allow invalid region When in the old setup with a single S3 backend an invalid region was provided in a request this would just be ignored as the target region would always be mapped to the configured target region. For the multi-backend setup specifying an invalid region would result in an internal server error. This change introduces a feature flag ENABLE_LEGACY_BEHAVIOR_INVALID_REGION_TO_DEFAULT_REGION which can be set to true to go back to the old behavior by falling back to the default backend (as specified in backend-config.yaml). If not set the request would be answered with a Bad Request response explaining to the user that an invalid region was returned. cleanup: legacy environment variable --- cmd/almost-e2e_test.go | 78 +++++++++++++++++++++++++++++++++++---- cmd/backendconfig.go | 8 +++- cmd/config.go | 12 +++++- cmd/handler-builder.go | 6 ++- cmd/s3-errors.go | 6 +++ cmd/s3errorcode_string.go | 5 ++- etc/.env | 1 - 7 files changed, 103 insertions(+), 13 deletions(-) diff --git a/cmd/almost-e2e_test.go b/cmd/almost-e2e_test.go index 5a4eb11..0e8cf28 100644 --- a/cmd/almost-e2e_test.go +++ b/cmd/almost-e2e_test.go @@ -2,6 +2,7 @@ package cmd import ( "context" + "errors" "fmt" "io" "net/http" @@ -11,11 +12,14 @@ import ( "github.com/VITObelgium/fakes3pp/presign" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/s3" + "github.com/aws/smithy-go" + "github.com/spf13/viper" ) const testRegion1 = "tst-1" const testRegion2 = "eu-test-2" +var defaultBakendIdAlmostE2ETests = testRegion2 var backendTestRegions = []string{testRegion1, testRegion2} var testingBucketNameBackenddetails = "backenddetails" var testingRegionTxtObjectKey = "region.txt" @@ -32,7 +36,7 @@ s3backends: file: ../etc/creds/otc_creds.yaml endpoint: http://localhost:5001 default: %s -`, testRegion1, testRegion2, testRegion2)) +`, testRegion1, testRegion2, defaultBakendIdAlmostE2ETests)) //Set the configurations as expected for the testingbackends @@ -103,7 +107,7 @@ func getCredentialsFromTestStsProxy(t *testing.T, token, sessionName, roleArn st } //region object is setup in the backends and matches the region name of the backend -func getRegionObjectContent(t *testing.T, region string, creds aws.Credentials) string{ +func getRegionObjectContent(t *testing.T, region string, creds aws.Credentials) (string, smithy.APIError){ client := getS3ClientAgainstS3Proxy(t, region, creds) max1Sec, cancel := context.WithTimeout(context.Background(), 1000 * time.Second) @@ -115,13 +119,19 @@ func getRegionObjectContent(t *testing.T, region string, creds aws.Credentials) defer cancel() s3ObjectOutput, err := client.GetObject(max1Sec, &input) if err != nil { - t.Errorf("encountered error getting region file for %s: %s", region, err) + var oe smithy.APIError + if !errors.As(err, &oe) { + t.Errorf("Could not convert smity error") + t.FailNow() + } + return "", oe } bytes, err := io.ReadAll(s3ObjectOutput.Body) if err != nil { - t.Errorf("encountered error reading region file content for %s: %s", region, err) + t.Error("Reading body should not fail unless issue with test environment") + t.FailNow() } - return string(bytes) + return string(bytes), nil } @@ -140,13 +150,67 @@ func TestMakeSureCorrectBackendIsSelected(t *testing.T) { for _, backendRegion := range backendTestRegions { - regionContent := getRegionObjectContent(t, backendRegion, creds) - if regionContent != backendRegion { + regionContent, err := getRegionObjectContent(t, backendRegion, creds) + if err != nil { + t.Errorf("Could not get region content due to error %s", err) + } else if regionContent != backendRegion { t.Errorf("when retrieving region file for %s we got %s", backendRegion, regionContent) } } } +//When requests are made with an invalid region generally it is expected to have the requests fail. +//for the legacy implementation only supporting a single backend that was not the case and the region +//information was ignored. It is recommended to discourage usage of wrong regions by region out to users +//who are using an invalid region. But to allow for a grace period where not breaking old usages you can also +//ENABLE_LEGACY_BEHAVIOR_INVALID_REGION_TO_DEFAULT_REGION +func TestAllowFallbackToDefaultBackend(t *testing.T) { + //Given legacy behavior mode enabled + viper.Set(enableLegacyBehaviorInvalidRegionToDefaultRegion, true) + + tearDown, getSignedToken := testingFixture(t) + defer tearDown() + token := getSignedToken("mySubject", time.Minute * 20, AWSSessionTags{PrincipalTags: map[string][]string{"org": {"a"}}}) + //Given the policy Manager that has roleArn for the testARN + pm = *NewTestPolicyManagerAllowAll() + //Given credentials for that role + creds := getCredentialsFromTestStsProxy(t, token, "my-session", testPolicyAllowAllARN) + + // When a request is done to an invalid region + regionContent, err := getRegionObjectContent(t, "invalidRegion", creds) + // The response is as if the request was set to the default region + if err != nil { + t.Errorf("Could not get region content due to error %s", err) + } else if regionContent != defaultBakendIdAlmostE2ETests { + t.Errorf("when retrieving region file for %s we got %s but expected %s", "invalidRegion", regionContent, testDefaultBackendRegion) + } +} + +//When not allowing fallback an invalid region should have clear indication that it is a user err +func TestIfNoFallbackToDefaultBackendBadRequestShouldBeReturned(t *testing.T) { + //Given legacy behavior mode enabled + viper.Set(enableLegacyBehaviorInvalidRegionToDefaultRegion, false) + + tearDown, getSignedToken := testingFixture(t) + defer tearDown() + token := getSignedToken("mySubject", time.Minute * 20, AWSSessionTags{PrincipalTags: map[string][]string{"org": {"a"}}}) + //Given the policy Manager that has roleArn for the testARN + pm = *NewTestPolicyManagerAllowAll() + //Given credentials for that role + creds := getCredentialsFromTestStsProxy(t, token, "my-session", testPolicyAllowAllARN) + + // When a request is done to an invalid region + regionContent, err := getRegionObjectContent(t, "invalidRegion", creds) + // The response is as if the request was set to the default region + if err == nil { + t.Errorf("Should not have succeeded but I got %s without error", regionContent) + } + if err.ErrorMessage() != "The provided region is not valid." { + t.Errorf("Unexpected error message: %s", err.ErrorMessage()) + t.FailNow() + } +} + func TestSigv4PresignedUrlsWork(t *testing.T) { //Given a running proxy and credentials against that proxy that allow access for the get operation tearDown, getSignedToken := testingFixture(t) diff --git a/cmd/backendconfig.go b/cmd/backendconfig.go index e4847c5..605dd27 100644 --- a/cmd/backendconfig.go +++ b/cmd/backendconfig.go @@ -147,6 +147,8 @@ type backendsConfig struct { defaultBackend string } +var errInvalidBackendErr = errors.New("invalid BackendId") + func (cfg* backendsConfig) getBackendConfig(backendId string) (cfgEntry backendConfigEntry, err error) { if cfg == nil { return cfgEntry, errors.New("backendsConfig not initialised") @@ -158,7 +160,11 @@ func (cfg* backendsConfig) getBackendConfig(backendId string) (cfgEntry backendC if ok { return backendCfg, nil } else { - return cfgEntry, fmt.Errorf("no such backend: %s", backendId) + if viper.GetBool(enableLegacyBehaviorInvalidRegionToDefaultRegion) && backendId != cfg.defaultBackend{ + return cfg.getBackendConfig(cfg.defaultBackend) + } else { + return cfgEntry, errInvalidBackendErr + } } } diff --git a/cmd/config.go b/cmd/config.go index b66d002..33533aa 100644 --- a/cmd/config.go +++ b/cmd/config.go @@ -48,6 +48,8 @@ const( s3BackendConfigFile = "s3BackendConfigFile" stsMaxDurationSeconds = "stsMaxDurationSeconds" signedUrlGraceTimeSeconds = "signedUrlGraceTimeSeconds" + enableLegacyBehaviorInvalidRegionToDefaultRegion = "enableLegacyBehaviorInvalidRegionToDefaultRegion" + //Environment variables are upper cased //Unless they are wellknown environment variables they should be prefixed @@ -67,6 +69,7 @@ const( FAKES3PP_ROLE_POLICY_PATH = "FAKES3PP_ROLE_POLICY_PATH" FAKES3PP_STS_MAX_DURATION_SECONDS = "FAKES3PP_STS_MAX_DURATION_SECONDS" FAKES3PP_SIGNEDURL_GRACE_TIME_SECONDS = "FAKES3PP_SIGNEDURL_GRACE_TIME_SECONDS" + ENABLE_LEGACY_BEHAVIOR_INVALID_REGION_TO_DEFAULT_REGION = "ENABLE_LEGACY_BEHAVIOR_INVALID_REGION_TO_DEFAULT_REGION" ) var envVarDefs = []envVarDef{ @@ -180,7 +183,14 @@ var envVarDefs = []envVarDef{ FAKES3PP_SIGNEDURL_GRACE_TIME_SECONDS, false, "The maximum duration in seconds a signed url can be valid past the lifetime of the credentials used to generate it", - []string{}, + []string{proxys3}, + }, + { + enableLegacyBehaviorInvalidRegionToDefaultRegion, + ENABLE_LEGACY_BEHAVIOR_INVALID_REGION_TO_DEFAULT_REGION, + false, + "If set to true invalid regions will not necessarily fail but will try default region", + []string{proxys3}, }, } diff --git a/cmd/handler-builder.go b/cmd/handler-builder.go index 9782058..d390849 100644 --- a/cmd/handler-builder.go +++ b/cmd/handler-builder.go @@ -335,7 +335,11 @@ func logRequest(ctx context.Context, apiAction string, r *http.Request) { func justProxy(ctx context.Context, w http.ResponseWriter, r *http.Request, targetBackendId string) { err := reTargetRequest(r, targetBackendId) - if err != nil { + if err == errInvalidBackendErr { + slog.Warn("Invalid region was specified in the request", "error", err, xRequestIDStr, getRequestID(ctx), "backendId", targetBackendId) + writeS3ErrorResponse(ctx, w, ErrS3InvalidRegion, nil) + return + } else if err != nil { slog.Error("Could not re-target request with permanent credentials", "error", err, xRequestIDStr, getRequestID(ctx), "backendId", targetBackendId) writeS3ErrorResponse(ctx, w, ErrS3InternalError, nil) return diff --git a/cmd/s3-errors.go b/cmd/s3-errors.go index 5078238..914743e 100644 --- a/cmd/s3-errors.go +++ b/cmd/s3-errors.go @@ -82,6 +82,7 @@ const ( ErrS3InvalidAccessKeyId ErrS3InvalidSignature ErrS3InvalidSecurity + ErrS3InvalidRegion ) @@ -127,4 +128,9 @@ var s3ErrCodes = s3ErrorCodeMap{ Description: "The provided security credentials are not valid.", HTTPStatusCode: http.StatusForbidden, }, + ErrS3InvalidRegion: { + Code: "InvalidRegion", + Description: "The provided region is not valid.", + HTTPStatusCode: http.StatusBadRequest, + }, } \ No newline at end of file diff --git a/cmd/s3errorcode_string.go b/cmd/s3errorcode_string.go index e2e1e1a..1a28205 100644 --- a/cmd/s3errorcode_string.go +++ b/cmd/s3errorcode_string.go @@ -15,11 +15,12 @@ func _() { _ = x[ErrS3InvalidAccessKeyId-4] _ = x[ErrS3InvalidSignature-5] _ = x[ErrS3InvalidSecurity-6] + _ = x[ErrS3InvalidRegion-7] } -const _S3ErrorCode_name = "S3NoneS3AccessDeniedS3InternalErrorS3UpstreamErrorS3InvalidAccessKeyIdS3InvalidSignatureS3InvalidSecurity" +const _S3ErrorCode_name = "S3NoneS3AccessDeniedS3InternalErrorS3UpstreamErrorS3InvalidAccessKeyIdS3InvalidSignatureS3InvalidSecurityS3InvalidRegion" -var _S3ErrorCode_index = [...]uint8{0, 6, 20, 35, 50, 70, 88, 105} +var _S3ErrorCode_index = [...]uint8{0, 6, 20, 35, 50, 70, 88, 105, 120} func (i S3ErrorCode) String() string { if i < 0 || i >= S3ErrorCode(len(_S3ErrorCode_index)-1) { diff --git a/etc/.env b/etc/.env index 3504d2f..8470ea6 100644 --- a/etc/.env +++ b/etc/.env @@ -4,7 +4,6 @@ FAKES3PP_S3_PROXY_KEY_FILE=../etc/key.pem FAKES3PP_S3_PROXY_CERT_FILE=../etc/cert.pem FAKES3PP_S3_PROXY_JWT_PRIVATE_RSA_KEY=../etc/jwt_testing_rsa FAKES3PP_S3_PROXY_JWT_PUBLIC_RSA_KEY=../etc/jwt_testing_rsa.pub -FAKES3PP_S3_PROXY_TARGET="s3.example.com" FAKES3PP_STS_PROXY_FQDN=localhost FAKES3PP_STS_PROXY_PORT=8444 FAKES3PP_STS_PROXY_KEY_FILE=../etc/key.pem