From f9350d6415d45c3cc2f9c0b4f7cd6f8f219907f2 Mon Sep 17 00:00:00 2001 From: Bayan Taani <86984560+btaani@users.noreply.github.com> Date: Fri, 5 Apr 2024 16:46:16 +0200 Subject: [PATCH] fix(operator): Improve validation of provided S3 storage configuration (#12181) Co-authored-by: Robert Jacob Co-authored-by: Joao Marcal --- operator/CHANGELOG.md | 1 + .../handlers/internal/storage/secrets.go | 50 +++++++++++- .../handlers/internal/storage/secrets_test.go | 79 +++++++++++++++++-- .../handlers/internal/storage/storage_test.go | 2 +- .../lokistack_create_or_update_test.go | 2 +- 5 files changed, 120 insertions(+), 14 deletions(-) diff --git a/operator/CHANGELOG.md b/operator/CHANGELOG.md index a6f7cae8bab2..ef3d8ccc0cdb 100644 --- a/operator/CHANGELOG.md +++ b/operator/CHANGELOG.md @@ -1,5 +1,6 @@ ## Main +- [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 - [12333](https://github.com/grafana/loki/pull/12333) **periklis**: Bump max OpenShift version to next release diff --git a/operator/internal/handlers/internal/storage/secrets.go b/operator/internal/handlers/internal/storage/secrets.go index 570415236a40..7188216311df 100644 --- a/operator/internal/handlers/internal/storage/secrets.go +++ b/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,11 @@ 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("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") + 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 +56,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 ( @@ -416,17 +426,17 @@ 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) } - return cfg, nil case lokiv1.CredentialModeStatic: cfg.Endpoint = string(endpoint) - if len(endpoint) == 0 { - return nil, fmt.Errorf("%w: %s", errSecretMissingField, storage.KeyAWSEndpoint) + if err := validateS3Endpoint(string(endpoint), string(region)); err != nil { + return nil, err } if len(id) == 0 { return nil, fmt.Errorf("%w: %s", errSecretMissingField, storage.KeyAWSAccessKeyID) @@ -450,6 +460,38 @@ func extractS3ConfigSecret(s *corev1.Secret, credentialMode lokiv1.CredentialMod } } +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 extractS3SSEConfig(d map[string][]byte) (storage.S3SSEConfig, error) { var ( sseType storage.S3SSEType diff --git a/operator/internal/handlers/internal/storage/secrets_test.go b/operator/internal/handlers/internal/storage/secrets_test.go index 80b8b48dd241..a85e2f6911d6 100644 --- a/operator/internal/handlers/internal/storage/secrets_test.go +++ b/operator/internal/handlers/internal/storage/secrets_test.go @@ -392,7 +392,8 @@ func TestS3Extract(t *testing.T) { name: "missing access_key_id", secret: &corev1.Secret{ Data: map[string][]byte{ - "endpoint": []byte("here"), + "endpoint": []byte("https://s3.test-region.amazonaws.com"), + "region": []byte("test-region"), "bucketnames": []byte("this,that"), }, }, @@ -402,7 +403,8 @@ func TestS3Extract(t *testing.T) { name: "missing access_key_secret", secret: &corev1.Secret{ Data: map[string][]byte{ - "endpoint": []byte("here"), + "endpoint": []byte("https://s3.test-region.amazonaws.com"), + "region": []byte("test-region"), "bucketnames": []byte("this,that"), "access_key_id": []byte("id"), }, @@ -413,7 +415,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 +428,8 @@ 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.test-region.amazonaws.com"), + "region": []byte("test-region"), "bucketnames": []byte("this,that"), "access_key_id": []byte("id"), "access_key_secret": []byte("secret"), @@ -441,7 +444,8 @@ func TestS3Extract(t *testing.T) { secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{Name: "test"}, Data: map[string][]byte{ - "endpoint": []byte("here"), + "endpoint": []byte("https://s3.test-region.amazonaws.com"), + "region": []byte("test-region"), "bucketnames": []byte("this,that"), "access_key_id": []byte("id"), "access_key_secret": []byte("secret"), @@ -456,7 +460,8 @@ func TestS3Extract(t *testing.T) { secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{Name: "test"}, Data: map[string][]byte{ - "endpoint": []byte("here"), + "endpoint": []byte("https://s3.test-region.amazonaws.com"), + "region": []byte("test-region"), "bucketnames": []byte("this,that"), "access_key_id": []byte("id"), "access_key_secret": []byte("secret"), @@ -472,7 +477,8 @@ func TestS3Extract(t *testing.T) { secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{Name: "test"}, Data: map[string][]byte{ - "endpoint": []byte("here"), + "endpoint": []byte("https://s3.test-region.amazonaws.com"), + "region": []byte("test-region"), "bucketnames": []byte("this,that"), "access_key_id": []byte("id"), "access_key_secret": []byte("secret"), @@ -486,7 +492,8 @@ func TestS3Extract(t *testing.T) { secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{Name: "test"}, Data: map[string][]byte{ - "endpoint": []byte("here"), + "endpoint": []byte("https://s3.test-region.amazonaws.com"), + "region": []byte("test-region"), "bucketnames": []byte("this,that"), "access_key_id": []byte("id"), "access_key_secret": []byte("secret"), @@ -530,6 +537,62 @@ func TestS3Extract(t *testing.T) { }, wantCredentialMode: lokiv1.CredentialModeToken, }, + { + name: "endpoint is just hostname", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + 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"), + }, + }, + wantError: "endpoint for S3 must be an HTTP or HTTPS URL", + }, + { + name: "endpoint unsupported scheme", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + 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"), + }, + }, + wantError: "scheme of S3 endpoint URL is unsupported: invalid", + }, + { + name: "s3 region used in endpoint URL is incorrect", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + 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"), + }, + }, + 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 diff --git a/operator/internal/handlers/internal/storage/storage_test.go b/operator/internal/handlers/internal/storage/storage_test.go index 3e805c60a5be..857d0be4de4a 100644 --- a/operator/internal/handlers/internal/storage/storage_test.go +++ b/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"), diff --git a/operator/internal/handlers/lokistack_create_or_update_test.go b/operator/internal/handlers/lokistack_create_or_update_test.go index bef5ffc9efb7..c7677e49a05a 100644 --- a/operator/internal/handlers/lokistack_create_or_update_test.go +++ b/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"),