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

Jump to bottom

crypto/x509: certificates with AKID don't chain to parents without SKID #30079

Closed
FiloSottile opened this issue Feb 4, 2019 · 8 comments
Closed

crypto/x509: certificates with AKID don't chain to parents without SKID #30079

FiloSottile opened this issue Feb 4, 2019 · 8 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@FiloSottile
Copy link
Contributor

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 NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Feb 4, 2019
@FiloSottile FiloSottile added this to the Go1.12 milestone Feb 4, 2019
@FiloSottile
Copy link
Contributor Author

FiloSottile commented Feb 4, 2019 •
edited

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

This was referenced Feb 4, 2019
Closed
Closed
@gopherbot
Copy link

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
Copy link

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
@FiloSottile
crypto/x509: consider parents by Subject if AKID has no match
95e5b07 
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
Copy link
Contributor Author

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

nebulabox pushed a commit to nebulabox/go that referenced this issue Feb 18, 2019
@FiloSottile @nebulabox
crypto/x509: consider parents by Subject if AKID has no match
d2a6079 
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 pushed a commit to nebulabox/go that referenced this issue Feb 20, 2019
@FiloSottile @nebulabox
crypto/x509: consider parents by Subject if AKID has no match
b1d3df6 
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
Copy link
Contributor

@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
Copy link
Contributor Author

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

@gopherbot
Copy link

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
@FiloSottile
[release-branch.go1.11] crypto/x509: consider parents by Subject if A…
aa95a1e 
…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
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@FiloSottile FiloSottile modified the milestones: Backlog, Go1.15 Mar 31, 2020
@FiloSottile FiloSottile self-assigned this Mar 31, 2020
@FiloSottile FiloSottile modified the milestones: Go1.15, Backlog May 1, 2020
@gopherbot
Copy link

Change https://golang.org/cl/232993 mentions this issue: crypto/x509: prioritize potential parents in chain building

@networkimprov networkimprov mentioned this issue Sep 29, 2020
Closed
@golang golang locked and limited conversation to collaborators Sep 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
5 participants
@rsc @andybons @FiloSottile @dmitshur @gopherbot