Skip to content

Commit

Permalink
helmrepo: adopt Kubernetes TLS secrets for .spec.certSecretRef
Browse files Browse the repository at this point in the history 
Adopt Kubernetes TLS secrets API to check for TLS data in the Secret
referred to by `.spec.certSecretRef`, i.e. check for keys `tls.crt` and
`tls.key` for the certificate and private key. Use `ca.crt` for the CA
certificate.

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
  • Loading branch information
@aryan9600
aryan9600 committed Aug 16, 2023
1 parent d218731 commit 0361779
Show file tree
Hide file tree
Showing 6 changed files with 97 additions and 59 deletions.
36 changes: 19 additions & 17 deletions docs/spec/v1beta2/helmrepositories.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -464,25 +464,26 @@ a deprecation warning will be logged.

### Cert secret reference

`.spec.certSecretRef.name` is an optional field to specify a secret containing TLS
certificate data. The secret can contain the following keys:
`.spec.certSecretRef.name` is an optional field to specify a secret containing
TLS certificate data. The secret can contain the following keys:

* `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
the other will lead to an error.
* `caFile`, to specify the CA certificate used to verify the server, which is required
if the server is using a self-signed certificate.
* `tls.crt` and `tls.key`, to specify the client certificate and private key used
for TLS client authentication. These must be used in conjunction, i.e.
specifying one without the other will lead to an error.
* `ca.crt`, to specify the CA certificate used to verify the server, which is
required if the server is using a self-signed certificate.

If the server is using a self-signed certificate and has TLS client authentication enabled,
all three values are required.
If the server is using a self-signed certificate and has TLS client
authentication enabled, all three values are required.

All the files in the secret are expected to be [PEM-encoded][pem-encoding]. Assuming you have
three files; `client.key`, `client.crt` and `ca.crt` for the client private key, client
certificate and the CA certificate respectively, you can generate the required secret using
the `flux creat secret helm` command:
The Secret should be of type Opaque or TLS. All the files in the Secret are
expected to be [PEM-encoded][pem-encoding]. Assuming you have three files;
`client.key`, `client.crt` and `ca.crt` for the client private key, client
certificate and the CA certificate respectively, you can generate the required
Secret using the `flux create secret tls` command:

```sh
flux create secret helm tls --key-file=client.key --cert-file=client.crt --ca-file=ca.crt
flux create secret tls --tls-key-file=client.key --tls-crt-file=client.crt --ca-crt-file=ca.crt
```

Example usage:
Expand All @@ -505,11 +506,12 @@ kind: Secret
metadata:
name: example-tls
namespace: default
type: kubernetes.io/tls # or Opaque
data:
certFile: <BASE64>
keyFile: <BASE64>
tls.crt: <BASE64>
tls.key: <BASE64>
# NOTE: Can be supplied without the above values
caFile: <BASE64>
ca.crt: <BASE64>
```

### Pass credentials
Expand Down
10 changes: 5 additions & 5 deletions internal/controller/helmchart_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2368,11 +2368,11 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
Name: "certs-secretref",
},
Data: map[string][]byte{
"caFile": []byte("invalid caFile"),
"ca.crt": []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"),
*conditions.TrueCondition(sourcev1.FetchFailedCondition, "Unknown", "unknown build error: failed to construct Helm client's TLS config: cannot append certificate into certificate pool: invalid CA certificate"),
},
},
{
Expand All @@ -2398,9 +2398,9 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
Name: "certs-secretref",
},
Data: map[string][]byte{
"caFile": tlsCA,
"certFile": clientPublicKey,
"keyFile": clientPrivateKey,
"ca.crt": tlsCA,
"tls.crt": clientPublicKey,
"tls.key": clientPrivateKey,
},
},
assertConditions: []metav1.Condition{
Expand Down
10 changes: 5 additions & 5 deletions internal/controller/helmrepository_controller_oci_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -325,12 +325,12 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) {
Name: "certs-secretref",
},
Data: map[string][]byte{
"caFile": []byte("invalid caFile"),
"ca.crt": []byte("invalid caFile"),
},
},
assertConditions: []metav1.Condition{
*conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingWithRetryReason, "processing object: new generation 0 -> 1"),
*conditions.FalseCondition(meta.ReadyCondition, sourcev1.AuthenticationFailedReason, "cannot append certificate into certificate pool: invalid caFile"),
*conditions.FalseCondition(meta.ReadyCondition, sourcev1.AuthenticationFailedReason, "cannot append certificate into certificate pool: invalid CA certificate"),
},
},
{
Expand All @@ -356,9 +356,9 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) {
Name: "certs-secretref",
},
Data: map[string][]byte{
"caFile": tlsCA,
"certFile": clientPublicKey,
"keyFile": clientPrivateKey,
"ca.crt": tlsCA,
"tls.crt": clientPublicKey,
"tls.key": clientPrivateKey,
},
},
assertConditions: []metav1.Condition{
Expand Down
8 changes: 4 additions & 4 deletions internal/controller/helmrepository_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) {
Name: "ca-file",
},
Data: map[string][]byte{
"caFile": tlsCA,
"ca.crt": tlsCA,
},
},
beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev digest.Digest) {
Expand Down Expand Up @@ -502,7 +502,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) {
Name: "invalid-ca",
},
Data: map[string][]byte{
"caFile": []byte("invalid"),
"ca.crt": []byte("invalid"),
},
},
beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev digest.Digest) {
Expand All @@ -512,7 +512,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) {
},
wantErr: true,
assertConditions: []metav1.Condition{
*conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "cannot append certificate into certificate pool: invalid caFile"),
*conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "cannot append certificate into certificate pool: invalid CA certificate"),
*conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"),
*conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"),
},
Expand Down Expand Up @@ -769,7 +769,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) {
if tt.url != "" {
repoURL = tt.url
}
tlsConf, _, serr = getter.TLSClientConfigFromSecret(*secret, repoURL)
tlsConf, _, serr = getter.TLSClientConfigFromSecret(*secret, repoURL, true)
if serr != nil {
validSecret = false
}
Expand Down
54 changes: 39 additions & 15 deletions internal/helm/getter/client_opts.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ const (
certFileName = "cert.pem"
keyFileName = "key.pem"
caFileName = "ca.pem"
caCertKey = "ca.crt"
)

var ErrDeprecatedTLSConfig = errors.New("TLS configured in a deprecated manner")
Expand Down Expand Up @@ -105,7 +106,7 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit
return nil, "", fmt.Errorf("failed to get TLS authentication secret '%s/%s': %w", obj.GetNamespace(), obj.Spec.CertSecretRef.Name, err)
}

