Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Helm OCI: Add support for TLS registries with self-signed certs #1097

Merged
merged 1 commit into from
Aug 7, 2023
Merged

Helm OCI: Add support for TLS registries with self-signed certs #1097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions docs/spec/v1beta2/helmrepositories.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,8 +459,6 @@ a deprecation warning will be logged.

### Cert secret reference

**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:

Expand Down
27 changes: 19 additions & 8 deletions internal/controller/helmchart_controller.go
View file
Open in desktop
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,15 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
return sreconcile.ResultEmpty, e
}
if certsTmpDir != "" {
defer func() {
if err := os.RemoveAll(certsTmpDir); err != nil {
r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason,
"failed to delete temporary certificates directory: %s", err)
}
}()
}

getterOpts := clientOpts.GetterOpts

// Initialize the chart repository
Expand All @@ -536,7 +546,7 @@ 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)
registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry())
aryan9600 marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
e := &serror.Event{
Err: fmt.Errorf("failed to construct Helm client: %w", err),
Expand Down Expand Up @@ -585,8 +595,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 clientOpts.MustLoginToRegistry() {
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 +993,15 @@ 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
}
getterOpts := clientOpts.GetterOpts

var chartRepo repository.Downloader
if helmreg.IsOCI(normalizedURL) {
registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.RegLoginOpt != nil)
registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry())
if err != nil {
return nil, fmt.Errorf("failed to create registry client: %w", err)
}
Expand All @@ -1002,6 +1012,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL, repository.WithOCIGetter(r.Getters),
repository.WithOCIGetterOptions(getterOpts),
repository.WithOCIRegistryClient(registryClient),
repository.WithCertificatesStore(certsTmpDir),
repository.WithCredentialsFile(credentialsFile))
if err != nil {
errs = append(errs, fmt.Errorf("failed to create OCI chart repository: %w", err))
Expand All @@ -1016,8 +1027,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 clientOpts.MustLoginToRegistry() {
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
161 changes: 125 additions & 36 deletions internal/controller/helmchart_controller_test.go
View file
Open in desktop
souleb marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -1109,7 +1109,7 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
g.Expect(err).NotTo(HaveOccurred())

// Upload the test chart
metadata, err := loadTestChartToOCI(chartData, chartPath, testRegistryServer)
metadata, err := loadTestChartToOCI(chartData, testRegistryServer, "", "", "")
g.Expect(err).NotTo(HaveOccurred())

storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords)
Expand Down Expand Up @@ -2244,53 +2244,74 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
url string
registryOpts registryOptions
secretOpts secretOptions
secret *corev1.Secret
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,
},
secretOpts: secretOptions{
username: testRegistryUsername,
password: testRegistryPassword,
},
secret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "auth-secretref",
},
Type: corev1.SecretTypeDockerConfigJson,
Data: map[string][]byte{},
},
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 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,
},
secretOpts: secretOptions{
username: "wrong-pass",
password: "wrong-pass",
},
secret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "auth-secretref",
},
Type: corev1.SecretTypeDockerConfigJson,
Data: map[string][]byte{},
},
assertConditions: []metav1.Condition{
*conditions.TrueCondition(sourcev1.FetchFailedCondition, "Unknown", "unknown build error: failed to login to OCI registry"),
},
},
{
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 @@ -2303,16 +2324,87 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
registryOpts: registryOptions{
withBasicAuth: true,
},
insecure: true,
secretOpts: secretOptions{
username: testRegistryUsername,
password: testRegistryPassword,
},
secret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "auth-secretref",
},
Type: corev1.SecretTypeDockerConfigJson,
Data: map[string][]byte{},
},
provider: "azure",
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: "HTTPS With invalid CA cert",
wantErr: true,
registryOpts: registryOptions{
withTLS: true,
withClientCertAuth: true,
},
secretOpts: secretOptions{
username: testRegistryUsername,
password: testRegistryPassword,
},
secret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "auth-secretref",
},
Type: corev1.SecretTypeDockerConfigJson,
souleb marked this conversation as resolved.
Show resolved Hide resolved
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 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,
withClientCertAuth: true,
},
secretOpts: secretOptions{
username: testRegistryUsername,
password: testRegistryPassword,
},
secret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "auth-secretref",
},
Type: corev1.SecretTypeDockerConfigJson,
souleb marked this conversation as resolved.
Show resolved Hide resolved
Data: map[string][]byte{},
},
certsecret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "certs-secretref",
},
Data: map[string][]byte{
"caFile": tlsCA,
"certFile": clientPublicKey,
"keyFile": clientPrivateKey,
},
},
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'"),
},
},
}

for _, tt := range tests {
Expand All @@ -2325,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 @@ -2337,7 +2431,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())

// Upload the test chart
metadata, err := loadTestChartToOCI(chartData, chartPath, server)
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 All @@ -2364,25 +2458,26 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
}

if tt.secretOpts.username != "" && tt.secretOpts.password != "" {
secret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "auth-secretref",
},
Type: corev1.SecretTypeDockerConfigJson,
Data: map[string][]byte{
".dockerconfigjson": []byte(fmt.Sprintf(`{"auths": {%q: {"username": %q, "password": %q}}}`,
server.registryHost, tt.secretOpts.username, tt.secretOpts.password)),
},
}
tt.secret.Data[".dockerconfigjson"] = []byte(fmt.Sprintf(`{"auths": {%q: {"username": %q, "password": %q}}}`,
server.registryHost, tt.secretOpts.username, tt.secretOpts.password))
}

if tt.secret != nil {
repo.Spec.SecretRef = &meta.LocalObjectReference{
Name: secret.Name,
Name: tt.secret.Name,
}
clientBuilder.WithObjects(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 @@ -2456,7 +2551,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T
g.Expect(err).ToNot(HaveOccurred())

// Upload the test chart
metadata, err := loadTestChartToOCI(chartData, chartPath, server)
metadata, err := loadTestChartToOCI(chartData, server, "", "", "")
g.Expect(err).NotTo(HaveOccurred())

storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords)
Expand Down Expand Up @@ -2687,30 +2782,24 @@ func extractChartMeta(chartData []byte) (*hchart.Metadata, error) {
return ch.Metadata, nil
}

func loadTestChartToOCI(chartData []byte, chartPath string, server *registryClientTestServer) (*hchart.Metadata, error) {
func loadTestChartToOCI(chartData []byte, server *registryClientTestServer, certFile, keyFile, cafile string) (*hchart.Metadata, error) {
// Login to the registry
err := server.registryClient.Login(server.registryHost,
helmreg.LoginOptBasicAuth(testRegistryUsername, testRegistryPassword),
helmreg.LoginOptInsecure(true))
if err != nil {
return nil, err
}

// Load a test chart
chartData, err = os.ReadFile(chartPath)
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
Open in desktop
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