Implement device-attestations for yubikeys #741
Conversation
Currently the kms yubikey implements the attestation interface, this experimental commit uses that interface to request a certificate using ACME with the device-attest-01 challenge.
github-actions
bot
added
the
needs triage
The signature verifies proof of possession of private key.
| @@ -16,6 +16,11 @@ import ( | |||
| "go.step.sm/crypto/pemutil" | |||
| ) | |||
|
|
|||
| type Attestor interface { | |||
| crypto.Signer | |||
There was a problem hiding this comment.
Embedding this interface could be an issue for a TPM EK, but I'll have to check out its usage.
utils/cautils/acmeutils.go
Outdated
| @@ -193,6 +203,7 @@ func serveAndValidateHTTPChallenge(ctx *cli.Context, ac *ca.ACMEClient, ch *acme | |||
| } | |||
|
|
|||
| func authorizeOrder(ctx *cli.Context, ac *ca.ACMEClient, o *acme.Order) error { | |||
| attest := (ctx.String("attest") != "") | |||
There was a problem hiding this comment.
This can probably be done with ctx.IsSet("attest") instead
There was a problem hiding this comment.
What I don't like about that is that they are not equivalent. You get IsSet set to true if you do --attest ""
There was a problem hiding this comment.
Can we call this variable isAttest or something to indicate that it's a boolean? Normally I wouldn't mind, but the flag input is a string and we're not consistent about how we use it.
There was a problem hiding this comment.
Fixed with 377194e
internal/cryptoutil/cryptoutil.go
Outdated
| @@ -33,6 +38,12 @@ func CreateSigner(kms, name string, opts ...pemutil.Options) (crypto.Signer, err | |||
| return newKMSSigner(kms, name) | |||
| } | |||
|
|
|||
| // CreateAttestor creates an attestor that will use `step-kms-plugin` with the | |||
| // given kms and uri. | |||
There was a problem hiding this comment.
It says given kms and uri, but it takes a kms and name.
There was a problem hiding this comment.
Fixed with 6ad024e
| if err != nil { | ||
| return errors.WithStack(err) | ||
| } | ||
| ui.PrintSelected("Private Key", keyFile) |
There was a problem hiding this comment.
It may be an option to indicate that the private key is created and stored in the KMS for the attestation flow?
There was a problem hiding this comment.
Fixed with 9a45100
| @@ -398,6 +398,11 @@ flag exists so it can be configured in $STEPPATH/config/defaults.json.`, | |||
| Name: "kms", | |||
| Usage: "The <uri> to configure a Cloud KMS or an HSM.", | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Is attest descriptive enough? Should it be something like attestation-uri?
There was a problem hiding this comment.
Fixed with 4fc5893
internal/cryptoutil/cryptoutil.go
Outdated
| @@ -16,6 +16,13 @@ import ( | |||
| "go.step.sm/crypto/pemutil" | |||
| ) | |||
|
|
|||
| // Attestor is the interface implemented by step-kms-plugin using the key, sign | |||
There was a problem hiding this comment.
| // Attestor is the interface implemented by step-kms-plugin using the key, sign | |
| // Attestor is the interface implemented by step-kms-plugin using the key, sign, |
There was a problem hiding this comment.
Fixed with 389fef8
Description
Currently the yubikey KMS implements the attestation interface, this experimental commit uses that interface to request a certificate using ACME with the device-attest-01 challenge.