diff --git a/internal/controller/bucket_controller.go b/internal/controller/bucket_controller.go index d67c10f9..7852d196 100644 --- a/internal/controller/bucket_controller.go +++ b/internal/controller/bucket_controller.go @@ -18,7 +18,7 @@ package controller import ( "context" - stdtls "crypto/tls" + "crypto/tls" "errors" "fmt" "net/url" @@ -50,6 +50,7 @@ import ( "github.com/fluxcd/pkg/runtime/patch" "github.com/fluxcd/pkg/runtime/predicates" rreconcile "github.com/fluxcd/pkg/runtime/reconcile" + "github.com/fluxcd/pkg/runtime/secrets" "github.com/fluxcd/pkg/sourceignore" sourcev1 "github.com/fluxcd/source-controller/api/v1" @@ -58,7 +59,6 @@ import ( "github.com/fluxcd/source-controller/internal/index" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" "github.com/fluxcd/source-controller/internal/reconcile/summarize" - "github.com/fluxcd/source-controller/internal/tls" "github.com/fluxcd/source-controller/pkg/azure" "github.com/fluxcd/source-controller/pkg/gcp" "github.com/fluxcd/source-controller/pkg/minio" @@ -155,6 +155,15 @@ type BucketProvider interface { Close(context.Context) } +// bucketCredentials contains all credentials and configuration needed for bucket providers. +type bucketCredentials struct { + secret *corev1.Secret + proxyURL *url.URL + tlsConfig *tls.Config + stsSecret *corev1.Secret + stsTLSConfig *tls.Config +} + // bucketReconcileFunc is the function type for all the v1.Bucket // (sub)reconcile functions. The type implementations are grouped and // executed serially to perform the complete reconcile of the object. @@ -421,162 +430,47 @@ func (r *BucketReconciler) reconcileStorage(ctx context.Context, sp *patch.Seria // the provider. If this fails, it records v1.FetchFailedCondition=True on // the object and returns early. func (r *BucketReconciler) reconcileSource(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.Bucket, index *index.Digester, dir string) (sreconcile.Result, error) { - secret, err := r.getSecret(ctx, obj.Spec.SecretRef, obj.GetNamespace()) - if err != nil { - e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - // Return error as the world as observed may change - return sreconcile.ResultEmpty, e - } - proxyURL, err := r.getProxyURL(ctx, obj) + creds, err := r.setupCredentials(ctx, obj) if err != nil { e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) return sreconcile.ResultEmpty, e } - // Construct provider client - var provider BucketProvider - switch obj.Spec.Provider { - case sourcev1.BucketProviderGoogle: - if err = gcp.ValidateSecret(secret); err != nil { + provider, err := r.createBucketProvider(ctx, obj, creds) + if err != nil { + var stallingErr *serror.Stalling + var genericErr *serror.Generic + if errors.As(err, &stallingErr) { + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, stallingErr.Reason, "%s", stallingErr) + return sreconcile.ResultEmpty, stallingErr + } else if errors.As(err, &genericErr) { + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, genericErr.Reason, "%s", genericErr) + return sreconcile.ResultEmpty, genericErr + } else { e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) return sreconcile.ResultEmpty, e } - var opts []gcp.Option - if secret != nil { - opts = append(opts, gcp.WithSecret(secret)) - } - if proxyURL != nil { - opts = append(opts, gcp.WithProxyURL(proxyURL)) - } - if provider, err = gcp.NewClient(ctx, opts...); err != nil { - e := serror.NewGeneric(err, "ClientError") - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } - case sourcev1.BucketProviderAzure: - if err = azure.ValidateSecret(secret); err != nil { - e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } - var opts []azure.Option - if secret != nil { - opts = append(opts, azure.WithSecret(secret)) - } - if proxyURL != nil { - opts = append(opts, azure.WithProxyURL(proxyURL)) - } - if provider, err = azure.NewClient(obj, opts...); err != nil { - e := serror.NewGeneric(err, "ClientError") - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } - default: - if err = minio.ValidateSecret(secret); err != nil { - e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } - tlsConfig, err := r.getTLSConfig(ctx, obj.Spec.CertSecretRef, obj.GetNamespace(), obj.Spec.Endpoint) - if err != nil { - e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } - stsSecret, err := r.getSTSSecret(ctx, obj) - if err != nil { - e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } - stsTLSConfig, err := r.getSTSTLSConfig(ctx, obj) - if err != nil { - err := fmt.Errorf("failed to get STS TLS config: %w", err) - e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } - if sts := obj.Spec.STS; sts != nil { - if err := minio.ValidateSTSProvider(obj.Spec.Provider, sts); err != nil { - e := serror.NewStalling(err, sourcev1.InvalidSTSConfigurationReason) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } - if _, err := url.Parse(sts.Endpoint); err != nil { - err := fmt.Errorf("failed to parse STS endpoint '%s': %w", sts.Endpoint, err) - e := serror.NewStalling(err, sourcev1.URLInvalidReason) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } - if err := minio.ValidateSTSSecret(sts.Provider, stsSecret); err != nil { - e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } - } - var opts []minio.Option - if secret != nil { - opts = append(opts, minio.WithSecret(secret)) - } - if tlsConfig != nil { - opts = append(opts, minio.WithTLSConfig(tlsConfig)) - } - if proxyURL != nil { - opts = append(opts, minio.WithProxyURL(proxyURL)) - } - if stsSecret != nil { - opts = append(opts, minio.WithSTSSecret(stsSecret)) - } - if stsTLSConfig != nil { - opts = append(opts, minio.WithSTSTLSConfig(stsTLSConfig)) - } - if provider, err = minio.NewClient(obj, opts...); err != nil { - e := serror.NewGeneric(err, "ClientError") - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e - } } - - // Fetch etag index - if err = fetchEtagIndex(ctx, provider, obj, index, dir); err != nil { + changed, err := r.syncBucketArtifacts(ctx, provider, obj, index, dir) + if err != nil { e := serror.NewGeneric(err, sourcev1.BucketOperationFailedReason) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) return sreconcile.ResultEmpty, e } - // Check if index has changed compared to current Artifact revision. - var changed bool - if artifact := obj.Status.Artifact; artifact != nil && artifact.Revision != "" { - curRev := digest.Digest(artifact.Revision) - changed = curRev.Validate() != nil || curRev != index.Digest(curRev.Algorithm()) - } - - // Fetch the bucket objects if required to. - if artifact := obj.GetArtifact(); artifact == nil || changed { - // Mark observations about the revision on the object - defer func() { - // As fetchIndexFiles can make last-minute modifications to the etag - // index, we need to re-calculate the revision at the end - revision := index.Digest(intdigest.Canonical) - - message := fmt.Sprintf("new upstream revision '%s'", revision) - if obj.GetArtifact() != nil { - conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "%s", message) - } - rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, "building artifact: %s", message) - if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { - ctrl.LoggerFrom(ctx).Error(err, "failed to patch") - return - } - }() - - if err = fetchIndexFiles(ctx, provider, obj, index, dir); err != nil { - e := serror.NewGeneric(err, sourcev1.BucketOperationFailedReason) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return sreconcile.ResultEmpty, e + // Update artifact status if changes were detected + if changed { + revision := index.Digest(intdigest.Canonical) + message := fmt.Sprintf("new upstream revision '%s'", revision) + if obj.GetArtifact() != nil { + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "%s", message) + } + rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, "building artifact: %s", message) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + ctrl.LoggerFrom(ctx).Error(err, "failed to patch") + return sreconcile.ResultEmpty, err } } @@ -736,85 +630,6 @@ func (r *BucketReconciler) garbageCollect(ctx context.Context, obj *sourcev1.Buc return nil } -// getSecret attempts to fetch a Secret reference if specified. It returns any client error. -func (r *BucketReconciler) getSecret(ctx context.Context, secretRef *meta.LocalObjectReference, - namespace string) (*corev1.Secret, error) { - if secretRef == nil { - return nil, nil - } - secretName := types.NamespacedName{ - Namespace: namespace, - Name: secretRef.Name, - } - secret := &corev1.Secret{} - if err := r.Get(ctx, secretName, secret); err != nil { - return nil, fmt.Errorf("failed to get secret '%s': %w", secretName.String(), err) - } - return secret, nil -} - -// getTLSConfig attempts to fetch a TLS configuration from the given -// Secret reference, namespace and endpoint. -func (r *BucketReconciler) getTLSConfig(ctx context.Context, - secretRef *meta.LocalObjectReference, namespace, endpoint string) (*stdtls.Config, error) { - certSecret, err := r.getSecret(ctx, secretRef, namespace) - if err != nil || certSecret == nil { - return nil, err - } - tlsConfig, _, err := tls.KubeTLSClientConfigFromSecret(*certSecret, endpoint) - if err != nil { - return nil, fmt.Errorf("failed to create TLS config: %w", err) - } - if tlsConfig == nil { - return nil, fmt.Errorf("certificate secret does not contain any TLS configuration") - } - return tlsConfig, nil -} - -// getProxyURL attempts to fetch a proxy URL from the object's proxy secret -// reference. -func (r *BucketReconciler) getProxyURL(ctx context.Context, obj *sourcev1.Bucket) (*url.URL, error) { - namespace := obj.GetNamespace() - proxySecret, err := r.getSecret(ctx, obj.Spec.ProxySecretRef, namespace) - if err != nil || proxySecret == nil { - return nil, err - } - proxyData := proxySecret.Data - address, ok := proxyData["address"] - if !ok { - return nil, fmt.Errorf("invalid proxy secret '%s/%s': key 'address' is missing", - namespace, obj.Spec.ProxySecretRef.Name) - } - proxyURL, err := url.Parse(string(address)) - if err != nil { - return nil, fmt.Errorf("failed to parse proxy address '%s': %w", address, err) - } - user, hasUser := proxyData["username"] - password, hasPassword := proxyData["password"] - if hasUser || hasPassword { - proxyURL.User = url.UserPassword(string(user), string(password)) - } - return proxyURL, nil -} - -// getSTSSecret attempts to fetch the secret from the object's STS secret -// reference. -func (r *BucketReconciler) getSTSSecret(ctx context.Context, obj *sourcev1.Bucket) (*corev1.Secret, error) { - if obj.Spec.STS == nil { - return nil, nil - } - return r.getSecret(ctx, obj.Spec.STS.SecretRef, obj.GetNamespace()) -} - -// getSTSTLSConfig attempts to fetch the certificate secret from the object's -// STS configuration. -func (r *BucketReconciler) getSTSTLSConfig(ctx context.Context, obj *sourcev1.Bucket) (*stdtls.Config, error) { - if obj.Spec.STS == nil { - return nil, nil - } - return r.getTLSConfig(ctx, obj.Spec.STS.CertSecretRef, obj.GetNamespace(), obj.Spec.STS.Endpoint) -} - // eventLogf records events, and logs at the same time. // // This log is different from the debug log in the EventRecorder, in the sense @@ -943,3 +758,168 @@ func fetchIndexFiles(ctx context.Context, provider BucketProvider, obj *sourcev1 return nil } + +// setupCredentials retrieves and validates secrets for authentication, TLS configuration, and proxy settings. +// It returns all credentials needed for bucket providers. +func (r *BucketReconciler) setupCredentials(ctx context.Context, obj *sourcev1.Bucket) (*bucketCredentials, error) { + var secret *corev1.Secret + if obj.Spec.SecretRef != nil { + secretName := types.NamespacedName{ + Namespace: obj.GetNamespace(), + Name: obj.Spec.SecretRef.Name, + } + secret = &corev1.Secret{} + if err := r.Get(ctx, secretName, secret); err != nil { + return nil, fmt.Errorf("failed to get secret '%s': %w", secretName, err) + } + } + + var stsSecret *corev1.Secret + if obj.Spec.STS != nil && obj.Spec.STS.SecretRef != nil { + secretName := types.NamespacedName{ + Namespace: obj.GetNamespace(), + Name: obj.Spec.STS.SecretRef.Name, + } + stsSecret = &corev1.Secret{} + if err := r.Get(ctx, secretName, stsSecret); err != nil { + return nil, fmt.Errorf("failed to get STS secret '%s': %w", secretName, err) + } + } + + var ( + err error + proxyURL *url.URL + tlsConfig *tls.Config + stsTLSConfig *tls.Config + ) + + if obj.Spec.ProxySecretRef != nil { + secretRef := types.NamespacedName{ + Namespace: obj.GetNamespace(), + Name: obj.Spec.ProxySecretRef.Name, + } + proxyURL, err = secrets.ProxyURLFromSecretRef(ctx, r.Client, secretRef) + if err != nil { + return nil, fmt.Errorf("failed to get proxy URL: %w", err) + } + } + + if obj.Spec.CertSecretRef != nil { + secretRef := types.NamespacedName{ + Namespace: obj.GetNamespace(), + Name: obj.Spec.CertSecretRef.Name, + } + tlsConfig, err = secrets.TLSConfigFromSecretRef(ctx, r.Client, secretRef, obj.Spec.Endpoint, secrets.WithSystemCertPool()) + if err != nil { + return nil, fmt.Errorf("failed to get TLS config: %w", err) + } + } + + if obj.Spec.STS != nil && obj.Spec.STS.CertSecretRef != nil { + secretRef := types.NamespacedName{ + Namespace: obj.GetNamespace(), + Name: obj.Spec.STS.CertSecretRef.Name, + } + stsTLSConfig, err = secrets.TLSConfigFromSecretRef(ctx, r.Client, secretRef, obj.Spec.STS.Endpoint, secrets.WithSystemCertPool()) + if err != nil { + return nil, fmt.Errorf("failed to get STS TLS config: %w", err) + } + } + + return &bucketCredentials{ + secret: secret, + proxyURL: proxyURL, + tlsConfig: tlsConfig, + stsSecret: stsSecret, + stsTLSConfig: stsTLSConfig, + }, nil +} + +// createBucketProvider creates a provider-specific bucket client using the given credentials and configuration. +// It handles different bucket providers (AWS, GCP, Azure, generic) and returns the appropriate client. +func (r *BucketReconciler) createBucketProvider(ctx context.Context, obj *sourcev1.Bucket, creds *bucketCredentials) (BucketProvider, error) { + switch obj.Spec.Provider { + case sourcev1.BucketProviderGoogle: + if err := gcp.ValidateSecret(creds.secret); err != nil { + return nil, err + } + var opts []gcp.Option + if creds.secret != nil { + opts = append(opts, gcp.WithSecret(creds.secret)) + } + if creds.proxyURL != nil { + opts = append(opts, gcp.WithProxyURL(creds.proxyURL)) + } + return gcp.NewClient(ctx, opts...) + + case sourcev1.BucketProviderAzure: + if err := azure.ValidateSecret(creds.secret); err != nil { + return nil, err + } + var opts []azure.Option + if creds.secret != nil { + opts = append(opts, azure.WithSecret(creds.secret)) + } + if creds.proxyURL != nil { + opts = append(opts, azure.WithProxyURL(creds.proxyURL)) + } + return azure.NewClient(obj, opts...) + + default: + if err := minio.ValidateSecret(creds.secret); err != nil { + return nil, err + } + if sts := obj.Spec.STS; sts != nil { + if err := minio.ValidateSTSProvider(obj.Spec.Provider, sts); err != nil { + return nil, serror.NewStalling(err, sourcev1.InvalidSTSConfigurationReason) + } + if _, err := url.Parse(sts.Endpoint); err != nil { + return nil, serror.NewStalling(fmt.Errorf("failed to parse STS endpoint '%s': %w", sts.Endpoint, err), sourcev1.URLInvalidReason) + } + if err := minio.ValidateSTSSecret(sts.Provider, creds.stsSecret); err != nil { + return nil, serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) + } + } + var opts []minio.Option + if creds.secret != nil { + opts = append(opts, minio.WithSecret(creds.secret)) + } + if creds.tlsConfig != nil { + opts = append(opts, minio.WithTLSConfig(creds.tlsConfig)) + } + if creds.proxyURL != nil { + opts = append(opts, minio.WithProxyURL(creds.proxyURL)) + } + if creds.stsSecret != nil { + opts = append(opts, minio.WithSTSSecret(creds.stsSecret)) + } + if creds.stsTLSConfig != nil { + opts = append(opts, minio.WithSTSTLSConfig(creds.stsTLSConfig)) + } + return minio.NewClient(obj, opts...) + } +} + +// syncBucketArtifacts handles etag index retrieval and bucket object fetching. +// It fetches the etag index from the provider and downloads objects to the specified directory. +// Returns true if changes were detected and artifacts were updated. +func (r *BucketReconciler) syncBucketArtifacts(ctx context.Context, provider BucketProvider, obj *sourcev1.Bucket, index *index.Digester, dir string) (bool, error) { + if err := fetchEtagIndex(ctx, provider, obj, index, dir); err != nil { + return false, err + } + var changed bool + if artifact := obj.Status.Artifact; artifact != nil && artifact.Revision != "" { + curRev := digest.Digest(artifact.Revision) + changed = curRev.Validate() != nil || curRev != index.Digest(curRev.Algorithm()) + } + + // Fetch the bucket objects if required to. + if artifact := obj.GetArtifact(); artifact == nil || changed { + if err := fetchIndexFiles(ctx, provider, obj, index, dir); err != nil { + return false, err + } + return true, nil + } + + return false, nil +} diff --git a/internal/controller/bucket_controller_test.go b/internal/controller/bucket_controller_test.go index 7563d6e9..4114050e 100644 --- a/internal/controller/bucket_controller_test.go +++ b/internal/controller/bucket_controller_test.go @@ -522,7 +522,7 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { wantErr: true, assertIndex: index.NewDigester(), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret '/dummy': secrets \"dummy\" not found"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get TLS config: secret '/dummy' not found"), *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, @@ -547,7 +547,7 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "certificate secret does not contain any TLS configuration"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get TLS config: secret '/dummy' must contain either 'ca.crt' or both 'tls.crt' and 'tls.key'"), }, }, { @@ -563,7 +563,7 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { wantErr: true, assertIndex: index.NewDigester(), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret '/dummy': secrets \"dummy\" not found"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get proxy URL: secret '/dummy' not found"), *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, @@ -575,6 +575,7 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "dummy", }, + Data: map[string][]byte{}, }, beforeFunc: func(obj *sourcev1.Bucket) { obj.Spec.ProxySecretRef = &meta.LocalObjectReference{ @@ -588,7 +589,7 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "invalid proxy secret '/dummy': key 'address' is missing"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get proxy URL: secret '/dummy': key 'address' not found"), }, }, { @@ -604,7 +605,7 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { wantErr: true, assertIndex: index.NewDigester(), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret '/dummy': secrets \"dummy\" not found"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get STS secret '/dummy': secrets \"dummy\" not found"), *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, @@ -648,7 +649,7 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { wantErr: true, assertIndex: index.NewDigester(), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret '/dummy': secrets \"dummy\" not found"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get STS TLS config: secret '/dummy' not found"), *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, @@ -676,7 +677,7 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get STS TLS config: certificate secret does not contain any TLS configuration"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get STS TLS config: secret '/dummy' must contain either 'ca.crt' or both 'tls.crt' and 'tls.key'"), }, }, { @@ -1073,7 +1074,7 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { wantErr: true, assertIndex: index.NewDigester(), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret '/dummy': secrets \"dummy\" not found"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get proxy URL: secret '/dummy' not found"), *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, @@ -1097,7 +1098,7 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { wantErr: true, assertIndex: index.NewDigester(), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "invalid proxy secret '/dummy': key 'address' is missing"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get proxy URL: secret '/dummy': key 'address' not found"), *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, @@ -1503,7 +1504,6 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-bucket-", Generation: 1, - Namespace: "default", }, Spec: sourcev1.BucketSpec{ Timeout: &metav1.Duration{Duration: timeout}, @@ -1751,191 +1751,6 @@ func TestBucketReconciler_notify(t *testing.T) { } } -func TestBucketReconciler_getProxyURL(t *testing.T) { - tests := []struct { - name string - bucket *sourcev1.Bucket - objects []client.Object - expectedURL string - expectedErr string - }{ - { - name: "empty proxySecretRef", - bucket: &sourcev1.Bucket{ - Spec: sourcev1.BucketSpec{ - ProxySecretRef: nil, - }, - }, - }, - { - name: "non-existing proxySecretRef", - bucket: &sourcev1.Bucket{ - Spec: sourcev1.BucketSpec{ - ProxySecretRef: &meta.LocalObjectReference{ - Name: "non-existing", - }, - }, - }, - expectedErr: "failed to get secret '/non-existing': secrets \"non-existing\" not found", - }, - { - name: "missing address in proxySecretRef", - bucket: &sourcev1.Bucket{ - Spec: sourcev1.BucketSpec{ - ProxySecretRef: &meta.LocalObjectReference{ - Name: "dummy", - }, - }, - }, - objects: []client.Object{ - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", - }, - Data: map[string][]byte{}, - }, - }, - expectedErr: "invalid proxy secret '/dummy': key 'address' is missing", - }, - { - name: "invalid address in proxySecretRef", - bucket: &sourcev1.Bucket{ - Spec: sourcev1.BucketSpec{ - ProxySecretRef: &meta.LocalObjectReference{ - Name: "dummy", - }, - }, - }, - objects: []client.Object{ - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", - }, - Data: map[string][]byte{ - "address": {0x7f}, - }, - }, - }, - expectedErr: "failed to parse proxy address '\x7f': parse \"\\x7f\": net/url: invalid control character in URL", - }, - { - name: "no user, no password", - bucket: &sourcev1.Bucket{ - Spec: sourcev1.BucketSpec{ - ProxySecretRef: &meta.LocalObjectReference{ - Name: "dummy", - }, - }, - }, - objects: []client.Object{ - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", - }, - Data: map[string][]byte{ - "address": []byte("http://proxy.example.com"), - }, - }, - }, - expectedURL: "http://proxy.example.com", - }, - { - name: "user, no password", - bucket: &sourcev1.Bucket{ - Spec: sourcev1.BucketSpec{ - ProxySecretRef: &meta.LocalObjectReference{ - Name: "dummy", - }, - }, - }, - objects: []client.Object{ - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", - }, - Data: map[string][]byte{ - "address": []byte("http://proxy.example.com"), - "username": []byte("user"), - }, - }, - }, - expectedURL: "http://user:@proxy.example.com", - }, - { - name: "no user, password", - bucket: &sourcev1.Bucket{ - Spec: sourcev1.BucketSpec{ - ProxySecretRef: &meta.LocalObjectReference{ - Name: "dummy", - }, - }, - }, - objects: []client.Object{ - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", - }, - Data: map[string][]byte{ - "address": []byte("http://proxy.example.com"), - "password": []byte("password"), - }, - }, - }, - expectedURL: "http://:password@proxy.example.com", - }, - { - name: "user, password", - bucket: &sourcev1.Bucket{ - Spec: sourcev1.BucketSpec{ - ProxySecretRef: &meta.LocalObjectReference{ - Name: "dummy", - }, - }, - }, - objects: []client.Object{ - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummy", - }, - Data: map[string][]byte{ - "address": []byte("http://proxy.example.com"), - "username": []byte("user"), - "password": []byte("password"), - }, - }, - }, - expectedURL: "http://user:password@proxy.example.com", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - c := fakeclient.NewClientBuilder(). - WithScheme(testEnv.Scheme()). - WithObjects(tt.objects...). - Build() - - r := &BucketReconciler{ - Client: c, - } - - u, err := r.getProxyURL(ctx, tt.bucket) - if tt.expectedErr == "" { - g.Expect(err).To(BeNil()) - } else { - g.Expect(err.Error()).To(ContainSubstring(tt.expectedErr)) - } - if tt.expectedURL == "" { - g.Expect(u).To(BeNil()) - } else { - g.Expect(u.String()).To(Equal(tt.expectedURL)) - } - }) - } -} - func TestBucketReconciler_APIServerValidation_STS(t *testing.T) { tests := []struct { name string diff --git a/internal/tls/config.go b/internal/tls/config.go deleted file mode 100644 index 841c9538..00000000 --- a/internal/tls/config.go +++ /dev/null @@ -1,161 +0,0 @@ -/* -Copyright 2023 The Flux authors - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package tls - -import ( - "crypto/tls" - "crypto/x509" - "fmt" - neturl "net/url" - - corev1 "k8s.io/api/core/v1" -) - -const CACrtKey = "ca.crt" - -// TLSBytes contains the bytes of the TLS files. -type TLSBytes struct { - // CertBytes is the bytes of the certificate file. - CertBytes []byte - // KeyBytes is the bytes of the key file. - KeyBytes []byte - // CABytes is the bytes of the CA file. - CABytes []byte -} - -// KubeTLSClientConfigFromSecret returns a TLS client config as a `tls.Config` -// object and in its bytes representation. The secret is expected to have the -// following keys: -// - tls.key, for the private key -// - tls.crt, for the certificate -// - ca.crt, for the CA certificate -// -// Secrets with no certificate, private key, AND CA cert are ignored. If only a -// certificate OR private key is found, an error is returned. The Secret type -// can be blank, Opaque or kubernetes.io/tls. -func KubeTLSClientConfigFromSecret(secret corev1.Secret, url string) (*tls.Config, *TLSBytes, error) { - return tlsClientConfigFromSecret(secret, url, true, true) -} - -// TLSClientConfigFromSecret returns a TLS client config as a `tls.Config` -// object and in its bytes representation. The secret is expected to have the -// following keys: -// - keyFile, for the private key -// - certFile, for the certificate -// - caFile, for the CA certificate -// -// Secrets with no certificate, private key, AND CA cert are ignored. If only a -// certificate OR private key is found, an error is returned. The Secret type -// can be blank, Opaque or kubernetes.io/tls. -func TLSClientConfigFromSecret(secret corev1.Secret, url string) (*tls.Config, *TLSBytes, error) { - return tlsClientConfigFromSecret(secret, url, false, true) -} - -// LegacyTLSClientConfigFromSecret returns a TLS client config as a `tls.Config` -// object and in its bytes representation. The secret is expected to have the -// following keys: -// - keyFile, for the private key -// - certFile, for the certificate -// - caFile, for the CA certificate -// -// Secrets with no certificate, private key, AND CA cert are ignored. If only a -// certificate OR private key is found, an error is returned. -func LegacyTLSClientConfigFromSecret(secret corev1.Secret, url string) (*tls.Config, *TLSBytes, error) { - return tlsClientConfigFromSecret(secret, url, false, false) -} - -// tlsClientConfigFromSecret attempts to construct and return a TLS client -// config from the given Secret. If the Secret does not contain any TLS -// data, it returns nil. -// -// kubernetesTLSKeys is a boolean indicating whether to check the Secret -// for keys expected to be present in a Kubernetes TLS Secret. Based on its -// value, the Secret is checked for the following keys: -// - tls.key/keyFile for the private key -// - tls.crt/certFile for the certificate -// - ca.crt/caFile for the CA certificate -// The keys should adhere to a single convention, i.e. a Secret with tls.key -// and certFile is invalid. -// -// checkType is a boolean indicating whether to check the Secret type. If true -// and the Secret's type is not blank, Opaque or kubernetes.io/tls, then an -// error is returned. -func tlsClientConfigFromSecret(secret corev1.Secret, url string, kubernetesTLSKeys bool, checkType bool) (*tls.Config, *TLSBytes, error) { - if checkType { - // Only Secrets of type Opaque and TLS are allowed. We also allow Secrets with a blank - // type, to avoid having to specify the type of the Secret for every test case. - // Since a real Kubernetes Secret is of type Opaque by default, its safe to allow this. - switch secret.Type { - case corev1.SecretTypeOpaque, corev1.SecretTypeTLS, "": - default: - return nil, nil, fmt.Errorf("cannot use secret '%s' to construct TLS config: invalid secret type: '%s'", secret.Name, secret.Type) - } - } - - var certBytes, keyBytes, caBytes []byte - if kubernetesTLSKeys { - certBytes, keyBytes, caBytes = secret.Data[corev1.TLSCertKey], secret.Data[corev1.TLSPrivateKeyKey], secret.Data[CACrtKey] - } else { - certBytes, keyBytes, caBytes = secret.Data["certFile"], secret.Data["keyFile"], secret.Data["caFile"] - } - - switch { - case len(certBytes)+len(keyBytes)+len(caBytes) == 0: - return nil, nil, nil - case (len(certBytes) > 0 && len(keyBytes) == 0) || (len(keyBytes) > 0 && len(certBytes) == 0): - return nil, nil, fmt.Errorf("invalid '%s' secret data: both certificate and private key need to be provided", - secret.Name) - } - - tlsConf := &tls.Config{ - MinVersion: tls.VersionTLS12, - } - if len(certBytes) > 0 && len(keyBytes) > 0 { - cert, err := tls.X509KeyPair(certBytes, keyBytes) - if err != nil { - return nil, nil, err - } - tlsConf.Certificates = append(tlsConf.Certificates, cert) - } - - if len(caBytes) > 0 { - cp, err := x509.SystemCertPool() - if err != nil { - return nil, nil, fmt.Errorf("cannot retrieve system certificate pool: %w", err) - } - if !cp.AppendCertsFromPEM(caBytes) { - return nil, nil, fmt.Errorf("cannot append certificate into certificate pool: invalid CA certificate") - } - - tlsConf.RootCAs = cp - } - - if url != "" { - u, err := neturl.Parse(url) - if err != nil { - return nil, nil, fmt.Errorf("cannot parse repository URL: %w", err) - } - - tlsConf.ServerName = u.Hostname() - } - - return tlsConf, &TLSBytes{ - CertBytes: certBytes, - KeyBytes: keyBytes, - CABytes: caBytes, - }, nil -} diff --git a/internal/tls/config_test.go b/internal/tls/config_test.go deleted file mode 100644 index 949142a0..00000000 --- a/internal/tls/config_test.go +++ /dev/null @@ -1,189 +0,0 @@ -/* -Copyright 2023 The Flux authors - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package tls - -import ( - "crypto/rand" - "crypto/rsa" - "crypto/x509" - "encoding/pem" - "fmt" - "math/big" - "net/url" - "testing" - - . "github.com/onsi/gomega" - corev1 "k8s.io/api/core/v1" -) - -func Test_tlsClientConfigFromSecret(t *testing.T) { - kubernetesTlsSecretFixture := validTlsSecret(t, true) - tlsSecretFixture := validTlsSecret(t, false) - - tests := []struct { - name string - secret corev1.Secret - modify func(secret *corev1.Secret) - tlsKeys bool - checkType bool - url string - wantErr bool - wantNil bool - }{ - { - name: "tls.crt, tls.key and ca.crt", - secret: kubernetesTlsSecretFixture, - modify: nil, - tlsKeys: true, - url: "https://example.com", - }, - { - name: "certFile, keyFile and caFile", - secret: tlsSecretFixture, - modify: nil, - tlsKeys: false, - url: "https://example.com", - }, - { - name: "without tls.crt", - secret: kubernetesTlsSecretFixture, - modify: func(s *corev1.Secret) { delete(s.Data, "tls.crt") }, - tlsKeys: true, - wantErr: true, - wantNil: true, - }, - { - name: "without tls.key", - secret: kubernetesTlsSecretFixture, - modify: func(s *corev1.Secret) { delete(s.Data, "tls.key") }, - tlsKeys: true, - wantErr: true, - wantNil: true, - }, - { - name: "without ca.crt", - secret: kubernetesTlsSecretFixture, - modify: func(s *corev1.Secret) { delete(s.Data, "ca.crt") }, - tlsKeys: true, - }, - { - name: "empty secret", - secret: corev1.Secret{}, - tlsKeys: true, - wantNil: true, - }, - { - name: "docker config secret with type checking enabled", - secret: tlsSecretFixture, - modify: func(secret *corev1.Secret) { secret.Type = corev1.SecretTypeDockerConfigJson }, - tlsKeys: false, - checkType: true, - wantErr: true, - wantNil: true, - }, - { - name: "docker config secret with type checking disabled", - secret: tlsSecretFixture, - modify: func(secret *corev1.Secret) { secret.Type = corev1.SecretTypeDockerConfigJson }, - tlsKeys: false, - url: "https://example.com", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - secret := tt.secret.DeepCopy() - if tt.modify != nil { - tt.modify(secret) - } - - tlsConfig, _, err := tlsClientConfigFromSecret(*secret, tt.url, tt.tlsKeys, tt.checkType) - g.Expect(err != nil).To(Equal(tt.wantErr), fmt.Sprintf("expected error: %v, got: %v", tt.wantErr, err)) - g.Expect(tlsConfig == nil).To(Equal(tt.wantNil)) - if tt.url != "" { - u, _ := url.Parse(tt.url) - g.Expect(u.Hostname()).To(Equal(tlsConfig.ServerName)) - } - }) - } -} - -// validTlsSecret creates a secret containing key pair and CA certificate that are -// valid from a syntax (minimum requirements) perspective. -func validTlsSecret(t *testing.T, kubernetesTlsKeys bool) corev1.Secret { - t.Helper() - key, err := rsa.GenerateKey(rand.Reader, 2048) - if err != nil { - t.Fatal("Private key cannot be created.", err.Error()) - } - - certTemplate := x509.Certificate{ - SerialNumber: big.NewInt(1337), - } - cert, err := x509.CreateCertificate(rand.Reader, &certTemplate, &certTemplate, &key.PublicKey, key) - if err != nil { - t.Fatal("Certificate cannot be created.", err.Error()) - } - - ca := &x509.Certificate{ - SerialNumber: big.NewInt(7331), - IsCA: true, - ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, - KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, - } - - caPrivKey, err := rsa.GenerateKey(rand.Reader, 4096) - if err != nil { - t.Fatal("CA private key cannot be created.", err.Error()) - } - - caBytes, err := x509.CreateCertificate(rand.Reader, ca, ca, &caPrivKey.PublicKey, caPrivKey) - if err != nil { - t.Fatal("CA certificate cannot be created.", err.Error()) - } - - keyPem := pem.EncodeToMemory(&pem.Block{ - Type: "RSA PRIVATE KEY", - Bytes: x509.MarshalPKCS1PrivateKey(key), - }) - - certPem := pem.EncodeToMemory(&pem.Block{ - Type: "CERTIFICATE", - Bytes: cert, - }) - - caPem := pem.EncodeToMemory(&pem.Block{ - Type: "CERTIFICATE", - Bytes: caBytes, - }) - - crtKey := corev1.TLSCertKey - pkKey := corev1.TLSPrivateKeyKey - caKey := CACrtKey - if !kubernetesTlsKeys { - crtKey = "certFile" - pkKey = "keyFile" - caKey = "caFile" - } - return corev1.Secret{ - Data: map[string][]byte{ - crtKey: []byte(certPem), - pkKey: []byte(keyPem), - caKey: []byte(caPem), - }, - } -}