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 CertificateRequest as a source #144

Open
Jamstah opened this issue Jun 27, 2023 · 19 comments
Open

Add CertificateRequest as a source #144

Jamstah opened this issue Jun 27, 2023 · 19 comments
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.

Comments

@Jamstah
Copy link
Contributor

Jamstah commented Jun 27, 2023

A common pattern to build a locally trusted CA with cert-manager is to create a self-signed issuer, use it to issue a CA certificate, then use that certificate with a ca issuer to issue leaf certificates. A common request with this configuration is to be able to supply a trust bundle to clients so they can trust the CA, and support rotation of that CA.

When a certificate is issued by cert-manager, it will create a CertificateRequest, whose status is populated with the public certificate that is issued. By default, this resource is not automatically garbage collected unless the Certificate object is deleted. The certificate requests are linked to the certificate via owner references.

By adding support for certificate requests as trust-manager sources, an end user could easily build a trust bundle for every certificate issued by the self-signed issuer, automatically extending it as the root certificate is rotated. If a specific issued root certificate is considered compromised, the user could delete the specific CertificateRequest and it would be removed from the trust bundle.

Suggested API:

spec:
  sources:
  - certificateRequests:
      certificate: "my-self-signed-ca"

The trust-manager would use owner references to identify the matching certificate requests (similar to how cert-manager does it in the issuing flow) , and use the certificate field from the status of the request to populate the trust bundle.

Alternatively, we could extend cert-manager to provide a CertificateRequest template option to be able to label requests, and select them that way.

We could also include the ability to select a specific CertificateRequest, but I'm not sure why we would do that.

@SgtCoDFish
Copy link
Member

Thanks for taking the time to raise an issue 😁

I worry that CertificateRequests are significantly more disposable than you've listed here - many orgs choose to garbage collect then with some regularity.

Even if they weren't at risk of being garbage collected, I'm squeamish about relying on their information being correct. We know it's safe for a self signed cert, but I can't think of any other situation/issuer where it would be safe. That's because SelfSigned issuers are weird and probably shouldn't exist (there should instead be a selfSigned: true field on Certificate resources).

If the only reason to do this would be for SelfSigned issuers, I'm inlined to say that we're not solving the right problem by adding CertificateRequests as sources 🤔 What do you think?

@Jamstah
Copy link
Contributor Author

Jamstah commented Jun 27, 2023

Thanks for taking the time to review it :)

many orgs choose to garbage collect then with some regularity.

I agree that orgs choose to garbage collect them, and there is an option to have cert-manager garbage collect older revisions too, but the key word here is choose. If an organisation decided to rely on these for their trust bundle, then they would choose to not garbage collect them, or to garbage collect differently (perhaps on expiry).

SelfSigned issuers are weird and probably shouldn't exist

I sort of agree here, but also understand that they exist because of the cert flow - by having them we keep the flow and the responsible controllers the same for all issuance, instead of having to treat them as a special case. Either way - this proposal doesn't actually involve the issuer at all, we're trusting revisions of a certificate.

If the only reason to do this would be for SelfSigned issuers, I'm inlined to say that we're not solving the right problem by adding CertificateRequests as sources thinking What do you think?

I agree that this solution is targeted at self signed issuers, with other issuers the root ca certs that need to be in a bundle can't be discovered in the same way.

I've seen numerous requests for a way to handle root rotation in these scenarios. Many solutions I've seen proposed end up being a little convoluted and only work in specific cases, I thought this suggestion might be a composable way satisfy the need using a fairly generic mechanism that already exists.

I can think of one scenario where restricting to the self-signed issuer is not necessarily the right thing to do. For example, lets say we have a root CA for an organisation, and we use that to sign intermediate CAs that are for specific teams/services. Its possible that an application would want to recognise certificates for a specific team/service but not for the organisation as a whole, so would want to use this on a CA issuer to build the trust bundle based on the intermediate not the root. I don't know of anyone doing this (I expect subject checking is more sensible).

Thoughts on restricting to self-signed

(split this out as my comment got too long!)

If we want to restrict this to the self-signed issuer, we could potentially update the issuer instead with a field like PublicCertificateHistory: true so that all issued certificates are copied automatically into ConfigMap objects that can then be collected by trust-manager. That could help make using this pattern more explicit and stop users trying to do this with other issuers, but could confuse users as multiple Certificates using the same issuer would end up being in the same trust bundle automatically.

We could also recognise the use case as a more first class use case and create a new issuer SelfSignedCA that exhibits the above behaviour. That feels like a good way to make the user think about what they're doing, but feels more complicated than it needs to be.

The other way to restrict it to the self-signed issuer would be to check the issuer type when building the bundle, and refuse (or add a condition) when its not the self-signed issuer.

As an aside: The first iteration of this I thought of was to add a field to the Certificate resource called something like PublicCertificatePrefix, and on issuing a certificate, to also create a ConfigMap named <prefix>-<revision> containing the public cert, user specified labels, and no owner references. Then we could add label selectors to trust-manager to collect those into a bundle based on those labels. It was while I was investigating how that would be implemented that I found that CertificateRequest resources already follow the same pattern.

@SgtCoDFish
Copy link
Member

On the Proposal

This is the only bit that really matters, but I added some more thoughts below!

If we want to restrict this to the self-signed issuer, we could potentially update the issuer instead with a field like PublicCertificateHistory: true...

That actually seems like a really attractive option. It applies nicely to all certificates, and it seems like something people would want for different certs too. It helps with this kind of trust-manager problem, but also helps for auditing purposes (e.g. "I've had 30 different Let's Encrypt certs on this cluster, here they all are").

I'd actually add it to the Certificate resource rather than an issuer, I think.

Unfortunately, I'm not sure if any of the maintainers would have time to implement (and it's possible that there might be blockers to implementing it which I haven't thought of!).

On Trusting Intermediates

I can think of one scenario where restricting to the self-signed issuer is not necessarily the right thing to do. For example, lets say we have a root CA for an organisation, and we use that to sign intermediate CAs that are for specific teams/services.

I tend to think that this is an antipattern and I'd advise against it if I saw it, because this turns the structure of an org's PKI into an implicit auth decision.

It's so easy for a team to trust the root certificate and have everything work, only to discover later that trusting the root means trusting everything. It also makes rotation much more difficult (because every intermediate has to be treated carefully).

This little bit isn't really relevant to the problem we're discussing, but I find that I weirdly care about PKI and I like talking about it 😂

On SelfSigned

I sort of agree here, but also understand that they exist because of the cert flow - by having them we keep the flow and the responsible controllers the same for all issuance, instead of having to treat them as a special case. Either way - this proposal doesn't actually involve the issuer at all, we're trusting revisions of a certificate.

My thing is that SelfSigned is already a special case which has to be treated differently, because it's the only issuer where the issuer needs to know the private key of the cert to be signed. All other issuers use CSRs. SelfSigned is already implemented in a weird way where the private key has to be shared with the issuer!

You're totally correct that the proposal doesn't involve the issuer, of course! My reason for bringing up the weirdness of SelfSigned is that I tend to be wary of anything which treats it as more of a special case. Because it's weird, if I see a proposal which seems to apply mostly to SelfSigned I'm extra careful!

@Jamstah
Copy link
Contributor Author

Jamstah commented Jul 3, 2023

That actually seems like a really attractive option. It applies nicely to all certificates, and it seems like something people would want for different certs too. It helps with this kind of trust-manager problem, but also helps for auditing purposes (e.g. "I've had 30 different Let's Encrypt certs on this cluster, here they all are").

I'd actually add it to the Certificate resource rather than an issuer, I think.

That was my first thought, to add that to the Certificate. It was as I was looking at the code to see how complicated it would be that I realised that CSRs already had the required data in them. Those CSRs could also already be used for audit purposes if they aren't cleaned up, and cleaning them up is an active decision that gets made by the user, so if they want to use them for audit, they just don't clean them up.

