Cleanup deprecations and fix client/server usage #49
Conversation
ehelms
force-pushed
the
cleanup-deprecations
branch
from
74511a9 to
589785e
Compare
ehelms
force-pushed
the
cleanup-deprecations
branch
from
589785e to
6003568
Compare
|
Includes #50 to fix test runs in CI |
| @@ -375,14 +375,14 @@ def figureDEFS_distinguishing(options): | |||
| [ req_server_x509_extensions ] | |||
| basicConstraints = CA:false | |||
| keyUsage = digitalSignature, keyEncipherment | |||
| extendedKeyUsage = serverAuth, clientAuth | |||
| extendedKeyUsage = serverAuth | |||
There was a problem hiding this comment.
I'm not sure if this is correct. Are you sure we're not mixing them? Like the Smart Proxy using the same certs for both client and serer.
There was a problem hiding this comment.
If you are asking "does Katello use them in a mixed usage way" the answer is no. We generate server and client certificates and strictly use them.
There was a problem hiding this comment.
Question is: should we use them in a mixed way? Does it hurt to have this here? Do we even check on the usage?
I'd feel more comfortable merging this just the deprecation removal for now (because that's obviously a good thing) and discuss the key usage separate (because I don't know how to think about it yet).
There was a problem hiding this comment.
That's a tough question. My reading has led me to conclude this comes down to how strict we want to be. Being strict about these usages is an implementation of the principal of least privilege. Do we want our client certificates to be able to be used for servers and vice versa, do we want our server certificates to be able to be used as client certificates?
If we said yes - then we would only need to generate one set of certificates per hostname and use it for Apache, foreman-proxy, foreman client certs, etc.
If we said no, then we could roughly stay with the structure we have now and fix this inconsistency as I have noted it here.
You can see, that we even expect this in puppet-certs: https://github.com/theforeman/puppet-certs/blob/master/manifests/foreman.pp#L29
We test for it -- https://github.com/theforeman/puppet-certs/blob/master/spec/acceptance/apache_spec.rb#L36
I ran a pipeline, implementing this and all bats tests passed so our deployment model even expects it.
I think as I understand it, you can also go one step further and mark extensions as critical in which the CA certificate will enforce the purpose. In some of my testing, even without the critical I was able to see Apache enforcing the purpose - that is, trying to use a serverAuth only certificate to perform a client action was rejected.
Here is an example of testing that:
[root@certs vagrant]# curl https://`hostname`/pulp/api/v3/users/ --cert /etc/foreman/client_cert.pem --key /etc/pki/katello/private/
certs.wareagle.example.com-foreman-proxy-client-bundle.pem katello-apache.key
[root@certs vagrant]# curl https://`hostname`/pulp/api/v3/users/ --cert /etc/pki/katello/certs/katello-apache.crt --key /etc/pki/katello/private/katello-apache.key
curl: (56) OpenSSL SSL_read: error:0A000413:SSL routines::sslv3 alert unsupported certificate, errno 0
Where each of these has a single purpose as outlined in this PR.
See https://docs.openssl.org/1.1.1/man5/x509v3_config/#deprecated-extensions for the noted deprecations being removed.
The code was also correctly setting
nsCertTypeto the right purpose, but failing to match this withextendedKeyUsage. This fixes that inconsistency.