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

WordPress Setup: Add Nginx ssl_client_certificate #869

Merged
merged 1 commit into from
Sep 10, 2017

Conversation

tangrufus
Copy link
Collaborator

@tangrufus tangrufus commented Aug 16, 2017

Usage:

wordpress_sites:
  example.com:
    site_hosts:
      - canonical: example.com
    ssl:
      enabled: true
      provider: self-signed
      client_cert_url: 'https://support.cloudflare.com/hc/en-us/article_attachments/201243967/origin-pull-ca.pem'

See:

@swalkinshaw
Copy link
Member

Seems fairly simple and it's a generic Nginx setting.

My only concern is the downloading with force being set.

Ansible docs say

Generally should be yes only for small local files.

Not exactly sure what the alternative is though or how much it matters. Obviously Trellis "downloads" many things during a provision but this won't be idempotent.

@fullyint
Copy link
Contributor

Idempotent reporting. I was delightfully surprised that even with force, the get_url task reporting shows ok if the file is unchanged, versus always showing changed. I believe that to force download of the temp file every time enables calculating and comparing checksums.

Copy module more flexible? Could users want a ssl_client_certificate that is not accessible via URL? Using the copy module may accommodate more scenarios. We could let users specify a local path to their own origin-pull-ca.pem they've previously downloaded to their machine, as is done with the provider: manual certs and keys. Using the copy module would avoid downloading every time and the related potential for playbook failure due to network connectivity issues.

Capturing updates via partial hash in dest filename. If you stick with get_url, I assume the intent in using force is to detect and incorporate changes in the content at the URL. I think we could still capture updates even when force: no by appending to the dest filename a few characters from a hash of the URL. Ansible would detect that dest does not exist and proceed to get_url.

This assumes that the update would be at a new URL, NOT the same URL. The source URL doesn't strike me as official/canonical being in Cloudflare's support article_attachments: https://support.cloudflare.com/hc/en-us/article_attachments/201243967/origin-pull-ca.pem. I couldn't find any other URL. I suspect any updates to origin-pull-ca.pem could likely appear at a new URL.

@tangrufus
Copy link
Collaborator Author

Updated:

  • Using md5 hash as file name
  • Not using force anymore, only download at the first provision
  • Using ssl_client_cert_url as global default

Usage 1:

wordpress_sites:
  example.com:
    ssl:
      enabled: true
      client_cert_url: 'https://example.com/origin-pull-ca.pem'

Usage 2:

ssl_client_cert_url: 'https://another-example.com/origin-pull-ca.pem'

wordpress_sites:
  example.com:
    ssl:
      enabled: true

Usage 3:

ssl_client_cert_url: 'https://another-example.com/origin-pull-ca.pem'

wordpress_sites:
  example.com:
    ssl:
      enabled: true
      client_cert_url: false

Usage 4:

ssl_client_cert_url: 'https://another-example.com/origin-pull-ca.pem'

wordpress_sites:
  example.com:
    ssl:
      enabled: true
      client_cert_url: 'https://example.com/origin-pull-ca.pem'

About using ansible copy module: maybe another pull request for another time

Thanks!

Copy link
Member

@swalkinshaw swalkinshaw left a comment

Choose a reason for hiding this comment

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

Is it even worth having the global option? Wondering if we should only allow the per site one and just let people define the same one if need be.

