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 self signed certificates subdomains #812

Merged

Conversation

medfreeman
Copy link
Contributor

@medfreeman medfreeman commented Mar 30, 2017

closes #809

Add support for all alternate hosts and subdomains in self-signed certificates
The subjectAltName certificate field now contains the DNS entries of all canonical and redirect hosts of a site, along with all wildcard domain names (for a site with multisite and subdomains enabled).

Add multisite_subdomains_wildcards helper variable
Replace server_name expression in wordpress sites template by multisite_subdomains_wildcards helper variable for clearer code

Copy link
Contributor Author

@medfreeman medfreeman left a comment

Choose a reason for hiding this comment

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

Quick testing procedure:

vagrant up
vagrant ssh

[vagrant@www]

sudo rm -f /etc/nginx/ssl/example.com.*
exit
ansible-playbook dev.yml -i .vagrant/provisioners/ansible/inventory/vagrant_ansible_inventory --tags wordpress-setup-self-signed-certificate

ℹ️ i got ERROR! Unexpected Exception: 'Host' object has no attribute 'remove_group' while executing this command, do you have an idea why ?

ℹ️ In my repo, the hosts/development inventory file is a symlink to vagrant inventory file, and the command works properly without the -i flag

Adding the certificate to the os trust store (tested with chrome):

openssl s_client -connect example.dev:443 < /dev/null > example.raw
openssl x509 -inform PEM -in example.raw -text -out example.dev.crt
rm -f example.raw

Ubuntu 16.04

sudo apt-get -y install libnss3-tools
certutil -d sql:$HOME/.pki/nssdb -A -t "P,," -n example.dev.crt -i example.dev.crt
rm -f example.dev.crt

ℹ️ Restart the browser or reload the site in a new tab, the security warning should have disappeared

MacOSX

sudo security add-trusted-cert -d -r trustRoot -k $HOME/Library/Keychains/login.keychain example.dev.crt
rm -f example.dev.crt

openssl req -subj "/CN={{ item.value.site_hosts[0].canonical }}" -new
-newkey rsa:2048 -days 3650 -nodes -x509 -sha256
-keyout {{ item.key }}.key -out {{ item.key }}.cert
shell: "openssl req -subj \"/CN={{ item.value.site_hosts[0].canonical }}\" -new \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change of command style was mandatory for a few reasons, first one is that the brackets characters (l.7-10) have to be escaped or ansible throws a syntax error.

The original Block scalar style using a > removes the newlines within the string, thus not allowing the config openssl section to work properly.

The choice of the Flow scalar style using a " (double-quote) is the only style in yaml multi-line string allowing the use of inline newlines (\n) and spaceless newlines (trailing \) (c.f. http://stackoverflow.com/questions/3790454/in-yaml-how-do-i-break-a-string-over-multiple-lines#answer-21699210).

I could have also used the Flow literal style using a |, but that would have forced all the dashed parameters of the
openssl command to be written on the same line since a spaceless newline (trailing \) can't be added in this mode.

In the chosen style, the double quotes " and backslash \ have to be escaped though.

args:
executable: "/bin/bash"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bash shell has to be used, since virtual files use the subshell operator () (l.5), which produces a syntax error using the default sh shell.
Perhaps there is a way to make this work with sh, but i didn't find it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps that can work with backticks, i didn't realise it until now. I'll try this during the afternoon.

cat <<\" EOF\"\n
[req]\n
distinguished_name = dn\n
[ dn ]\n
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Providing an empty dn section is mandatory, even with the subject provided in command-line (l.3), while using a config file, or openssl throws an error.

shell: "openssl req -subj \"/CN={{ item.value.site_hosts[0].canonical }}\" -new \
-newkey rsa:2048 -days 3650 -nodes -x509 -sha256 \
-extensions req_ext -config <( \
cat <<\" EOF\"\n
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ansible issues ansible/ansible#12856 and ansible/ansible#8512 prevent the use of EOF or -EOF as file delimiter, since a space character is inserted by ansible at the start of each line while parsing the shell string.
If found the solution here in the second issue.

I don't know if you have a better understanding of the problem than me, but i couldn't find a better solution here or elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the cmd shell parameter instead of providing the command directly to the shell module yields the same error while using EOF instead of \" EOF\".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-keyout {{ item.key }}.key -out {{ item.key }}.cert
shell: "openssl req -subj \"/CN={{ item.value.site_hosts[0].canonical }}\" -new \
-newkey rsa:2048 -days 3650 -nodes -x509 -sha256 \
-extensions req_ext -config <( \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The extensions parameter is mandatory to have openssl x509 key generation take into account the subjectAltName parameter (it references the req_ext section l.10).

@swalkinshaw
Copy link
Member

@medfreeman can you take a look at #818 as well? Solves some (all?) of the same problem.

The method of using an Openssl config might be a much nicer option than the shell command.

@medfreeman
Copy link
Contributor Author

medfreeman commented Apr 8, 2017

@swalkinshaw sure.

While #818 can solve the syntax problem, it only adds a second task for each site to achieve the same result (config file vs command-line virtual config via here-now document, since subjectAltName cannot directly passed as a command-line argument).

