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

crypto/x509: AKI (Authority Key Id) exclusion logic is broken #62060

Open
furkanmustafa opened this issue Aug 16, 2023 · 8 comments
Open

crypto/x509: AKI (Authority Key Id) exclusion logic is broken #62060

furkanmustafa opened this issue Aug 16, 2023 · 8 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@furkanmustafa
Copy link

furkanmustafa commented Aug 16, 2023

Fix for a previous issue #15194, in 2016, introduces a breaking change,
where AKIs suddenly stops appearing in created certificates after the
golang upgrade.

The main case for the original issue is probably not an issue for
actually self-signed certificates. But; the logic for excluding AKI
from output, instead of comparing KeyIds, it compares Subject(CN)s.

So if you use same CNs for a CA and a certificate to sign, the
signed certificate will not have AKI included, even though it is not
self-signed at all.

This change caused us quite a deal of confusion, and months of delay
for upgrading our tooling. Workaround wasn't obvious, we just tried
changing CN fields after so many things. And it just worked, which
confused us even more.

go version

1.7+

Does this issue reproduce with the latest release?

I believe so, but did not do a through testing.

What did you do?

The problem was observed when upgrading CFSSL from 1.2 to 1.6.
Which indicates this change in the README:158.

What did you expect to see?

AKI omission decision to be made by checking if KeyIds are actually same or not.

if len(parent.SubjectKeyId) > 0 && template.SubjectKeyId != parent.SubjectKeyId ...

What did you see instead?

to decide KeyId is same, comparison is done on Subject fields :(

if !bytes.Equal(asn1Issuer, asn1Subject) && len(parent.SubjectKeyId) > 0 ...
@dmitshur
Copy link
Contributor

CC @golang/security.

@dmitshur dmitshur added this to the Backlog milestone Aug 16, 2023
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 16, 2023
@FiloSottile
Copy link
Contributor

This comes up from time to time. My understanding of the specifications is that a certificate with Issuer equal to the Subject is, by definition, a self-signed certificate. Path building will or at least should stop at such a certificate.

If you make a certificate with Issuer equal to Subject but signed by a different public key, it's just a self-signed certificate with a broken signature, it doesn't stop being a self-signed certificate.

AKI makes no sense in a self-signed certificate because, well, it's supposed to be signed by itself.

@furkanmustafa
Copy link
Author

furkanmustafa commented Aug 17, 2023

(Disclaimer: I do not have enough knowledge to talk about actual "correct" way things should be)

Better expression of our case;

  • Root CA (CN=kubernetes, keyId=1, self-signed by keyId=1)
    • admin (CN=kubernetes-admin, O=system:masters), keyId=2, signed by Root CA(CN=kubernetes, keyId=1)
    • api (CN=kubernetes, +some SANS,IPs and such), keyId=3, signed by Root CA(CN=kubernetes, keyId=1)

In above scenario, by the mentioned change;

  • ✅ AKI is omitted for Root CA's cert, which is fine.
  • ✅ AKI is included for admin's cert, which is expected behavior.
  • ❌ AKI was omitted for api's cert, this was not expected at all.

So in that case; KeyId's are different. but accidentally CNs are same. but it is not self-signed (by the same key). It could be said that "it is self-signed" in the sense of actually same organization is signing it, yes. but it is definitely not self-signed by its own key.

Key Id, in my literal reading, refers to identity of the key, not the identity of the signing authority. Reading the RFC (5280) also gives me that implication, it says, the AKI will have same content when it is self-signed. Furthermore, it suggests two ways for deriving a key id;

  • a hash or something that describes the key
  • or Subject AND serial

Which, in my reading, also defines "what is self-signed", which is a certificate signed by the same exact key, or again the same exact key (of same subject with same serial).

--

For another reference; all other tooling, including openssl, browsers, https clients, works just fine with the way it was (CA, and the signed cert, having the same CN).

In our case, initial CN overlap problem was descended from following "Kubernetes the hard way", where it build the PKI stack using a CN "kubernetes" for CA, and then it uses the same CN again in one of the signed certificates (API certificate) later on. (link) Which might sound like a problem, but it was not, so far.

@FiloSottile
Copy link
Contributor

"Self-signed" is a technical term of art, not just a statement about it being signed by the same key. You could have the same key in two certificates with different Subjects and if you sign one with the other it will not be "self-signed".

In X.509, what's dispositive for chaining is the Subject and the Issuer. SKI and AKI are hints to the chain building process, but they don't override Subject/Issuer chaining.

The api certificate is technically a self-signed certificate because it is a certificate for CN=kubernetes signed by CN=kubernetes.

Other tooling might be tolerant of this because this is a common point of confusion, but crypto/x509 tries to stay simple by targeting the WebPKI and well-formed PKIs that follow the same profile.

I remember another issue where we looked into the definition of self-signed certificate, but I can't find it. I did find a duplicate though #20002.

@furkanmustafa
Copy link
Author

I did find a duplicate though #20002.

whoops, sorry. I did look for a duplicate but apparently oversaw that one. Looks like an exact duplicate.

@furkanmustafa
Copy link
Author

"Self-signed" is a technical term of art, not just a statement about it being signed by the same key. You could have the same key in two certificates with different Subjects and if you sign one with the other it will not be "self-signed".

I looked around to find any information about what defines self-signed, but there is almost zero information on it.

One hint I could found was on NIST;

A public-key certificate whose digital signature may be verified by the public key contained within the certificate.

It kinda matches with the RFC's (my-own-reading) intentions, to omit AKI, because the subject key id and authority key id would be exactly same, only in that case.

There is one exception;
where a CA distributes its public key in the form of a "self-signed"
certificate, the authority key identifier MAY be omitted. The
signature on a self-signed certificate is generated with the private
key associated with the certificate's subject public key. (This
proves that the issuer possesses both the public and private keys.)

It goes on to say;

In this case, the subject and authority key identifiers would be
identical
, but only the subject key identifier is needed for
certification path building.

Above two paragraphs says, "self-signed" here, means the certificate is signed by it's own private key, thus it can be verified with the public key it contains.

You could have the same key in two certificates with different Subjects and if you sign one with the other it will not be "self-signed"

Would mean this case is also "self-signed", because it can be verified with the public key it contains, also SKI and AKI would be identical, so AKI information can be omitted.


On the other hand; allowing omittance of AKI, would only make sense if it is same with SKI, regardless of what "self-signed" actually means. (AFAIU)

I intend to test what openssl produces as well soon. I can share results when I do.

I think that's my limit on what I could understand so far from the situation. I hope a nicer and compatible solution could be implemented in golang.

@rolandshoemaker
Copy link
Member

I'm not particularly opposed to making this change, it seems unlikely to really break anything, and perhaps would fix this for a handful of people. That said it seems like we've only heard this is an issue twice since the initial change was made, so I'm not sure how worthwhile the additional complexity is.

In the described use case above, is there any particular reason that you cannot just use a different CN?

(I don't think the definition of self-signed is a particularly useful discussion to have here, it's been incredibly vaguely defined since the beginnings of X509, and various technical forums tend to define it differently or use the term to refer to different things. Trying to find a concrete, well accepted definition in any of the multitude of specifications is a rabbit hole with no end.)

@furkanmustafa
Copy link
Author

In the described use case above, is there any particular reason that you cannot just use a different CN?

No. That's what I did at the end. But two things along the way were difficult;

First, understanding that this was the cause was very difficult;

  • I had to learn about (vague) details X509
  • I had to byte-by-byte compare generated certificates with different versions and tools
  • I had to start reading golang's x509 code (also had to figure out I need to suspect the cause would be there)

Second, being able to make the change;

The original guide instructed such setup, and we have been running stuff that way for years. So even after lucky random change I made made things work, I couldn't be confident that it is not going to break things in unexpected ways.

for a handful of people. That said it seems like we've only heard this is an issue twice

That is feels interesting for me as well, but my guess is that, for any related problem people might be having, it is extremely hard to escalate and finally link to here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants