globalsign / est

An implementation of the Enrollment over Secure Transport (EST) certificate enrollment protocol
MIT License
42 stars 25 forks source link

cacerts - support individual file per CA, more flexible filtering #21

Closed Jmennius closed 3 years ago

Jmennius commented 3 years ago

This implements two features - more flexible output filtering and writing CA certs to individual files.

  1. Flexible output filtering Using one of roots or intermediates flags allows the user to output only root or intermediate CA certificates respectively.

    In contrast with rootout flag, roots flag also supports outputting multiple root certificates.

  2. Writing every CA cert to separate file:

    A new flag, separate, enables this mode. It is possible to set filename prefix with out flag. Files are written in a format <prefix>-<index>.pem, ca is the default prefix, prefix can also end with .pem.

TODO:

Jmennius commented 3 years ago

I'd like to somehow specify that sepout is basically in conflict with rootout/out flags.. how can we do that?

paulgriffiths commented 3 years ago

I'd like to somehow specify that sepout is basically in conflict with rootout/out flags.. how can we do that?

I'd be inclined to first investigate whether it's possible to make usage more simple by just removing the conflict. For example:

paulgriffiths commented 3 years ago
<prefix>-ca-root.pem - for root CA
<prefix>-ca-<index>.pem - for subordinate CA

Would it not be simpler to avoid special handling for the root CA filename and just have:

<prefix>-1.pem   // Root CA (required by RFC 7030 4.1.3) is always index 1
<prefix>-2.pem   // First intermediate CA, if present
<prefix>-3.pem   // Second intermediate CA, if present
. . .
paulgriffiths commented 3 years ago

So I think the whole idea of output a single sorted chain of CA certificates is not always as simple as it might look at first.

<prefix>-ca-<index>.pem - for subordinate CA, where index is based on ascending order of NotBefore

Sorting in this way could give incorrect results if there were two CA certificates with the same NotBefore date in the chain - this is probably unlikely, but not impossible as far as I can tell. If we wanted to ensure the correct order of a single chain, we'd at a minimum need to make sure each certificate in the constructed chain cryptographically verifies the next one, I'd think.

Moreover, RFC 5272 section 4.1 states (emphasis added):

Servers SHOULD include all intermediate certificates needed to form complete certification paths to one or more trust anchors

so it's possible that there is more than one valid chain and more than one valid root CA in the returned certificates (if one of the CAs is cross-signed, for example), and in those cases assuming we have a single chain is going to lead to incorrect results (even if they all have distinct NotBefore dates). The current -rootout flag also outputs a single root CA, of course, but in cases where you just want a valid root CA and you don't care which one, this will still give acceptable results if more than one root CA is present in the response.

(It's not clear what position RFC 7030 takes on this, if any, by the way - section 4.1.3 says the /cacerts response is "as defined in RFC5272", but then talks about "the current root CA certificate", and "the root EST CA" as if it's expecting there to always be only one).

Finally, RFC 7030 section 4.1.3 states:

The EST server SHOULD include the three "Root CA Key Update" certificates OldWithOld, OldWithNew, and NewWithOld in the response chain.

In this case, there will be at least two root CAs (i.e. OldWithOld and NewWithNew) and at least one "intermediate" CA which will not appear in any valid certificate chain (i.e. "OldWithNew").

All this means that splitting up the CA certificates can be more complicated that it may at first look, and this is one reason why the current implementation just passes through the list of certificates as received from the EST server, letting the caller decide how it wants to handle those potential complications since this is a general-purpose EST client (although correctly handling and extracting the three "Root CA Key Update" certificates is definitely a potential future enhancement).

So, if we wanted to implement the proposal, we'd have to decide how to handle these complications, for example:

If we had an actual EST-issued certificate to verify, then dumping all the CA certificates into an x509.VerifyOptions and running x509.Certificate.Verify could do most or all of the work of creating a list of valid chains for us, but since we generally call /cacerts before requesting a certificate, in general we're not going to have an issued cert available for this purpose, so more work would be required.

So I'm open to suggestions, but I think the current proposal doesn't sufficiently address these complications.

Jmennius commented 3 years ago

I'd like to somehow specify that sepout is basically in conflict with rootout/out flags.. how can we do that?

I'd be inclined to first investigate whether it's possible to make usage more simple by just removing the conflict. For example:

* instead of accepting a string `-sepout` could just be a boolean flag, maybe renamed to `-separate` or `-multiple`
* if `-sepout` is set, then `-out` now specifies a prefix, instead of a full file name. For convenience, we could check if the "prefix" has a .pem suffix and remove it if it does, so a user can naturally say `-sepout -out cacert.pem` but still get just `cacert-ca-root-pem` instead of `cacert.pem-ca-root.pem`, for example.

* if `-sepout` and `-rootout` are both set, then only the root certificate is output, the only difference is that it's named exactly as it would have been if `-rootout` hadn't been set.

That's what I've implemented initially, but I didn't like it. Most importantly, the duality of -out flag is the worst IMO - in one case it a full file name and in another one, it is a prefix. You then have to additionally handle the case when there is no prefix (to avoid filename starting with -) and there is a question of how to document it. I feel like having a separate flag for this mode of operation that provides a prefix is both cleaner and easier to understand for users.


<prefix>-ca-root.pem - for root CA
<prefix>-ca-<index>.pem - for subordinate CA

Would it not be simpler to avoid special handling for the root CA filename and just have:

<prefix>-1.pem   // Root CA (required by RFC 7030 4.1.3) is always index 1
<prefix>-2.pem   // First intermediate CA, if present
<prefix>-3.pem   // Second intermediate CA, if present
. . .

It probably would (although not by much; and we still have to make sure that the root goes first). But I think it is better when it is explicit. In my use case, I want to distinguish between root and intermediate CAs (sometimes I install all of them and sometimes only the root) - it's easier when they do not follow the same filename pattern (I can glob for intermediates CAs only). Although, to be fair, that's probably not a huge deal for me - I'm ready to change it if it is your preference.

Jmennius commented 3 years ago

So I think the whole idea of output a single sorted chain of CA certificates is not always as simple as it might look at first.

<prefix>-ca-<index>.pem - for subordinate CA, where index is based on ascending order of NotBefore

Sorting in this way could give incorrect results if there were two CA certificates with the same NotBefore date in the chain - this is probably unlikely, but not impossible as far as I can tell. If we wanted to ensure the correct order of a single chain, we'd at a minimum need to make sure each certificate in the constructed chain cryptographically verifies the next one, I'd think.

Moreover, RFC 5272 section 4.1 states (emphasis added):

Servers SHOULD include all intermediate certificates needed to form complete certification paths to one or more trust anchors

so it's possible that there is more than one valid chain and more than one valid root CA in the returned certificates (if one of the CAs is cross-signed, for example), and in those cases assuming we have a single chain is going to lead to incorrect results (even if they all have distinct NotBefore dates). The current -rootout flag also outputs a single root CA, of course, but in cases where you just want a valid root CA and you don't care which one, this will still give acceptable results if more than one root CA is present in the response.

(It's not clear what position RFC 7030 takes on this, if any, by the way - section 4.1.3 says the /cacerts response is "as defined in RFC5272", but then talks about "the current root CA certificate", and "the root EST CA" as if it's expecting there to always be only one).

Finally, RFC 7030 section 4.1.3 states:

The EST server SHOULD include the three "Root CA Key Update" certificates OldWithOld, OldWithNew, and NewWithOld in the response chain.

In this case, there will be at least two root CAs (i.e. OldWithOld and NewWithNew) and at least one "intermediate" CA which will not appear in any valid certificate chain (i.e. "OldWithNew").

All this means that splitting up the CA certificates can be more complicated that it may at first look, and this is one reason why the current implementation just passes through the list of certificates as received from the EST server, letting the caller decide how it wants to handle those potential complications since this is a general-purpose EST client (although correctly handling and extracting the three "Root CA Key Update" certificates is definitely a potential future enhancement).

So, if we wanted to implement the proposal, we'd have to decide how to handle these complications, for example:

* Just let the output be wrong in all but the simplest of cases (not really desirable)

* Attempt to detect if more than more chain is present, and return an error instead of giving wrong output

* Try to detect the individual chains, but then decide on a single one to output

* Try to detect the individual chains, and then output them all with different prefixes

If we had an actual EST-issued certificate to verify, then dumping all the CA certificates into an x509.VerifyOptions and running x509.Certificate.Verify could do most or all of the work of creating a list of valid chains for us, but since we generally call /cacerts before requesting a certificate, in general we're not going to have an issued cert available for this purpose, so more work would be required.

So I'm open to suggestions, but I think the current proposal doesn't sufficiently address these complications.

I didn't mean to sort it in any meaningful way (I don't think the order really matters anywhere) - only to provide stability (i.e. that between invocations of cacerts, prefix-sub-ca-1.pem and prefix-sub-ca-2.pem do not swap/change contents). And I didn't understand how Root CA Key Update is handled in cacerts - I didn't find any specific handling for it, so I've assumed that it is not really supported (cause -rootout supported only one certificate)...

So, from what I understand, what we can do to greatly simplify things - is to use the filename format that you've proposed, which doesn't distinguish between root and intermediate CAs, i.e. prefix-ca-idx.pem? But should we do any reordering then (even to make root first one)?

Jmennius commented 3 years ago

I was looking for clarification regarding the order of certificates:

RFC 7030, 4.1.3. CA Certificates Response:

A successful response MUST be a certs-only CMC Simple PKI Response, as defined in [RFC5272], containing the certificates described in the following paragraph.

RFC 5272, 4.1. Simple PKI Response:

Clients MUST NOT assume the certificates are in any order. Servers SHOULD include all intermediate certificates needed to form complete certification paths to one or more trust anchors, not just the newly issued certificate(s).

So according to RFC 5272 the order is insignificant and clients should be able to handle that.

Jmennius commented 3 years ago

What do you think about the following?

paulgriffiths commented 3 years ago

And I didn't understand how Root CA Key Update is handled in cacerts - I didn't find any specific handling for it, so I've assumed that it is not really supported (cause -rootout supported only one certificate)...

Correct - nothing specific is currently done with them, although if present they'll be included in the returned set of CA certs.

paulgriffiths commented 3 years ago

What do you think about the following?

  • output all root CAs in format <prefix>-root-ca-<index>.pem, the order is not important, index from 1 to (max) 4
  • output all intermediate CAs in format <prefix>-sub-ca-<index>.pem, the order is not important, index from 1 to N (count of intermediate CAs), there is no binding between intermediate CAs and root - they are all part of a single PKI

I'm hesitant at this kind of scheme because I think there are about as many different potential naming/grouping schemes as there are potential users, and for a general purpose EST client I'd rather users be able to choose their own way than us impose a way on them. I think adding an index is the most structure we'd want to impose, here.

How about this as a more general solution:

and in all cases the order is not guaranteed, other than to be in the same relative order that they were received from the server.

(We can also apply these two flags to the situation where -separate is not specified, and in both cases we either output all the roots or all the intermediates, but they go into a single PEM-encoded file, or to standard out. This is in contrast to the current -rootout, which only outputs a single root, regardless of how many might be present in the CA certs.)

With this scheme, prefix-1.pem could be a root CA when run with the -roots option, but it could also be an intermediate CA when run with the -intermediates option. The user is of course free to specify a different prefix for each invocation, so:

estclient cacerts -out root-ca.pem -separate -roots
estclient cacerts -out inter-ca.pem -separate -intermediates

would yield root-ca-1.pem, root-ca-2.pem, etc, and inter-ca-1.pem, inter-ca-2.pem etc, without the client having to impose any suffix except the index itself. You wouldn't get this same set of filenames if you omitted both -roots and -intermediates, but if getting that consistent set of filenames is important to a particular user, then they can just ensure they always use one of those two flags. If another user doesn't care about the distinction between roots and intermediates and they just want to extract a number list of certs to pass onto another process, they can omit both flags and just get a simple list. This seems to preserve maximum flexibility, and everybody wins.

Most importantly, the duality of -out flag is the worst IMO - in one case it a full file name and in another one, it is a prefix.

I get your point but I'm not too troubled by this. I think a case can be made in the other way, e.g. output usually goes to standard out if -out is not specified, so if you require -prefix but don't require -out then you may leave users wondering if their output is going to go to files or not. Similarly, you may confuse users by sometimes basing the filename on -out, and sometimes on -prefix. I don't think there's much in it, but requiring one less flag seems slightly more useful and avoiding the double meaning, to me.

Jmennius commented 3 years ago

I understand that you are concerned that we are enforcing/limiting something, but I would argue that:

Anyway, I am on board with your proposal to have -roots, -intermediates and -separate flags - I think it's quite sensible and flexible (and implementation should be nice too).

Ok, by items:

  1. implement -roots and -intermediates
  2. implement -separate
  3. how do we handle the prefix? 3.1. do you prefer reusing -out for prefix? Or should -separate provide a prefix? 3.2. should the prefix include extension?
  4. Add multiple root CAs handling on the way. BTW, do you have a setup where we can test this?
paulgriffiths commented 3 years ago

3.1. do you prefer reusing -out for prefix? Or should -separate provide a prefix?

Let's reuse -out.

3.2. should the prefix include extension?