As is this pr won't also work with multiple site keys, because the last site will have overwritten previous openssl configurations.

To follow this path, two options are possible:

  • Generate a specific openssl config file for each site
    -> As stated that only duplicates the cert task and will produce additional files, is this really warranted ? Moreover the original config works already, and i don't like duplicating a common config with args that can be passed in the command-line

  • Generate a single openssl config for all sites in one go
    -> I personally prefer having them separate for consistency, and keeps working as-is with one per-site (and subdomains) certificate pair

I personaly am in favor of writing a simpler syntax for my PR, but perhaps it is because i have written it.
Please tell me what you think.

@medfreeman medfreeman force-pushed the add-self-signed-certificates-subdomains branch from 8032299 to 15bae9c Compare April 8, 2017 10:16
@medfreeman
Copy link
Contributor Author

Ok, i made the syntax cleaner by avoiding unecessary quoting, including commonName in the configuration, and using single quotes for EOF.
Imo the only thing that goes in favor of #818 is that this PR will break when ansible/ansible/issues/12034 is resolved.

@heyfletch
Copy link
Contributor

heyfletch commented May 11, 2017

I got this to work. Didn't try #818. This has become critical now that Chrome 58 will not load the local https site without Subject Alternative Name.

I had a rough go of this, so reiterating some points here for others:

  1. Start with @medfreeman's testing procedure and be sure the certs and keys are gone (those commands didn't clear them for me, so the reprovisioning was not working)
  2. The ansible-playbook provision line didn't work either, so did: vagrant reload --provision instead
  3. If sudo security add-trusted-cert on macOS doesn't work, do it manually by opening dev site, viewing certificate, dragging to desktop, dragging into keychain.

@heyfletch
Copy link
Contributor

My cheatsheet for new site with https/SSL with Subject Alternative Name on a local Mac:

  1. Incorporate this pull request's changes:
  2. Create and trust the local certificate:
openssl s_client -connect example.localdev:443 < /dev/null > example.raw
openssl x509 -inform PEM -in example.raw -text -out example.localdev.crt
sudo security add-trusted-cert -d -k /Library/Keychains/System.keychain example.localdev.crt
rm -f example.raw example.localdev.crt

Haven't tried this on multisite.

Curious, anything else we need to do to merge this? Any way I can help?

@fullyint
Copy link
Contributor

I'm only a novice on the topic but tried to thoroughly review. @medfreeman I really appreciate your exceptionally thorough research and explanation. This works in my tests and I don't have any requested changes.

heredoc vs. config file. I slightly prefer the here document approach here in #812 over the templated openssl config file approach in #818. I find @medfreeman's concern about #818's approach to be convincing (especially for projects with 10–20+ sites):

[by creating openssl config files, #818] duplicates the cert task and will produce additional files... and i don't like duplicating a common config with args that can be passed in the command-line --ref

extensions. The other main difference I noticed was that #818 has additional config file sections:

  • req_extensions = v3_req
  • x509_extensions = v3_ca

As best I could tell these extensions are considered optional for webservers and probably of no additional benefit to non-CA self-signed certs aimed at local development. @scherii perhaps you can correct me if I've misunderstood.

#812 appears preferable and ready. I'm not opposed to #818 but I lean toward #812 for the reasons above and because I think #818 needs more discussion:

Huge thanks to @scherii and @medfreeman for such awesome work. Having to discuss which of two excellent PRs to pick is a great problem to have.

@fullyint fullyint merged commit 624ad4e into roots:master Jun 22, 2017
@fullyint
Copy link
Contributor

Thank you again @medfreeman @scherii @heyfletch
This is a great improvement to Trellis!

primozcigler added a commit to primozcigler/trellis that referenced this pull request Jun 26, 2017
* master: (120 commits)
  Reload Nginx with updates to manual cert or key (roots#843)
  Add self signed certificates subdomains (roots#812)
  Add support for SMTP without authentication
  Remove output about running WP-CLI and Composer
  Option to install WP-CLI packages (roots#837)
  Update WP theme paths only when template_root in releases_path (roots#840)
  Update wp-cli to 1.2.1 (roots#838)
  skip plugins with WP-CLI, to avoid crappy plugins breaking deploys
  Add options for sources and version
  Add re-run vagrant up message
  Remove plugins from README
  Auto-install Vagrant plugins
  Fixes
  Add Vagrant config support
  Use python slice on password salt instead of Jinja2 truncate()
  Avoid single var containing jinja delimiters in when parameter
  Remove jinja delimiters from tasks' when parameter
  Update connection test for new dense.py callback in Ansible 2.3
  Deploys: Make wp core update-db optional (roots#827)
  Use a block
  ...

# Conflicts:
#	Vagrantfile
#	ansible.cfg
@medfreeman medfreeman deleted the add-self-signed-certificates-subdomains branch August 4, 2017 08:31
skemantix pushed a commit to skemantix/trellis that referenced this pull request Sep 4, 2017
* Add support for all alternate hosts and subdomains in self-signed certificates
* Replace server_name expression by new `multisite_subdomains_wildcards` helper variable
* Use `nginx_ssl_path` helper variable
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.

Add subdomains and all canonical names to self-signed certificates
4 participants