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 a p12 command to create PKCS12 store files #372

Merged
merged 2 commits into from
Oct 19, 2020
Merged

Add a p12 command to create PKCS12 store files #372

merged 2 commits into from
Oct 19, 2020

Conversation

doug-fitzmaurice-rowden
Copy link
Contributor

@doug-fitzmaurice-rowden doug-fitzmaurice-rowden commented Oct 15, 2020

Description

This PR adds a step certificate p12 subcommand to support bundling keys and certificates together into PKCS12 / p12 / pfx files, as discussed in #244

These are commonly used for installing client certificates into Firefox / Windows, and for configuring server certificates for Java applications.

The arguments / options are an attempt at being easy to use, without forcing the user to understand all the different types of p12 that exist.

A password is specifiable on the CLI as one is required for .p12, but is a bit pointless as the encryption is weak. The author of the upstream library this PR uses suggests using a nominal password then protecting the result file using a stronger external method:
https://github.com/SSLMate/go-pkcs12/blob/master/pkcs12.go#L33

@CLAassistant
Copy link

CLAassistant commented Oct 15, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Oct 15, 2020

Codecov Report

Merging #372 into master will decrease coverage by 0.27%.
The diff coverage is 42.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #372      +/-   ##
==========================================
- Coverage   64.20%   63.93%   -0.28%     
==========================================
  Files          55       56       +1     
  Lines        6613     6696      +83     
==========================================
+ Hits         4246     4281      +35     
- Misses       2141     2189      +48     
  Partials      226      226              
Impacted Files Coverage Δ
command/certificate/p12.go 41.46% <41.46%> (ø)
command/certificate/certificate.go 100.00% <100.00%> (ø)
flags/flags.go 30.18% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51e2230...ac07ffb. Read the comment docs.

@maraino maraino self-requested a review October 15, 2020 18:17
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.

@doug-fitzmaurice-rowden This looks great.
I've proposed a couple of changes for consistency.

Comment on lines 59 to 62
cli.StringFlag{
Name: "password",
Usage: `Set the password to encrypt the .p12 file.`,
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency with other commands, it makes more sense to use --password-file, and then just use utils.ReadStringPasswordFromFile(filename string) (string, error).

Comment on lines 112 to 115
x509Cert, err := pemutil.ReadCertificate(crtFile)
if err != nil {
return errors.Wrap(err, "error reading certificate")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

An X.509 certificate is usually bundled with an intermediary, we do that by default if you are using step-ca. I think it makes sense that if we pass a bundled certificate, we should include the intermediary automatically in the list of CAs. Don't you think?
You can use pemutil.ReadCertificateBundle(filename string) ([]*x509.Certificate, error) to get all certificates.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What would happen if a CA is added multiple times? For example one from the cert and one from the --ca flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure - I suspect it will be fine as PKCS12 is a dumb collection of certificates. I'll build a few different types and do some tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding bundles - if a bundle is passed to the --ca option it's easy - append all the certs to the CA list.
If one is passed to <crt_file>, we need to work out which one the key applies to, and which is the intermediate as they get stored in different places in the result.

Is the best way to do this extract the public key from the certs and the private key, and compare to find a match? I had a look around the codebase but I couldnt see any existing code that does such a check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A X.509 certificate is usually a bundle like:

-----BEGIN CERTIFICATE-----
MII.... the server certificate
-----END CERTIFICATE-----
-----BEGIN CERTIFICATE-----
MII.... the intermediate certificate
-----END CERTIFICATE-----

It can also include multiple intermediates at the end. For example, in TLS this is required because your browser only knows the root certificates and he needs the intermediate to build the validation path.

Check for example:

step certificate inspect --format pem --bundle https://www.google.com/

So using a PKI with the structure root->intermediate->leaf and using a CA like step-ca it will make sense to create pkcs#12 certificate like:

step ca certificate mariano@smallstep.com mariano.crt mariano.key
step certificate p12 mariano.crt mariano.key --ca $(step path)/certs/root_ca.crt

Copy link
Collaborator

@maraino maraino Oct 16, 2020

Choose a reason for hiding this comment

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

The server cert is always the first. The list of intermediates are in order at the end.
server ->signed by-> int1 -> signed by ->int2-> ... and then signed by --ca

@doug-fitzmaurice-rowden
Copy link
Contributor Author

Thanks @maraino - I've pushed another commit with those changes.

@maraino maraino self-requested a review October 19, 2020 17:21
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.

@maraino maraino merged commit 3eaafc7 into smallstep:master Oct 19, 2020
@maraino
Copy link
Collaborator

maraino commented Oct 19, 2020

@doug-fitzmaurice-rowden the PR is merged to master, we will probably do a new release soon that will include step certificates p12.
Thanks again.

@doug-fitzmaurice-rowden
Copy link
Contributor Author

Fantastic, thanks so much!

I have a couple of PRs pending on the upstream library that will hopefully improve compatibility with some Java tools that consume these .p12s.

If / when those are merged I'll put up another quick PR here to update the version of go-pkcs12 used.

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.

3 participants