Skip to content

Commit

Permalink
refactor TLSconfig funcs
Browse files Browse the repository at this point in the history 
Signed-off-by: Soule BA <soule@weave.works>
  • Loading branch information
@souleb
souleb committed Aug 3, 2023
1 parent 22942cb commit a5a1399
Show file tree
Hide file tree
Showing 22 changed files with 448 additions and 422 deletions.
3 changes: 0 additions & 3 deletions api/v1beta2/zz_generated.deepcopy.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 0 additions & 30 deletions docs/spec/v1beta2/helmrepositories.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -459,16 +459,8 @@ a deprecation warning will be logged.

### Cert secret reference

<<<<<<< HEAD
**Note:** TLS authentication is not yet supported by OCI Helm repositories.

`.spec.certSecretRef.name` is an optional field to specify a secret containing TLS
certificate data. The secret can contain the following keys:
=======
To provide TLS credentials to use while connecting with the Helm repository,
the referenced Secret is expected to contain `.data.certFile` and
`.data.keyFile`, and/or `.data.caFile` values.
>>>>>>> 3df4c49 (refactoring and fix tests)

* `certFile` and `keyFile`, to specify the client certificate and private key used for
TLS client authentication. These must be used in conjunction, i.e. specifying one without
Expand Down Expand Up @@ -515,28 +507,6 @@ data:
caFile: <BASE64>
```

#### Provide TLS credentials in a secret of type kubernetes.io/dockerconfigjson

For OCI Helm repositories, Kubernetes secrets of type [kubernetes.io/dockerconfigjson](https://kubernetes.io/docs/concepts/configuration/secret/#secret-types)
are also supported. It is possible to append TLS credentials to the secret data.

For example:

```yaml
apiVersion: v1
kind: Secret
metadata:
name: example-tls
namespace: default
type: kubernetes.io/dockerconfigjson
data:
.dockerconfigjson: <BASE64>
certFile: <BASE64>
keyFile: <BASE64>
# NOTE: Can be supplied without the above values
caFile: <BASE64>
```

### Pass credentials

`.spec.passCredentials` is an optional field to allow the credentials from the
Expand Down
23 changes: 15 additions & 8 deletions internal/controller/helmchart_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,8 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
if err != nil {
return chartRepoConfigErrorReturn(err, obj)
}
clientOpts, err := getter.GetClientOpts(ctxTimeout, r.Client, repo, normalizedURL)

clientOpts, certsTmpDir, err := getter.GetClientOpts(ctxTimeout, r.Client, repo, normalizedURL)
if err != nil && !errors.Is(err, getter.ErrDeprecatedTLSConfig) {
e := &serror.Event{
Err: err,
Expand All @@ -521,6 +522,8 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
return sreconcile.ResultEmpty, e
}
defer os.RemoveAll(certsTmpDir)

getterOpts := clientOpts.GetterOpts

// Initialize the chart repository
Expand All @@ -536,7 +539,8 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
// this is needed because otherwise the credentials are stored in ~/.docker/config.json.
// TODO@souleb: remove this once the registry move to Oras v2
// or rework to enable reusing credentials to avoid the unneccessary handshake operations
registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.RegLoginOpt != nil)
shouldLogin := len(clientOpts.RegLoginOpts) > 0 && clientOpts.RegLoginOpts[0] != nil
registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, shouldLogin)
if err != nil {
e := &serror.Event{
Err: fmt.Errorf("failed to construct Helm client: %w", err),
Expand Down Expand Up @@ -585,8 +589,8 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *

// If login options are configured, use them to login to the registry
// The OCIGetter will later retrieve the stored credentials to pull the chart
if clientOpts.RegLoginOpt != nil {
err = ociChartRepo.Login(clientOpts.RegLoginOpt)
if shouldLogin {
err = ociChartRepo.Login(clientOpts.RegLoginOpts...)
if err != nil {
e := &serror.Event{
Err: fmt.Errorf("failed to login to OCI registry: %w", err),
Expand Down Expand Up @@ -983,15 +987,18 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
ctxTimeout, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration)
defer cancel()

clientOpts, err := getter.GetClientOpts(ctxTimeout, r.Client, obj, normalizedURL)
clientOpts, certsTmpDir, err := getter.GetClientOpts(ctxTimeout, r.Client, obj, normalizedURL)
if err != nil && !errors.Is(err, getter.ErrDeprecatedTLSConfig) {
return nil, err
}
defer os.RemoveAll(certsTmpDir)

getterOpts := clientOpts.GetterOpts

var chartRepo repository.Downloader
if helmreg.IsOCI(normalizedURL) {
registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.RegLoginOpt != nil)
shouldLogin := len(clientOpts.RegLoginOpts) > 0 && clientOpts.RegLoginOpts[0] != nil
registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, shouldLogin)
if err != nil {
return nil, fmt.Errorf("failed to create registry client: %w", err)
}
Expand All @@ -1016,8 +1023,8 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont

// If login options are configured, use them to login to the registry
// The OCIGetter will later retrieve the stored credentials to pull the chart
if clientOpts.RegLoginOpt != nil {
err = ociChartRepo.Login(clientOpts.RegLoginOpt)
if shouldLogin {
err = ociChartRepo.Login(clientOpts.RegLoginOpts...)
if err != nil {
errs = append(errs, fmt.Errorf("failed to login to OCI chart repository: %w", err))
// clean up the credentialsFile
Expand Down
73 changes: 50 additions & 23 deletions internal/controller/helmchart_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2245,24 +2245,27 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
registryOpts registryOptions
secretOpts secretOptions
secret *corev1.Secret
withTLS bool
certsecret *corev1.Secret
insecure bool
provider string
providerImg string
want sreconcile.Result
wantErr bool
assertConditions []metav1.Condition
}{
{
name: "HTTP without basic auth",
want: sreconcile.ResultSuccess,
name: "HTTP without basic auth",
want: sreconcile.ResultSuccess,
insecure: true,
assertConditions: []metav1.Condition{
*conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"),
*conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"),
},
},
{
name: "HTTP with basic auth secret",
want: sreconcile.ResultSuccess,
name: "HTTP with basic auth secret",
want: sreconcile.ResultSuccess,
insecure: true,
registryOpts: registryOptions{
withBasicAuth: true,
},
Expand All @@ -2283,9 +2286,10 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
},
},
{
name: "HTTP registry - basic auth with invalid secret",
want: sreconcile.ResultEmpty,
wantErr: true,
name: "HTTP registry - basic auth with invalid secret",
want: sreconcile.ResultEmpty,
wantErr: true,
insecure: true,
registryOpts: registryOptions{
withBasicAuth: true,
},
Expand All @@ -2307,6 +2311,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
{
name: "with contextual login provider",
wantErr: true,
insecure: true,
provider: "aws",
providerImg: "oci://123456789000.dkr.ecr.us-east-2.amazonaws.com/test",
assertConditions: []metav1.Condition{
Expand All @@ -2319,6 +2324,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
registryOpts: registryOptions{
withBasicAuth: true,
},
insecure: true,
secretOpts: secretOptions{
username: testRegistryUsername,
password: testRegistryPassword,
Expand All @@ -2340,9 +2346,9 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
name: "HTTPS With invalid CA cert",
wantErr: true,
registryOpts: registryOptions{
withTLS: true,
withTLS: true,
withClientCertAuth: true,
},
withTLS: true,
secretOpts: secretOptions{
username: testRegistryUsername,
password: testRegistryPassword,
Expand All @@ -2352,21 +2358,27 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
Name: "auth-secretref",
},
Type: corev1.SecretTypeDockerConfigJson,
Data: map[string][]byte{},
},
certsecret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "certs-secretref",
},
Data: map[string][]byte{
"caFile": []byte("invalid caFile"),
},
},
assertConditions: []metav1.Condition{
*conditions.TrueCondition(sourcev1.FetchFailedCondition, "Unknown", "unknown build error: failed to create TLS client config with secret data: cannot append certificate into certificate pool: invalid caFile"),
*conditions.TrueCondition(sourcev1.FetchFailedCondition, "Unknown", "unknown build error: failed to construct Helm client's TLS config: cannot append certificate into certificate pool: invalid caFile"),
},
},
{
name: "HTTPS With CA cert",
want: sreconcile.ResultSuccess,
registryOpts: registryOptions{
withTLS: true,
withTLS: true,
withClientCertAuth: true,
},
withTLS: true,
secretOpts: secretOptions{
username: testRegistryUsername,
password: testRegistryPassword,
Expand All @@ -2376,10 +2388,16 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
Name: "auth-secretref",
},
Type: corev1.SecretTypeDockerConfigJson,
Data: map[string][]byte{},
},
certsecret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "certs-secretref",
},
Data: map[string][]byte{
"caFile": tlsCA,
"certFile": tlsPublicKey,
"keyFile": tlsPrivateKey,
"certFile": clientPublicKey,
"keyFile": clientPrivateKey,
},
},
assertConditions: []metav1.Condition{
Expand All @@ -2399,7 +2417,9 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {

workspaceDir := t.TempDir()

tt.registryOpts.disableDNSMocking = true
if tt.insecure {
tt.registryOpts.disableDNSMocking = true
}
server, err := setupRegistryServer(ctx, workspaceDir, tt.registryOpts)
g.Expect(err).NotTo(HaveOccurred())
t.Cleanup(func() {
Expand All @@ -2411,7 +2431,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())

// Upload the test chart
metadata, err := loadTestChartToOCI(chartData, server, "testdata/certs/server.pem", "testdata/certs/server-key.pem", "testdata/certs/ca.pem")
metadata, err := loadTestChartToOCI(chartData, server, "testdata/certs/client.pem", "testdata/certs/client-key.pem", "testdata/certs/ca.pem")
g.Expect(err).ToNot(HaveOccurred())

repo := &helmv1.HelmRepository{
Expand Down Expand Up @@ -2446,11 +2466,18 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
repo.Spec.SecretRef = &meta.LocalObjectReference{
Name: tt.secret.Name,
}
clientBuilder.WithObjects(tt.secret, repo)
} else {
clientBuilder.WithObjects(repo)
clientBuilder.WithObjects(tt.secret)
}

if tt.certsecret != nil {
repo.Spec.CertSecretRef = &meta.LocalObjectReference{
Name: tt.certsecret.Name,
}
clientBuilder.WithObjects(tt.certsecret)
}

clientBuilder.WithObjects(repo)

obj := &helmv1.HelmChart{
ObjectMeta: metav1.ObjectMeta{
GenerateName: "auth-strategy-",
Expand Down Expand Up @@ -2761,18 +2788,18 @@ func loadTestChartToOCI(chartData []byte, server *registryClientTestServer, cert
helmreg.LoginOptBasicAuth(testRegistryUsername, testRegistryPassword),
helmreg.LoginOptTLSClientConfig(certFile, keyFile, cafile))
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to login to OCI registry: %w", err)
}
metadata, err := extractChartMeta(chartData)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to extract chart metadata: %w", err)
}

// Upload the test chart
ref := fmt.Sprintf("%s/testrepo/%s:%s", server.registryHost, metadata.Name, metadata.Version)
_, err = server.registryClient.Push(chartData, ref)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to push chart: %w", err)
}

return metadata, nil
Expand Down
2 changes: 1 addition & 1 deletion internal/controller/helmrepository_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc
return sreconcile.ResultEmpty, e
}

clientOpts, err := getter.GetClientOpts(ctx, r.Client, obj, normalizedURL)
clientOpts, _, err := getter.GetClientOpts(ctx, r.Client, obj, normalizedURL)
if err != nil {
if errors.Is(err, getter.ErrDeprecatedTLSConfig) {
ctrl.LoggerFrom(ctx).
Expand Down
Loading

0 comments on commit a5a1399

Please sign in to comment.