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

Jump to bottom

refactor: certificates generation package and secrets #777

Merged
merged 7 commits into from
Jun 26, 2024
Merged

refactor: certificates generation package and secrets #777

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8bead45
refactor: rework cert package
fabriziosestito Jun 21, 2024
bc8e152
refactor: ca and certificates reconciler methods
fabriziosestito Jun 21, 2024
7b9da28
test: remove ca secret unit test
fabriziosestito Jun 21, 2024
4e213d0
chore: format policyserver_controller_test
fabriziosestito Jun 21, 2024
82b12a9
test: update PolicyServer secrets integration test
fabriziosestito Jun 21, 2024
d4a3b61
ci: tune varnamelen linter
fabriziosestito Jun 21, 2024
c5a217e
chore: move certs to a single file; use constant for bit size
fabriziosestito Jun 24, 2024
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: 2 additions & 0 deletions .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,5 @@ linters-settings:
desc: not allowed
- pkg: "github.com/pkg/errors"
desc: Should be replaced by standard lib errors package
varnamelen:
ignore-map-index-ok: true
46 changes: 30 additions & 16 deletions controllers/policyserver_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ var _ = Describe("PolicyServer controller", func() {
})

When("deleting a PolicyServer", func() {

BeforeEach(func() {
createPolicyServerAndWaitForItsService(policyServerFactory(policyServerName))
})
Expand Down Expand Up @@ -135,7 +134,6 @@ var _ = Describe("PolicyServer controller", func() {
return err
}, timeout, pollInterval).Should(notFound())
})

})

Context("with assigned policies", func() {
Expand Down Expand Up @@ -201,7 +199,6 @@ var _ = Describe("PolicyServer controller", func() {
HaveField("Finalizers", Not(ContainElement(constants.KubewardenFinalizerPre114))),
HaveField("Finalizers", ContainElement(IntegrationTestsFinalizer)),
))

})

It("should not delete its managed resources until all the scheduled policies are gone", func() {
Expand Down Expand Up @@ -248,15 +245,11 @@ var _ = Describe("PolicyServer controller", func() {
}, timeout, pollInterval).ShouldNot(
HaveField("Finalizers", ContainElement(constants.KubewardenFinalizer)),
)

})

})

})

When("creating a PolicyServer", func() {

It("should use the policy server affinity configuration in the policy server deployment", func() {
policyServer := policyServerFactory(policyServerName)
policyServer.Spec.Affinity = corev1.Affinity{
Expand Down Expand Up @@ -316,7 +309,8 @@ var _ = Describe("PolicyServer controller", func() {
"RunAsGroup": BeNil(),
"ProcMount": BeNil(),
"SeccompProfile": BeNil(),
}))})))
})),
})))
By("checking the deployment pod security context")
Expect(deployment.Spec.Template.Spec.SecurityContext).To(PointTo(MatchFields(IgnoreExtras, Fields{
"SELinuxOptions": BeNil(),
Expand All @@ -328,7 +322,8 @@ var _ = Describe("PolicyServer controller", func() {
"FSGroup": BeNil(),
"Sysctls": BeNil(),
"FSGroupChangePolicy": BeNil(),
"SeccompProfile": BeNil()})))
"SeccompProfile": BeNil(),
})))

By("checking the deployment affinity")
Expect(deployment.Spec.Template.Spec.Affinity).To(BeNil())
Expand Down Expand Up @@ -366,7 +361,8 @@ var _ = Describe("PolicyServer controller", func() {
"RunAsGroup": BeNil(),
"ProcMount": BeNil(),
"SeccompProfile": BeNil(),
}))})))
})),
})))
Expect(deployment.Spec.Template.Spec.SecurityContext).To(PointTo(MatchFields(IgnoreExtras, Fields{
"SELinuxOptions": BeNil(),
"WindowsOptions": BeNil(),
Expand All @@ -377,7 +373,8 @@ var _ = Describe("PolicyServer controller", func() {
"FSGroup": BeNil(),
"Sysctls": BeNil(),
"FSGroupChangePolicy": BeNil(),
"SeccompProfile": BeNil()})))
"SeccompProfile": BeNil(),
})))
})

