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

Add epoxy_certs utility for certificate rotation #113

Merged
merged 7 commits into from
Jan 3, 2024

Conversation

stephen-soltesz
Copy link
Contributor

@stephen-soltesz stephen-soltesz commented Jan 2, 2024

This change adds a new utility for creating a private ca for the boot server and rotating the epoxy server certificates.

The logic in this PR is identical to that used to originally generate the current epoxy private ca server certificates, with minor changes for style and lint warnings. Taken directly from https://github.com/stephen-soltesz/epoxy/blob/master/src/epoxy/cmd/epoxycerts/main.go


This change is Reviewable

Copy link
Contributor

@nkinkade nkinkade left a comment

Choose a reason for hiding this comment

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

:lgtm:, I only did a very light scan across the code for obvious errors that I might notice. Instead I'm mostly relying on compiling the code from this PR and using epoxy_certs to generate a key and certificate and then inspecting them to be sure they are what we expect, and they do appear to be.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @stephen-soltesz)


cmd/epoxy_certs/certutil.go line 1 at r1 (raw file):

// Package certutil implements utility methods for managing x509 certificates and RSA keys.

Tiny nit: this comment references package certutil, when the actual package name is main.

Copy link
Contributor Author

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @nkinkade)


cmd/epoxy_certs/certutil.go line 1 at r1 (raw file):

Previously, nkinkade wrote…

Tiny nit: this comment references package certutil, when the actual package name is main.

Good catch, fixed.

Copy link
Contributor

@nkinkade nkinkade left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @stephen-soltesz)


cmd/epoxy_certs/README.md line 50 at r3 (raw file):

```sh
cat ca-cert.pem >> server-cert.pem

This helps, but one small addition would be helpful to mention would be that the user needs to somehow get hold of the CA root cert and private key. I was wondering how it was going to work when I first ran it, and epoxy_certs complained that it couldn't find ca-cert.pem, which is when I reached out to you for their location.

Copy link
Contributor Author

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nkinkade)


cmd/epoxy_certs/README.md line 50 at r3 (raw file):

Previously, nkinkade wrote…

This helps, but one small addition would be helpful to mention would be that the user needs to somehow get hold of the CA root cert and private key. I was wondering how it was going to work when I first ran it, and epoxy_certs complained that it couldn't find ca-cert.pem, which is when I reached out to you for their location.

Done. PTAL?

Copy link
Contributor

@nkinkade nkinkade left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, 1 of 1 files at r5, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @stephen-soltesz)


cmd/epoxy_certs/README.md line 53 at r5 (raw file):

```sh
cat ca-cert.pem >> server-cert.pem

On second though, one other small change would be for this command to instead be:

cat server-cert.pem ca-cert.pem > server-certs.pem

The file generated by epoxy_certs is server-cert.pem (singular), while the required file in the private bucket is server-certs.pem (plural).

In this vein, it might also be useful to mention here where the new files should be placed in GCS, and that a redeployment of epoxy will after that be sufficient to deploy them.

Copy link
Contributor

@nkinkade nkinkade left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @stephen-soltesz)

Copy link
Contributor Author

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @nkinkade)


cmd/epoxy_certs/README.md line 53 at r5 (raw file):

Previously, nkinkade wrote…

On second though, one other small change would be for this command to instead be:

cat server-cert.pem ca-cert.pem > server-certs.pem

The file generated by epoxy_certs is server-cert.pem (singular), while the required file in the private bucket is server-certs.pem (plural).

In this vein, it might also be useful to mention here where the new files should be placed in GCS, and that a redeployment of epoxy will after that be sufficient to deploy them.

That's a very good idea!

I've also added a hint about uploading to GCS for M-Lab deployments.

@stephen-soltesz stephen-soltesz merged commit 459c96b into main Jan 3, 2024
4 checks passed
@stephen-soltesz stephen-soltesz deleted the sandbox-soltesz-add-epoxy_certs branch January 3, 2024 21:32
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.

None yet

2 participants