hrOpts.TlsConfig, tlsBytes, err = TLSClientConfigFromSecret(*certSecret, url)
hrOpts.TlsConfig, tlsBytes, err = TLSClientConfigFromSecret(*certSecret, url, true)
if err != nil {
return nil, "", fmt.Errorf("failed to construct Helm client's TLS config: %w", err)
}
Expand All @@ -129,7 +130,7 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit
// If the TLS config is nil, i.e. one couldn't be constructed using `.spec.certSecretRef`
// then try to use `.spec.secretRef`.
if hrOpts.TlsConfig == nil {
hrOpts.TlsConfig, tlsBytes, err = TLSClientConfigFromSecret(*authSecret, url)
hrOpts.TlsConfig, tlsBytes, err = TLSClientConfigFromSecret(*authSecret, url, false)
if err != nil {
return nil, "", fmt.Errorf("failed to construct Helm client's TLS config: %w", err)
}
Expand Down Expand Up @@ -199,20 +200,43 @@ func fetchSecret(ctx context.Context, c client.Client, name, namespace string) (
}

// TLSClientConfigFromSecret attempts to construct a TLS client config
// for the given v1.Secret. It returns the TLS client config or an error.
// for the given Secret. It returns the TLS client config or an error.
//
// Secrets with no certFile, keyFile, AND caFile are ignored, if only a
// certBytes OR keyBytes is defined it returns an error.
func TLSClientConfigFromSecret(secret corev1.Secret, repositoryUrl string) (*tls.Config, *TLSBytes, error) {
certBytes, keyBytes, caBytes := secret.Data["certFile"], secret.Data["keyFile"], secret.Data["caFile"]
// 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.
// 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 TLSClientConfigFromSecret(secret corev1.Secret, repositoryUrl string, kubernetesTLSKeys bool) (*tls.Config, *TLSBytes, error) {
var certBytes, keyBytes, caBytes []byte
if kubernetesTLSKeys {
certBytes, keyBytes, caBytes = secret.Data[corev1.TLSCertKey], secret.Data[corev1.TLSPrivateKeyKey], secret.Data[caCertKey]
} 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: fields 'certFile' and 'keyFile' require each other's presence",
return nil, nil, fmt.Errorf("invalid '%s' secret data: both certificate and private key need to be provided",
secret.Name)
}

// 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)
}

tlsConf := &tls.Config{}
if len(certBytes) > 0 && len(keyBytes) > 0 {
cert, err := tls.X509KeyPair(certBytes, keyBytes)
Expand All @@ -228,21 +252,21 @@ func TLSClientConfigFromSecret(secret corev1.Secret, repositoryUrl string) (*tls
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 caFile")
return nil, nil, fmt.Errorf("cannot append certificate into certificate pool: invalid CA certificate")
}

tlsConf.RootCAs = cp
}