It("should create the policy server configmap empty if no policies are assigned ", func() {
Expand Down Expand Up @@ -465,7 +462,6 @@ var _ = Describe("PolicyServer controller", func() {
})

It("should create PodDisruptionBudget when policy server has MinAvailable configuration set", func() {

policyServer := policyServerFactory(policyServerName)
minAvailable := intstr.FromInt(2)
policyServer.Spec.MinAvailable = &minAvailable
Expand Down Expand Up @@ -603,9 +599,24 @@ var _ = Describe("PolicyServer controller", func() {
}).Should(Succeed())
})

It("should create secret with owner reference", func() {
It("should create the policy server secrets", func() {
policyServer := policyServerFactory(policyServerName)
createPolicyServerAndWaitForItsService(policyServer)

Eventually(func() error {
secret, err := getTestPolicyServerCASecret()
if err != nil {
return err
}

By("creating a secret containing the CA certificate and key")
Expect(secret.Data).To(HaveKey(constants.PolicyServerCARootCACert))
Expect(secret.Data).To(HaveKey(constants.PolicyServerCARootPemName))
Expect(secret.Data).To(HaveKey(constants.PolicyServerCARootPrivateKeyCertName))

return nil
}).Should(Succeed())

Eventually(func() error {
secret, err := getTestPolicyServerSecret(policyServerName)
if err != nil {
Expand All @@ -615,6 +626,12 @@ var _ = Describe("PolicyServer controller", func() {
if err != nil {
return err
}

By("creating a secret containing the TLS certificate and key")
Expect(secret.Data).To(HaveKey(constants.PolicyServerTLSCert))
Expect(secret.Data).To(HaveKey(constants.PolicyServerTLSKey))

By("setting the secret owner reference")
Expect(secret.OwnerReferences).To(ContainElement(
MatchFields(IgnoreExtras, Fields{
"UID": Equal(policyServer.GetUID()),
Expand Down Expand Up @@ -690,9 +707,7 @@ var _ = Describe("PolicyServer controller", func() {
}
return nil
}, timeout, pollInterval).Should(Succeed())

})

})

When("updating the PolicyServer", func() {
Expand Down Expand Up @@ -968,6 +983,5 @@ var _ = Describe("PolicyServer controller", func() {
),
)
})

})
})
8 changes: 8 additions & 0 deletions controllers/utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,14 @@ func getTestPolicyServerService(policyServerName string) (*corev1.Service, error
return &service, nil
}

func getTestPolicyServerCASecret() (*corev1.Secret, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is retrieving the Root CA created by our controller. I suggest to rename this helper function to reflect that.

Maybe something like getInternalRootCASecret ?

secret := corev1.Secret{}
if err := reconciler.APIReader.Get(ctx, client.ObjectKey{Name: constants.PolicyServerCARootSecretName, Namespace: DeploymentsNamespace}, &secret); err != nil {
return nil, errors.Join(errors.New("could not find the PolicyServer CA secret"), err)
}
return &secret, nil
}

func getTestPolicyServerSecret(policyServerName string) (*corev1.Secret, error) {
secretName := getPolicyServerNameWithPrefix(policyServerName)
secret := corev1.Secret{}
Expand Down
76 changes: 47 additions & 29 deletions internal/pkg/admission/policy-server-ca-secret.go
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something for later, but I would also rename this file since it can mislead into thinking each Policy Server has a dedicated CA

Original file line number Diff line number Diff line change
Expand Up @@ -12,21 +12,18 @@
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"

"github.com/kubewarden/kubewarden-controller/internal/pkg/admissionregistration"
"github.com/kubewarden/kubewarden-controller/internal/pkg/certs"
"github.com/kubewarden/kubewarden-controller/internal/pkg/constants"
)

type generateCAFunc = func() (*admissionregistration.CA, error)
type pemEncodeCertificateFunc = func(certificate []byte) ([]byte, error)
type generateCertFunc = func(ca []byte, commonName string, extraSANs []string, CAPrivateKey *rsa.PrivateKey) ([]byte, []byte, error)

func (r *Reconciler) fetchOrInitializePolicyServerCASecret(ctx context.Context, policyServer *policiesv1.PolicyServer, caSecret *corev1.Secret, generateCert generateCertFunc) error {
func (r *Reconciler) fetchOrInitializePolicyServerCASecret(ctx context.Context, policyServer *policiesv1.PolicyServer, caSecret *corev1.Secret) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, there's a bit of naming confusion here we inherited from the past.

This method creates the certificate used by a specific Policy Server instance, there's no CA associated with a Policy Server

policyServerSecret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Namespace: r.DeploymentsNamespace,
Name: policyServer.NameWithPrefix(),
},
}

_, err := controllerutil.CreateOrPatch(ctx, r.Client, policyServerSecret, func() error {
if err := controllerutil.SetOwnerReference(policyServer, policyServerSecret, r.Client.Scheme()); err != nil {
return errors.Join(errors.New("failed to set policy server secret owner reference"), err)
Expand All @@ -36,22 +33,34 @@
_, hasTLSCert := policyServerSecret.Data[constants.PolicyServerTLSCert]
_, hasTLSKey := policyServerSecret.Data[constants.PolicyServerTLSKey]
if !hasTLSCert || !hasTLSKey {
admissionregCA, err := extractCaFromSecret(caSecret)
caCert, caPrivateKey, err := extractCAFromSecret(caSecret)
if err != nil {
return err
}
servingCert, servingKey, err := generateCert(
admissionregCA.CaCert,

cert, privateKey, err := certs.GenerateCert(
caCert,
fmt.Sprintf("%s.%s.svc", policyServer.NameWithPrefix(), r.DeploymentsNamespace),
[]string{fmt.Sprintf("%s.%s.svc", policyServer.NameWithPrefix(), r.DeploymentsNamespace)},
admissionregCA.CaPrivateKey)
caPrivateKey)
if err != nil {
return fmt.Errorf("cannot generate policy-server %s certificate: %w", policyServer.NameWithPrefix(), err)
}

certPEM, err := certs.PEMEncodeCertificate(cert)
if err != nil {
return fmt.Errorf("cannot PEM encode policy-server %s certificate: %w", policyServer.NameWithPrefix(), err)

Check warning on line 52 in internal/pkg/admission/policy-server-ca-secret.go

View check run for this annotation

Codecov / codecov/patch

internal/pkg/admission/policy-server-ca-secret.go#L52 

Added line #L52 was not covered by tests
}

privateKeyPEM, err := certs.PEMEncodePrivateKey(privateKey)
if err != nil {
return fmt.Errorf("cannot PEM encode policy-server %s private key: %w", policyServer.NameWithPrefix(), err)

Check warning on line 57 in internal/pkg/admission/policy-server-ca-secret.go

View check run for this annotation

Codecov / codecov/patch

internal/pkg/admission/policy-server-ca-secret.go#L57 

Added line #L57 was not covered by tests
}

policyServerSecret.Type = corev1.SecretTypeOpaque
policyServerSecret.StringData = map[string]string{
constants.PolicyServerTLSCert: string(servingCert),
constants.PolicyServerTLSKey: string(servingKey),
constants.PolicyServerTLSCert: string(certPEM),
constants.PolicyServerTLSKey: string(privateKeyPEM),
}
}

Expand All @@ -65,6 +74,7 @@
)
return errors.Join(errors.New("cannot fetch or initialize Policy Server CA secret"), err)
}

setTrueConditionType(
&policyServer.Status.Conditions,
string(policiesv1.PolicyServerCASecretReconciled),
Expand All @@ -73,37 +83,40 @@
return nil
}

func extractCaFromSecret(caSecret *corev1.Secret) (*admissionregistration.CA, error) {
func extractCAFromSecret(caSecret *corev1.Secret) ([]byte, *rsa.PrivateKey, error) {
caCert, ok := caSecret.Data[constants.PolicyServerCARootCACert]
if !ok {
return nil, fmt.Errorf("CA could not be extracted from secret %s", caSecret.Kind)
return nil, nil, fmt.Errorf("CA could not be extracted from secret %s", caSecret.Kind)

Check warning on line 89 in internal/pkg/admission/policy-server-ca-secret.go

View check run for this annotation

Codecov / codecov/patch

internal/pkg/admission/policy-server-ca-secret.go#L89 

Added line #L89 was not covered by tests
}
caPrivateKeyBytes, ok := caSecret.Data[constants.PolicyServerCARootPrivateKeyCertName]

privateKeyBytes, ok := caSecret.Data[constants.PolicyServerCARootPrivateKeyCertName]
if !ok {
return nil, fmt.Errorf("CA private key bytes could not be extracted from secret %s", caSecret.Kind)
return nil, nil, fmt.Errorf("CA private key bytes could not be extracted from secret %s", caSecret.Kind)

Check warning on line 94 in internal/pkg/admission/policy-server-ca-secret.go

View check run for this annotation

Codecov / codecov/patch

internal/pkg/admission/policy-server-ca-secret.go#L94 

Added line #L94 was not covered by tests
}

caPrivateKey, err := x509.ParsePKCS1PrivateKey(caPrivateKeyBytes)
privateKey, err := x509.ParsePKCS1PrivateKey(privateKeyBytes)
if err != nil {
return nil, fmt.Errorf("CA private key could not be extracted from secret %s", caSecret.Kind)
return nil, nil, fmt.Errorf("CA private key could not be extracted from secret %s", caSecret.Kind)

Check warning on line 99 in internal/pkg/admission/policy-server-ca-secret.go

View check run for this annotation

Codecov / codecov/patch

internal/pkg/admission/policy-server-ca-secret.go#L99 

Added line #L99 was not covered by tests
}
return &admissionregistration.CA{CaCert: caCert, CaPrivateKey: caPrivateKey}, nil

return caCert, privateKey, nil
}

func (r *Reconciler) fetchOrInitializePolicyServerCARootSecret(ctx context.Context, policyServer *policiesv1.PolicyServer, generateCA generateCAFunc, pemEncodeCertificate pemEncodeCertificateFunc) (*corev1.Secret, error) {
func (r *Reconciler) fetchOrInitializePolicyServerCARootSecret(ctx context.Context, policyServer *policiesv1.PolicyServer) (*corev1.Secret, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, the naming is confusing since no CA is stored inside of the Secret that is returned

policyServerSecret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Namespace: r.DeploymentsNamespace,
Name: constants.PolicyServerCARootSecretName,
},
}

_, err := controllerutil.CreateOrPatch(ctx, r.Client, policyServerSecret, func() error {
_, hasCARootCert := policyServerSecret.Data[constants.PolicyServerCARootCACert]
_, hasCARootPem := policyServerSecret.Data[constants.PolicyServerCARootPemName]
_, hasCARootPrivateKey := policyServerSecret.Data[constants.PolicyServerCARootPrivateKeyCertName]

if !hasCARootCert || !hasCARootPem || !hasCARootPrivateKey {
return updateSecretCA(policyServerSecret, generateCA, pemEncodeCertificate)
return updateCASecret(policyServerSecret)
}
return nil
})
Expand All @@ -115,29 +128,34 @@
)
return nil, errors.Join(errors.New("cannot fetch or initialize Policy Server CA secret"), err)
}

setTrueConditionType(
&policyServer.Status.Conditions,
string(policiesv1.PolicyServerCARootSecretReconciled),
)

return policyServerSecret, nil
}

func updateSecretCA(policyServerSecret *corev1.Secret, generateCA generateCAFunc, pemEncodeCertificate pemEncodeCertificateFunc) error {
caRoot, err := generateCA()
func updateCASecret(policyServerSecret *corev1.Secret) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is always generating the CA, it doesn't look into the contents of the secret that is passed as parameter.

Why don't we change the signature of the function to be something like that?

func createInternalCASecret() (policyServerSecret *corev1.Secret, error)

caCert, privateKey, err := certs.GenerateCA()
if err != nil {
return fmt.Errorf("cannot generate policy-server secret CA: %w", err)
}
caPEMEncoded, err := pemEncodeCertificate(caRoot.CaCert)

caCertPEM, err := certs.PEMEncodeCertificate(caCert)
if err != nil {
return fmt.Errorf("cannot encode policy-server secret CA: %w", err)
return fmt.Errorf("cannot PEM encode policy-server secret CA certificate: %w", err)

Check warning on line 148 in internal/pkg/admission/policy-server-ca-secret.go

View check run for this annotation

Codecov / codecov/patch

internal/pkg/admission/policy-server-ca-secret.go#L148 

Added line #L148 was not covered by tests
}
caPrivateKeyBytes := x509.MarshalPKCS1PrivateKey(caRoot.CaPrivateKey)

privateKeyBytes := x509.MarshalPKCS1PrivateKey(privateKey)
secretContents := map[string][]byte{
constants.PolicyServerCARootCACert: caRoot.CaCert,
constants.PolicyServerCARootPemName: caPEMEncoded,
constants.PolicyServerCARootPrivateKeyCertName: caPrivateKeyBytes,
constants.PolicyServerCARootCACert: caCert,
constants.PolicyServerCARootPemName: caCertPEM,
constants.PolicyServerCARootPrivateKeyCertName: privateKeyBytes,
}
policyServerSecret.Type = corev1.SecretTypeOpaque
policyServerSecret.Data = secretContents

return nil
}
Loading
Loading