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 --ca-kms flag to step certificate create #942

Merged
merged 19 commits into from
Aug 9, 2023
Merged

Conversation

hslatman
Copy link
Member

@hslatman hslatman commented May 17, 2023

The --ca-kms flag can be used to specify a different KMS to be used for the CA signer.

@maraino What are your thoughts on this? Replacing kms with caKMS is a backwards incompatible change. Not a big issue I think, because this flow is most probably used when invoking step manually. I think being able to specify a different KMS for the CA signing the certificate is useful, because the current assumption seems to be that keys will be in the same KMS. That may not be true necessarily, if someone wants their root offline (e.g. an USB token) and the intermediate to be in an online system.

An alternative would be to override the --kms flag with the value from --ca-kms if it's set. One thing that won't work in that case is when we don't specify a --ca-kms, but still want to use the SoftKMS implementation. We could catch the --ca-kms softkms: case before invoking step-kms-plugin to allow for that? Another option might be toprovide another flag (e.g. --ignore-kms) to indicate that the --kms flag must not be used as the CA KMS.

This PR includes the changes from #945 and #946.

The `--ca-kms` flag can be used to specify a different KMS to
be used for the CA signer.
@github-actions github-actions bot added the needs triage Waiting for discussion / prioritization by team label May 17, 2023
@hslatman hslatman changed the title Add --ca-kms flag Add --ca-kms flag to step certificate create May 17, 2023
Before this commit the CSR (used internally to prepare the final
certificate to be signed) was signed using the key that would sign
the final certificate instead of by the key to be signed. This
commit passes in `priv` instead of `signer`.

This could lead to non-backwards compatible issues, but I think
those shouldn't happen often.
@maraino
Copy link
Collaborator

maraino commented May 18, 2023

As we talked, it makes sense to have both --kms and --ca-kms, one for creating the CSR and the other for signing it.

A better solution would be not to require them, but this will require changes in other places.

hslatman and others added 3 commits May 25, 2023 15:59
When creating a certificate for a public key backed by a KMS
that doesn't allow the key to also be used for signing, or in
cases where the private key isn't readily available to sign the
CSR, `--skip-csr-signature` can be passed to skip signing the
(internally used) CSR.

This option is not compatible with `--csr`, because that requires
a CSR with a valid signature to be produced.
Instead of relying on a new implementation based on generics,
smallstep/crypto#248 was created to have
a minimal implementation for supporting signing public keys.
@hslatman
Copy link
Member Author

hslatman commented May 26, 2023

After the changes in smallstep/crypto#240 are available here, --ca-kms may no longer be required. A key in a different KMS can then be specified using the --key <kms:>name notation. We could keep it around, though.

#946 will be merged into this first, then this PR will be updated.

@hslatman hslatman removed the needs triage Waiting for discussion / prioritization by team label May 26, 2023
@maraino
Copy link
Collaborator

maraino commented May 30, 2023

After the changes in smallstep/crypto#240 are available here, --ca-kms may no longer be required. A key in a different KMS can then be specified using the --key kms:name notation. We could keep it around, though.

To remove the need of --ca-kms we will need to change the cryptoutil package to directly support URIs. See

func CreateSigner(kms, name string, opts ...pemutil.Options) (crypto.Signer, error) {
if kms == "" {
s, err := pemutil.Read(name, opts...)
if err != nil {
return nil, err
}
if sig, ok := s.(crypto.Signer); ok {
return sig, nil
}
return nil, fmt.Errorf("file %s does not contain a valid private key", name)
}
return newKMSSigner(kms, name)
}

@hslatman hslatman requested a review from maraino May 31, 2023 20:53
@hslatman hslatman enabled auto-merge May 31, 2023 22:08
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.

Add new flags at the list of flags at the beginning of UsageText.

This is not 100% backward compatible with the previous use of --kms but this might be ok. The problem is that you might need to specify both flags when before you only needed one. It was convenient for the same KMS but was problematic for cross-kms signing.

@hslatman hslatman added this to the v0.24.5 milestone Aug 1, 2023
@hslatman hslatman self-assigned this Aug 1, 2023
@hslatman hslatman requested a review from maraino August 9, 2023 11:02
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.

The current behavior is not backward compatible as the changelog says, but the help still shows examples that now will fail.

profile = ctx.String("profile")
template = ctx.String("template")
kms = ctx.String("kms")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change makes the command incompatible with at least one example in the help:

# Create an intermediate certificate using step-kms-plugin:

$ step kms create \
  --kms 'pkcs11:module-path=/usr/local/lib/softhsm/libsofthsm2.so;token=smallstep?pin-value=password' \
  'pkcs11:id=4001;object=intermediate-key'
$ step certificate create \
  --profile intermediate-ca \
  --kms 'pkcs11:module-path=/usr/local/lib/softhsm/libsofthsm2.so;token=smallstep?pin-value=password' \
  --ca root_ca.crt --ca-key 'pkcs11:id=4000' \
  --key 'pkcs11:id=4001' \
  'My KMS Intermediate' intermediate_ca.crt

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 290f81d

@hslatman hslatman requested a review from maraino August 9, 2023 19:27
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.

lgtm

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.

2 participants