tlsConf.BuildNameToCertificate()
if repositoryUrl != "" {
u, err := url.Parse(repositoryUrl)
if err != nil {
return nil, nil, fmt.Errorf("cannot parse repository URL: %w", err)
}

u, err := url.Parse(repositoryUrl)
if err != nil {
return nil, nil, fmt.Errorf("cannot parse repository URL: %w", err)
tlsConf.ServerName = u.Hostname()
}

tlsConf.ServerName = u.Hostname()

return tlsConf, &TLSBytes{
CertBytes: certBytes,
KeyBytes: keyBytes,
Expand Down
38 changes: 25 additions & 13 deletions internal/helm/getter/client_opts_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func TestGetClientOpts(t *testing.T) {
Name: "ca-file",
},
Data: map[string][]byte{
"caFile": tlsCA,
"ca.crt": tlsCA,
},
},
authSecret: &corev1.Secret{
Expand Down Expand Up @@ -161,20 +161,24 @@ func TestGetClientOpts(t *testing.T) {
}

func Test_tlsClientConfigFromSecret(t *testing.T) {
tlsSecretFixture := validTlsSecret(t)
kubernetesTlsSecretFixture := validTlsSecret(t, true)
tlsSecretFixture := validTlsSecret(t, false)

tests := []struct {
name string
secret corev1.Secret
modify func(secret *corev1.Secret)
tlsKeys bool
wantErr bool
wantNil bool
}{
{"certFile, keyFile and caFile", tlsSecretFixture, nil, false, false},
{"without certFile", tlsSecretFixture, func(s *corev1.Secret) { delete(s.Data, "certFile") }, true, true},
{"without keyFile", tlsSecretFixture, func(s *corev1.Secret) { delete(s.Data, "keyFile") }, true, true},
{"without caFile", tlsSecretFixture, func(s *corev1.Secret) { delete(s.Data, "caFile") }, false, false},
{"empty", corev1.Secret{}, nil, false, true},
{"tls.crt, tls.key and ca.crt", kubernetesTlsSecretFixture, nil, true, false, false},
{"certFile, keyFile and caFile", tlsSecretFixture, nil, false, false, false},
{"without tls.crt", kubernetesTlsSecretFixture, func(s *corev1.Secret) { delete(s.Data, "tls.crt") }, true, true, true},
{"without tls.key", kubernetesTlsSecretFixture, func(s *corev1.Secret) { delete(s.Data, "tls.key") }, true, true, true},
{"without ca.crt", kubernetesTlsSecretFixture, func(s *corev1.Secret) { delete(s.Data, "ca.crt") }, true, false, false},
{"empty", corev1.Secret{}, nil, true, false, true},
{"invalid secret type", kubernetesTlsSecretFixture, func(s *corev1.Secret) { s.Type = corev1.SecretTypeBasicAuth }, true, true, true},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand All @@ -183,7 +187,7 @@ func Test_tlsClientConfigFromSecret(t *testing.T) {
tt.modify(secret)
}

got, _, err := TLSClientConfigFromSecret(*secret, "")
got, _, err := TLSClientConfigFromSecret(*secret, "", tt.tlsKeys)
if (err != nil) != tt.wantErr {
t.Errorf("TLSClientConfigFromSecret() error = %v, wantErr %v", err, tt.wantErr)
return
Expand Down Expand Up @@ -215,7 +219,7 @@ func TestGetClientOpts_registryTLSLoginOption(t *testing.T) {
Name: "ca-file",
},
Data: map[string][]byte{
"caFile": tlsCA,
"ca.crt": tlsCA,
},
},
authSecret: &corev1.Secret{
Expand Down Expand Up @@ -310,7 +314,7 @@ func TestGetClientOpts_registryTLSLoginOption(t *testing.T) {

// validTlsSecret creates a secret containing key pair and CA certificate that are
// valid from a syntax (minimum requirements) perspective.
func validTlsSecret(t *testing.T) corev1.Secret {
func validTlsSecret(t *testing.T, kubernetesTlsKeys bool) corev1.Secret {
key, err := rsa.GenerateKey(rand.Reader, 2048)
if err != nil {
t.Fatal("Private key cannot be created.", err.Error())
Expand Down Expand Up @@ -356,11 +360,19 @@ func validTlsSecret(t *testing.T) corev1.Secret {
Bytes: caBytes,
})

crtKey := "tls.crt"
pkKey := "tls.key"
caKey := "ca.crt"
if !kubernetesTlsKeys {
crtKey = "certFile"
pkKey = "keyFile"
caKey = "caFile"
}
return corev1.Secret{
Data: map[string][]byte{
"certFile": []byte(certPem),
"keyFile": []byte(keyPem),
"caFile": []byte(caPem),
crtKey: []byte(certPem),
pkKey: []byte(keyPem),
caKey: []byte(caPem),
},
}
}

0 comments on commit 0361779

Please sign in to comment.