Skip to content

Commit

Permalink
fix(operator): Improve validation of provided S3 storage configuration (
Browse files Browse the repository at this point in the history 
#12181)

Co-authored-by: Robert Jacob <rojacob@redhat.com>
Co-authored-by: Joao Marcal <joao.marcal12@gmail.com>
  • Loading branch information
@btaani @xperimental @JoaoBraveCoding
3 people committed Apr 5, 2024
1 parent 705379c commit f9350d6
Show file tree
Hide file tree
Showing 5 changed files with 120 additions and 14 deletions.
1 change: 1 addition & 0 deletions operator/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -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

Expand Down
50 changes: 46 additions & 4 deletions operator/internal/handlers/internal/storage/secrets.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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")

Expand All @@ -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 (
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down
79 changes: 71 additions & 8 deletions operator/internal/handlers/internal/storage/secrets_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
},
},
Expand All @@ -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"),
},
Expand All @@ -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"),
Expand All @@ -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"),
Expand All @@ -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"),
Expand All @@ -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"),
Expand All @@ -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"),
Expand All @@ -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"),
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion operator/internal/handlers/internal/storage/storage_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down
2 changes: 1 addition & 1 deletion operator/internal/handlers/lokistack_create_or_update_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down

0 comments on commit f9350d6

Please sign in to comment.