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 certificate extract command #574

Closed
wants to merge 1 commit into from
Closed

Conversation

6293
Copy link

@6293 6293 commented Nov 6, 2021

Fixes #487

Description

Add step certificate extract command to extract a certificate and private key from a .p12 file, with corresponding integration tests.

@CLAassistant
Copy link

CLAassistant commented Nov 6, 2021

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the needs triage Waiting for discussion / prioritization by team label Nov 6, 2021
@dopey
Copy link
Contributor

dopey commented Nov 10, 2021

Hey @z8674558 thanks for the PR! Love that you added unit tests. You'll notice that we're not great at that ourselves.

The first bit of feedback is that we'd like this command to be under the step certificate format command group. So p12 would be an option under format (the way that PEM and DER are). Is that a change you wouldn't mind making?

@6293
Copy link
Author

6293 commented Nov 11, 2021

hi @dopey, that makes sense. I updated my code.

do we want to also move step certificate p12 command to format?

@6293
Copy link
Author

6293 commented Nov 11, 2021

To avoid breaking backward compatibility, I added --crt, --key, --ca option to format command. Presence of these options means that the positional argument is .p12 file.

@maraino
Copy link
Collaborator

maraino commented Nov 11, 2021

We don't want to move step certificate p12 but we might want to add support for it in format too. My proposal would be something like:

# Current functionality:
## Format PEM to DER
step certificate format internal.pem [-out internal.der]
## Format DER to PEM
step certificate format internal.der [-out internal.pem]

# New functionality:
## Format PEM to P12
## We can default to p12 if --key or --ca are passed
step certificate format internal.pem [--format p12] --key internal.key --ca ca.pem [-out internal.p12]
## Format P12 to PEM
## We can default to PEM if the input is p12
### Print crt, ca, and key to stdout or file (in that order)
step certificate format internal.p12 [--format pem] [--out internal.pem]
### Print crt, ca, and key to different files
step certificate format internal.p12 --crt internal.pem --key internal.key --ca ca.pem
step certificate format internal.p12 --out internal.pem --key internal.key --ca ca.pem
### If this is confusing we might want to do
step certificate format internal.p12 --out internal.pem --out-key internal.key --out-ca ca.pem

If you don't pass the out it will print in standard out. And of course, you can also do --format der, but in this case, you cannot print to stdout multiple files, just one.

@mmalone @dopey @z8674558 What do you think about this?

@6293
Copy link
Author

6293 commented Nov 12, 2021

@maraino Looks nice, I updated my code. One concern from me is that this approach makes hard to print a single file to stdout. e.g. one have to do the following to print only a certificate from a p12 file:

step certificate format foo.p12 --out-key /tmp/out.key --out-ca /tmp/out.ca

I would propose that instead of printing to stdout when the flags are absent, we allow empty value in the flags. In the above example,

step certificate format foo.p12 --out

will print the certificate to stdout, ignoring the key and the intermediate CAs. With this approach, And of course, you can also do --format der, but in this case, you cannot print to stdout multiple files, just one. would be simpler, too.

@mmalone
Copy link
Contributor

mmalone commented Nov 12, 2021

Yea, so, to state the obvious this is a little clunky because P12s can have CAs & keys while PEM/DER is typically just one cert. I think having both --out <file> and meaning "send everything to <file>" and --crt <crt-file> --key <key-file> --ca <ca-file> to split stuff out is fine.

If you combine these flags (e.g., --out foo --key bar --ca baz I'd be ok with either 1) failing with an error (--out is incompatible with the other three flags) or writing everything to foo (crt, key, ca) and writing the key to bar and the CA to baz. In other words, it feels more reasonable to keep the semantics of --out <file> as "write everything you have to file" even when combined with --crt, --key, or --ca rather than changing the semantics of the --out flag depending on whether those other flags are present.

@z8674558, for your use case (writing an individual file to stdout) could we support - as a value to the various flags. For example, step certificate format foo.p12 --crt - would write just the key to stdout? Or is that weird? Does - usually mean "read from stdin" or can it also mean "write to stdout"?

@6293
Copy link
Author

6293 commented Nov 15, 2021

@mmalone Thank you for the insight.

If you combine these flags (e.g., --out foo --key bar --ca baz I'd be ok with either 1) failing with an error (--out is incompatible with the other three flags) or writing everything to foo (crt, key, ca) and writing the key to bar and the CA to baz. In other words, it feels more reasonable to keep the semantics of --out as "write everything you have to file" even when combined with --crt, --key, or --ca rather than changing the semantics of the --out flag depending on whether those other flags are present.

I adopted the strategy 1) for I don't think many people want to save crt, key, and ca into a single file. And they can just cat when they want to.

@z8674558, for your use case (writing an individual file to stdout) could we support - as a value to the various flags. For example, step certificate format foo.p12 --crt - would write just the key to stdout? Or is that weird? Does - usually mean "read from stdin" or can it also mean "write to stdout"?

This looks nice and I tried it. It turned out, however, urfave/cli does not support a hyphen right after an option flag (this is a bug, see urfave/cli#1092). Maybe we have to get by with the workaround for now.

@dopey
Copy link
Contributor

dopey commented Nov 17, 2021

Hey @z8674558 we really appreciate the contribution!

We're still reviewing, but in the mean time we wanted to send you some swag to show our appreciation. If you're interested, ping me (max@smallstep.com). Cheers!

@dopey dopey removed the needs triage Waiting for discussion / prioritization by team label Nov 18, 2021
@dopey dopey requested review from dopey and maraino November 24, 2021 18:09
@dopey dopey self-assigned this Nov 24, 2021
Name: "format",
Action: command.ActionFunc(formatAction),
Usage: `reformat certificate`,
UsageText: `**step certificate format** <crt_file> [**--out**=<file>]`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's leave the first arg as <crt-file>. That name is consistent across the other commands, and I'm changing the other commands to use - instead of _.

Copy link
Author

Choose a reason for hiding this comment

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

done

## POSITIONAL ARGUMENTS

<crt_file>
: Path to a certificate or CSR file.
<src_file>
Copy link
Contributor

Choose a reason for hiding this comment

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

No description provided.

@github-actions github-actions bot added the needs triage Waiting for discussion / prioritization by team label Dec 2, 2021
@dopey
Copy link
Contributor

dopey commented Dec 2, 2021

Created a new branch / PR because I wanted to commit some documentation changes - #589

@dopey
Copy link
Contributor

dopey commented Dec 2, 2021

Going to close this one in favor of the other if that's all right. Let me know if I should reopen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage Waiting for discussion / prioritization by team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[add] a method to extract certs and keys from a .p12 file
5 participants