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.

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

feat(spin/certs): automate creating the default CA bundle secret #207

Merged
merged 2 commits into from
May 15, 2024

Conversation

adamreese
Copy link
Member

Supersedes #184

Automate the creation of a secret for a default CA root certificate bundle. A secret is created in each namespace that contains a spin application. If a secret already exists with the name spin-ca it will not be modified. This allows the default spin-ca secret to be overridden by the user.

The embedded CA bundle is fetched from https://curl.se/ca/cacert.pem and can be updated to the latest by running go generate ./....

There is no owner reference on the secret which means it will persist unless manually deleted. Meaning that if spin-operator is removed from the cluster it will not be included in the cascading deletion.

This commit injects a default CA bundle in the root filesystem for every
Spin application pod created, so host components can execute TLS
operations.

Signed-off-by: Radu Matei <[email protected]>
Copy link
Contributor

@calebschoepp calebschoepp left a comment

Choose a reason for hiding this comment

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

Great work Adam.

There is no owner reference on the secret which means it will persist unless manually deleted. Meaning that if spin-operator is removed from the cluster it will not be included in the cascading deletion.

Mind elaborating on this design choice. My gut feeling is that we would want it do be cleaned up automatically.

internal/controller/spinapp_controller.go Show resolved Hide resolved
internal/controller/spinapp_controller.go Outdated Show resolved Hide resolved
internal/cacerts/cacerts.go Outdated Show resolved Hide resolved
@adamreese
Copy link
Member Author

@calebschoepp Having no owner reference is because there isn't anything to assign ownership to. The secret is global for the namespace so it can't be owned by specific applications.

@calebschoepp
Copy link
Contributor

@calebschoepp Having no owner reference is because there isn't anything to assign ownership to. The secret is global for the namespace so it can't be owned by specific applications.

Could it be owned by the operator so that if the operator is removed the certs are removed too?

@adamreese
Copy link
Member Author

The operator isn't aware of it's own kubernetes deployment. It's just a running binary.

// Package cacerts provides an embedded CA root certificates bundle.
package cacerts

//go:generate curl -sfL https://curl.se/ca/cacert.pem -o ca-certificates.crt
Copy link
Contributor

Choose a reason for hiding this comment

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

LazyQ: What license are these under? if it involves attribution are we correctly doing so?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great question. It's built off the mozilla cert bundle which is under Mozilla Public License

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this means the cacerts package also has to be MPL-2.0 (because we embed the MPL-2.0 data in the sources), and MPL-2.0 is (albeit weakly) copyleft.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not exactly sure what we need to do for this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a comment on the variable that the contents are MPL-2.0 licensed might be good enough? (and having a link to the cert bundle is helpful for understanding what they are anyway)

Copy link
Contributor

@michelleN michelleN left a comment

Choose a reason for hiding this comment

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

+1 to adding fields to the SpinAppExecutor around defaulting behavior and secret reference for CA bundle

@bacongobbler
Copy link
Contributor

RE: #207 (comment)

Disclaimer: I am not a lawyer.

I think this covers the section under the MPL-2.0 terms for "Distribution of Source Form" (Section 3.1 of the MPL-2.0), but it does not cover "Distribution of Executable Form".

Section 3.2 of the MPL 2.0 states the following:

If You distribute Covered Software in Executable Form then:

a. such Covered Software must also be made available in Source Code Form, as described in Section 3.1, and You must inform recipients of the Executable Form how they can obtain a copy of such Source Code Form by reasonable means in a timely manner, at a charge no more than the cost of distribution to the recipient; and

b. You may distribute such Executable Form under the terms of this License, or sublicense it under different terms, provided that the license for the Executable Form does not attempt to limit or alter the recipients’ rights in the Source Code Form under this License.

Given that our users would refer to the LICENSE file provided with the executable, I think we need to include it there as well. Either that or include a NOTICE file referring to the MPL-2.0 license and its usage within spin-operator.

@bacongobbler
Copy link
Contributor

Hmm.. Re-reading that section, it doesn't state you need to sub-license the project or include its terms in a NOTICE file. You just need to make the "recipient of the Executable Form" aware of how to receive the source code.

I think we're good on that front... But again, not a lawyer. :)

Copy link
Contributor

@bacongobbler bacongobbler left a comment

Choose a reason for hiding this comment

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

Looks good to me! Left a few quick comments but I think this is safe to merge at this point.

//
// curl -sfL https://curl.se/ca/cacert.pem -o ca-certificates.crt

import _ "embed"
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL this is a part of the standard library now. That's awesome

// ConstructVolumeMountsForApp introspects the application and generates
// any required volume mounts. A generated runtime secret is mutually
// exclusive with a user-provided secret - this is to require _either_ a
// manual runtime-config or a generated one from the crd.
func ConstructVolumeMountsForApp(ctx context.Context, app *spinv1alpha1.SpinApp, generatedRuntimeSecret string) ([]corev1.Volume, []corev1.VolumeMount, error) {
func ConstructVolumeMountsForApp(ctx context.Context, app *spinv1alpha1.SpinApp, generatedRuntimeSecret, caSecretName string) ([]corev1.Volume, []corev1.VolumeMount, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish there was a way to make caSecretName use an Option<string> instead of an empty string to make it more clear to the caller that this is an optional value. I've been spoiled by Rust.

Copy link
Member

Choose a reason for hiding this comment

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

we can always use *string in golang. (before you ask Even I am not sure if I am kidding or being serious here.)

internal/controller/spinapp_controller.go Outdated Show resolved Hide resolved
Supersedes spinkube#184

Automate the creation of a secret for a default CA root certificate
bundle. A secret is created in each namespace that contains a spin
application. If a secret already exists with the name `spin-ca` it will
not be modified. This allows the default `spin-ca` secret to be
overridden by the user.

The embedded CA bundle is fetched from https://curl.se/ca/cacert.pem and
can be updated to the latest by running `go generate ./...`.

There is no owner reference on the secret which means it will persist
unless manually deleted. Meaning that if spin-operator is removed from
the cluster it will not be included in the cascading deletion.

Signed-off-by: Adam Reese <[email protected]>
@michelleN michelleN merged commit 7e8d78e into spinkube:main May 15, 2024
11 checks passed
@adamreese adamreese deleted the feat/inject-ca branch May 15, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants