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

Support issuing/renewing SSH Host Certificates #24

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ckwalsh
Copy link

@ckwalsh ckwalsh commented Dec 28, 2020

This pull request adds support for generating, distributing, and maintaining SSH Host certificates within smallstep/autocert. For ease of review, it is split into several diffs with various goals:

  1. Update step dependencies to 0.15.3. I'm not sure exactly when SSH host support landed, but I figured having the latest version wasn't a bad idea.
  2. Refactor the autocert controller to avoid assumptions it was working with TLS certificates. This logic was moved to structs that implement IdentityRequest, but the general structure/certificate issuing flow was not significantly changed.
  3. Add support for SSH Host certs. This adds the SSHIdentityRequest to the controller, a new bootstrapper, and a new renewer.

These changes should be backwards compatible, and should not break existing deployments.

Future Work:

  1. SSH renewer improvements. Unlike TLS certificates, step ssh doesn't have a renewal daemon mode. Instead, the renewer is a bash script that implements a while-true-sleep-work loop based on env vars. Assuming an ssh renewal daemon is added, it shouldn't be too hard to swap it in.
  2. Documentation and config file updates. Especially need to fill in the SSHHost* fields in the autocert.yaml config.
  3. SSH Client cert support. I implemented the code with the assumption it would be added, but didn't actually do so, as I don't need it. I'll leave it as an exercise to others.
  4. Testing. I updated the unit tests to confirm no breakages, and with some tweaked config files, ran this on my homelab k8s install.

Notes:

  1. This was my holiday project, and it was a lot of fun. I had heard of smallstep, and read yet another article about how certificates > passwords, and finally got around to diving into it a couple days before Christmas. Took a bit of time to grok the certificate distribution flow, but once I did, it's a pretty awesome project you guys built.
  2. This is my first foray into Golang, the code was really easy to understand, modify, and build. Please point out if I broke any significant Go idioms.

@CLAassistant
Copy link

CLAassistant commented Dec 28, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@codecov
Copy link

codecov bot commented Dec 28, 2020

Codecov Report

Merging #24 (e31da80) into master (ef4f27c) will decrease coverage by 1.63%.
The diff coverage is 11.73%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #24      +/-   ##
=========================================
- Coverage    8.00%   6.36%   -1.64%     
=========================================
  Files           2       2              
  Lines         500     581      +81     
=========================================
- Hits           40      37       -3     
- Misses        459     544      +85     
+ Partials        1       0       -1     
Impacted Files Coverage Δ
controller/main.go 6.95% <11.73%> (-2.04%) ⬇️
controller/client.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16d2f60...e31da80. Read the comment docs.

@maraino maraino self-requested a review December 28, 2020 20:31
@ckwalsh
Copy link
Author

ckwalsh commented Jan 16, 2021

Ping?

@maraino
Copy link
Collaborator

maraino commented Jan 20, 2021

@ckwalsh Pong, sorry it went out of my radar. I will take a look asap.

Copy link
Collaborator

@maraino maraino left a comment

Choose a reason for hiding this comment

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

Pretty much ok, I've added some comments, and some questions.

Comment on lines +171 to +175
if req.duration != "" {
b.Env = append(b.Env, corev1.EnvVar{
Name: "STEP_NOT_AFTER",
Value: req.duration,
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will work but I still see references to the DURATION environment variable in bootstrapper/tls/bootstrapper.sh

With the environment variable named STEP_NOT_AFTER we don't need the if condition in the bootstrapper.sh

controller/main.go Show resolved Hide resolved
controller/main.go Show resolved Hide resolved
Comment on lines +21 to +22
STEP_HOST=false step ssh config --roots > $USER_CA
STEP_HOST=true step ssh config --roots > $HOST_CA
Copy link
Collaborator

Choose a reason for hiding this comment

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

Easier to see if we do it like this:

step ssh config --roots > $USER_CA
step ssh config --host --roots > $HOST_CA

# Download the root certificate and set permissions
if [ "$STEP_HOST" == "" ];
then
KEY=$USER_KEY
Copy link
Collaborator

Choose a reason for hiding this comment

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

If user keys are not supported, why not just exit 0 here?


if [ "$STEP_HOST" == "" ];
then
KEY=$USER_KEY
Copy link
Collaborator

Choose a reason for hiding this comment

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

If user keys are not supported, why not just exit 0 here?

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.

3 participants