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

@@ -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)

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

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

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

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,
+	},
 }

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