Add section on supporting intermediate certificates

[jackkleeman]

I've written my proposal for how we could support intermediate certificates, which is already partially implemented in kubernetes/kubernetes#88741.

There's already been a lot of different ideas about the right design here, so this is just intended to spur discussion, I'm happy to write up whatever conclusion we come to.

Here's the bit thats causing confusion:
We have a signing certificate, currently inputed as a file, with another file for a key. We want users to be able to indicate, optionally, that the signing certificate should be presented back to the client. However, for this to be useful in cases with a long chain up to the root, we need to also allow the user to provide additional intermediate certificates (closer to the root than the signer), that can be used to build a chain to the root. There are a few options, and we'd love input from people who have worked with a similar system.

1. Instead of having a single signing certificate in an input file, we could allow a certificate chain, where only one certificate is the signer that should be combined with the key. We would need to infer what certificate is the signer - by convention, it could be the last cert in the file (is this a convention in any other signing systems? In Go cert chain parsing, the order usually doesn't matter), or we could walk the chain to find the 'deepest' cert (sounds tricky, but Go stdlib does this sort of thing to build TLS cert chains), or we could check the public key of each against the private key (but that might find multiple). Then, the user can provide a bool to suggest whether to append the full chain as given, including the signer.
2. We could maintain a single signing cert in that file, but add another optional file, which should contain a list of additional certs to append to the signed output, giving the user a lot of control. However, in all sign-with-an-intermediate cases, this file will need to contain the same cert as the main cert file, which is a bit of a weird user experience and leads to edge cases around file rotation that would be nice to avoid - we'd need to check on rotation that the chain file and the signing cert file still form a coherent chain, and in doing so we'd constrain the user to what we believe is a coherent chain. In the case where the signer is the child of a root, the files will be identical, which again is a bit odd.
3. We could maintain a single signing cert in the file, and append it if a boolean is set (or default to appending it if the signer isnt a root). Additionally, for cases where your signer is not a child of the root, we can have another file that contains certs that are further up the chain than the signer only. This avoids some of the user experience concern, although its still a bit weird to have two files comprising your chain. Some rotation concerns persist - unlike a key and a cert, its not 'obvious' that we have a mismatch between the chain certs and the signing cert, so we'd need some kind of chain verification logic.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jackkleeman
To complete the pull request process, please assign mikedanese
You can assign the PR to them by writing /assign @mikedanese in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• keps/sig-auth/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jackkleeman]

/assign @mikedanese

[mikedanese]

cc @deads2k @liggitt @munnerz

[munnerz]

nit: if you wrap at 80/120 LoC, it makes it a little easier to review/discuss :)

The additional certificates would only be used if the include signers option is set; they are not needed in signing.

It seems non-obvious to have the --cluster-signing-cert-file parameter specifically say that "some of these things actually won't be used for signing".

I may have missed something in previous discussion, but what's the rationale behind a bool vs an explicit --cluster-signing-cert-chain flag which points to a file containing the chain expected to be appended to the certificates the signer signs? I know this was talked about before and I thought the consensus was on an explicit file/flag.

To try and get an idea of what either configuration would look like either way...

With the bool:

