search

spiffe / spire

The SPIFFE Runtime Environment
https://spiffe.io
Apache License 2.0
1.8k stars 474 forks source link

DigitalSignature bit SHOULD NOT be set on a SPIRE CA cert #2811

Closed georgi-lozev closed 2 years ago

georgi-lozev commented 2 years ago

Following up on our discussion in slack, question 2., regarding DigitalSignature bit in the Key Usage extension being set on the SPIRE CA certs when running in a nested mode.

More context in https://datatracker.ietf.org/doc/html/rfc5280#page-31 section

If the subject public key is only to be used for verifying signatures on
   certificates and/or CRLs, then the digitalSignature and
   nonRepudiation bits SHOULD NOT be set.
azdagron commented 2 years ago

Hmm, yes, I agree with your assessment and frankly was surprised to see this (I didn't think we set this usage bit). Doing a little code archaeology, SPIRE CA has had digitalSignature set for a very long time, predating my time on the project. The default Upstream Authority (CA) plugin stopped adding digitalSignature back in 0.7.0, but self-signed CAs still have it. Downstream CAs have also had it since their inception.

My guess is this was an oversight, but maybe @evan2645 knows if there was some intentional reason for CA's having the digitalSignature key usage? Barring that, I think we're fine to change this behavior.

evan2645 commented 2 years ago

I also looked in git history, referenced SPIFFE spec and RFC 5280, and I believe this is an oversight.

I am not sure how we want to handle this compat-wise. There really shouldn't be anywhere that someone is leaning on this, and if there is then it should be stopped! On the other hand, it has been this way for a very long time and changing the shape of signing certificates is generally a hazardous proposition.

evan2645 commented 2 years ago

If it wasn't clear in my last message, I think we need to fix it 🙃

azdagron commented 2 years ago

Heh, yeah, i agree. Like you said, I'd be surprised if somebody was leaning on this. I'm also for fixing it and taking on the small risk here. We can call this out very clearly in the release notes.