-
Notifications
You must be signed in to change notification settings - Fork 255
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 handling of cnf
claim
#1092
Conversation
This commit allows to generate fingerprints for CSR files to the `step certificate fingerprint` command.
This commit allows passing confirmation claims to tokens to tie the tokens with a provided CSR or SSH public key. The confirmation claim is implemented in the token command as well as the com commands that uses a given CSR or ssh public key. Those are: - step ca token - step ca sign - step ssh certificate --sign Fixes smallstep/certificates#1637
// Confirmation is a cli.Flag used to add a confirmation claim in the token. | ||
Confirmation = cli.StringFlag{ | ||
Name: "cnf", | ||
Usage: `The <fingerprint> of the CSR to restrict this token for.`, | ||
} | ||
|
||
// ConfirmationFile is a cli.Flag used to add a confirmation claim in the | ||
// tokens. It will add a confirmation kid with the fingerprint of the CSR. | ||
ConfirmationFile = cli.StringFlag{ | ||
Name: "cnf-file", | ||
Usage: `The CSR <file> to restrict this token for.`, | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these include something about the SSH public key, or want to keep it simple?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They used to include it, but due to a comment in the certificates PR that suggested removing the cnf for SSH, I've removed it from here. Although I kept the SSH public key in the context.
if isSSH { | ||
sshPub, _, _, _, err := ssh.ParseAuthorizedKey(in) | ||
if err != nil { | ||
return errors.Wrap(err, "error parsing ssh public key") | ||
} | ||
tokenOpts = append(tokenOpts, cautils.WithSSHPublicKey(sshPub)) | ||
} else { | ||
csr, err := pemutil.ParseCertificateRequest(in) | ||
if err != nil { | ||
return errors.Wrap(err, "error parsing certificate request") | ||
} | ||
tokenOpts = append(tokenOpts, cautils.WithCertificateRequest(csr)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of passing the CSR or SSH public key, the claim itself could be calculated here, and just pass it as the fingerprint directly? Or is there some flow in which to defer calculation of the fingerprint is necessary? step ca sign
, maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
step ca sign
use cautils.WithCertificateRequest(csr)
and step ssh certificate
use cautils.WithSSHPublicKey(sshPub)
, although the SSH one is currently totally ignored. These fields go to the shared context of the cautils
package and end up in the token
package, which takes care of calculating the fingerprint and adding the proper attribute to the final token. Right now, only the CSR gets into the token package, but sending the type allows us to specify the right attribute now that we don't share just kid
.
Co-authored-by: Herman Slatman <hslatman@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK now. Not sure what's up with the test? It seems to want the old kid
claim still?
Allow to add confirmation claims to tokens
This commit allows passing confirmation claims to tokens to tie the
tokens with a provided CSR or SSH public key.
Fixes smallstep/certificates#1637
Related PR: