Merge pull request grafana#285 from btaani/LOG-5393

[release-5.7] Improve validation of provided S3 storage configuration

operator/CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## Release 5.7.13
 
+- [12181](https://github.com/grafana/loki/pull/12181) **btaani**: Improve validation of provided S3 storage configuration
 - [12370](https://github.com/grafana/loki/pull/12370) **periklis**: Update Loki operand to v2.9.6
 
 ## Release 5.7.12

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

@@ -5,7 +5,9 @@ import (
 	"crypto/sha1"
 	"errors"
 	"fmt"
+	"net/url"
 	"sort"
+	"strings"
 
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -23,8 +25,15 @@ var (
 	errSecretUnknownType  = errors.New("unknown secret type")
 	errSecretMissingField = errors.New("missing secret field")
 	errSecretHashError    = errors.New("error calculating hash for secret")
+
+	errS3EndpointUnparseable       = errors.New("can not parse S3 endpoint as URL")
+	errS3EndpointNoURL             = errors.New("endpoint for S3 must be an HTTP or HTTPS URL")
+	errS3EndpointUnsupportedScheme = errors.New("scheme of S3 endpoint URL is unsupported")
+	errS3EndpointAWSInvalid        = errors.New("endpoint for AWS S3 must include correct region")
 )
 
+const awsEndpointSuffix = ".amazonaws.com"
+
 func getSecret(ctx context.Context, k k8s.Client, stack *lokiv1.LokiStack) (*corev1.Secret, error) {
 	var storageSecret corev1.Secret
 	key := client.ObjectKey{Name: stack.Spec.Storage.Secret.Name, Namespace: stack.Namespace}
@@ -152,27 +161,65 @@ func extractS3ConfigSecret(s *corev1.Secret) (*storage.S3StorageConfig, error) {
 	if len(buckets) == 0 {
 		return nil, fmt.Errorf("%w: %s", errSecretMissingField, storage.KeyAWSBucketNames)
 	}
-	endpoint := s.Data[storage.KeyAWSEndpoint]
-	if len(endpoint) == 0 {
-		return nil, fmt.Errorf("%w: %s", errSecretMissingField, storage.KeyAWSEndpoint)
+
+	var (
+		// Fields related with static authentication
+		endpoint = s.Data[storage.KeyAWSEndpoint]
+		id       = s.Data[storage.KeyAWSAccessKeyID]
+		secret   = s.Data[storage.KeyAWSAccessKeySecret]
+		// Optional fields
+		region = s.Data[storage.KeyAWSRegion]
+	)
+
+	cfg := &storage.S3StorageConfig{
+		Buckets: string(buckets),
+		Region:  string(region),
+	}
+	cfg.Endpoint = string(endpoint)
+
+	if err := validateS3Endpoint(string(endpoint), string(region)); err != nil {
+		return nil, err
 	}
-	id := s.Data[storage.KeyAWSAccessKeyID]
 	if len(id) == 0 {
 		return nil, fmt.Errorf("%w: %s", errSecretMissingField, storage.KeyAWSAccessKeyID)
 	}
-	secret := s.Data[storage.KeyAWSAccessKeySecret]
 	if len(secret) == 0 {
 		return nil, fmt.Errorf("%w: %s", errSecretMissingField, storage.KeyAWSAccessKeySecret)
 	}
 
-	// Extract and validate optional fields
-	region := s.Data[storage.KeyAWSRegion]
+	return cfg, nil
+}
 
-	return &storage.S3StorageConfig{
-		Endpoint: string(endpoint),
-		Buckets:  string(buckets),
-		Region:   string(region),
-	}, nil
+func validateS3Endpoint(endpoint string, region string) error {
+	if len(endpoint) == 0 {
+		return fmt.Errorf("%w: %s", errSecretMissingField, storage.KeyAWSEndpoint)
+	}
+
+	parsedURL, err := url.Parse(endpoint)
+	if err != nil {
+		return fmt.Errorf("%w: %w", errS3EndpointUnparseable, err)
+	}
+
+	if parsedURL.Scheme == "" {
+		// Assume "just a hostname" when scheme is empty and produce a clearer error message
+		return errS3EndpointNoURL
+	}
+
+	if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" {
+		return fmt.Errorf("%w: %s", errS3EndpointUnsupportedScheme, parsedURL.Scheme)
+	}
+
+	if strings.HasSuffix(endpoint, awsEndpointSuffix) {
+		if len(region) == 0 {
+			return fmt.Errorf("%w: %s", errSecretMissingField, storage.KeyAWSRegion)
+		}
+
+		validEndpoint := fmt.Sprintf("https://s3.%s%s", region, awsEndpointSuffix)
+		if endpoint != validEndpoint {
+			return fmt.Errorf("%w: %s", errS3EndpointAWSInvalid, validEndpoint)
+		}
+	}
+	return nil
 }
 
 func extractSwiftConfigSecret(s *corev1.Secret) (*storage.SwiftStorageConfig, error) {

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

@@ -191,52 +191,109 @@ func TestGCSExtract(t *testing.T) {
 
 func TestS3Extract(t *testing.T) {
 	type test struct {
-		name      string
-		secret    *corev1.Secret
-		wantError string
+		name    string
+		secret  *corev1.Secret
+		wantErr string
 	}
 	table := []test{
-		{
-			name:      "missing bucketnames",
-			secret:    &corev1.Secret{},
-			wantError: "missing secret field: bucketnames",
-		},
 		{
 			name: "missing endpoint",
 			secret: &corev1.Secret{
 				Data: map[string][]byte{
 					"bucketnames": []byte("this,that"),
 				},
 			},
-			wantError: "missing secret field: endpoint",
+			wantErr: "missing secret field: endpoint",
+		},
+		{
+			name: "missing bucketnames",
+			secret: &corev1.Secret{
+				Data: map[string][]byte{
+					"endpoint": []byte("http://here"),
+				},
+			},
+			wantErr: "missing secret field: bucketnames",
 		},
 		{
 			name: "missing access_key_id",
 			secret: &corev1.Secret{
 				Data: map[string][]byte{
-					"endpoint":    []byte("here"),
+					"endpoint":    []byte("https://here"),
 					"bucketnames": []byte("this,that"),
 				},
 			},
-			wantError: "missing secret field: access_key_id",
+			wantErr: "missing secret field: access_key_id",
 		},
 		{
 			name: "missing access_key_secret",
 			secret: &corev1.Secret{
 				Data: map[string][]byte{
-					"endpoint":      []byte("here"),
+					"endpoint":      []byte("https://here"),
 					"bucketnames":   []byte("this,that"),
 					"access_key_id": []byte("id"),
 				},
 			},
-			wantError: "missing secret field: access_key_secret",
+			wantErr: "missing secret field: access_key_secret",
+		},
+		{
+			name: "endpoint is just hostname",
+			secret: &corev1.Secret{
+				Data: map[string][]byte{
+					"endpoint":          []byte("hostname.example.com"),
+					"region":            []byte("region"),
+					"bucketnames":       []byte("this,that"),
+					"access_key_id":     []byte("id"),
+					"access_key_secret": []byte("secret"),
+				},
+			},
+			wantErr: "endpoint for S3 must be an HTTP or HTTPS URL",
+		},
+		{
+			name: "endpoint unsupported scheme",
+			secret: &corev1.Secret{
+				Data: map[string][]byte{
+					"endpoint":          []byte("invalid://hostname"),
+					"region":            []byte("region"),
+					"bucketnames":       []byte("this,that"),
+					"access_key_id":     []byte("id"),
+					"access_key_secret": []byte("secret"),
+				},
+			},
+			wantErr: "scheme of S3 endpoint URL is unsupported: invalid",
+		},
+		{
+			name: "s3 region used in endpoint URL is incorrect",
+			secret: &corev1.Secret{
+				Data: map[string][]byte{
+					"endpoint":          []byte("https://s3.wrong.amazonaws.com"),
+					"region":            []byte("region"),
+					"bucketnames":       []byte("this,that"),
+					"access_key_id":     []byte("id"),
+					"access_key_secret": []byte("secret"),
+				},
+			},
+			wantErr: "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{
+				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"),
+				},
+			},
+			wantErr: "endpoint for AWS S3 must include correct region: https://s3.region.amazonaws.com",
 		},
 		{
 			name: "all set",
 			secret: &corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{Name: "test"},
 				Data: map[string][]byte{
-					"endpoint":          []byte("here"),
+					"endpoint":          []byte("https://s3.region.amazonaws.com"),
+					"region":            []byte("region"),
 					"bucketnames":       []byte("this,that"),
 					"access_key_id":     []byte("id"),
 					"access_key_secret": []byte("secret"),
@@ -250,13 +307,13 @@ func TestS3Extract(t *testing.T) {
 			t.Parallel()
 
 			opts, err := extractSecret(tst.secret, lokiv1.ObjectStorageSecretS3)
-			if tst.wantError == "" {
+			if tst.wantErr == "" {
 				require.NoError(t, err)
 				require.NotEmpty(t, opts.SecretName)
 				require.NotEmpty(t, opts.SecretSHA1)
 				require.Equal(t, opts.SharedStore, lokiv1.ObjectStorageSecretS3)
 			} else {
-				require.EqualError(t, err, tst.wantError)
+				require.EqualError(t, err, tst.wantErr)
 			}
 		})
 	}

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

@@ -38,7 +38,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

@@ -54,7 +54,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"),