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: support DirectoryName name constraints #15196

Open
vanbroup opened this Issue Apr 8, 2016 · 26 comments

Comments

Projects
None yet
8 participants
@vanbroup
Contributor

vanbroup commented Apr 8, 2016

I would like to request for the adoption of change 3230 which is in code review for a long time.

This change extents the Name Constraint properties by adding the Excluded property for DNSDomains and both the permitted and excluded properties for EmailDomains, IPAddresses and DirectoryNames as specified for the GeneralName property in RFC5280.

The selected properties are required to create or validate a fully constrained certificate.

The change also improves the validation of Name Constraints in general and allows the parsing of certificates that have Name Constraints marked as cirtial.

https://go-review.googlesource.com/#/c/3230/

Change-Id: Idaa7abafec372d5eb444cad7ee2ea5794aee3424

To be able to validate all certificates issued according to the CA / Browser Forum Baseline Requirements the full set of name constraints need to be available in GO.

A recent version of the CA / Browser Forum Baseline Requirements states that Technical Constraints in Subordinate CA Certificates MUST be applied via Name Constraints. To support strong and strict certificate path validation and to allow users to see the actual constraints it's important that GO supports the required Name Constraints.

Section 9.7 of the baseline requirements states:

"If the Subordinate CA Certificate includes the id-kp-serverAuth extended key usage, then the Subordinate CA Certificate MUST include the Name Constraints X.509v3 extension with constraints on dNSName, iPAddress and DirectoryName as follows:-"

The full requirements can be found on: https://cabforum.org/baseline-requirements-documents/

The DirectoryName is also needed in some Microsoft environments. Forbidding all directory names would enforce domain validated certificates even if all certificates under a specific root are used and issued by the same organisation. Or could enforce all certificates to be issued with the same OV certificate details. Email addresses are not specifically handled by the CABForum because they don't cover client auth or s/mime certificates currently but likely to have the same requirements and make the set of constraints supported complete.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Apr 9, 2016

Assigning to @agl to decide the fate of this. His last comment on CL 3230 was:

I don't think that there's a need to handle verifying email addresses, nor to process DirectoryNames (other than to accept forbidding all DirectoryNames).

@vanbroup

This comment has been minimized.

Contributor

vanbroup commented Apr 11, 2016

Not handling directory names is against RFC 5280 section 4.2.1.10:

Applications conforming to this profile MUST be able to process name
constraints that are imposed on the directoryName name form and
SHOULD be able to process name constraints that are imposed on the
rfc822Name, uniformResourceIdentifier, dNSName, and iPAddress name
forms.

Handeling these name constraints should have minimal impact as the extension is only processed when actually present in the certificate and we are processing a part of the extension currently anyway. Partly processing makes it confusing, I think we should support name constraints with all it's attributes or not support them at all.

@tadukurow

This comment has been minimized.

tadukurow commented May 3, 2017

Has there been any more thought regarding this issue?

We are seeing public CA certificates issued with critical name constraint (per RFC 5280) and Excluded subtrees for IP addresses (per the Baseline requirements https://cabforum.org/baseline-requirements-documents/).

I think x509 should be able to parse BR-compliant CAs.

@agl

This comment has been minimized.

Contributor

agl commented May 4, 2017

@tadukurow can you paste an example for testing?

I thought that I had done this but I see that I forgot to hit submit on https://go-review.googlesource.com/c/36900/. Also, that only supports it for names so I suspect that I'll want to extend it first.

@tadukurow

This comment has been minimized.

tadukurow commented May 4, 2017

Here is an example for a CA which does not allow ips in the san of its leaf certificates as per baseline requirements.

X509v3 Name Constraints: critical Excluded: IP:0.0.0.0/0.0.0.0 IP:0:0:0:0:0:0:0:0/0:0:0:0:0:0:0:0

-----BEGIN CERTIFICATE-----
MIIFojCCA4qgAwIBAgIJAOMWrsjU/s6/MA0GCSqGSIb3DQEBCwUAMD0xCzAJBgNV
BAYTAkdCMQ8wDQYDVQQHDAZMb25kb24xEDAOBgNVBAoMB0RlbW8gQ0ExCzAJBgNV
BAsMAklUMB4XDTE3MDUwNDE1NDQyM1oXDTM3MDQyOTE1NDQyM1owPTELMAkGA1UE
BhMCR0IxDzANBgNVBAcMBkxvbmRvbjEQMA4GA1UECgwHRGVtbyBDQTELMAkGA1UE
CwwCSVQwggIiMA0GCSqGSIb3DQEBAQUAA4ICDwAwggIKAoICAQCy6709MthSnIg6
jl9+fgSdqdok56ElUVVaWiF0skZy17skS05zkdLXT15Su4Ffqs7kRawO4IHZvNVc
oVGOkz4xnG4UT8w9dhtu10yUBbs/hi4/KWtdUFNvu2xU0vEzxuLjCZNyOmy5+rYB
TfD6RIZ3eaElE5dZv0syZfG3WoVJMCYZt3KL7O0FtXSd6lCqUNE+bcbxEZ6Av4aJ
NMGMKK+3ML1lDwmyDalUnkOlcTkxlUt2mBt4MUHzc3fmjY4GgfjlyjKp1bzrBVU5
sGrGGiZgH/kYvqABsUyaf3ejvg413YB66bAOr1sMvm1HGnqock8NpEYCJmWGDBox
2s6o/nNO1i1+eE11Iyty24wx1Wwx1VMXd08CYDPpdhkkfN9LK+8yK7CpOViOy6/a
Xc2h7W0QHvWXCHGoEPDjGPKhSVzB2YR/+Nxlei9SjiI1cOE0ThTH1ikO3/3+S3yT
KGZUJT8Mlra7IUBGfbwm63WRyEI9rlowhtGnA7bNtRPekiA95TqYmf5ZGYyksCso
wnYMU8Onm0JkDu5smeV+UGuqV5kXRiCOKoxdbGiJXWYZHNXEc1rnnDzN5TMJK3S2
qLh0IpAJtQwhS++Oi0oTD3bjsyF9RPtDFnxYbxQJYGO/ig5WG1MQ97QMcUu94ll6
vcjm8B14OUEkSzOVvxJtsaeldquRWQIDAQABo4GkMIGhMB0GA1UdDgQWBBQBMXxW
PYExW1n/j0gJPuO7idEwQDAfBgNVHSMEGDAWgBQBMXxWPYExW1n/j0gJPuO7idEw
QDAPBgNVHRMBAf8EBTADAQH/MA4GA1UdDwEB/wQEAwIBhjA+BgNVHR4BAf8ENDAy
oTAwCocIAAAAAAAAAAAwIocgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
AAAwDQYJKoZIhvcNAQELBQADggIBAAxNFDaTK70DtoX5NkWpfiNau1yyFGryygqb
cTQAwkm8U+qp3Ktwf59eQfCd6fwq0aZXHbF+oLTuJrF9SWrx7RG5maZjAHXQKC52
x5c6jYoWxhBiWcBGf9nYMTs16PKi0gRr5MLWaGsDWG8hMxZPeDW7S7c8Z1UXEul3
6AyHKMON56lqB7i2/b1VB18eDW8g6Y7fyWvNhJBJx5tgAq+zgQNBaKfmmwP3XoRG
2WZXN28+vX6rgpSG9oCei26A4/BYEp37tC2VMTaJ8rvAWaRr+B3jYes94a25E/q1
Vp1/IZBY8nCMZgsz5tusROI+rQPdZPV8AJVSgQsfPZFmBY9V9vMYkuiuTTtnzNCo
vrVU5sQWau+oOKYz/nsip0A7Bq1ZKhcSs3Bzpn1sh870yLWL1ALOLQf2SGrK/lRK
+jcPSifqcme7kUQqjX2+gSn9j0deM3UMoEEfVNt39jnzmDVlxyFzhszLCZFzE+X4
sKuKSsrSCVZdzqC3SKgWrJLTEJajoI5I3pFf4hM9kuinlijOQXtQjPERWeBiUhEH
BqFeLZOvDMcru4UPmaDe5G3J5vNDzk3jvNwc/w1hvESgb8/aiNZFYqNN0bL1RXAH
7LknjK/MZvCZ+nBvjWQMfLhbn6TZqiRHSo55e3DCiCCtffEdGjiPOUdjWRPd97YE
p0Oon62s
-----END CERTIFICATE-----
@vanbroup

This comment has been minimized.

Contributor

vanbroup commented Jun 30, 2017

Hi @agl,

I see that ExcludedDNSDomains is included in the beta for 1.9, but what about the remaining name constraints?

	PermittedEmailDomains       []string
	ExcludedEmailDomains        []string
	PermittedIPAddresses        []net.IPNet
	ExcludedIPAddresses         []net.IPNet
	PermittedDirectoryNames     []pkix.Name
	ExcludedDirectoryNames      []pkix.Name

These name constraints are needed to parse publicly trusted CA certs, here I have listed just some random publicly trusted CA certificates from different roots that together include all these types of name constraints:

https://ssl-tools.net/certificates/7ea2490df8ea1f80134498caf93dde770c4e195a.txt
https://ssl-tools.net/certificates/e12ba5aeb7613a72cc9652f1673017a5d8fc7479.txt
https://ssl-tools.net/certificates/f124f3083de686a14eec17389eee8990eafecbd2.txt
https://ssl-tools.net/certificates/36059970e35c0fd7030c7746b92168a3c29816f5.txt

@vanbroup

This comment has been minimized.

Contributor

vanbroup commented Jul 7, 2017

As also required by the Mozilla Root Store Policy:

If the certificate includes the id-kp-serverAuth extended key usage, then the certificate MUST be Name Constrained as described in section 7.1.5 of version 1.3 or later of the Baseline Requirements.

If the certificate includes the id-kp-emailProtection extended key usage, it MUST include the Name Constraints X.509v3 extension with constraints on rfc822Name, with at least one name in permittedSubtrees, each such name having its ownership validated according to section 3.2.2.4 of the Baseline Requirements.

https://www.mozilla.org/en-US/about/governance/policies/security-group/certs/policy/#technically-constrained

@agl

This comment has been minimized.

Contributor

agl commented Jul 7, 2017

The other types did not make it into 1.9, which is now frozen I'm afraid. Shouldn't be a stretch for 1.10, although we do not support validating against emails or directory names so some thought is required about how that would be exposed. (I.e. don't expose and ignore them, even if critical, since we'll never use them in the stdlib and let people extract the extension if they care?)

@bradfitz bradfitz modified the milestones: Go1.10, Unplanned Jul 7, 2017

@bradfitz bradfitz changed the title from crypto/x509: Adding Name Constraints to crypto/x509: add Name Constraints Jul 7, 2017

@briansmith

This comment has been minimized.

briansmith commented Jul 7, 2017

@agl Directory names are used to chain certificates together issuer<->subject, so directory name constraints are always relevant. Also the name constraint enforcement is supposed to be done independently of what kind of name is being validated. That is, if there is an email address constraint and that constraint is violated, then the certificate chain should be rejected even if you're validating a TLS server name.

@agl

This comment has been minimized.

Contributor

agl commented Jul 7, 2017

That is, if there is an email address constraint and that constraint is violated, then the certificate chain should be rejected

That's the "word of God" approach to certificate validation. I.e. that once validated, everything in the certificate is valid. Go does not take that approach and always considers validity in a specific context.

@vanbroup

This comment has been minimized.

Contributor

vanbroup commented Jul 8, 2017

I absolutely don't like the approach of ignoring the critical flag, these constraints and the flag is there with a good reason. While I see your argument that Go does not follow the "word of God", which is fine. Let's instead verify what "God" tells us and do more on top of that where possible. If we start leaving out parts of the standards and validation logic the mess is only getting bigger.

We can't say that the users need to parse the extension them selfs as this would mean that they also have to perform the full chain validation and certificate handling them self which is not even possible in all use cases. Name constraints are a key part of the certificate chain validation.

Instead of exposing all name constraint details in the x509.Certificate struct I can imagine that we would prefer to put the name constraints in its own linked struct like pkix.Name. We could also decide to include verification options in x509.VerifyOptions that let users choose to disable name constraint checking.

Another approach is to add a hook to the certificate validation logic to handle 'unknown' or override/extend known extension handling. This way users can fulfill any type of critial extension (also thinking about OCSP must-staple) and perform full name constraints validation in the certificate chain without the need to rewrite the x509 package.

@briansmith

This comment has been minimized.

briansmith commented Jul 9, 2017

Hi Adam, you probably already know this, but the design, (lack of) documentation, and code of the Verify() and VerifyHostname() functions suggest that it's good to do it the way I suggest.

For example, IIUC, VerifyOptions.DNSName isn't supposed to be required, but IIUC it is required now if and only if there are any “permitted” name constraints, which seems weird.

It also seems reasonable to call Verify() once (perhaps with a DNSName) and then call VerifyHostname() one or more times with different hostnames, for example if one is trying to implement the HTTP/2 connection coalescing feature. However, VerifyHostname() doesn't check the name it is given against the name constraints, IIUC.

I think if you want to continue doing things as you suggest then it might be better to do the name constraint check in VerifyHostname() instead of in isValid(). Note that isValid() calls VerifyHostname() when the DNSName option is given.

Sorry if I'm misunderstanding something.

@agl

This comment has been minimized.

Contributor

agl commented Jul 11, 2017

Brian, you're not misunderstanding. (Although I don't think Paul's argument that context-dependent validation is ok but ignoring these constraints is not, is sound. One implies the other unless we're adding contexts for email and so on.)

When the Go X.509 library was first written it was quite unclear whether context-dependent validations were going to be the norm or not, mostly because of DANE. With a DNSSEC-based system, it's not possible to implement a Voice of God-style validation because the authority is inherently limited.

But, as you note, the API was frozen long ago and isn't well demarcated. And, for a long time, it didn't really matter whether people got it right or not.

I've written a CL previously to switch Go over to Voice of God but it didn't end up landing because it was somewhat large and I was overtaken by other stuff. VOG is less surprising and, after Google decided to buttress X.509 rather than replace it, and with Let's Encrypt eliminating most of the friction for large numbers of people, VOG is looking fairly dominant.

So reviving that effort might be a sensible prereq here. I assume it would make Paul happy because VOG implies checking all constraints up-and-down the chain at verification time.

(Although there's still some wooly thinking happening from what I can see. If intermediates need to have negative constraints for all the common other SAN types, then how do new name types ever get added? By that scheme, it would need every constrained intermediate to be updated before it would be safe. At best, I guess, we can declare that all the existing name types got this wrong but, for new ones, any name constraints implicitly exclude them.)

@rsc

This comment has been minimized.

Contributor

rsc commented Aug 10, 2017

I've heard off-issue from some people working with SPIFFE that need URI-based Name Constraints, and SPIFFE in turn is hopefully going to be written in Go and used by a variety of systems, including gRPC and Istio.

I'm trying to understand the current state here. @agl, it sounds like you are okay with moving to the VOG approach to X.509 validation, and he may still have an old CL doing much of the work. The issue is titled "add Name Constraints", though. How would moving to VOG address that issue, or what new API would be needed?

Thanks.

@rsc

This comment has been minimized.

Contributor

rsc commented Aug 11, 2017

I spoke to @agl, and he confirmed that he believes moving to VOG is the right thing to do. He hopes to do that for Go 1.10, and then adding URI-based Name Constraints would follow. If there are crypto experts who want to help, please let us know.

@vanbroup

This comment has been minimized.

Contributor

vanbroup commented Aug 14, 2017

@rsc, @agl, my previously submitted patch to implement this can't merge and anymore but I can contribute some coding and/or testing time to fix this patch or to create something new. Have there been any further thoughts about the implementation?

I was thinking that instead of creating a list for 'each' general name type in x509.Certificate it might be cleaner to create one GeneralName interface in crypto/x509/pkix?

crypto/x509

type NameConstraints struct {
  Permitted pkix.GeneralName
  Excluded pkix.GeneralName
}

// Permitted verifies if the given GeneralName is allowed to be used according to the 
// name constraint rules. The name must be explicitly be Permitted or no other names 
// of the given type must be listed as permitted or excluded.
func (n *NameConstraints) Permitted(pkix.GeneralName) bool {
  ...
}

type Certificate struct {
  ...
  NameConstraintsCritical bool // if true then the name constraints are marked critical
  NameConstraints NameConstraints
  ...
}

NameConstraintsCritical could also be exposed as function Critital() on NameConstraints?

crypto/x509/pkix

type GeneralName interface{
  String() string
}

type RFC822Name string
type DNSName string
type DirectoryName pkix.Name
type UniformResourceIdentifier net.URL
type IPAddress net.IP
...
@gopherbot

This comment has been minimized.

gopherbot commented Oct 13, 2017

Change https://golang.org/cl/62693 mentions this issue: crypto/x509: enforce all name constraints and support IP, email and URI constraints

@vanbroup

This comment has been minimized.

Contributor

vanbroup commented Oct 13, 2017

@agl thanks for working on this, can we please include directoryName too?

directoryName is used a key restriction to prevent constrained sub-CAs to issue under a non-validated/allowed subject DN.

@gopherbot

This comment has been minimized.

gopherbot commented Oct 15, 2017

Change https://golang.org/cl/71030 mentions this issue: crypto/x509: enforce EKU nesting at chain-construction time.

@agl

This comment has been minimized.

Contributor

agl commented Oct 15, 2017

@agl thanks for working on this, can we please include directoryName too?

Based on internal feedback from people who know more about X.509 than I, directoryName constraints were less useful and so I didn't include them in the first CL. The other thing that needs to get done this cycle is EKU checking at chain building time (in order to match the constraints behaviour). After that, it's possible but November is past approaching.

gopherbot pushed a commit that referenced this issue Nov 7, 2017

crypto/x509: enforce all name constraints and support IP, email and U…
…RI constraints

This change makes crypto/x509 enforce name constraints for all names in
a leaf certificate, not just the name being validated. Thus, after this
change, if a certificate validates then all the names in it can be
trusted – one doesn't have a validate again for each interesting name.

Making extended key usage work in this fashion still remains to be done.

Updates #15196

Change-Id: I72ed5ff2f7284082d5bf3e1e86faf76cef62f9b5
Reviewed-on: https://go-review.googlesource.com/62693
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>

gopherbot pushed a commit that referenced this issue Nov 7, 2017

crypto/x509: enforce EKU nesting at chain-construction time.
crypto/x509 has always enforced EKUs as a chain property (like CAPI, but
unlike the RFC). With this change, EKUs will be checked at
chain-building time rather than in a target-specific way.

Thus mis-nested EKUs will now cause a failure in Verify, irrespective of
the key usages requested in opts. (This mirrors the new behaviour w.r.t.
name constraints, where an illegal name in the leaf will cause a Verify
failure, even if the verified name is permitted.).

Updates #15196

Change-Id: Ib6a15b11a9879a9daf5b1d3638d5ebbbcac506e5
Reviewed-on: https://go-review.googlesource.com/71030
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@gopherbot

This comment has been minimized.

gopherbot commented Nov 8, 2017

Change https://golang.org/cl/74270 mentions this issue: vendor: add golang.org/x/crypto/cryptobyte

gopherbot pushed a commit that referenced this issue Nov 8, 2017

vendor: add golang.org/x/crypto/cryptobyte
This change adds the cryptobyte package from x/crypto at git revision
faadfbdc035307d901e69eea569f5dda451a3ee3.

Updates #22616, #15196

Change-Id: Iffd0b022ca129d340ef429697e05b581f04e5c4f
Reviewed-on: https://go-review.googlesource.com/74270
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@rsc rsc added NeedsFix and removed NeedsDecision labels Nov 22, 2017

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 30, 2017

@agl, what remains here? Anything for Go 1.10?

albertito added a commit to albertito/chasquid that referenced this issue Dec 8, 2017

test: Make generate_cert use IDNA for certificate fields
In Go 1.10 the TLS library will start to reject DNS SANs which are not
properly formed; and in particular, if they're not IDNA-encoded. See:
 - golang/go#15196
 - golang/go@9e76ce7

The generate_cert utility will write non-IDNA DNS SANs, which the TLS
library does not like, causing our idna tests to fail.

This patch fixes this incompatibility by making generate_cert IDNA-encode
the host names when adding them to the certificate.

@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Jan 6, 2018

@jeffcpullen

This comment has been minimized.

jeffcpullen commented Feb 1, 2018

@agl Thank you for working on this, based on the milestone change, is it safe to assume this will hit with 1.11?

@bradfitz

This comment has been minimized.

Member

bradfitz commented Feb 1, 2018

@jeffcpullen, that's not a safe assumption. It previously had Milestone Go1.10 and wasn't included in Go 1.10.

@agl

This comment has been minimized.

Contributor

agl commented Feb 1, 2018

What's in 1.10 is everything except constraints on directory names, which seemed to be less important.

DNS, Email, and IP constraints should all be enforced, and EKUs are also enforced as a tree property. (Glancing over the big table in the test file should give some idea of what the behaviour will be.)

@rsc

This comment has been minimized.

Contributor

rsc commented Aug 17, 2018

Per @agl's earlier comment, all that's left here is DirectoryName name constraints.
It's unclear whether that will happen or not so changing back to NeedsDecision
and updating milestone to Go 1.12.

@rsc rsc modified the milestones: Go1.11, Go1.12 Aug 17, 2018

@rsc rsc changed the title from crypto/x509: add Name Constraints to crypto/x509: support DirectoryName name constraints Aug 17, 2018

@rsc rsc added NeedsDecision and removed NeedsFix labels Aug 17, 2018

@rsc rsc modified the milestones: Go1.12, Go1.13 Nov 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment