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: certificates with AKID don't chain to parents without SKID #30079

Open
FiloSottile opened this issue Feb 4, 2019 · 7 comments

Comments

@FiloSottile
Copy link
Member

commented Feb 4, 2019

The fix for #29233 broke certificate chains where a certificate has an AKID but the parent doesn't.

These chains are weird (where did the AKID come from if there's no SKID?) and invalid by RFC 5280 (parents MUST have AKID) but they are out there, and as much of the X.509 ecosystem, we need to live with them.

Reported by @abarisani in #29233 (comment).

In the review @sleevi pointed out that the old code was also subtly broken (if a parent with wrong subject but right AKID and one with right subject but no SKID were available, only the former would be considered, failing to build a chain) but that was a way rarer scenario.

We broke this in a minor (security!) release, which is not good, so we should backport a revert, and fix also the subtle case above in Go 1.13.

@FiloSottile FiloSottile added this to the Go1.12 milestone Feb 4, 2019

@FiloSottile

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2019

@gopherbot please open both backport issues. This is a regression introduced in a minor release.

@gopherbot

This comment has been minimized.

Copy link

commented Feb 4, 2019

Backport issue(s) opened: #30080 (for 1.10), #30081 (for 1.11).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot

This comment has been minimized.

Copy link

commented Feb 4, 2019

Change https://golang.org/cl/161097 mentions this issue: crypto/x509: consider parents by Subject if AKID has no match

gopherbot pushed a commit that referenced this issue Feb 7, 2019
crypto/x509: consider parents by Subject if AKID has no match
If a certificate somehow has an AKID, it should still chain successfully
to a parent without a SKID, even if the latter is invalid according to
RFC 5280, because only the Subject is authoritative.

This reverts to the behavior before #29233 was fixed in 7701306. Roots
with the right subject will still be shadowed by roots with the right
SKID and the wrong subject, but that's been the case for a long time, and
is left for a more complete fix in Go 1.13.

Updates #30079

Change-Id: If8ab0179aca86cb74caa926d1ef93fb5e416b4bb
Reviewed-on: https://go-review.googlesource.com/c/161097
Reviewed-by: Adam Langley <agl@golang.org>

@FiloSottile FiloSottile modified the milestones: Go1.12, Go1.13 Feb 7, 2019

@FiloSottile

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

Short term fix done for Go 1.12, leaving open on Go 1.13 for a more complete fix.

nebulabox added a commit to nebulabox/go that referenced this issue Feb 18, 2019
crypto/x509: consider parents by Subject if AKID has no match
If a certificate somehow has an AKID, it should still chain successfully
to a parent without a SKID, even if the latter is invalid according to
RFC 5280, because only the Subject is authoritative.

This reverts to the behavior before golang#29233 was fixed in 7701306. Roots
with the right subject will still be shadowed by roots with the right
SKID and the wrong subject, but that's been the case for a long time, and
is left for a more complete fix in Go 1.13.

Updates golang#30079

Change-Id: If8ab0179aca86cb74caa926d1ef93fb5e416b4bb
Reviewed-on: https://go-review.googlesource.com/c/161097
Reviewed-by: Adam Langley <agl@golang.org>
nebulabox added a commit to nebulabox/go that referenced this issue Feb 20, 2019
crypto/x509: consider parents by Subject if AKID has no match
If a certificate somehow has an AKID, it should still chain successfully
to a parent without a SKID, even if the latter is invalid according to
RFC 5280, because only the Subject is authoritative.

This reverts to the behavior before golang#29233 was fixed in 7701306. Roots
with the right subject will still be shadowed by roots with the right
SKID and the wrong subject, but that's been the case for a long time, and
is left for a more complete fix in Go 1.13.

Updates golang#30079

Change-Id: If8ab0179aca86cb74caa926d1ef93fb5e416b4bb
Reviewed-on: https://go-review.googlesource.com/c/161097
Reviewed-by: Adam Langley <agl@golang.org>
@dmitshur

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

@FiloSottile Is the short term fix what should be used for backporting purposes, and is it enough to resolve the two backport issues? Or should they wait for full fix before backporting?

@FiloSottile

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2019

@dmitshur Yeah, the short term fix is all we need to backport.

@gopherbot

This comment has been minimized.

Copy link

commented Feb 25, 2019

Change https://golang.org/cl/163739 mentions this issue: [release-branch.go1.11] crypto/x509: consider parents by Subject if AKID has no match

gopherbot pushed a commit that referenced this issue Feb 26, 2019
[release-branch.go1.11] crypto/x509: consider parents by Subject if A…
…KID has no match

If a certificate somehow has an AKID, it should still chain successfully
to a parent without a SKID, even if the latter is invalid according to
RFC 5280, because only the Subject is authoritative.

This reverts to the behavior before #29233 was fixed in 7701306. Roots
with the right subject will still be shadowed by roots with the right
SKID and the wrong subject, but that's been the case for a long time, and
is left for a more complete fix in Go 1.13.

Updates #30079
Fixes #30081

Change-Id: If8ab0179aca86cb74caa926d1ef93fb5e416b4bb
Reviewed-on: https://go-review.googlesource.com/c/161097
Reviewed-by: Adam Langley <agl@golang.org>
(cherry picked from commit 95e5b07)
Reviewed-on: https://go-review.googlesource.com/c/163739
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.