@@ -75,6 +75,15 @@ server {

add_header Strict-Transport-Security "max-age={{ [hsts_max_age, hsts_include_subdomains, hsts_preload] | reject('none') | join('; ') }}";

{% if item.value.ssl.client_cert_url is defined and item.value.ssl.client_cert_url -%}
Copy link
Member

Choose a reason for hiding this comment

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

Is this supposed to be ssl_client_cert_url in the first one? Just confused why it's checking the same variable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ssl_client_cert_url was the global option, removed now.

Copy link
Contributor

Choose a reason for hiding this comment

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

With the global option removed, I presume users would no longer disable client verification by setting client_cert_url: false. Rather, they simply wouldn't define client_cert_url for the relevant site.

If we don't need to check the bool interpretation of client_cert_url, could we make this change?

- {% if item.value.ssl.client_cert_url is defined and item.value.ssl.client_cert_url -%}
+ {% if item.value.ssl.client_cert_url is defined -%}

@@ -75,6 +75,15 @@ server {

add_header Strict-Transport-Security "max-age={{ [hsts_max_age, hsts_include_subdomains, hsts_preload] | reject('none') | join('; ') }}";

{% if item.value.ssl.client_cert_url is defined and item.value.ssl.client_cert_url -%}
ssl_client_certificate {{ nginx_ssl_path }}/{{ item.value.ssl.client_cert_url | hash('md5') }};
Copy link
Member

Choose a reason for hiding this comment

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

There's two spaces before { 👾

ssl_verify_client on;

{% elif ssl_client_cert_url is defined and not item.value.ssl.client_cert_url is defined -%}
ssl_client_certificate {{ nginx_ssl_path }}/{{ ssl_client_cert_url | hash('md5') }};
Copy link
Member

Choose a reason for hiding this comment

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

ditto here too

@tangrufus
Copy link
Collaborator Author

Removed the global option.
2 spaces before { because of alignment:
trellis-nginx-ssl

@tangrufus
Copy link
Collaborator Author

Will this be consider to be merged, or should I extract it to be a galaxy role?

Thanks!

Copy link
Contributor

@fullyint fullyint left a comment

Choose a reason for hiding this comment

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

@tangrufus Thanks for your great work on this! I think it's ready to go if you're willing to make a few minor changes.

- include: nginx-client-cert.yml
tags: wordpress-setup-nginx-client-cert
notify: reload nginx

Copy link
Contributor

@fullyint fullyint Sep 10, 2017

Choose a reason for hiding this comment

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

Two minor requests:

  • Could you move this include task right after the other similar include tasks in tasks/main.yml? That placement strikes me as more consistent. I like the idea of avoiding nested includes when it's easy.
  • Let's omit the notify here. Nginx only needs to reload if the conf file produced by our wordpress-site.conf.j2 changes. A change in the "Download client cert" task produces a new or renamed client cert, which in turn will trigger a changed template task which already has notify: reload nginx.

- name: Download client cert
get_url:
url: "{{ item.value.ssl.client_cert_url }}"
dest: "{{ nginx_ssl_path }}/{{ item.value.ssl.client_cert_url | hash('md5') }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a user who happens upon the server's nginx_ssl_path contents would find the filename more intuitive as client-4c1d4cc.crt instead of 4c1d4ccad291c1ac1f230ad263c4196c.

Could we make this change?

- dest: "{{ nginx_ssl_path }}/{{ item.value.ssl.client_cert_url | hash('md5') }}"
+ dest: "{{ nginx_ssl_path }}/client-{{ (item.value.ssl.client_cert_url | hash('md5'))[:7] }}.crt"

A similar filename change would be needed in wordpress-site.conf.j2.

@@ -75,6 +75,15 @@ server {

add_header Strict-Transport-Security "max-age={{ [hsts_max_age, hsts_include_subdomains, hsts_preload] | reject('none') | join('; ') }}";

{% if item.value.ssl.client_cert_url is defined and item.value.ssl.client_cert_url -%}
Copy link
Contributor

Choose a reason for hiding this comment

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

With the global option removed, I presume users would no longer disable client verification by setting client_cert_url: false. Rather, they simply wouldn't define client_cert_url for the relevant site.

If we don't need to check the bool interpretation of client_cert_url, could we make this change?

- {% if item.value.ssl.client_cert_url is defined and item.value.ssl.client_cert_url -%}
+ {% if item.value.ssl.client_cert_url is defined -%}

@tangrufus
Copy link
Collaborator Author

All set. Thanks!

@fullyint fullyint merged commit 8b3bc5a into roots:master Sep 10, 2017
@fullyint
Copy link
Contributor

Wonderful! Thank you @tangrufus!

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