Adding support for chained intermediate CAs in pki backend #1694

chrishoffman · 2016-08-05T18:01:08Z

The current pki backend does not have the ability to provide the full trust chain for the certificate authority. The changes that were made were all additive and should maintain backwards compatibility for everyone who does not provide this information to their backend.

This pull request includes a rewrite portions of the PEM parser to support reading, storing, and verifying the entire trust chain. In doing so, the parser is a little more prescriptive about how the PEM bundles should be constructed. Previously, the parser used some assumptions to populate the certificate bundle regardless of order. Now, certificates are required to be provided in order with the leaf certificate first. This follows what the tls.Certificate package does. As a benefit to this change, some of the code relating to incorrect assumptions the parser was making have been removed.

This PR relates to this original thread.

jefferai · 2016-08-18T18:56:36Z

Hi @chrishoffman ,

It doesn't seem to me like this needs to be separate endpoints. Is there any reason we don't simply accept a chain in the normal CA upload endpoint?

chrishoffman · 2016-08-19T01:01:38Z

I think you are right, we don't need the endpoints. It works in both places but I thought it was still required when you generated your private key using the /pki/intermediate/generate endpoint. It looks like you can provide the full certificate chain when uploading the signed cert with /pki/intermediate/set-signed or providing the full cert with key with /pki/config/ca.

I'll remove the /pki/config/chain endpoints.

jefferai · 2016-08-19T01:38:24Z

It looks like you can provide the full certificate chain when uploading the signed cert with /pki/intermediate/set-signed or providing the full cert with key with /pki/config/ca.

I think it doesn't now, but ought to be :-D Hence the point of this PR.

chrishoffman · 2016-08-19T02:26:45Z

The endpoints are out. I need add some documentation to the two upload endpoints but will wait to see if there are any major changes that need to be made.

jefferai · 2016-08-24T02:09:36Z

builtin/logical/pki/cert_util.go

+func fetchCAInfo(req *logical.Request) (*caInfoBundle, error) {
+	caBundle, err := fetchCABundle(req)
+	if err != nil {
+		return nil, errutil.InternalError{Err: fmt.Sprintf("unable to fetch CA bundle: %v", err)}


Just pass this through; the error might be Internal or User, and we don't want to lose that. The error message here isn't adding much.

I was waiting for more feedback since this was a simple one :), I knew there would be more.

jefferai · 2016-09-13T16:14:07Z

builtin/logical/pki/cert_util.go

-		result.IssuingCA = creationInfo.SigningBundle.Certificate
+		certPath := creationInfo.SigningBundle.GetCertificatePath()
+		result.IssuingCABytes = certPath[0].Bytes
+		result.IssuingCA = certPath[0].Certificate


For safety, can you ensure that the length of certPath is at least one here? Better to return an error than panic.

jefferai · 2016-09-13T16:38:08Z

Overall this is looking pretty good. Most of my comments are cleanup, a few potential behavioral changes.

chrishoffman · 2016-09-16T00:34:04Z

I updated the DER format logic to return the CA chain certificate when there is only one certificate in the chain. If we decide to keep the issuing CA out of the chain, this will still add one level to the DER format and unlimited for PEM.

@jefferai What do you think about the prepending the issuing CA to the chain? I'm not sure what the expectation should be.

jefferai · 2016-09-16T16:12:36Z

Weeeeeeeeeeeeeeeel...

Before Apache gained support for just reading appended PEM files containing the full bundle, it had SSLCertificateChainFile, which:

This directive sets the optional all-in-one file where you can assemble the certificates of Certification Authorities (CA) which form the certificate chain of the server certificate. This starts with the issuing CA certificate of the server certificate and can range up to the root CA certificate. Such a file is simply the concatenation of the various PEM-encoded CA Certificate files, usually in certificate chain order.

Of course, that's deprecated.

Then you have things like the Java keystore which form chains internally based on key relationships but you only add in one cert at a time.

Generally, non-Java services these days seem to trend towards full chains specified in the file, starting with the actual server cert (and maybe with the key elsewhere). Although if you have the root CA in the file, Qualsys will ding you.

I think, maybe, the right thing to do is the following. This would work across formats, with pem_bundle modifying certificate in the obvious way (cert + full chain). Note that this doesn't necessarily need to correspond to the internal representation; this is my suggestion for what is output across the wire.

certificate: The actual certificate
issuing_ca: The issuing CA
ca_chain: An array of CA certs, including the issuing CA

Having it be an array makes it harder to parse on the command line, but still very easy to deal with via the native JSON API; makes it simple to reason about what to do in the DER case; and anyone not wanting the immediate CA can just start indexing at 1 instead of 0.

And, I think the 99% case generally will be satisfied by certificate with the pem_bundle output format anyways.

I'm totally open for debate here, let me know what you think.

chrishoffman · 2016-09-16T16:58:36Z

Let's go with your suggestion. I can't disagree with anything you said and also believe that the pem_bundle is the majority use case. I'll try and finish this up today.

chrishoffman · 2016-09-20T16:16:32Z

I feel good about the current state and now that the major changes have stabilized, I may look at adding a few more tests. Let's let it burn in for a few days.

jefferai · 2016-09-20T16:25:21Z

Sounds good. We're looking to close 0.6.2 soonish so I'll give a ping in a few days.

stepanstipl · 2016-09-23T10:49:41Z

Hi, this looks great, thanks, pretty much exactly what I was missing in Vault.

I have one suggestion:

add option to retrieve ca certificate with a complete chain - /ca(/pem) endpoints, possibly add smth. like ?chain=true

jefferai · 2016-09-23T11:22:14Z

We don't support parameters on GET but this could be a special-cased name for the endpoint (just as 'ca' currently is).

chrishoffman · 2016-09-23T11:30:22Z

This also probably only makes sense as a concatenated pem bundle. We can't really support the chain with der certs due to the limitation of the format.

@jefferai Should we try and get this in for 0.6.2? How do you like /ca_chain or /chain? Should I just ignore the der format and just return pem from the base path?

I did review the tests and we seem reasonably covered. I may add a few minor tweaks.

stepanstipl · 2016-09-23T14:48:41Z

@jefferai Ah, I was just looking at /pki/certs/?list=true endpoint, but special endpoint works equally well of course (what about /ca/chain in that case?).

jefferai · 2016-09-23T14:53:38Z

@chrishoffman If you can get it in for 0.6.2 that'd be neat! I think ca_chain for explicitness sounds good, and yeah, I think ignore the DER format.

jefferai · 2016-09-23T14:54:24Z

@stepanstipl That's handled waaaay up in the chain; it's that way because we weren't sure if all software would be happy with using the HTTP LIST verb :-)

But down at the level of backend endpoints GET calls don't have parameters.

chrishoffman · 2016-09-24T01:12:42Z

I settled on /ca/chain for now and is ready to be reviewed.

jefferai · 2016-09-26T14:24:56Z

builtin/logical/pki/path_fetch.go

+			logical.ReadOperation: b.pathFetchReadCAChain,
+		},
+
+		HelpSynopsis:    pathFetchHelpSyn,


