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

CA hardening and status URI change support #105

Merged
merged 10 commits into from
Apr 25, 2017

Conversation

ypid
Copy link
Member

@ypid ypid commented Apr 17, 2017

Not sure yet if item.domain is a sensible name constraint default. Certainly restrictive but for most of my usecases, this works. Some feedback on this would be required before merging.

Closes: #87

ypid added 4 commits April 17, 2017 14:38
Tested:

```
/tmp/simple-https-server.py --certfile default.crt --keyfile default.key --hostname vz.example.com
Serving at https://vz.example.com/
```

```
curl https://vz.example.com --cacert CA.crt
curl: (60) SSL certificate problem: permitted subtree violation
More details here: http://curl.haxx.se/docs/sslcerts.html

curl performs SSL certificate verification by default, using a "bundle"
 of Certificate Authority (CA) public keys (CA certs). If the default
 bundle file isn't adequate, you can specify an alternate file
 using the --cacert option.
If this HTTPS server uses a certificate signed by a CA represented in
 the bundle, the certificate verification probably failed due to a
 problem with the certificate (it might be expired, or the name might
 not match the domain name in the URL).
If you'd like to turn off curl's verification of the certificate, use
 the -k (or --insecure) option.
```

Further refs:

* https://security.stackexchange.com/questions/31376/can-i-restrict-a-certification-authority-to-signing-certain-domains-only/130674#130674
* https://gist.github.com/JonathonReinhart/f26365364918b44d82bbd6b90269fbd6
Copy link
Member

@drybjed drybjed left a comment

Choose a reason for hiding this comment

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

Sounds good, however a brief description of the new parameters in the documentation could be nice. :-)

I'm not sure if nameConstraints field in the CA is used during certificate creation. It apparently should be done by openssl 1.0.0+ but I haven't tested it. And it only is supposed to matter during certificate verification and not signing? There's also this analysis which concludes that the nameConstraints field doesn't really matter at all.

Did you test this functionality and wether you were able to get a certificate with a not allowed domain by DebOps internal CA? Perhaps this should be implemented in a way that the script gets the list of allowed domains from the CA certificate, does the check itself against the request and then issues a certificate, ie. an application test vs openssl library test?

if [ -z "${config_ca_type}" ] || [ "${config_ca_type}" = "root" ] ; then
cat << EOF >> "${config_file}"
basicConstraints = critical, CA:TRUE
keyUsage = critical, keyCertSign, cRLSign
subjectKeyIdentifier = hash
$name_constraints
Copy link
Member

Choose a reason for hiding this comment

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

Should this and similar instances be written as ${name_constraints}?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, ${var} is generally preferred. Updated.

@ypid
Copy link
Member Author

ypid commented Apr 24, 2017

There's also this analysis which concludes that the nameConstraints field doesn't really matter at all.

That seems to be from 2012. Mozilla has the Web PKI x509 certificate primer recommending its use last updated 2015. This is not a too commonly used extension but it’s support should be mature and it is recommended to set it to critical which I did now. https://www.sysadmins.lv/blog-en/x509-name-constraints-certificate-extension-all-you-should-know.aspx

Yup, tested it, refer to the commit message:
cb8ed64

And it only is supposed to matter during certificate verification and not signing?

I have not researched how this is handled by OpenSSL further. My understanding is that it is mainly checked during certificate verification by clients but I also get "lookup:permitted subtree violation" errors on Ansible runs where I issued certs which are outside of nameConstraints for testing. So, it appears debops.pki might still issue such certs, but that is not really relevant here. The extesion is set to critical so it MUST be checked during cert verification. It could be enforced during singing as well of course (or checked if openssl can do this) but that is not really necessary (in contrast to #106).

Copy link
Member

@drybjed drybjed left a comment

Choose a reason for hiding this comment

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

Looks like the new changes work OK on Travis. I'll merge this PR and let's see if some issues show up before a new release.

@@ -230,3 +230,34 @@ respectively:
- 'uri:https://{{ ansible_domain }}/'
- 'dns:*.{{ ansible_domain }}'
- 'dns:{{ ansible_domain }}'

pki_authorities
---------------
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed that there was/is no documentation for pki_authorities at all. My bad, thanks for starting it. It will need to be expanded upon later.

@drybjed drybjed merged commit afa9b3c into debops:master Apr 25, 2017
@drybjed
Copy link
Member

drybjed commented Apr 25, 2017

Just as a follow up, this article has a paragraph:

If a client encounters something in a certificate that is marked critical, but the client does not know how to understand what it is seeing, then the client must drop the connection with an error. If a client encounters The HARICA’s CA, but the client does not understand the Name Constraints section, that’s OK with the client, because that section is not marked critical.

Since by default, debops.pki will add nameConstratints as critical, I wonder how many clients might not work properly... Although the article is from 2014, so things might have changed in the meantime. Perhaps dropping the critical status by default and making it optional could be benefical as making the role and its environment easier to use for new users? @ypid, what do you think?

@ypid
Copy link
Member Author

ypid commented Apr 25, 2017

I changed it to critical as I think that is the proper default. I also found reports/issues from years ago and but as it’s seems to be recommended to set critical for it now I went with it. I would just see if something actually breaks. And I guess if it does, the client software might not be up-to-date (so it should be updated).

Thanks for merging.

@drybjed
Copy link
Member

drybjed commented Apr 25, 2017

OK, let's leave it as is then, for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants