fix(operator): Improve validation of provided S3 storage configuration

operator/CHANGELOG.md

@@ -1,5 +1,6 @@
 ## Main
 
+- [12181](https://github.com/grafana/loki/pull/12181) **btaani**: Improve validation of provided S3 storage configuration
 - [12333](https://github.com/grafana/loki/pull/12333) **periklis**: Bump max OpenShift version to next release
 
 ## 0.6.0 (2024-03-19)

operator/internal/handlers/internal/storage/secrets.go

@@ -9,7 +9,9 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"net/url"
 	"sort"
+	"strings"
 
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -38,6 +40,10 @@ var (
 	errAzureInvalidEnvironment        = errors.New("azure environment invalid (valid values: AzureGlobal, AzureChinaCloud, AzureGermanCloud, AzureUSGovernment)")
 	errAzureInvalidAccountKey         = errors.New("azure account key is not valid base64")
 
+	errS3EndpointUnparseable       = errors.New("s3 endpoint is not parseable")
+	errS3EndpointUnsupportedScheme = errors.New("scheme of S3 endpoint URL is unsupported")
+	errS3EndpointAWSInvalid        = errors.New("endpoint for AWS S3 must include correct region")
+
 	errGCPParseCredentialsFile      = errors.New("gcp storage secret cannot be parsed from JSON content")
 	errGCPWrongCredentialSourceFile = errors.New("credential source in secret needs to point to token file")
 
@@ -49,7 +55,10 @@ var (
 	}
 )
 
-const gcpAccountTypeExternal = "external_account"
+const (
+	awsEndpointSuffix      = ".amazonaws.com"
+	gcpAccountTypeExternal = "external_account"
+)
 
 func getSecrets(ctx context.Context, k k8s.Client, stack *lokiv1.LokiStack, fg configv1.FeatureGates) (*corev1.Secret, *corev1.Secret, error) {
 	var (
@@ -408,6 +417,8 @@ func extractS3ConfigSecret(s *corev1.Secret, credentialMode lokiv1.CredentialMod
 		SSE:     sseCfg,
 	}
 
+	err = validateS3Endpoint(string(endpoint), string(region))
+
 	switch credentialMode {
 	case lokiv1.CredentialModeTokenCCO:
 		cfg.STS = true
@@ -416,17 +427,21 @@ func extractS3ConfigSecret(s *corev1.Secret, credentialMode lokiv1.CredentialMod
 		if len(roleArn) != 0 {
 			return nil, fmt.Errorf("%w: %s", errSecretFieldNotAllowed, storage.KeyAWSRoleArn)
 		}
+
 		// In the STS case region is not an optional field
 		if len(region) == 0 {
 			return nil, fmt.Errorf("%w: %s", errSecretMissingField, storage.KeyAWSRegion)
+		} else if len(endpoint) != 0 && err != nil {
+			return nil, err
 		}
-
 		return cfg, nil
 	case lokiv1.CredentialModeStatic:
 		cfg.Endpoint = string(endpoint)
 
 		if len(endpoint) == 0 {
 			return nil, fmt.Errorf("%w: %s", errSecretMissingField, storage.KeyAWSEndpoint)
+		} else if len(region) != 0 && err != nil {
+			return nil, err
 		}
 		if len(id) == 0 {
 			return nil, fmt.Errorf("%w: %s", errSecretMissingField, storage.KeyAWSAccessKeyID)
@@ -443,13 +458,32 @@ func extractS3ConfigSecret(s *corev1.Secret, credentialMode lokiv1.CredentialMod
 		// In the STS case region is not an optional field
 		if len(region) == 0 {
 			return nil, fmt.Errorf("%w: %s", errSecretMissingField, storage.KeyAWSRegion)
+		} else if len(endpoint) != 0 && err != nil {
+			return nil, err
 		}
 		return cfg, nil
 	default:
 		return nil, fmt.Errorf("%w: %s", errSecretUnknownCredentialMode, credentialMode)
 	}
 }
 
+func validateS3Endpoint(endpoint string, region string) error {
+	parsedURL, err := url.Parse(endpoint)
+	if err != nil {
+		return fmt.Errorf("%w: %w", errS3EndpointUnparseable, err)
+	}
+	if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" {
+		return fmt.Errorf("%w: %s", errS3EndpointUnsupportedScheme, parsedURL.Scheme)
+	}
+	if strings.HasSuffix(endpoint, awsEndpointSuffix) {
+		validEndpoint := fmt.Sprintf("https://s3.%s%s", region, awsEndpointSuffix)
+		if endpoint != validEndpoint {
+			return fmt.Errorf("%w: %s", errS3EndpointAWSInvalid, validEndpoint)
+		}
+	}
+	return nil
+}
+
 func extractS3SSEConfig(d map[string][]byte) (storage.S3SSEConfig, error) {
 	var (
 		sseType                    storage.S3SSEType

operator/internal/handlers/internal/storage/secrets_test.go

@@ -392,7 +392,7 @@ func TestS3Extract(t *testing.T) {
 			name: "missing access_key_id",
 			secret: &corev1.Secret{
 				Data: map[string][]byte{
-					"endpoint":    []byte("here"),
+					"endpoint":    []byte("https://s3.REGION.amazonaws.com"),
 					"bucketnames": []byte("this,that"),
 				},
 			},
@@ -402,7 +402,7 @@ func TestS3Extract(t *testing.T) {
 			name: "missing access_key_secret",
 			secret: &corev1.Secret{
 				Data: map[string][]byte{
-					"endpoint":      []byte("here"),
+					"endpoint":      []byte("https://s3.REGION.amazonaws.com"),
 					"bucketnames":   []byte("this,that"),
 					"access_key_id": []byte("id"),
 				},
@@ -413,7 +413,7 @@ func TestS3Extract(t *testing.T) {
 			name: "unsupported SSE type",
 			secret: &corev1.Secret{
 				Data: map[string][]byte{
-					"endpoint":          []byte("here"),
+					"endpoint":          []byte("https://s3.REGION.amazonaws.com"),
 					"bucketnames":       []byte("this,that"),
 					"access_key_id":     []byte("id"),
 					"access_key_secret": []byte("secret"),
@@ -426,7 +426,7 @@ func TestS3Extract(t *testing.T) {
 			name: "missing SSE-KMS kms_key_id",
 			secret: &corev1.Secret{
 				Data: map[string][]byte{
-					"endpoint":                   []byte("here"),
+					"endpoint":                   []byte("https://s3.REGION.amazonaws.com"),
 					"bucketnames":                []byte("this,that"),
 					"access_key_id":              []byte("id"),
 					"access_key_secret":          []byte("secret"),
@@ -441,7 +441,7 @@ func TestS3Extract(t *testing.T) {
 			secret: &corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{Name: "test"},
 				Data: map[string][]byte{
-					"endpoint":          []byte("here"),
+					"endpoint":          []byte("https://s3.REGION.amazonaws.com"),
 					"bucketnames":       []byte("this,that"),
 					"access_key_id":     []byte("id"),
 					"access_key_secret": []byte("secret"),
@@ -456,7 +456,7 @@ func TestS3Extract(t *testing.T) {
 			secret: &corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{Name: "test"},
 				Data: map[string][]byte{
-					"endpoint":                   []byte("here"),
+					"endpoint":                   []byte("https://s3.REGION.amazonaws.com"),
 					"bucketnames":                []byte("this,that"),
 					"access_key_id":              []byte("id"),
 					"access_key_secret":          []byte("secret"),
@@ -472,7 +472,7 @@ func TestS3Extract(t *testing.T) {
 			secret: &corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{Name: "test"},
 				Data: map[string][]byte{
-					"endpoint":          []byte("here"),
+					"endpoint":          []byte("https://s3.REGION.amazonaws.com"),
 					"bucketnames":       []byte("this,that"),
 					"access_key_id":     []byte("id"),
 					"access_key_secret": []byte("secret"),
@@ -486,7 +486,7 @@ func TestS3Extract(t *testing.T) {
 			secret: &corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{Name: "test"},
 				Data: map[string][]byte{
-					"endpoint":          []byte("here"),
+					"endpoint":          []byte("https://s3.REGION.amazonaws.com"),
 					"bucketnames":       []byte("this,that"),
 					"access_key_id":     []byte("id"),
 					"access_key_secret": []byte("secret"),
@@ -530,6 +530,48 @@ func TestS3Extract(t *testing.T) {
 			},
 			wantCredentialMode: lokiv1.CredentialModeToken,
 		},
+		{
+			name: "endpoint url missing http/https",
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Name: "test"},
+				Data: map[string][]byte{
+					"endpoint":          []byte("invalid"),
+					"region":            []byte("region"),
+					"bucketnames":       []byte("this,that"),
+					"access_key_id":     []byte("id"),
+					"access_key_secret": []byte("secret"),
+				},
+			},
+			wantError: "scheme of S3 endpoint URL is unsupported: ",
+		},
+		{
+			name: "s3 region used in endpoint URL is incorrect",
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Name: "test"},
+				Data: map[string][]byte{
+					"endpoint":          []byte("http://s3.wrong.amazonaws.com"),
+					"region":            []byte("region"),
+					"bucketnames":       []byte("this,that"),
+					"access_key_id":     []byte("id"),
+					"access_key_secret": []byte("secret"),
+				},
+			},
+			wantError: "endpoint for AWS S3 must include correct region: https://s3.region.amazonaws.com",
+		},
+		{
+			name: "s3 endpoint format is not a valid s3 URL",
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Name: "test"},
+				Data: map[string][]byte{
+					"endpoint":          []byte("http://region.amazonaws.com"),
+					"region":            []byte("region"),
+					"bucketnames":       []byte("this,that"),
+					"access_key_id":     []byte("id"),
+					"access_key_secret": []byte("secret"),
+				},
+			},
+			wantError: "endpoint for AWS S3 must include correct region: https://s3.region.amazonaws.com",
+		},
 	}
 	for _, tst := range table {
 		tst := tst

operator/internal/handlers/internal/storage/storage_test.go

@@ -39,7 +39,7 @@ var (
 			Namespace: "some-ns",
 		},
 		Data: map[string][]byte{
-			"endpoint":          []byte("s3://your-endpoint"),
+			"endpoint":          []byte("https://s3.a-region.amazonaws.com"),
 			"region":            []byte("a-region"),
 			"bucketnames":       []byte("bucket1,bucket2"),
 			"access_key_id":     []byte("a-secret-id"),

operator/internal/handlers/lokistack_create_or_update_test.go

@@ -53,7 +53,7 @@ var (
 			Namespace: "some-ns",
 		},
 		Data: map[string][]byte{
-			"endpoint":          []byte("s3://your-endpoint"),
+			"endpoint":          []byte("https://s3.a-region.amazonaws.com"),
 			"region":            []byte("a-region"),
 			"bucketnames":       []byte("bucket1,bucket2"),
 			"access_key_id":     []byte("a-secret-id"),