The text for this probably needs some changes to accommodate the new info.

I think I need to add the ca_chain to a few other documents too.

jefferai · 2016-09-26T14:27:06Z

builtin/logical/pki/path_fetch.go

+			errutil.InternalError{Err: fmt.Sprintf("Error fetching CA certificate: %s", err)}
+	}
+
+	caChain := caInfo.GetCAChain()


Should this return nil if there is no certs? Then you can easily fast-path that with a nil, nil response which will 204.

It may make sense. This is only used in two places right now and should be pretty easy to transition to this way.

Returning nil here bypasses the Response where we want to provide raw output (this is currently a raw endpoint). When I returned nil here, it fast-path to a default response of 404 with an empty error array. I think we should just keep it the way it is.

Oh, you're right -- a GET will return a 404 on no data returned, unlike a POST or DELETE.

Keeping the way it is sounds fine but if there's no data we should return 204, not 200.

jefferai · 2016-09-26T14:28:59Z

builtin/logical/pki/path_fetch.go

+		certs = append(certs, pem.EncodeToMemory(&block)...)
+	}
+
+	response := &logical.Response{


This tweaks me a little in a consistency sense.

ca returns a Vault response; ca/pem returns a raw response.
crl returns a Vault response; crl/pem returns a raw response.

It feels like ca/chain should return a Vault response and ca/chain/pem should return a raw response. Although in that scenario it's probably better to just use ca_chain and ca_chain/pem.

The reason for having both is so that the Vault CLI (and API clients, generally) can do a read against one of the endpoints and understand how to parse it.

Both ca and ca/pam return a raw response, ca being in DER format. I think you were thinking about cert/ca which returns a Vault response. Same for crl. The reason I didn't store the ca_chain by a special serial in the cert store was because it expects the bytes of a single certificate so there was no easy way to store the slice of []byte. I also can't support the DER format since it could be multiple certificates and that is why I left the /pem off the end.

You do bring up a valid point about API clients, which I didn't fully consider. I'll think about the best way to support the cert/ca_chain endpoint which would be more suitable for API clients.

I think you were thinking about cert/ca which returns a Vault response.
I was! Mea culpa. So this seems fine as-is, but it would be nice to support cert/ca_chain. It could just build up the chain rather than attempt to fetch one from the backend, unless this is difficult for a reason I'm not thinking of right now.

I was able to merge the fetching of the ca chain into the standard fetch method. It is a little different in how this is generated vs. the rest of the certs but I think it is ok.

jefferai · 2016-09-26T14:29:28Z

I know I'm requesting more, but this is close...I promise!

jefferai · 2016-09-27T02:07:07Z

Looking pretty good to me. Want to give it a couple of days to bake in your mind, or merge?

chrishoffman · 2016-09-27T13:11:58Z

I had a thought about the cert/ca_chain endpoint. Do you think we should be returning this as a list of certificates under ca_chain and maybe the pem bundle in certificate? This would provide parity with what is returned when issuing/signing a certificate. It may even make sense to return the chain on the cert/ca endpoint only and not provide a separate cert/ca_bundle endpoint. What do you think?

The raw endpoints can stay as they are.

chrishoffman · 2016-09-27T13:57:37Z

...and maybe we should return the chain for all certificates.

jefferai · 2016-09-27T16:46:23Z

I think this should be kept simple for now. Fetching is not generally (so far as I know) a common operation, it's mostly a nicety. The CA chain has the full chain, including the CA cert set in the backend, so it's really simple to just make two queries if you really need to -- and chances are if you already have the serial of the certificate, given that certificates are public, you only need one or the other.

jefferai · 2016-09-28T00:44:07Z

OK. Let me know when you want me to pull the trigger!

chrishoffman · 2016-09-28T00:44:57Z

I think it is ready to go.

jefferai · 2016-09-28T00:50:19Z

💥

chrishoffman · 2016-09-28T00:51:37Z

Woohoo!

jefferai · 2016-09-28T00:53:46Z

Great job and many thanks! It is also now officially in the changelog as a FEATURE of 0.6.2.

chrishoffman · 2016-09-28T01:03:37Z

Thanks! There may be a small tweak that is needed for the changelog. I've built full chains using internally generated certificates since you can set-signed without the key. Here is a gist that shows building a 4-level CA chain. https://gist.github.com/chrishoffman/acc60cf577e1e79f56beb63747466d3c

jefferai · 2016-09-28T01:05:15Z

Oh neato, so set-signed also handles chains? I didn't realize that. Will adjust!

jefferai added this to the 0.6.2 milestone Aug 5, 2016

chrishoffman changed the title ~~Adding support for chained CAs in pki backend~~ Adding support for chained intermediate CAs in pki backend Aug 8, 2016

jefferai reviewed Aug 24, 2016
View reviewed changes

jefferai self-assigned this Sep 13, 2016

jefferai reviewed Sep 13, 2016
View reviewed changes

chrishoffman force-pushed the pki-ca-chain branch from 85ae728 to 88e5e85 Compare September 20, 2016 03:04

chrishoffman added 6 commits September 19, 2016 23:13

Added Issuing CA Chain support

8cafd44

fixing parsing of CA chain

150ca81

Adding config path for CA Chain

63b4866

Store the CA Chain in the cert bundle

8260062

Adding issuing_ca_chain to secret data and pem_bundle

2cd3859

Adding delete endpoint for CA chain

3e50925

chrishoffman added 2 commits September 23, 2016 21:04

Update test with chain

a19f428

Adding endpiont to retrieve CA chain

ade4d2a

jefferai requested changes Sep 26, 2016

View reviewed changes

chrishoffman added 3 commits September 26, 2016 19:58

Support returning CA Chain in both raw format and Vault response

12719db

Updating documentation

04d3681

minor cleanup

b402e4d

jefferai approved these changes Sep 27, 2016

View reviewed changes

jefferai merged commit 10c8024 into hashicorp:master Sep 28, 2016

jefferai mentioned this pull request Nov 22, 2016

Issuing CA not included in bundle #2102

Closed

Adding support for chained intermediate CAs in pki backend #1694

Adding support for chained intermediate CAs in pki backend #1694

Conversation

chrishoffman commented Aug 5, 2016

jefferai commented Aug 18, 2016

chrishoffman commented Aug 19, 2016

jefferai commented Aug 19, 2016

chrishoffman commented Aug 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefferai commented Sep 13, 2016

chrishoffman commented Sep 16, 2016

jefferai commented Sep 16, 2016

chrishoffman commented Sep 16, 2016

chrishoffman commented Sep 20, 2016

jefferai commented Sep 20, 2016

stepanstipl commented Sep 23, 2016

jefferai commented Sep 23, 2016 via email

chrishoffman commented Sep 23, 2016

stepanstipl commented Sep 23, 2016

jefferai commented Sep 23, 2016

jefferai commented Sep 23, 2016

chrishoffman commented Sep 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefferai commented Sep 26, 2016

jefferai commented Sep 27, 2016

chrishoffman commented Sep 27, 2016

chrishoffman commented Sep 27, 2016

jefferai commented Sep 27, 2016

jefferai commented Sep 28, 2016

chrishoffman commented Sep 28, 2016

jefferai commented Sep 28, 2016

chrishoffman commented Sep 28, 2016

jefferai commented Sep 28, 2016

chrishoffman commented Sep 28, 2016 • edited Loading

jefferai commented Sep 28, 2016

chrishoffman commented Sep 28, 2016 •

edited

Loading