Why do you think it would be better to use a PublicCertificateHistory field that duplicates each public cert to a configmap, instead of using the existing CertificateSigningRequest that already follows that pattern? (I don't mind contributing it, I would probably open an issue on cert-manager first to get more opinions before writing code though, and that's a question I expect would come up!)

Also, I'm not expecting anyone to code it for me, I'm happy to contribute, but wanted to get some feedback before doing a lot of work. I have an untested changeset for trust-manager to pick up data from the CSR already, and have put some thought into how PublicCertificateHistory would need to change the Certificate issuing flow in cert-manager, but with no code behind it yet.

I tend to think that this is an antipattern and I'd advise against it if I saw it, because this turns the structure of an org's PKI into an implicit auth decision.

Yeah, you're right and my example was stretching. Your example (audit trail) is more appropriate.

@Jamstah
Copy link
Contributor Author

Jamstah commented Jul 3, 2023

This little bit isn't really relevant to the problem we're discussing, but I find that I weirdly care about PKI and I like talking about it 😂

Yes, there's something about PKI I enjoy too, I've never been able to explain it. My favourite module at university was the crypto module.

@Jamstah
Copy link
Contributor Author

Jamstah commented Jul 18, 2023

Before I start a discussion over in the cert-manager repo, one question that might help me write that request:

Why do you think it would be better to use a PublicCertificateHistory field that duplicates each public cert to a configmap, instead of using the existing CertificateSigningRequest that already follows that pattern?

@SgtCoDFish
Copy link
Member

Hey! Sorry, been a crazy couple of weeks and I've been meaning to get back to this.

I guess the main reason I'd suggest a CertificateHistory field would be API design. CertificateHistory is incredibly specific, and it's not overloaded in any way. It would be clear to me that I could use that history ConfigMap to trust every iteration of a cert in trust-manager, for example.

CertificateRequest objects are used for requesting certs, and so using them for trust purposes would be overloading their purpose. They're also liable to being cleaned up / garbage collected - it's possible to imagine a workaround to prevent them being garbage collected, but equally it's possible to imagine a well-meaning engineer cleaning them up manually, not realising how they're being used.

@Jamstah
Copy link
Contributor Author

Jamstah commented Jul 19, 2023

Raised a suggestion in cert-manager here: cert-manager/cert-manager#6224

@james-w
Copy link

james-w commented Jul 28, 2023

I think that linking cert-manager with trust-manager in general is going to be a very useful thing. However I think there's another issue that is important and I don't think it's been touched on here yet.

There is a differentiation between which CA cert is being used to sign, and which CA cert(s) should be trusted. This is obvious during a rotation. You will need to start trusting the new CA before it is being used to sign, and stop trusting the old one after there are no more certs signed with it in use.

The CertificateHistory idea is starting to touch on this, but I think the solution is more complex than that allows.

Having automatic connection between cert-manager and trust-manager would be really useful, but I wonder if the use cases would be limited without something quite a bit more complex.

@Jamstah
Copy link
Contributor Author

Jamstah commented Jul 28, 2023

Having automatic connection between cert-manager and trust-manager would be really useful, but I wonder if the use cases would be limited without something quite a bit more complex.

Can you describe how you think the use cases would be limited?

@james-w
Copy link

james-w commented Jul 28, 2023

Can you describe how you think the use cases would be limited?

If you just trust the current signing cert then you will have issues at rotation time. If you trust all historical certs then you are trusting more than you want to ideally. These two extremes may work for some use cases, but anything more nuanced needs a more complex approach IMO.

@Jamstah
Copy link
Contributor Author

Jamstah commented Jul 28, 2023

Right, so you think there needs to be a way to determine when all the certs issued by a CA have expired? That's a difficult question to answer :)

Trusting an expired CA isn't ideal, but shouldn't typically be unsafe.

@SgtCoDFish
Copy link
Member

