[go] crypto/x509: add directory name constraints (WIP)
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded this change for review.
crypto/x509: add directory name constraints (WIP)
It is still missing tests.
Fixes #15196
This is my first day using golang. So, this PR might have newbie codes that might require improvements.
I'm reparsing Subject in order to compare it. However, Certificate class could keep a RDNSequence and save some cycles.
It might be a better way to do the dirname compassion (without using three nested for).
Change-Id: Ie8d0d2a7bd865c0cf7b5ac2296e09d8248945d7e
GitHub-Last-Rev: 4f8fbd34448d9208a05279206e406d59b53a5896
GitHub-Pull-Request: golang/go#39639
---
M src/crypto/x509/verify.go
M src/crypto/x509/x509.go
2 files changed, 82 insertions(+), 19 deletions(-)
diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go
index be11e73..57ed57a 100644
--- a/src/crypto/x509/verify.go
+++ b/src/crypto/x509/verify.go
@@ -6,6 +6,8 @@
import (
"bytes"
+ "crypto/x509/pkix"
+ "encoding/asn1"
"errors"
"fmt"
"net"
@@ -516,6 +518,34 @@
return true, nil
}
+func matchDirNameConstraint(dirname pkix.RDNSequence, constraint *pkix.RDNSequence) (bool, error) {
+
+ if len(*constraint) > len(dirname) {
+ return false, nil
+ }
+ for i, rdn := range *constraint {
+ if len(rdn) > len(dirname[i]) {
+ return false, nil
+ }
+ for j, const_tv := range rdn {
+ dirname_tv := dirname[i][j]
+ if len(const_tv.Type) != len(dirname_tv.Type) {
+ return false, nil
+ }
+ for k, _ := range const_tv.Type {
+ if const_tv.Type[k] != dirname_tv.Type[k] {
+ return false, nil
+ }
+ }
+ if const_tv.Value != dirname_tv.Value {
+ return false, nil
+ }
+ }
+ }
+
+ return true, nil
+}
+
// checkNameConstraints checks that c permits a child certificate to claim the
// given name, of type nameType. The argument parsedName contains the parsed
// form of name, suitable for passing to the match function. The total number
@@ -623,6 +653,24 @@
}
checkNameConstraints := (certType == intermediateCertificate || certType == rootCertificate) && c.hasNameConstraints()
+ if checkNameConstraints {
+ var leafSubject pkix.RDNSequence
+
+ // leaf.Subject.ToRDNSequence cannot be used as it ignores unknown RDN
+ if rest, err := asn1.Unmarshal(leaf.RawSubject, &leafSubject); err != nil {
+ return err
+ } else if len(rest) != 0 {
+ return errors.New("x509: trailing data after X.509 subject")
+ }
+
+ if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "directory Name", leaf.Subject.String(), leafSubject,
+ func(parsedName, constraint interface{}) (bool, error) {
+ return matchDirNameConstraint(parsedName.(pkix.RDNSequence), constraint.(*pkix.RDNSequence))
+ }, c.PermittedDirNames, c.ExcludedDirNames); err != nil {
+ return err
+ }
+ }
+
if checkNameConstraints && leaf.commonNameAsHostname() {
// This is the deprecated, legacy case of depending on the commonName as
// a hostname. We don't enforce name constraints against the CN, but
diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go
index 338b488..cdfef07 100644
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -748,6 +748,8 @@
// Name constraints
PermittedDNSDomainsCritical bool // if true then the name constraints are marked critical.
+ PermittedDirNames []*pkix.RDNSequence
+ ExcludedDirNames []*pkix.RDNSequence
PermittedDNSDomains []string
ExcludedDNSDomains []string
PermittedIPRanges []*net.IPNet
@@ -1211,27 +1213,28 @@
return false, errors.New("x509: empty name constraints extension")
}
- getValues := func(subtrees cryptobyte.String) (dnsNames []string, ips []*net.IPNet, emails, uriDomains []string, err error) {
+ getValues := func(subtrees cryptobyte.String) (dirNames []*pkix.RDNSequence, dnsNames []string, ips []*net.IPNet, emails, uriDomains []string, err error) {
for !subtrees.Empty() {
var seq, value cryptobyte.String
var tag cryptobyte_asn1.Tag
if !subtrees.ReadASN1(&seq, cryptobyte_asn1.SEQUENCE) ||
!seq.ReadAnyASN1(&value, &tag) {
- return nil, nil, nil, nil, fmt.Errorf("x509: invalid NameConstraints extension")
+ return nil, nil, nil, nil, nil, fmt.Errorf("x509: invalid NameConstraints extension")
}
var (
- dnsTag = cryptobyte_asn1.Tag(2).ContextSpecific()
- emailTag = cryptobyte_asn1.Tag(1).ContextSpecific()
- ipTag = cryptobyte_asn1.Tag(7).ContextSpecific()
- uriTag = cryptobyte_asn1.Tag(6).ContextSpecific()
+ dirNameTag = cryptobyte_asn1.Tag(4).ContextSpecific().Constructed()
+ dnsTag = cryptobyte_asn1.Tag(2).ContextSpecific()
+ emailTag = cryptobyte_asn1.Tag(1).ContextSpecific()
+ ipTag = cryptobyte_asn1.Tag(7).ContextSpecific()
+ uriTag = cryptobyte_asn1.Tag(6).ContextSpecific()
)
switch tag {
case dnsTag:
domain := string(value)
if err := isIA5String(domain); err != nil {
- return nil, nil, nil, nil, errors.New("x509: invalid constraint value: " + err.Error())
+ return nil, nil, nil, nil, nil, errors.New("x509: invalid constraint value: " + err.Error())
}
trimmedDomain := domain
@@ -1243,10 +1246,22 @@
trimmedDomain = trimmedDomain[1:]
}
if _, ok := domainToReverseLabels(trimmedDomain); !ok {
- return nil, nil, nil, nil, fmt.Errorf("x509: failed to parse dnsName constraint %q", domain)
+ return nil, nil, nil, nil, nil, fmt.Errorf("x509: failed to parse dnsName constraint %q", domain)
}
dnsNames = append(dnsNames, domain)
+ case dirNameTag:
+
+ var dirName pkix.RDNSequence
+
+ if rest, err := asn1.Unmarshal(value, &dirName); err != nil {
+ return nil, nil, nil, nil, nil, err
+ } else if len(rest) != 0 {
+ return nil, nil, nil, nil, nil, errors.New("x509: trailing data after dirname constraint")
+ }
+
+ dirNames = append(dirNames, &dirName)
+
case ipTag:
l := len(value)
var ip, mask []byte
@@ -1261,11 +1276,11 @@
mask = value[16:]
default:
- return nil, nil, nil, nil, fmt.Errorf("x509: IP constraint contained value of length %d", l)
+ return nil, nil, nil, nil, nil, fmt.Errorf("x509: IP constraint contained value of length %d", l)
}
if !isValidIPMask(mask) {
- return nil, nil, nil, nil, fmt.Errorf("x509: IP constraint contained invalid mask %x", mask)
+ return nil, nil, nil, nil, nil, fmt.Errorf("x509: IP constraint contained invalid mask %x", mask)
}
ips = append(ips, &net.IPNet{IP: net.IP(ip), Mask: net.IPMask(mask)})
@@ -1273,14 +1288,14 @@
case emailTag:
constraint := string(value)
if err := isIA5String(constraint); err != nil {
- return nil, nil, nil, nil, errors.New("x509: invalid constraint value: " + err.Error())
+ return nil, nil, nil, nil, nil, errors.New("x509: invalid constraint value: " + err.Error())
}
// If the constraint contains an @ then
// it specifies an exact mailbox name.
if strings.Contains(constraint, "@") {
if _, ok := parseRFC2821Mailbox(constraint); !ok {
- return nil, nil, nil, nil, fmt.Errorf("x509: failed to parse rfc822Name constraint %q", constraint)
+ return nil, nil, nil, nil, nil, fmt.Errorf("x509: failed to parse rfc822Name constraint %q", constraint)
}
} else {
// Otherwise it's a domain name.
@@ -1289,7 +1304,7 @@
domain = domain[1:]
}
if _, ok := domainToReverseLabels(domain); !ok {
- return nil, nil, nil, nil, fmt.Errorf("x509: failed to parse rfc822Name constraint %q", constraint)
+ return nil, nil, nil, nil, nil, fmt.Errorf("x509: failed to parse rfc822Name constraint %q", constraint)
}
}
emails = append(emails, constraint)
@@ -1297,11 +1312,11 @@
case uriTag:
domain := string(value)
if err := isIA5String(domain); err != nil {
- return nil, nil, nil, nil, errors.New("x509: invalid constraint value: " + err.Error())
+ return nil, nil, nil, nil, nil, errors.New("x509: invalid constraint value: " + err.Error())
}
if net.ParseIP(domain) != nil {
- return nil, nil, nil, nil, fmt.Errorf("x509: failed to parse URI constraint %q: cannot be IP address", domain)
+ return nil, nil, nil, nil, nil, fmt.Errorf("x509: failed to parse URI constraint %q: cannot be IP address", domain)
}
trimmedDomain := domain
@@ -1313,7 +1328,7 @@
trimmedDomain = trimmedDomain[1:]
}
if _, ok := domainToReverseLabels(trimmedDomain); !ok {
- return nil, nil, nil, nil, fmt.Errorf("x509: failed to parse URI constraint %q", domain)
+ return nil, nil, nil, nil, nil, fmt.Errorf("x509: failed to parse URI constraint %q", domain)
}
uriDomains = append(uriDomains, domain)
@@ -1322,13 +1337,13 @@
}
}
- return dnsNames, ips, emails, uriDomains, nil
+ return dirNames, dnsNames, ips, emails, uriDomains, nil
}
- if out.PermittedDNSDomains, out.PermittedIPRanges, out.PermittedEmailAddresses, out.PermittedURIDomains, err = getValues(permitted); err != nil {
+ if out.PermittedDirNames, out.PermittedDNSDomains, out.PermittedIPRanges, out.PermittedEmailAddresses, out.PermittedURIDomains, err = getValues(permitted); err != nil {
return false, err
}
- if out.ExcludedDNSDomains, out.ExcludedIPRanges, out.ExcludedEmailAddresses, out.ExcludedURIDomains, err = getValues(excluded); err != nil {
+ if out.ExcludedDirNames, out.ExcludedDNSDomains, out.ExcludedIPRanges, out.ExcludedEmailAddresses, out.ExcludedURIDomains, err = getValues(excluded); err != nil {
return false, err
}
out.PermittedDNSDomainsCritical = e.Critical
To view, visit change 238362. To unsubscribe, or for help writing mail filters, visit settings.
Gobot Gobot (Gerrit)
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
A maintainer will review your change and provide feedback. See
https://golang.org/doc/contribute.html#review for more info and tips to get your
patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://golang.org/s/release
for more details.
Paul van Brouwershaven (Gerrit)
Thanks for your contribution, I would really appreciate if we can finally handle directory name constraints in Go.
Don't forget to extend the test cases to cover different scenarios with directory based name constraints!
Besides the ones I listed in #15196, you can find some example certificates with directory name constraints here:
https://censys.io/certificates?q=parsed.extensions.name_constraints.excluded_directory_names%3A%2A
But maybe it's easier to just generate some certificate to cover the different directory name constraint test scenarios.
Paul van Brouwershaven (Gerrit)
Sorry, included the wrong Censys search query, this one includes the permitted ones which make more sense.
Luiz Angelo Daros de Luca (Gerrit)
Thanks Paul, some example certs might help speed up tests. Anyway, I know how to do it from scratch.
I just posted it sooner without tests to get some feedback on code. This is my first day with golang. I just want to know if this is the right track to follow and it just need some fix to get accepted.
I will try to get some tests working.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #2 to this change.
crypto/x509: add directory name constraints (WIP)
It is still missing tests.
Fixes #15196
This is my first day using golang. So, this PR might have newbie codes that might require improvements.
I'm reparsing Subject in order to compare it. However, Certificate class could keep a RDNSequence and save some cycles.
It might be a better way to do the dirname compassion (without using three nested for).
Change-Id: Ie8d0d2a7bd865c0cf7b5ac2296e09d8248945d7e
GitHub-Last-Rev: 70bf4843fb8f65303d86321abacad172424cd537
GitHub-Pull-Request: golang/go#39639
---
M src/crypto/x509/verify.go
M src/crypto/x509/verify_test.go
M src/crypto/x509/x509.go
3 files changed, 1,179 insertions(+), 19 deletions(-)
To view, visit change 238362. To unsubscribe, or for help writing mail filters, visit settings.
Luiz Angelo Daros de Luca (Gerrit)
I added tests. I created a bunch of certificates and validated them against openssl.
After that, I added the same test to golang.
It uncovered a bug where dirname constraints was only validating leaf certificate against the chain,
while it should also validate intermediate CAs against it.
I still want to test if I a subCA could (wrongly) relax a constraint (adding extra permitted).
I didn't check the code but it looks like all permmited/excluded constraints are grouped into the leaf certificate instead of individually checking each CA for restrictions. For example:
- RootCA permit example.com
- SubCA permit x.example.com and permit x.example2.com
If we join permits, you'll allow leaf certificates to use example.com and example2.com while it should only allow x.example.com
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #3 to this change.
crypto/x509: add directory name constraints (WIP)
It is still missing tests.
Fixes #15196
This is my first day using golang. So, this PR might have newbie codes that might require improvements.
I'm reparsing Subject in order to compare it. However, Certificate class could keep a RDNSequence and save some cycles.
It might be a better way to do the dirname compassion (without using three nested for).
Change-Id: Ie8d0d2a7bd865c0cf7b5ac2296e09d8248945d7e
GitHub-Last-Rev: 7285b7bfdb8708d280100ab01e0d640da162116d
GitHub-Pull-Request: golang/go#39639
---
M src/crypto/x509/verify.go
M src/crypto/x509/verify_test.go
M src/crypto/x509/x509.go
3 files changed, 1,179 insertions(+), 19 deletions(-)
To view, visit change 238362. To unsubscribe, or for help writing mail filters, visit settings.
Luiz Angelo Daros de Luca (Gerrit)
I still want to test if I a subCA could (wrongly) relax a constraint (adding extra permitted).
All good. I added some more tests related to this and they were all clear.
Luiz Angelo Daros de Luca (Gerrit)
I would like to update commit message (just removing extra commends)
Paul van Brouwershaven (Gerrit)
Thanks for adding these tests, this should really help to get this go through when some of the Go team look at this.
I also noticed that you currently only parse and validate the directory name constraints, can you make sure that they will also be marshalled when creating a new certificate?
DO NOT USE (Gerrit)
Luiz, thank you for contributing. Before we can move to the implementation and code review, we should decide on the issue tracker that we want to add this feature.
You can help that discussion by explaining there what you need it for.
Luiz Angelo Daros de Luca (Gerrit)
You can help that discussion by explaining there what you need it for.
You mean to discuss here?
https://github.com/golang/go/issues/15196#issuecomment-645079361
I already posted there before this PR. In summary:
1. It's mandatory (MUST) by RFC5280 in contrast of all other name constraints (which Go already implements). Does 'not respecting RFC5280' qualify this issue as a bug?
2. It breaks any PKI that uses directory name constraints (like my case). It's not a simple 'does not check'. It rejects any TLS conn.
3. It is generally used in Windows AD CA to limit SubCA for subdomains, avoiding them to generate a certificate for a high level domain.
Paul van Brouwershaven (Gerrit)
I would say another good argument for this is that there are over 180 publicly trusted and non expired issuing CA's with directory name constraints (see my Censys link earlier).
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #4 to this change.
crypto/x509: add directory name constraints (WIP)
It is still missing tests.
Fixes #15196
This is my first day using golang. So, this PR might have newbie codes that might require improvements.
I'm reparsing Subject in order to compare it. However, Certificate class could keep a RDNSequence and save some cycles.
It might be a better way to do the dirname compassion (without using three nested for).
Change-Id: Ie8d0d2a7bd865c0cf7b5ac2296e09d8248945d7e
GitHub-Last-Rev: e44eb9fbe75aaa50a29ad5abb61b2eb110fed46e
GitHub-Pull-Request: golang/go#39639
---
M src/crypto/x509/verify.go
M src/crypto/x509/verify_test.go
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
4 files changed, 1,454 insertions(+), 23 deletions(-)
To view, visit change 238362. To unsubscribe, or for help writing mail filters, visit settings.
Luiz Angelo Daros de Luca (Gerrit)
I also noticed that you currently only parse and validate the directory name constraints, can you make sure that they will also be marshalled when creating a new certificate?
I didn't know go API was also capable of creating certificates. I added the needed code and also expanded the existing mashall/unmarshall test.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #5 to this change.
crypto/x509: add directory name constraints
It is still missing tests.
Fixes #15196
This is my first day using golang. So, this PR might have newbie codes that might require improvements.
I'm reparsing Subject in order to compare it. However, Certificate class could keep a RDNSequence and save some cycles.
It might be a better way to do the dirname compassion (without using three nested for).
Change-Id: Ie8d0d2a7bd865c0cf7b5ac2296e09d8248945d7e
GitHub-Last-Rev: e44eb9fbe75aaa50a29ad5abb61b2eb110fed46e
GitHub-Pull-Request: golang/go#39639
---
M src/crypto/x509/verify.go
M src/crypto/x509/verify_test.go
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
4 files changed, 1,454 insertions(+), 23 deletions(-)
To view, visit change 238362. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #6 to this change.
crypto/x509: add directory name constraints
Fixes #15196
Change-Id: Ie8d0d2a7bd865c0cf7b5ac2296e09d8248945d7e
GitHub-Last-Rev: e44eb9fbe75aaa50a29ad5abb61b2eb110fed46e
GitHub-Pull-Request: golang/go#39639
---
M src/crypto/x509/verify.go
M src/crypto/x509/verify_test.go
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
4 files changed, 1,454 insertions(+), 23 deletions(-)
To view, visit change 238362. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #7 to this change.
crypto/x509: add directory name constraints
Fixes #15196
Change-Id: Ie8d0d2a7bd865c0cf7b5ac2296e09d8248945d7e
GitHub-Last-Rev: a3b93f5e2b0c082ca72d39083b4be60339f69c40
GitHub-Pull-Request: golang/go#39639
---
M src/crypto/x509/verify.go
M src/crypto/x509/verify_test.go
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
4 files changed, 1,454 insertions(+), 23 deletions(-)
To view, visit change 238362. To unsubscribe, or for help writing mail filters, visit settings.
Luiz Angelo Daros de Luca (Gerrit)
1 comment:
Patchset:
Hello,
As go 1.16 was released, wouldn't it be a good time to merge?
To view, visit change 238362. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #8 to this change.
crypto/x509: add directory name constraints
Fixes #15196
Change-Id: Ie8d0d2a7bd865c0cf7b5ac2296e09d8248945d7e
GitHub-Last-Rev: eef1cd3020577110ca4fd0138240771bb4dad45f
GitHub-Pull-Request: golang/go#39639
---
M src/crypto/x509/parser.go
M src/crypto/x509/verify.go
M src/crypto/x509/verify_test.go
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
5 files changed, 1,454 insertions(+), 23 deletions(-)
To view, visit change 238362. To unsubscribe, or for help writing mail filters, visit settings.
Gopher Robot (Gerrit)
Gopher Robot abandoned this change.
To view, visit change 238362. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Ian Lance Taylor restored this change.
To view, visit change 238362. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #9 to this change.
crypto/x509: add directory name constraints
Fixes #15196
Change-Id: Ie8d0d2a7bd865c0cf7b5ac2296e09d8248945d7e
GitHub-Last-Rev: 56134a2cda0f92a46f6b6075d3cf84f9bf4cb60a
GitHub-Pull-Request: golang/go#39639
---
M src/crypto/x509/parser.go
M src/crypto/x509/verify.go
M src/crypto/x509/verify_test.go
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
5 files changed, 1,466 insertions(+), 23 deletions(-)
To view, visit change 238362. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Filippo Valsorda.
Gerrit Bot uploaded patch set #10 to this change.
crypto/x509: add directory name constraints
Fixes #15196
Change-Id: Ie8d0d2a7bd865c0cf7b5ac2296e09d8248945d7e
GitHub-Last-Rev: 6ba02dd14e5d7756b54853f760fdf797ef02e6be
GitHub-Pull-Request: golang/go#39639
---
M src/crypto/x509/parser.go
M src/crypto/x509/verify.go
M src/crypto/x509/verify_test.go
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
5 files changed, 1,453 insertions(+), 23 deletions(-)
To view, visit change 238362. To unsubscribe, or for help writing mail filters, visit settings.
Luiz Angelo Daros de Luca (Gerrit)
Attention is currently required from: Filippo Valsorda.
2 comments:
Patchset:
Hello, […]
What do I need to fix to get this merged (or someone provides a similar feature). It is not funny to maintain out-of-tree patches.
Patchset:
What do I need to fix to get this merged (or someone provides a similar feature). It is not funny to maintain out-of-tree patches.
To view, visit change 238362. To unsubscribe, or for help writing mail filters, visit settings.
t hepudds (Gerrit)
Attention is currently required from: Filippo Valsorda, Luiz Angelo Daros de Luca.
1 comment:
Patchset:
What do I need to fix to get this merged (or someone provides a similar feature). […]
Hi Luiz, I can't comment on whether or not your changes are desirable (I'm just a random gopher from the broader community), but a few quick comments that might help:
1. In general, it's easier for a reviewer to review a change if the commit message contains more details than just "Fixes #nnnn". As the [Contribution Guide](https://go.dev/doc/contribute#main_content) suggests, the commit description should provide context and rationale for the change and explain what it does.
2. In this particular case, issue #15196 is somewhat old and involved, including it is long enough that GitHub has started to hide some of the comments by default until they are manually expanded. It would probably be worthwhile to at least briefly summarize or touch some of the history in the commit message for this CL. (Maybe as short as -- "Adam Langley added support for XXXX in CL YYYY, but left ZZZZ as something for the future, which this CL now attempts to address.").
3. The #15196 issue is still listed as "NeedsDecision", so it might help to get a decision on the GitHub issue. (As the Contribution Guide also says -- "Consensus should have been reached on the tracker before proceeding... Gerrit reviews do not discuss the merit of the change, just its implementation").
4. Finally, in case this helps -- the commit title and commit message body come from the GitHub PR title and description, and must be edited in the GitHub web interface (not via git). For instructions, see [here](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message).
To view, visit change 238362. To unsubscribe, or for help writing mail filters, visit settings.
Luiz Angelo Daros de Luca (Gerrit)
Attention is currently required from: Filippo Valsorda, t hepudds.
1 comment:
Patchset:
Hi Luiz, I can't comment on whether or not your changes are desirable (I'm just a random gopher from the broader community), but a few quick comments that might help:
1. In general, it's easier for a reviewer to review a change if the commit message contains more details than just "Fixes #nnnn". As the [Contribution Guide](https://go.dev/doc/contribute#main_content) suggests, the commit description should provide context and rationale for the change and explain what it does.
There isn't too much to add. It implements the MANDATORY directoryName constraints according to https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.10 . Why? Because it breaks validation when the certificate uses that feature. Because it is MANDATORY to any X509 library.
2. In this particular case, issue #15196 is somewhat old and involved, including it is long enough that GitHub has started to hide some of the comments by default until they are manually expanded. It would probably be worthwhile to at least briefly summarize or touch some of the history in the commit message for this CL. (Maybe as short as -- "Adam Langley added support for XXXX in CL YYYY, but left ZZZZ as something for the future, which this CL now attempts to address.").
I'll add some comments about Adam Langley patch.
3. The #15196 issue is still listed as "NeedsDecision", so it might help to get a decision on the GitHub issue. (As the Contribution Guide also says -- "Consensus should have been reached on the tracker before proceeding... Gerrit reviews do not discuss the merit of the change, just its implementation").
From this comment https://github.com/smallstep/cli/issues/961#issuecomment-1593498226, it might indicate that somewhere the merit was already considered.
4. Finally, in case this helps -- the commit title and commit message body come from the GitHub PR title and description, and must be edited in the GitHub web interface (not via git). For instructions, see [here](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message).
I'll update both the commit message and the github text.
To view, visit change 238362. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Filippo Valsorda, t hepudds.
Gerrit Bot uploaded patch set #11 to this change.
crypto/x509: add directory name constraints
Adam Langley implemented the optional part of name constraints
(9e76ce70701ceef8fbccfb953b33a2ae7fe0367c) left the directory name
validation, which is a mandatory part of RFC5280, section 4.2.1.10.
Fixes #15196
Change-Id: Ie8d0d2a7bd865c0cf7b5ac2296e09d8248945d7e
GitHub-Last-Rev: d8148096c4a5ab2f49ceeb4aeba58c0a0ca2e651
GitHub-Pull-Request: golang/go#39639
---
M src/crypto/x509/parser.go
M src/crypto/x509/verify.go
M src/crypto/x509/verify_test.go
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
5 files changed, 1,453 insertions(+), 23 deletions(-)
To view, visit change 238362. To unsubscribe, or for help writing mail filters, visit settings.