• single self signed root: configure as we do today (no --cluster-signing-include-signers flag
• root -> intermediate: --cluster-signing-cert-file=<path-to-intermediate> --cluster-signing-include-signers=true
• root -> intermediate -> intermediate: --cluster-signing-cert-file=<path-to-file-with-both-intermediates> --cluster-signing-include-signers=true

& with the additional file flag:

• single self signed root: configure as we do today (no --cluster-signing-chain-file flag
• root -> intermediate: --cluster-signing-cert-file=<path-to-intermediate> --cluster-signing-chain-file=<path-to-intermediate>
• root -> intermediate1 -> intermediate2: --cluster-signing-cert-file=<path-to-intermediate2-file> --cluster-signing-chain-file=<path-to-file-with-both-intermediates>

---

I think the bool option isn't actually that unclear for anyone with some PKI background (who is familiar with concatenating PEM files together), although the fact that --cluster-signing-key-file and --cluster-signing-cert-file now are a bit ambiguous could definitely cause confusion (one of these can contain multiple certificates, the other contains a private key that must be valid with the first intermediate only).

The additional file option seems a bit more verbose, but at least it is explicit (and validation in kube-controller-manager could check to ensure that only a single PEM is included in the --cluster-signing-cert-file too)

More of a brain dump than a concrete answer/review :)

[jackkleeman]

I like the file option too, but the spanner in the works for me was:

1. In probably(?) the most common case where you're not signing with a root, you have root -> intermediate. In that case, you provide the same file to two different parameters, which just feels weird to me as an end user
2. As an addendum to that, managing rotation of these files becomes more complicated; we now have two files that can contain overlapping content or may be the same file, where we're generally interested in the deduplicated sum of both files. We can't set up individual file rotation controllers on them, we'd need to do something bespoke to watch them together, although there will technically always be a race condition that needs to be handled

It just feels simpler to me to have all the certs in one place, and let people decide whether to include them in the cert response. To be honest, I'd advocate to include signing certs by default, maybe filtering out any root certs, but its not as explicit.

[deads2k]

Intermediates can include a set larger than the simply the certificate that was used for signing. If we're going to add a flag, it makes sense for it cover the full use-case.

In addition, with four distinct signers from the migration of a single-signer, flags should take into account the reality of managing distinct signers. There is clear value to having these separate signers with their own signing cert/keys (trust, revocation, and duration of the signer validity) and each would logically have their own intermediates.

We get a choice between a proliferation of flags or names configurable in a repeated flag. Either can be constrained via validation to avoid runaway, "sign all the things" usage.

I'd like to see this enhancement updated to reflect the larger context of the problem-space it exists in.

[jackkleeman]

Intermediates can include a set larger than the simply the certificate that was used for signing. If we're going to add a flag, it makes sense for it cover the full use-case.

Currently the controller only allows a single signing cert in cluster-signing-cert-file, so this approach would only work in the case where the signing certificate is a child of the root CA, in which case it is the only certificate that needs to be provided to the client. To support further nested signing certificates, we additionally need to relax the contraint of the cert file parameter to accept multiple signers in a chain.

Does this part of my patch cover this particular concern? I agree we need to allow a set of intermediates that can also be provided to the client, and I wonder if we can do this by simply allowing a full chain to be provided as the cert file.

[deads2k]

Does this part of my patch cover this particular concern? I agree we need to allow a set of intermediates that can also be provided to the client, and I wonder if we can do this by simply allowing a full chain to be provided as the cert file.

Not really. The first concern is that intermediates encompass more than just the signer, which implies another flag. The second concern is that we would need such a parallel flag for each signer we support configuring.

I don't know enough about cert signing standards to know if it is normal to auto-choose which certificate in a bundle to sign with. It sounds unusual to me, but I'm not sure if it is normal and simply uncommon enough that I haven't seen it.

[mikedanese]

Please include changes you intend to make to the CertificateSigningRequest API Definition (even if they are only doc changes).

[jackkleeman]

@mikedanese I think its already covered by the comment in this kep:

  // Certificate is populated by the signer with the issued certificate in PEM format.
  // In JSON and YAML output, this entire field is base64-encoded, so it consists of:
  // base64(
  // -----BEGIN CERTIFICATE-----
  // MII...Pb7Yu/E=
  // -----END CERTIFICATE-----
  // optional intermediate certificate blocks
  // -----BEGIN CERTIFICATE-----
  // MII...MGKB
  // -----END CERTIFICATE-----
  // -----BEGIN CERTIFICATE-----
  // MII...AY1M
  // -----END CERTIFICATE-----
  // )
  //
  // In v1beta1, this field is unvalidated.
  //
  // In v1, modified content in this field is validated:
  // * Content must contain one or more PEM blocks
  // * All PEM blocks must have the "CERTIFICATE" label, contain no headers, and the encoded data
  //   must be a BER-encoded ASN.1 Certificate structure as described in section 4 of RFC5280.
  // * Non-PEM content may appear before or after the CERTIFICATE PEM blocks and is unvalidated,
  //   to allow for explanatory text as described in section 5.2 of RFC7468.
  //
  // If more than one PEM block is present, and the definition of the requested spec.signerName
  // does not indicate otherwise, the first block is the issued certificate,
  // and subsequent blocks should be treated as intermediate certificates and presented in TLS handshakes.

but this is not currently the comment in k8s/k8s. The main concern is how we set up the configuration, I believe the kep already covers conceptually how intermediates are handled in the api.

[sftim]

A couple of questions for clarification.

[sftim]

I recommend unambiguously specify the order in which intermediate certs are added into CertificateSigningRequest's status field or stating that the order is arbitrary / random.

[jackkleeman]

Hmm, we could say it's the order given in the file in the controller, but I'm tempted to not make any guarantees here. I can update to say that it's arbitrary

[sftim]

Is this a boolean field in the CSR spec or a boolean command line argument to the kube-controller-manager?

[jackkleeman]

The latter, I can clarify

[mikedanese]

Another compelling reason to add a new flag for the intermediate bundle is that we can't easily introduce the full set of validation we could do on an existing flag. I do like the idea of validating that intermediate bundle is a single, well formed chain up to a root. This validation would break kcms that are configured with an intermediate cert today if we reuse --cluster-signing-cert-file.

I don't know enough about cert signing standards to know if it is normal to auto-choose which certificate in a bundle to sign with. It sounds unusual to me, but I'm not sure if it is normal and simply uncommon enough that I haven't seen it.

I don't see support for auto-selection in openssl or cfssl. We would be relaxing the validation of the signing configuration in order to reuse an option for the bundling phase. I'm leaning towards a separate options.

[mikedanese]

The main concern is how we set up the configuration, I believe the kep already covers conceptually how intermediates are handled in the api.

Should signers include or omit root certs in the bundle? The doc indicates that it won't. Is that what we want?

[jackkleeman]

Should signers include or omit root certs in the bundle? The doc indicates that it won't. Is that what we want?

Personally I see no reason to send the root given that there's no reason for clients to present it, but I guess it wouldn't hurt, if others disagree

[liggitt]

I would avoid sending the root in the CSR... that seems likely to lead to inappropriate use as a trust distribution mechanism

[mikedanese]

Not including roots makes sense to me.

[mrogers950]

We would need to infer what certificate is the signer - by convention, it could be the last cert in the file (is this a convention in any other signing systems? In Go cert chain parsing, the order usually doesn't matter), or we could walk the chain to find the 'deepest' cert (sounds tricky, but Go stdlib does this sort of thing to build TLS cert chains), or we could check the public key of each against the private key (but that might find multiple). Then, the user can provide a bool to suggest whether to append the full chain as given, including the signer.

NSS handles a similar situation (designation of a valid signer in a full chain) with a trust flag setting, where one of the flags means "this CA can be used to sign". So on the signer side of things, having a full chain but providing an option to select the CA is not unheard of.

I'd say it's better to not send the root in a chain back to the requester, it's implied that they have the root by other means (i.e. browser bundle), but may not have the intermediate chain that was the CA's choosing. Although, if you have a chain of root->sub1->sub2->sub3 and sub3 is used to sign, you might not want to even send back sub1 and sub2 if they can be used alone as valid trust anchors (golang clients allow this). So depending on the clients, sending intermediates may also be used as trust distribution..

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[k8s-ci-robot]

@jackkleeman: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[sftim]

@jackkleeman - do you have time to rebase this and help it move forward? It sounds like a good idea.

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.