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.

Sign up for GitHub

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

Jump to bottom

Cleanup deprecations and fix client/server usage #49

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Cleanup deprecations and fix client/server usage #49

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
51b30a1
Use almalinux 8 for EL8 testing
ehelms Aug 27, 2024
3fc6f2b
Remove deprecated nsComment from OpenSSL config
ehelms Aug 27, 2024
d56893c
Drop use of deprecated nsCertType
ehelms Aug 27, 2024
6003568
Align extendedKeyUsage to the intended type for server and client
ehelms Aug 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,24 @@ jobs:
fail-fast: false
matrix:
centos:
- stream8
- stream9
container:
image: quay.io/centos/centos:${{ matrix.centos }}
steps:
- uses: actions/checkout@v2
- name: Run tests
run: ./test.sh

almalinux:
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
almalinux:
- 8
container:
image: almalinux:${{ matrix.almalinux }}
steps:
- uses: actions/checkout@v2
- name: Run tests
run: ./test.sh
1 change: 0 additions & 1 deletion katello-certs-sign
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,6 @@ emailAddress = optional
[ usr_cert ]
basicConstraints = CA:false
extendedKeyUsage = serverAuth,clientAuth
nsCertType = server
keyUsage = digitalSignature, keyEncipherment

# PKIX recommendations harmless if included in all certificates.
Expand Down
20 changes: 4 additions & 16 deletions katello_certs_tools/sslToolConfig.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,29 +369,20 @@ def figureDEFS_distinguishing(options):
basicConstraints = CA:true
keyUsage = digitalSignature, keyEncipherment, keyCertSign, cRLSign
extendedKeyUsage = serverAuth, clientAuth
nsCertType = server, sslCA
# PKIX recommendations harmless if included in all certificates.
nsComment = "Katello SSL Tool Generated Certificate"
subjectKeyIdentifier = hash
authorityKeyIdentifier = keyid, issuer:always

[ req_server_x509_extensions ]
basicConstraints = CA:false
keyUsage = digitalSignature, keyEncipherment
extendedKeyUsage = serverAuth, clientAuth
nsCertType = server
# PKIX recommendations harmless if included in all certificates.
nsComment = "Katello SSL Tool Generated Certificate"
extendedKeyUsage = serverAuth
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

subjectKeyIdentifier = hash
authorityKeyIdentifier = keyid, issuer:always

[ req_client_x509_extensions ]
basicConstraints = CA:false
keyUsage = digitalSignature, keyEncipherment
extendedKeyUsage = serverAuth, clientAuth
nsCertType = client
# PKIX recommendations harmless if included in all certificates.
nsComment = "Katello SSL Tool Generated Certificate"
extendedKeyUsage = clientAuth
subjectKeyIdentifier = hash
authorityKeyIdentifier = keyid, issuer:always
#===========================================================================
Expand All @@ -416,10 +407,7 @@ def figureDEFS_distinguishing(options):
[ req_server_x509_extensions ]
basicConstraints = CA:false
keyUsage = digitalSignature, keyEncipherment
extendedKeyUsage = serverAuth, clientAuth
nsCertType = %s
# PKIX recommendations harmless if included in all certificates.
nsComment = "Katello SSL Tool Generated Certificate, got it?"
extendedKeyUsage = serverAuth
subjectKeyIdentifier = hash
authorityKeyIdentifier = keyid, issuer:always

Expand Down Expand Up @@ -717,7 +705,7 @@ def save(self, d, caYN=0, verbosity=0):
)
else:
openssl_cnf = CONF_TEMPLATE_SERVER \
% (gen_req_distinguished_name(rdn), d['--purpose'], gen_req_alt_names(d, rdn['CN']))
% (gen_req_distinguished_name(rdn), gen_req_alt_names(d, rdn['CN']))

try:
rotated = rotateFile(filepath=self.filename, verbosity=verbosity)
Expand Down
2 changes: 1 addition & 1 deletion test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ PYTHON=python3

if [[ -f /etc/redhat-release ]]; then
. /etc/os-release
if [[ $VERSION_ID == 8 ]] ; then
if [[ $VERSION_ID == "8.10" ]] ; then
REPOS="--enablerepo=powertools"
else
REPOS=""
Expand Down
Loading