Setup SSL/TLS client auth without overly broad trusts for client certificates #1680
Conversation
|
commented on ticket |
|
I don't understand why the test is failing. Is it inheriting the defaults from the first test listed? How would i set it up to have a brand new setup to run into? |
|
@epackorigan extra space happens to the best of us. I'd be happy to talk to you about running acceptance tests but maybe email or chat would be a better forum for that. |
| @@ -19,23 +30,12 @@ | |||
| <%- if @ssl_crl -%> | |||
| SSLCARevocationFile "<%= @ssl_crl %>" | |||
| <%- end -%> | |||
| <%- if @ssl_verify_depth -%> | |||
| SSLVerifyDepth <%= @ssl_verify_depth %> | |||
There was a problem hiding this comment.
what was the purpose of changing the order of these directives?
There was a problem hiding this comment.
I was trying to regroup the directives that have no effect when client certs are not being looked at/needed... I just moved them all around into one block to make it easier to read (and missed that one the first time around).
There was a problem hiding this comment.
it's not as clear as i'd like from the diff...
the set of directives that i moved in here are only effective is "ssl_verify_client" on line 19 is set. That "if" statement ends on line 39. That means that all the directives that are "ssl client auth specific" are only included in the config when they are actually needed/used.
This might be out of scope from the pull request perspective, but will make it clear that those are only to be used when the setup enforce verification of client certificates for authentication.
| ssl_verify_client => 'test', | ||
| } | ||
| EOS | ||
| apply_manifest(pp, :catch_failures => true) |
There was a problem hiding this comment.
in our acceptance tests, we use a pattern to ensure idempotency that applies the manifest twice; the second time ensures nothing has changed.
apply_manifest(pp, :catch_failures => true)
apply_manifest(pp, :catch_changes => true)
There was a problem hiding this comment.
There's even a shared_examples "a idempotent resource" for this in the spec helper so you can simply use it_behaves_like "a idempotent resource" here instead.
There was a problem hiding this comment.
Added the idempotency tests to all 4 tests modified.
|
@epackorigan , please remove the lines you added to the change log. We definitely appreciate you including them but those will get taken care of when we release this patch and will go under different headings. |
If $ssl_certs_dir defaults to a location that contains certificates, apache will trust implicitly all the certificates presented by the client for auth that were issues by *any* certificates in that location. Note: * In Gentoo, $ssl_certs_dir was pointing to /etc/apache2/ssl, which may have been safe, or used for storing CA certs to check client auth. Gentoo users may need to set that value explicitly after upgrade. fix extra space Only include directives when needed. Do not include directives that have no effect. SSLCA* directives are only meaningful when 'ssl_verify_client' is set (when you want to handle client cert authentication, and how to verify those certs being presented to the server by the client). missed one of the directives for client auth missed one directive that doesn't need to be included unless you are doing ssl client authentication. Update README.md test for ssl_certs_dir and ssl_ca There should probably be tests for each variation (`ssl_verify_client` + any of the client authentication directives. Updated to simpler/clearer tests hopefully this will make more sense. Run tests on different files. not quite sure how that works. rename vhost for each additional test rename the apache::vhost for each test. (maint) remove extra space on line 146 adding idempotent verifications to each test. making sure that the resources are idempotent. changes reflected in the changelog mention in the changelog * the fix for auth * the defaults being changed * the directives no longer being included unless needed. reverting changes to changelog.
eputnam
force-pushed
the
apache-broad-client-auth-fix
branch
from
03a5678
to
7bb35c2
Compare
…nt-auth-fix Setup SSL/TLS client auth without overly broad trusts for client certificates
If
$ssl_certs_dirdefaults to a location that contains certificates, apache will trust implicitly all the certificates presented by the client for auth that were issues by any certificates in that location. Setting the default to the location where all root certificates is stored enabled anyone with a key and cert from any of the SSL certificate provider to connect.Note:
$ssl_certs_dirwas pointing to /etc/apache2/ssl, which may have been safe, or used for storing CA certs to check client auth. Gentoo users may need to set that value explicitly depending on how it was used in prior versions.For more details, see [MODULE-5471]