Since we're always going to be outputting PEM-encoded certificates here, I think we should allow that extension, but not require it. If the prefix ends with ".pem" (case-insensitive) then remove those four characters and the prefix is what remains. In all other cases, the prefix is the entire string specified by -out.

  1. Add multiple root CAs handling on the way. BTW, do you have a setup where we can test this?

By packaging the behavior up into testable functions and having the cacerts function call those we should be able to test all the behavior involved here except the flag parsing and the actual output. If we want to test from start to finish, it's easy enough to spin up a test EST server with any desired properties and test by calling the cacerts function directly - there's an example in function newTestServer in est_test.go in the main package. I think I might look at adding this kind of test it, just for cacerts at first, and then maybe extending it to the other ops later.

Jmennius commented 3 years ago

I've updated the PR with new implementation.

Note, that I've changed the criteria for a 'root' certificate (as noted in commit msg). Please share your thoughts.

paulgriffiths commented 3 years ago

Note, that I've changed the criteria for a 'root' certificate (as noted in commit msg). Please share your thoughts.

I don't agree with this change - a root certificate is one which can form the root of a chain of trust (i.e. which can begin a certification path), and which is therefore directly trusted without requiring any additional cryptographic verification, i.e.:

Self-signed certificates are used to convey a public key for use to begin certification paths

per RFC 5280 section 3.2, and:

Self-signed CAs SHALL be considered as directly trusted CAs. Recognizing whether a non-self-signed CA is supposed to be directly trusted for some end entities is a matter of CA policy and is thus beyond the scope of this document.

per RFC 4210 section 4.4.

In terms of the "Root CA Key Update" procedure as defined in Section 4.4 of RFC 4210, both "new with old" and "old with new" by definition require at least one other certificate earlier in the certification path, and therefore they are not root certificates. They are both self-issued certificates which "are generated to support changes in policy or operations" per RFC 5280, but self-signed, root certificates are still required in order to trust and use them. The remaining certificates (i.e. "old with old" and "new with new") are self-signed and fall under the existing definition of 'root'.

In general, an issue should be opened for any proposed changes of this nature, so that they can be properly discussed and decided upon before anyone spends time implementing them, and because PRs become more difficult and time-consuming to process when they contain other, non-essential changes.

Jmennius commented 3 years ago

Note, that I've changed the criteria for a 'root' certificate (as noted in commit msg). Please share your thoughts.

I don't agree with this change - a root certificate is one which can form the root of a chain of trust (i.e. which can begin a certification path), and which is therefore directly trusted without requiring any additional cryptographic verification, i.e.:

Self-signed certificates are used to convey a public key for use to begin certification paths

per RFC 5280 section 3.2, and:

Self-signed CAs SHALL be considered as directly trusted CAs. Recognizing whether a non-self-signed CA is supposed to be directly trusted for some end entities is a matter of CA policy and is thus beyond the scope of this document.

per RFC 4210 section 4.4.

In terms of the "Root CA Key Update" procedure as defined in Section 4.4 of RFC 4210, both "new with old" and "old with new" by definition require at least one other certificate earlier in the certification path, and therefore they are not root certificates. They are both self-issued certificates which "are generated to support changes in policy or operations" per RFC 5280, but self-signed, root certificates are still required in order to trust and use them. The remaining certificates (i.e. "old with old" and "new with new") are self-signed and fall under the existing definition of 'root'.

In general, an issue should be opened for any proposed changes of this nature, so that they can be properly discussed and decided upon before anyone spends time implementing them, and because PRs become more difficult and time-consuming to process when they contain other, non-essential changes.

I have no problem with revering this (it really didn't deserve that much text :) ).

Jmennius commented 3 years ago

Fixed all comments:

P.S. I hope you're OK with me force-pushing (GitHub doesn't make it nice)...

Jmennius commented 3 years ago

Oh, that happened fast.. :) I was just adding a few lines to README regarding those changes and also was going to ask your opinion regarding this 'deprecation' of rootout (should the README be changed? if not - it probably shouldn't be 'deprecated' if README points to it).

paulgriffiths commented 3 years ago

Oh, that happened fast.. :) I was just adding a few lines to README regarding those changes and also was going to ask your opinion regarding this 'deprecation' of rootout (should the README be changed? if not - it probably shouldn't be 'deprecated' if README points to it).

Yes, good point, the use of -rootout should probably be replaced with -roots in the README. I don't think we need to say much else about these changes in the README, if the examples there don't actually use them, since it's not intended to be a comprehensive reference.