Initial PKI backend implementation by jefferai · Pull Request #310 · hashicorp/vault

jefferai · 2015-06-03T14:09:28Z

Pinging #125.

Complete:

Up-to-date API documents
Backend configuration (root certificate and private key)
Highly granular role configuration
Certificate generation
CN checking against role
IP and DNS subject alternative names
Server, client, and code signing usage types
Later certificate (but not private key) retrieval
CRL creation and update
CRL/CA bare endpoints (for cert extensions)
Revocation (both Vault-native and by serial number)
CRL force-rotation endpoint
Acceptance tests (a few more could be added, but they're fairly comprehensive‡)

Missing:

OCSP support (can't implement without changes in Vault)

‡ Note: I believe revocation tests are impossible with the current logical testing framework, so this is a big missing piece of acceptance tests.

armon · 2015-06-03T21:36:05Z

Awesome! I will review shortly and provide any feedback I have!

jefferai · 2015-06-04T19:14:00Z

I've created a small test program for an internal demo here, and thought I'd share it: https://gist.github.com/jefferai/e2bebc3bb97fed521666.

You'll obviously need correct roles set up, and an appropriate CA certificate (see advice about CRL endpoints in the documentation).

Also, I pushed some bug fixes into this -- since there are no reviews/comments yet, I simply rebased. Once there are reviews/comments I will commit fixes normally.

jefferai · 2015-06-05T21:01:18Z

@armon With a few minor exceptions, acceptance tests are done. At some point we should discuss the current testing framework because I don't see any way to actually do revocation testing.

Complete: * Up-to-date API documents * Backend configuration (root certificate and private key) * Highly granular role configuration * Certificate generation * CN checking against role * IP and DNS subject alternative names * Server, client, and code signing usage types * Later certificate (but not private key) retrieval * CRL creation and update * CRL/CA bare endpoints (for cert extensions) * Revocation (both Vault-native and by serial number) * CRL force-rotation endpoint Missing: * OCSP support (can't implement without changes in Vault) * Unit tests Commit contents (C)2015 Akamai Technologies, Inc. <opensource@akamai.com>

* CA bundle uploading * Basic role creation * Common Name restrictions * IP SAN restrictions * EC + RSA keys * Various key usages * Lease times * CA fetching in various formats * DNS SAN handling Also, fix a bug when trying to get code signing certificates. Not tested: * Revocation (I believe this is impossible with the current testing framework) Commit contents (C)2015 Akamai Technologies, Inc. <opensource@akamai.com>

armon · 2015-06-11T21:36:35Z

It would be amazing if /pki/config/ca support an option to just generated a new CA. This way, you can let Vault auto-generate the CA, and pull out the certificate and install it on new clients dynamically (say with Terraform). No need to ever touch OpenSSL.

armon · 2015-06-11T21:38:15Z

This is crl/rotate in the docs, not sure which it should be

Hah, the problem with not yet having unit tests for revocation. I landed on "rotate" so I'll update that here (it's that path in the actual Path struct, just didn't update it here).

armon · 2015-06-11T22:13:15Z

Left a few comments, but this looks SUPER solid! Very excited to get it in

…use an RWMutex but punted, because it's not worth trying to save some milliseconds with the possibility of getting something wrong. So the entire operations are now wrapped, which is minimally slower but very safe. Commit contents (C)2015 Akamai Technologies, Inc. <opensource@akamai.com>

jefferai · 2015-06-12T02:32:24Z

@armon I agree that it would be nice if the backend generated a certificate for you; however, I wanted to avoid dealing with the number of variables, or with templating, and with having to have users then configure a base domain to use when generating CRL endpoints, and so on. It seems like a good future enhancement...it requires actually processing a good amount of data if you want certs that support all of the various configuration options, and someone that will be using this will probably not be too scared by a quick openssl command especially using easy-rsa...

…o that it is usable by other backends that want to use it to get the necessary data for TLS auth. Also, enhance the raw cert bundle => parsed cert bundle to make it more useful and perform more validation checks. More refactoring could be done within the PKI backend itself, but that can wait. Commit contents (C)2015 Akamai Technologies, Inc. <opensource@akamai.com>

jefferai · 2015-06-17T20:27:49Z

I've updated the gist (https://gist.github.com/jefferai/e2bebc3bb97fed521666) to use the functions in helper/certutil to deal with the returned data. The result is a 60-line (33%) reduction in code and significantly less complexity for most users; this hopefully shows how moving these functions to a helper location is a win.

Note that although it has helper functions to directly parse the output from the PKI backend, in the Cassandra backend I'm coding it actually doesn't need to use these; the user can upload the returned JSON from the PKI backend or upload a PEM bundle directly with a cert that does not need to be sourced from the PKI backend; in either case the functions are still extremely useful, and huge time/LoC savers.

The gist is now a fully error-checking cert-fetching client/server TLS auth program in 125 lines.

armon · 2015-06-18T00:10:03Z

We should have tests for all the methods in the certutil package!

All of that code is tested by the tests in the pki package, since that package is heavily dependent upon it and the code was split out from it. But if you want separate tests, they could happen.

It would be great to get the tests in this package separately. This way we can refactor each layer of the code without trying to remember what package tests what other packages.

… up. This is useful/important for e.g. the Cassandra backend, where you may want to do TLS with a specific CA cert for server validation, but not actually do client authentication with a client cert. Commit contents (C)2015 Akamai Technologies, Inc. <opensource@akamai.com>

armon · 2015-06-18T21:57:43Z

If the certificate is revoked just due to age (e.g. expiration time hit) then we probably do not need to generate a CRL entry. The underlying certificate is already invalid anyways. It's only for an early revocation that CRL must be generated. (Correct me if I'm wrong)

Certificates that are expired are not added to the CRL. (In fact, when building the CRL, any revoked certificates that are expired are simply removed from the underlying store.)

armon · 2015-06-18T22:34:52Z

@jefferai Left a bit more feedback. It looks super solid. The major feedback points would be:

Testing for the individual packages. This way you can modify a single package and run its tests without needing to understand the interdependencies.
Do we need the revoked/ endpoints?
Please add a short comment to every function describing what it does for style

Otherwise we are pretty much on the finish line!

* Add comments to every non-obvious (e.g. not basic read/write handler type) function * Remove revoked/ endpoint, at least for now * Add configurable CRL lifetime * Cleanup * Address some comments from code review Commit contents (C)2015 Akamai Technologies, Inc. <opensource@akamai.com>

jefferai · 2015-06-19T16:49:50Z

At this point I believe I have addressed all feedback except for testing for the certutil package.

jefferai · 2015-06-19T20:08:10Z

Unit tests for certutil are done. I think that solves all outstanding issues.

armon · 2015-06-21T10:12:18Z

Awesome work! Let's get this in!

Initial PKI backend implementation

jefferai · 2015-06-22T14:03:29Z

@armon You're supposed to be on vacation. :-)

jefferai force-pushed the f-pki branch from 2fd8694 to 657cd15 Compare June 3, 2015 19:55

jefferai force-pushed the f-pki branch 2 times, most recently from 2b04841 to 203b994 Compare June 4, 2015 17:15

jefferai mentioned this pull request Jun 4, 2015

PKI secret backend #125

Closed

jefferai force-pushed the f-pki branch 3 times, most recently from 08156fa to 3f3f3df Compare June 5, 2015 20:58

jefferai force-pushed the f-pki branch from 3f3f3df to de4486e Compare June 5, 2015 21:22

Jeff Mitchell added 2 commits June 8, 2015 00:06

jefferai force-pushed the f-pki branch from de4486e to 20ac7a4 Compare June 8, 2015 04:06

armon reviewed Jun 11, 2015
View reviewed changes

Jeff Mitchell added 3 commits June 11, 2015 21:17

Fix some out-of-date examples.

0ee9735

Commit contents (C)2015 Akamai Technologies, Inc. <opensource@akamai.com>

Address most of Armon's initial feedback.

722eca1

Commit contents (C)2015 Akamai Technologies, Inc. <opensource@akamai.com>

Erp, forgot this feedback...

8b55d33

Commit contents (C)2015 Akamai Technologies, Inc. <opensource@akamai.com>

jefferai force-pushed the f-pki branch 3 times, most recently from c6f510f to 817c406 Compare June 17, 2015 19:44

jefferai force-pushed the f-pki branch from 817c406 to 31e6800 Compare June 17, 2015 20:07

armon reviewed Jun 18, 2015
View reviewed changes

Restructure a little bit to make the helper library fully standalone.…

c425660

… This makes it easier to move around later if desired, and for use by external programs. Commit contents (C)2015 Akamai Technologies, Inc. <opensource@akamai.com>

jefferai force-pushed the f-pki branch 4 times, most recently from bd9bcb8 to 473d997 Compare June 18, 2015 19:21

jefferai force-pushed the f-pki branch from 473d997 to 23ba605 Compare June 18, 2015 19:23

armon reviewed Jun 18, 2015
View reviewed changes

Merge remote-tracking branch 'upstream/master' into f-pki

d8ed14a

jefferai mentioned this pull request Jun 19, 2015

Cassandra logical backend #363

Merged

Add unit tests for certutil, and fix a whitespace stripping issue.

1559456

Commit contents (C)2015 Akamai Technologies, Inc. <opensource@akamai.com>

armon added a commit that referenced this pull request Jun 21, 2015

Merge pull request #310 from jefferai/f-pki

01592c0

Initial PKI backend implementation

armon merged commit 01592c0 into hashicorp:master Jun 21, 2015

kgutwin mentioned this pull request Jun 30, 2015

Error 500 when creating PKI role from CLI #381

Closed

Conversation

jefferai commented Jun 3, 2015

Uh oh!

armon commented Jun 3, 2015

Uh oh!

jefferai commented Jun 4, 2015

Uh oh!

jefferai commented Jun 5, 2015

Uh oh!

armon commented Jun 11, 2015

Uh oh!

armon Jun 11, 2015

Choose a reason for hiding this comment

Uh oh!

jefferai Jun 12, 2015

Choose a reason for hiding this comment

Uh oh!

armon commented Jun 11, 2015

Uh oh!

jefferai commented Jun 12, 2015

Uh oh!

jefferai commented Jun 17, 2015

Uh oh!

armon Jun 18, 2015

Choose a reason for hiding this comment

Uh oh!

jefferai Jun 18, 2015

Choose a reason for hiding this comment

Uh oh!

armon Jun 18, 2015

Choose a reason for hiding this comment

Uh oh!

armon Jun 18, 2015

Choose a reason for hiding this comment

Uh oh!

jefferai Jun 19, 2015

Choose a reason for hiding this comment

Uh oh!

armon commented Jun 18, 2015

Uh oh!

jefferai commented Jun 19, 2015

Uh oh!

jefferai commented Jun 19, 2015

Uh oh!

armon commented Jun 21, 2015

Uh oh!

jefferai commented Jun 22, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants