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: CANotAuthorizedForExtKeyUsage is premature #24590

Closed
robstradling opened this issue Mar 29, 2018 · 11 comments
Closed

crypto/x509: CANotAuthorizedForExtKeyUsage is premature #24590

robstradling opened this issue Mar 29, 2018 · 11 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@robstradling
Copy link
Contributor

What version of Go are you using (go version)?

1.10.

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

Linux amd64.

What did you do?

https://play.golang.org/p/ITwuMz9qN4z

What did you expect to see?

No error.

What did you see instead?

verification failure: x509: a root or intermediate certificate is not authorized for an extended key usage: EKU not permitted: asn1.ObjectIdentifier{2, 16, 840, 1, 113741, 1, 2, 3}

@ianlancetaylor
Copy link
Contributor

CC @FiloSottile

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 29, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Mar 29, 2018
@robstradling
Copy link
Contributor Author

@robstradling
Copy link
Contributor Author

EKU "nesting" is not part of RFC5280. However, several platforms (MS CryptoAPI, NSS, etc) (mis)use the EKU extension in CA certificates to constrain the set of purpose(s) for which leaf certificates may be used.
When an "un-nested" EKU OID appears in a certificate further down the chain, these platforms won't accept that certificate for the "un-nested" purpose, but they will still accept the certificate for the "nested" purpose(s).
Including "un-nested" EKU OIDs in certificates further down the chain is not an error. All the platforms (except Go 1.10!) that I'm aware of will accept the example certificate (see also https://crt.sh/?id=369642622) for the server authentication purpose. The "un-nested" EKU OID, 2.16.840.1.113741.1.2.3, is only recognized by the Intel vPro/AMT platform, which doesn't implement EKU "nesting". It doesn't matter that 2.16.840.1.113741.1.2.3 is "un-nested", because the platforms (except Go 1.10!) that do EKU "nesting" don't recognize it and so wouldn't behave any differently even if it was "nested".

@FiloSottile
Copy link
Contributor

The EKU nesting check is documented and matched by other major platforms, as you said, so I don't see that changing.

type VerifyOptions struct {
        // KeyUsage specifies which Extended Key Usage values are acceptable.
        // An empty list means ExtKeyUsageServerAuth. Key usage is considered a
        // constraint down the chain which mirrors Windows CryptoAPI behavior,
        // but not the spec. To accept any key usage, include ExtKeyUsageAny.
        KeyUsages []ExtKeyUsage

However, I can see why ignoring those EKUs instead of erroring out might be the right choice, in particular if that's the other platforms behavior.

While name constraints are documented to be verified in Voice of God mode (which is necessary for subsequent calls to VerifyHostname, which has no chain visibility), the EKU validation can reasonably be per-Verify. Here's all the docs say:

Extended Key Usage values are enforced down a chain, so an intermediate or root that enumerates EKUs prevents a leaf from asserting an EKU not in that list.

@agl opinions?

@FiloSottile FiloSottile added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 29, 2018
@agl
Copy link
Contributor

agl commented Mar 29, 2018

We've enforced nesting of requested EKUs for a long time. When redoing it, I asked sleevi how he would like to see it work and, having double checked with him, I had misunderstood and he would be fine with only enforcing the nesting of requested EKUs.

So we could relax this again, although the argument that I find compelling is that there could be a path (that we didn't find) which makes the nesting valid. The argument above seems to be that it's ok to not nest them because some verifiers don't check this and it's fine that some EKUs could never be accepted by a verifier that does. That seems to painting those verifiers into a corner where they could never enforce this, which doesn't seem like a positive move.

I also agree with @FiloSottile that a CT verifier would have sufficiently different goals that it would probably not want to use verify.go.

So while the overall request might be valid based on my updated understanding from sleevi, this is not a strong test case.

@rsc
Copy link
Contributor

rsc commented Apr 9, 2018

Leaving for @FiloSottile and @agl to decide.

@nodo
Copy link
Contributor

nodo commented May 4, 2018

We have also encountered this error. While waiting for a fix upstream, is there a workaround that we can use to ignore this validation error?

@FiloSottile
Copy link
Contributor

Based on discussion with @agl, we will go back to only enforcing nesting (and returning CANotAuthorizedForExtKeyUsage) when the EKU is being asserted in Verify.

@gopherbot, please open a tracking issue for backporting to 1.10.

@gopherbot
Copy link

Backport issue(s) opened: #25258 (for 1.10).

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

@FiloSottile FiloSottile self-assigned this May 4, 2018
@FiloSottile FiloSottile added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels May 4, 2018
jamesjoshuahill pushed a commit to cloudfoundry/backup-and-restore-sdk-release that referenced this issue May 16, 2018
- golang 1.10 has a bug parsing x509 certificates, see
  golang/go#24590

[#157626761]

Signed-off-by: Mirah Gary <mgary@pivotal.io>
cholick added a commit to vmware-archive/kibosh that referenced this issue May 16, 2018
@gopherbot
Copy link

Change https://golang.org/cl/113475 mentions this issue: crypto/x509: check EKUs like 1.9.

@agl agl changed the title crypto/x509: CANotAuthorizedForExtKeyUsage is a bogus error crypto/x509: CANotAuthorizedForExtKeyUsage is premature May 16, 2018
kieron-dev pushed a commit to pivotal-cf/service-backup-release that referenced this issue May 18, 2018
See golang/go#24590

[#157663527, #157286799]

Co-authored-by: Kieron Browne <kbrowne@pivotal.io>
@agl
Copy link
Contributor

agl commented May 21, 2018

We're going to change back to the 1.9 behaviour w.r.t. EKUs. But this is because Go generally doesn't try to be aggressive in fixing these ecosystem issues. If you hit chains that did not have nested EKUs, that's probably a landmind that will bite you in the future and something to be addressed.

blgm added a commit to pivotal-cf/cf-rabbitmq-multitenant-broker-release that referenced this issue May 23, 2018
- this is because of a [bug in
1.10](golang/go#24590)

[#157768129]
@golang golang locked and limited conversation to collaborators May 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants