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

Add name constraints to CA #14206

Merged

Conversation

franziskuskiefer
Copy link
Contributor

@franziskuskiefer franziskuskiefer commented Nov 23, 2018

Something like this should fix issue #11075 (fixes #11075).
I don't know how to run this so I can't tell for sure. But adding something of the form nameConstraints=permitted;DNS:.example.com,permitted;DNS:.example.org will give you what you want.

@annevk
Copy link
Member

annevk commented Nov 23, 2018

Thanks @franziskuskiefer!

@jgraham @gsnedders @jugglinmike would you be willing to iterate on this further?

@foolip foolip assigned gsnedders and unassigned jugglinmike Nov 23, 2018
@foolip
Copy link
Member

foolip commented Nov 23, 2018

@gsnedders, I assigned to you since #11075 is on our list of important infra issues.

@foolip foolip mentioned this pull request Nov 23, 2018
@jugglinmike
Copy link
Contributor

We recently received a feature request to support arbitrary subdomains. We're not doing that (yet), but it might be wise to account for the possibility in this patch by relaxing the constraints a bit. As far as I know (which is not very far), it will be no less secure to constrain to:

  • web-platform.test
  • .web-platform.test
  • not-web-platform.test
  • .not-web-platform.test

Though now I'm wondering if we could simply constrain to .test.

@gsnedders
Copy link
Member

@jugglinmike

Though now I'm wondering if we could simply constrain to .test.

I'd strongly oppose this, given I (and probably others) have other local dev environments served on .test.

@jgraham
Copy link
Contributor

jgraham commented Nov 27, 2018

I don't think the arbitary subdomains thing needs to be considered here; if we change the range of supported hosts it will in any case be necessary to update the SAN line on the certs so there is barely any advantage in preempting that change for the constraints line only.

@foolip
Copy link
Member

foolip commented Dec 4, 2018

Ping @gsnedders for review.

Copy link
Member

@gsnedders gsnedders left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@foolip I actually started on this yesterday, but couldn't figure out why it wasn't working. :)

@jgraham @franziskuskiefer This PR should probably also update tools/certs, preferably also providing some documentation on how to update them!

else:
san_line = "subjectAltName = %s" % make_alt_names(hosts)
constraints_line = "nameConstraints = " + make_name_constraints(hosts)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will lead to trying to put nameConstraints on the server cert, rather than the CA cert.

From _generate_ca we call get_config with hosts=None:

  web-platform-tests/tools/wptserve/wptserve/sslutils/openssl.py(399)_generate_host_cert()
-> self._generate_ca()
 web-platform-tests/tools/wptserve/wptserve/sslutils/openssl.py(342)_generate_ca()
-> with self._config_openssl(None) as openssl:
  web-platform-tests/tools/wptserve/wptserve/sslutils/openssl.py(43)__enter__()
-> f.write(get_config(self.base_path, self.hosts, self.duration))

(interestingly pdb seems to be omitting _config_openssl in that!)

As such, this doesn't actually fix the underlying issue! I think we need to have a is_ca arg here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now puts both nameConstraints and subjectAltName in the config for both certs, which leads to having both on the CA cert and only the latter on the non-CA cert. I think this behaviour is fine?

@gsnedders
Copy link
Member

@jgraham what do I need to do to update tools/certs? I presume I also need to make it have longer validity?

@jgraham
Copy link
Contributor

jgraham commented Dec 19, 2018

Updating the certs is a bit of a black art at the moment. You need to change the wptserve config to use openssl as the config type and add a duration key to the openssl part of the config with the desired duration. Then if you run the server you can copy the generated files out of the temporary location and into the irectory containing the pregenerated certs.

@foolip
Copy link
Member

foolip commented Dec 19, 2018

This PR is blocked on the required "Travis CI - Pull Request" check after #14499. In order to trigger it, I will close and reopen this PR.

@foolip foolip closed this Dec 19, 2018
@foolip foolip reopened this Dec 19, 2018
Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subject to .gitignore change.

@@ -0,0 +1,3 @@
/0*.pem
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add this to the root .gitignore please (some tooling we have only works with the root).

@gsnedders gsnedders merged commit d742ec3 into web-platform-tests:master Dec 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is the Certificate Authority name-constrained somehow?
8 participants