Skip to content

crypto/x509: add directory name constraints #39639

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

luizluca
Copy link

@luizluca luizluca commented Jun 17, 2020

Adam Langley implemented the optional part of name constraints
(9e76ce7) left the directory name
validation, which is a mandatory part of RFC5280, section 4.2.1.10.

Fixes #15196

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no Used by googlebot to label PRs as having an invalid CLA. The text of this label should not change. label Jun 17, 2020
@luizluca
Copy link
Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. and removed cla: no Used by googlebot to label PRs as having an invalid CLA. The text of this label should not change. labels Jun 17, 2020
@gopherbot
Copy link
Contributor

This PR (HEAD: 4f8fbd3) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/238362 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 1:

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.


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Paul van Brouwershaven:

Patch Set 1:

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.


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Paul van Brouwershaven:

Patch Set 1:

Sorry, included the wrong Censys search query, this one includes the permitted ones which make more sense.

https://censys.io/certificates?q=parsed.extensions.name_constraints.excluded_directory_names%3A*+OR+parsed.extensions.name_constraints.permitted_directory_names%3A*


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Luiz Angelo Daros de Luca:

Patch Set 1:

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.


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 70bf484) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/238362 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Luiz Angelo Daros de Luca:

Patch Set 2:

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


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 7285b7b) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/238362 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Luiz Angelo Daros de Luca:

Patch Set 2:

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.


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Luiz Angelo Daros de Luca:

Patch Set 3:

I would like to update commit message (just removing extra commends)


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Paul van Brouwershaven:

Patch Set 3:

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?


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from DO NOT USE:

Patch Set 3:

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.


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Luiz Angelo Daros de Luca:

Patch Set 3:

You can help that discussion by explaining there what you need it for.

You mean to discuss here?
#15196 (comment)

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.

Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Paul van Brouwershaven:

Patch Set 3:

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).


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: e44eb9f) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/238362 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Luiz Angelo Daros de Luca:

Patch Set 3:

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.


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@luizluca luizluca changed the title crypto/x509: add directory name constraints (WIP) crypto/x509: add directory name constraints Jul 30, 2020
@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 1:

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.


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: a3b93f5) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/238362 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@luizluca
Copy link
Author

Rebasing and conflict fix

@luizluca
Copy link
Author

@heschi , could you please tell me why this was closed?

@ianlancetaylor
Copy link
Contributor

It was due to #50197. Reopening.

@luizluca
Copy link
Author

It was due to #50197. Reopening.

Thanks!

@gopherbot
Copy link
Contributor

This PR (HEAD: 56134a2) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/238362 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

This PR (HEAD: 6ba02dd) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/238362 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 1:

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.


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Paul van Brouwershaven:

Patch Set 1:

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.


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Paul van Brouwershaven:

Patch Set 1:

Sorry, included the wrong Censys search query, this one includes the permitted ones which make more sense.

https://censys.io/certificates?q=parsed.extensions.name_constraints.excluded_directory_names%3A*+OR+parsed.extensions.name_constraints.permitted_directory_names%3A*


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Luiz Angelo Daros de Luca:

Patch Set 1:

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.


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Luiz Angelo Daros de Luca:

Patch Set 2:

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


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Luiz Angelo Daros de Luca:

Patch Set 2:

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.


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Luiz Angelo Daros de Luca:

Patch Set 3:

I would like to update commit message (just removing extra commends)


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Paul van Brouwershaven:

Patch Set 3:

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?


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Filippo Valsorda:

Patch Set 3:

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.


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Luiz Angelo Daros de Luca:

Patch Set 3:

You can help that discussion by explaining there what you need it for.

You mean to discuss here?
#15196 (comment)

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.

Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Paul van Brouwershaven:

Patch Set 3:

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).


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Luiz Angelo Daros de Luca:

Patch Set 3:

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.


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Luiz Angelo Daros de Luca:

Patch Set 7:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Luiz Angelo Daros de Luca:

Patch Set 10:

(2 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from t hepudds:

Patch Set 10:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

Adam Langley implemented the optional part of name constraints
(9e76ce7) left the directory name
validation, which is a mandatory part of RFC5280, section 4.2.1.10.

Fixes golang#15196
@luizluca
Copy link
Author

@agl , this complements the name constraint you did before.

@gopherbot
Copy link
Contributor

This PR (HEAD: d814809) has been imported to Gerrit for code review.

Please visit Gerrit at https://go-review.googlesource.com/c/go/+/238362.

Important tips:

  • Don't comment on this PR. All discussion takes place in Gerrit.
  • You need a Gmail or other Google account to register for Gerrit.
  • To change your code in response to feedback:
    • Push a new commit to the branch used by your GitHub PR.
    • A new "patch set" will then appear in Gerrit.
    • Respond to each comment by marking as Done in Gerrit if implemented as suggested. You can alternatively write a reply.
    • Critical: you must click the blue Reply button near the top to publish your Gerrit responses.
    • Multiple commits in the PR will be squashed by GerritBot.
  • The title and description of the GitHub PR are used to construct the final commit message.
    • Edit these as needed via the GitHub web interface (not via Gerrit or git).
    • You should word wrap the PR description at ~76 characters unless you need longer lines (e.g., for tables or URLs).
  • See the Sending a change via GitHub and Reviews sections of the Contribution Guide as well as the FAQ for details.

@gopherbot
Copy link
Contributor

Message from Luiz Angelo Daros de Luca:

Patch Set 10:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/238362.
After addressing review feedback, remember to publish your drafts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crypto/x509: support DirectoryName name constraints
5 participants