Trusting an expired CA isn't ideal, but shouldn't typically be unsafe.

That could actually be a useful standalone trust-manager improvement. There's no reason to include expired CAs in our output I can think of (unless people are running massively different clocks on different machines, but I don't think that's worth us supporting).

I think we could filter anything expired out of the ConfigMap we generate.

In any case, I think you're correct that it's safe. If an attacker had the ability to modify the time on a box, they could maybe make use of an expired cert - but I think it's reasonable to assume that attackers cannot modify time in our threat model (AFAIK changing time requires privileged access in most cases, so it's already game over if they can do that)

The issue really is distrusting a still-valid cert that's been rotated. James makes a good point that PublicCertificateHistory makes it very easy to still trust a cert that's been rotated.

Right, so you think there needs to be a way to determine when all the certs issued by a CA have expired? That's a difficult question to answer :)

I think more generally, it would be required to find out when all certs issued by a CA were no longer used, with it being a safe assumption that an expired cert is no longer being used (and that no cert issued by an expired CA is being used). Doing that is absolutely gonna be very complex though, as you correctly note!

@Jamstah
Copy link
Contributor Author

Jamstah commented Jul 29, 2023

That could actually be a useful standalone trust-manager improvement. There's no reason to include expired CAs in our output I can think of...

I believe a cert is valid as long as it was signed by the CA while it was valid, and that an expired CA can indeed be used as part of a valid trust chain.

I don't think we can just not include any expired certs :(

@SgtCoDFish
Copy link
Member

I believe a cert is valid as long as it was signed by the CA while it was valid, and that an expired CA can indeed be used as part of a valid trust chain.

The first part of this surprised me! I definitely agree that an expired CA can be used in a valid chain, but I think it would have no effect. I tested this by writing a program which generates a root and leaf certificate.

First I generated a root which hadn't expired to test that the chain was valid. This test was done before 2023-07-31T10:00:00Z.

# Root cert:
# Validity
#   Not Before: Jul 31 09:37:36 2022 GMT
#   Not After : Aug 10 09:37:36 2023 GMT
-----BEGIN CERTIFICATE-----
MIIBBjCBuaADAgECAgIFOTAFBgMrZXAwETEPMA0GA1UEChMGdGVzdE9VMB4XDTIy
MDczMTA5MzczNloXDTIzMDgxMDA5MzczNlowETEPMA0GA1UEChMGdGVzdE9VMCow
BQYDK2VwAyEA9i8wVyrym754KyvVasojACJNBYELhbBaA54jxi1yJp2jNTAzMBIG
A1UdEwEB/wQIMAYBAf8CAQIwHQYDVR0OBBYEFHb9oGnwr7cm0df4WBuLfTD8H7qi
MAUGAytlcANBAIZHRZDfzCLmU5vnwr3Q+vMoq6al6E//G/EQ3WfgxZKs1lndiCv5
Y03GwGoKVfvM7EZxVlaF3GXWqLBR/AuOUAs=
-----END CERTIFICATE-----

# Leaf cert
# Validity
#   Not Before: Jul 26 09:37:36 2023 GMT
#   Not After : Aug  5 09:37:36 2023 GMT
-----BEGIN CERTIFICATE-----
MIHrMIGeoAMCAQICAhyjMAUGAytlcDARMQ8wDQYDVQQKEwZ0ZXN0T1UwHhcNMjMw
NzI2MDkzNzM2WhcNMjMwODA1MDkzNzM2WjARMQ8wDQYDVQQKEwZ0ZXN0T1UwKjAF
BgMrZXADIQCClq2g4Io7jhtThwL4Lp/rhkWmP7J6CslB34Rm0teMUqMaMBgwFgYD
VR0RBA8wDYILZXhhbXBsZS5jb20wBQYDK2VwA0EAeslRHiTu/W853rWQUdU04Sxd
GdFtvHZNVLrBnE0IA1UwCldCuIRuuhApXBFrVg/Jcnu0vmQCWdmJM43T0XuSAQ==
-----END CERTIFICATE-----

I ran openssl verify to check this was valid, and it was:

$ openssl verify -CAfile root.pem chain.pem
chain.pem: OK

Then I regenerated the chain with a root cert which had expired but with a leaf cert which was still unexpired:

# Root cert
# Validity
#   Not Before: Jul 31 09:41:14 2022 GMT
#   Not After : Jul 21 09:41:14 2023 GMT
-----BEGIN CERTIFICATE-----
MIIBBjCBuaADAgECAgIFOTAFBgMrZXAwETEPMA0GA1UEChMGdGVzdE9VMB4XDTIy
MDczMTA5NDExNFoXDTIzMDcyMTA5NDExNFowETEPMA0GA1UEChMGdGVzdE9VMCow
BQYDK2VwAyEAJ7fX2joHtI/uV/eVFGArjjNZcSCV7GSVYNUdZ8MSQEijNTAzMBIG
A1UdEwEB/wQIMAYBAf8CAQIwHQYDVR0OBBYEFHVc2w1ggTX4qWr3x23xobwQYzk8
MAUGAytlcANBALoMDsLTbusM6qZm8Op+i6Wv+972pYf3S6ZUexv6FUPAbG9/K/6j
pIWec/Ll4IpIE7kfsUNhBvZDPR+0JFEnDAg=
-----END CERTIFICATE-----

# Leaf cert
# Validity
#   Not Before: Jul 26 09:41:14 2023 GMT
#   Not After : Aug  5 09:41:14 2023 GMT
-----BEGIN CERTIFICATE-----
MIHrMIGeoAMCAQICAhyjMAUGAytlcDARMQ8wDQYDVQQKEwZ0ZXN0T1UwHhcNMjMw
NzI2MDk0MTE0WhcNMjMwODA1MDk0MTE0WjARMQ8wDQYDVQQKEwZ0ZXN0T1UwKjAF
BgMrZXADIQAx6lQrUzwFtoIJIAqX5OuPCbGmWXNMGU1zAtgPuJFAkqMaMBgwFgYD
VR0RBA8wDYILZXhhbXBsZS5jb20wBQYDK2VwA0EAaY49vM7wGBOgu7ubD13yHsPz
rdKiG/aN6ywSgw5QThIxprSomK28mPaaO/wILWgbzVE4+0WNJB4DrpbNwfSmCw==
-----END CERTIFICATE-----

Then I tried to verify again:

$ openssl verify -CAfile root.pem chain.pem
O = testOU
error 10 at 0 depth lookup: certificate has expired
error chain.pem: verification failed

I'm now convinced that if a CA in the chain has expired, the leaf will not verify!

@Jamstah
Copy link
Contributor Author

Jamstah commented Jul 31, 2023

In openssl :)

It's not in the spec, and the signature is based on the CA key. There is nothing to stop a new ca cert being made with the new key, afaik.

Good post on the subject here: https://security.stackexchange.com/questions/120469/can-a-ssl-certificate-have-longer-validity-period-than-its-parent-in-x-509-chain

I think it would make a good addition to the spec.

@Jamstah
Copy link
Contributor Author

Jamstah commented Jul 31, 2023

Have improved my understanding and this is indeed wrong:

I believe a cert is valid as long as it was signed by the CA while it was valid, and that an expired CA can indeed be used as part of a valid trust chain.

The spec just doesn't say the validity period needs to nest. It does say all the certs need to be valid at the time of validation.

The leaf could still be valid, but would require a new ca cert with the same key to validate a whole chain.

So yes, I think we could not include expired CA certs in a trust bundle.

@cert-manager-bot
Copy link
Contributor

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.
/lifecycle stale

@cert-manager-prow cert-manager-prow bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 6, 2024
@cert-manager-bot
Copy link
Contributor

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.
/lifecycle rotten
/remove-lifecycle stale

@cert-manager-prow cert-manager-prow bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Development

No branches or pull requests

4 participants