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 for conversion between P12, PEM, and DER #589

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

Conversation

dopey
Copy link
Contributor

@dopey dopey commented Dec 2, 2021

Original PR #574

@dopey dopey requested a review from maraino December 2, 2021 19:03
@github-actions github-actions bot added the needs triage Waiting for discussion / prioritization by team label Dec 2, 2021
@dopey
Copy link
Contributor Author

dopey commented Dec 2, 2021

@maraino @z8674558 I've reviewed and generally everything looks good to me. I committed some grammar / documentation changes on top.

The only additional question / comment I have is whether we should use --decrypt-password-file and --encrypt-password-file? Otherwise I can see it being confusing whether --password-file is the flag to decrypt the old file or to encrypt the new p12 or key file. For example, you may want password-file to decrypt the input and --no-password to leave the output unencrypted - currently this would throw an error.

Maybe our answer to the above is that we always use the same encryption on the output as was used on the input, but then there's no need for --no-password and --insecure.

@dopey dopey self-assigned this Dec 2, 2021
@dopey dopey removed the needs triage Waiting for discussion / prioritization by team label Dec 2, 2021
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.

Usage is not clear, sometimes flags are used as input sometimes as output, It should be consistent and print always to standard output or use the --out flag.

command/certificate/format.go Show resolved Hide resolved
command/certificate/format.go Show resolved Hide resolved
command/certificate/format.go Outdated Show resolved Hide resolved
command/certificate/format.go Outdated Show resolved Hide resolved
command/certificate/format.go Outdated Show resolved Hide resolved
command/certificate/format.go Show resolved Hide resolved
@dopey
Copy link
Contributor Author

dopey commented Dec 3, 2021

Usage is not clear, sometimes flags are used as input sometimes as output, It should be consistent and print always to standard output or use the --out flag.

@maraino Doesn't that contradict what we laid out here: #574 (comment)?

@github-actions github-actions bot added the needs triage Waiting for discussion / prioritization by team label Dec 3, 2021
@dopey dopey requested a review from maraino December 8, 2021 18:19
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.

It looks like the command is not backward compatible.

Comment on lines +68 to +78
Convert a .p12 file to a certificate and private key:

'''
$ step certificate format foo.p12 --crt foo.crt --key foo.key --format pem
'''

Convert a .p12 file to a certificate, private key and intermediate certificates:

'''
$ step certificate format foo.p12 --crt foo.crt --key foo.key --ca intermediate.crt --format pem
'''
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like if the flag --format is not passed it will write PEM files, as it should be. We should show this in one of these two examples, explaining that is the default behavior.


// If format is PEM or DER (not P12) then an input certificate file is required.
if format != "p12" {
return errors.Errorf("flag --format with value '%s' requires a certificate file as positional argument", format)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Commands like these will stop working:

$ cat cert.pem | step certificate format
... der data ...
$ cat cert.der | step certificate format
... pem data ...

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.

3 participants