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

Fixes #36755 - Send full certificate chain to clients #874

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

jpasqualetto
Copy link

Add option SSLExtraChainCert to the Webrick webserver, so the full certificate chain is sent to clients during the SSL handkshake.

lib/launcher.rb Outdated Show resolved Hide resolved
@ekohl
Copy link
Member

ekohl commented Sep 18, 2023

Test failures look related.

lib/launcher.rb Show resolved Hide resolved
@@ -62,6 +69,10 @@ def https_app(https_port, plugins = https_plugins)
logger.error "Unable to read #{settings.ssl_ca_file}. Are the values correct in settings.yml and do permissions allow reading?"
end

unless File.readable?(settings.foreman_ssl_ca)
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed you're suing foreman_ssl_ca but you should use ssl_ca_file. foreman_ssl_ca is used to communicate with Foreman, while falling back to ssl_ca_file. ssl_ca_file is used to serve. So this check would be redundant.

Copy link
Author

Choose a reason for hiding this comment

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

Hello @ekohl

I'm using foreman_ssl_ca intentionally. ssl_ca_file will always contain only the internal CA. When a deployment is using self-signed certs, ssl_ca_file and foreman_ssl_ca are identical and contain a certificate chain that can validate ssl_certificate

# openssl verify -CAfile ./foreman_ssl_ca.pem ssl_cert.pem 
ssl_cert.pem: OK
# openssl verify -CAfile ./ssl_ca.pem ssl_cert.pem 
ssl_cert.pem: OK

However, when a deployment uses custom certificates, then ssl_ca_file only contains internal CA while foreman_ssl_ca contains the CA bundle which forms the full certificate chain for ssl_certificate

# openssl verify -CAfile ./foreman_ssl_ca.pem ssl_cert.pem 
ssl_cert.pem: OK
# openssl verify -CAfile ./ssl_ca.pem ssl_cert.pem 
CN = satellite.jpasqualetto.local
error 20 at 0 depth lookup: unable to get local issuer certificate
error ssl_cert.pem: verification failed

So, if we add ssl_ca_file into SSLExtraChainCert this will work fine for deployments with self-signed certs but will break on deployments with custom certs.

If we add foreman_ssl_ca into SSLExtraChainCert it works for both use cases.

With all this being said, the check to see if we can read foreman_ssl_ca seems necessary, just as checking ssl_ca_file is.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, but if that's the case then that's just a bug in how the installer deploys things then. The foreman_ssl_ca is the CA used to verify the connection to Foreman. ssl_ca_file is the CA that must match the public and private key. I'm very firm in saying that it must be ssl_ca_file.

Looking at the certs we have this bit that deploys it:
https://github.com/theforeman/puppet-certs/blob/5d7679a1add25a087d8dd925c1bae11d003852b5/manifests/foreman_proxy.pp#L95-L115

If $proxy_key and $proxy_cert are signed by $server_ca_cert, then $proxy_ca_cert should also use $server_ca_cert as a source.

Now that I look closer, we already send SSLCACertificateFile so perhaps this whole PR is not needed, but the real bug is in puppet-certs.

Copy link
Member

Choose a reason for hiding this comment

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

I think theforeman/puppet-certs#413 is the actual fix for this bug.

Copy link
Author

Choose a reason for hiding this comment

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

The foreman_ssl_ca is the CA used to verify the connection to Foreman.

That's true. As the same certificate deployed on Apache (where we got o talk to foreman) is used by foreman-proxy, it also can be used to verify the connection to foreman-proxy.

ssl_ca_file is the CA that must match the public and private key. I'm very firm in saying that it must be ssl_ca_file

This is only true for self-signed certificates. For custom certificates, ssl_ca_file does not validate ssl_cert.pem and ssl_key.pem.

If ssl_ca_file is modified to include the CA bundle, which would validate ssl_cert.pem and ssl_key.pem, then we will create a problem to authenticate client connections with certificates because they are supposed to be validated against ssl_ca_file

I understand your point of view, but I just don't see a simple solution without using foreman_ssl_ca.

Maybe deploy another file to be used as SSLExtraChainCert ? It would need to be identical to foreman_ssl_ca to work.

Now that I look closer, we already send SSLCACertificateFile so perhaps this whole PR is not needed

Not sure why, but SSLCACertificateFile is not sent during the handshake, only ssl_certificate is (no chain at all). The entire chain is only sent when we define SSLExtraChainCert.

This is what a client receives (original code, without my patch):

# echo|openssl s_client -connect $(hostname -f):9090 -showcerts
CONNECTED(00000003)
depth=2 CN = Joniel Internal ROOT CA
verify return:1
depth=1 CN = Joniel Intermediate CA
verify return:1
depth=0 CN = satellite.jpasqualetto.local
verify return:1
---
Certificate chain
 0 s:CN = satellite.jpasqualetto.local
   i:CN = Joniel Intermediate CA
-----BEGIN CERTIFICATE-----

(server certificate hash)

-----END CERTIFICATE-----
---
Server certificate
subject=CN = satellite.jpasqualetto.local

issuer=CN = Joniel Intermediate CA

---
No client certificate CA names sent
Requested Signature Algorithms: ECDSA+SHA256:ECDSA+SHA384:ECDSA+SHA512:Ed25519:Ed448:RSA-PSS+SHA256:RSA-PSS+SHA256:RSA-PSS+SHA384:RSA-PSS+SHA384:RSA-PSS+SHA512:RSA-PSS+SHA512:RSA+SHA256:RSA+SHA384:RSA+SHA512:ECDSA+SHA224:RSA+SHA224:ECDSA+SHA1:RSA+SHA1
Shared Requested Signature Algorithms: ECDSA+SHA256:ECDSA+SHA384:ECDSA+SHA512:Ed25519:Ed448:RSA-PSS+SHA256:RSA-PSS+SHA256:RSA-PSS+SHA384:RSA-PSS+SHA384:RSA-PSS+SHA512:RSA-PSS+SHA512:RSA+SHA256:RSA+SHA384:RSA+SHA512
Peer signing digest: SHA256
Peer signature type: RSA-PSS
Server Temp Key: X25519, 253 bits
---
SSL handshake has read 1563 bytes and written 433 bytes
Verification: OK
---
New, TLSv1.3, Cipher is TLS_AES_256_GCM_SHA384
Server public key is 2048 bit
Secure Renegotiation IS NOT supported
Compression: NONE
Expansion: NONE
No ALPN negotiated
Early data was not sent
Verify return code: 0 (ok)
---
DONE

This is what it would look like with my patch:

# echo|openssl s_client -connect $(hostname -f):9090 -showcerts
CONNECTED(00000003)
depth=2 CN = Joniel Internal ROOT CA
verify return:1
depth=1 CN = Joniel Intermediate CA
verify return:1
depth=0 CN = satellite.jpasqualetto.local
verify return:1
---
Certificate chain
 0 s:CN = satellite.jpasqualetto.local
   i:CN = Joniel Intermediate CA
-----BEGIN CERTIFICATE-----

( server certificate hash)

-----END CERTIFICATE-----
 1 s:CN = Joniel Internal ROOT CA
   i:CN = Joniel Internal ROOT CA
-----BEGIN CERTIFICATE-----

(root CA hash)

-----END CERTIFICATE-----
 2 s:CN = Joniel Intermediate CA
   i:CN = Joniel Internal ROOT CA
-----BEGIN CERTIFICATE-----

(intermediate CA hash)

-----END CERTIFICATE-----
---
Server certificate
subject=CN = satellite.jpasqualetto.local

issuer=CN = Joniel Intermediate CA

---
No client certificate CA names sent
Requested Signature Algorithms: ECDSA+SHA256:ECDSA+SHA384:ECDSA+SHA512:Ed25519:Ed448:RSA-PSS+SHA256:RSA-PSS+SHA256:RSA-PSS+SHA384:RSA-PSS+SHA384:RSA-PSS+SHA512:RSA-PSS+SHA512:RSA+SHA256:RSA+SHA384:RSA+SHA512:ECDSA+SHA224:RSA+SHA224:ECDSA+SHA1:RSA+SHA1
Shared Requested Signature Algorithms: ECDSA+SHA256:ECDSA+SHA384:ECDSA+SHA512:Ed25519:Ed448:RSA-PSS+SHA256:RSA-PSS+SHA256:RSA-PSS+SHA384:RSA-PSS+SHA384:RSA-PSS+SHA512:RSA-PSS+SHA512:RSA+SHA256:RSA+SHA384:RSA+SHA512
Peer signing digest: SHA256
Peer signature type: RSA-PSS
Server Temp Key: X25519, 253 bits
---
SSL handshake has read 4102 bytes and written 433 bytes
Verification: OK
---
New, TLSv1.3, Cipher is TLS_AES_256_GCM_SHA384
Server public key is 2048 bit
Secure Renegotiation IS NOT supported
Compression: NONE
Expansion: NONE
No ALPN negotiated
Early data was not sent
Verify return code: 0 (ok)
---
DONE

This is what it would look like using ssl_ca_file:

# echo|openssl s_client -connect $(hostname -f):9090 -showcerts
CONNECTED(00000003)
depth=2 CN = Joniel Internal ROOT CA
verify return:1
depth=1 CN = Joniel Intermediate CA
verify return:1
depth=0 CN = satellite.jpasqualetto.local
verify return:1
---
Certificate chain
 0 s:CN = satellite.jpasqualetto.local
   i:CN = Joniel Intermediate CA
-----BEGIN CERTIFICATE-----

(server certificate hash)

-----END CERTIFICATE-----
 1 s:C = US, ST = North Carolina, L = Raleigh, O = Katello, OU = SomeOrgUnit, CN = sat613.jpasqualetto.local
   i:C = US, ST = North Carolina, L = Raleigh, O = Katello, OU = SomeOrgUnit, CN = sat613.jpasqualetto.local
-----BEGIN CERTIFICATE-----

(internal CA certificate hash which didn't sign the server certificate)

-----END CERTIFICATE-----
---
Server certificate
subject=CN = satellite.jpasqualetto.local

issuer=CN = Joniel Intermediate CA

---
No client certificate CA names sent
Requested Signature Algorithms: ECDSA+SHA256:ECDSA+SHA384:ECDSA+SHA512:Ed25519:Ed448:RSA-PSS+SHA256:RSA-PSS+SHA256:RSA-PSS+SHA384:RSA-PSS+SHA384:RSA-PSS+SHA512:RSA-PSS+SHA512:RSA+SHA256:RSA+SHA384:RSA+SHA512:ECDSA+SHA224:RSA+SHA224:ECDSA+SHA1:RSA+SHA1
Shared Requested Signature Algorithms: ECDSA+SHA256:ECDSA+SHA384:ECDSA+SHA512:Ed25519:Ed448:RSA-PSS+SHA256:RSA-PSS+SHA256:RSA-PSS+SHA384:RSA-PSS+SHA384:RSA-PSS+SHA512:RSA-PSS+SHA512:RSA+SHA256:RSA+SHA384:RSA+SHA512
Peer signing digest: SHA256
Peer signature type: RSA-PSS
Server Temp Key: X25519, 253 bits
---
SSL handshake has read 3372 bytes and written 433 bytes
Verification: OK
---
New, TLSv1.3, Cipher is TLS_AES_256_GCM_SHA384
Server public key is 2048 bit
Secure Renegotiation IS NOT supported
Compression: NONE
Expansion: NONE
No ALPN negotiated
Early data was not sent
Verify return code: 0 (ok)
---
DONE

Copy link
Member

Choose a reason for hiding this comment

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

@ehelms pointed out we also do both verification of client certs (which is the default CA), so it's one I need to think further about.

Copy link
Member

Choose a reason for hiding this comment

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

Note to self: perhaps we can provide the cert including the full chain in SSLCertificate.

@@ -95,6 +106,7 @@ def https_app(https_port, plugins = https_plugins)
:SSLVerifyClient => OpenSSL::SSL::VERIFY_PEER,
:SSLPrivateKey => load_ssl_private_key(settings.ssl_private_key),
:SSLCertificate => load_ssl_certificate(settings.ssl_certificate),
:SSLExtraChainCert => load_fullchain(settings.foreman_ssl_ca),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:SSLExtraChainCert => load_fullchain(settings.foreman_ssl_ca),
:SSLExtraChainCert => load_fullchain(settings.ssl_ca_file),

Add option SSLExtraChainCert to the Webrick webserver, so the full
certificate chain is sent to clients during the SSL handkshake.
@ekohl
Copy link
Member

ekohl commented Sep 26, 2023

Some further testing on my Fedora 38 box: if you provide the full chain in ssl_certificate (so cert + CA) then that is provided to the client. I think this is a cleaner solution because it doesn't require any code/config modifications. Reusing foreman_ssl_ca for this is a hack that only happens to work. Let's further look into how to solve this in the installer.

@jpasqualetto
Copy link
Author

How did you do that? Simply changing the content of the file? I couldn't get this approach to work on a RHEL8, still only get the server certificate when establishing a connection.

Besides that, I don't think that's how the webrick's SSLCertificate option is supposed to be used. The documentation describes the option SSLCertificate1 as "The SSL certificate for the server". I don't think replacing the certificate by a bundle containing the certificate and its chain is a clean solution.

Since you're willing to modify the installer to fix this, I propose that we create a new file containing the full chain and then load it into SSLExtraChainCert, which is a parameter expecting an array of certificates to build the chain.

@ekohl
Copy link
Member

ekohl commented Sep 26, 2023

I explicitly noted I only tested on Fedora 38, so perhaps this is a side effect of the newer Ruby, OpenSSL or both.

Reading the docs, I'm confused that there's SSLClientCA and SSLExtraChainCert. The documentation on this is minimal, to say the least.

Since you're willing to modify the installer to fix this, I propose that we create a new file containing the full chain and then load it into SSLExtraChainCert, which is a parameter expecting an array of certificates to build the chain.

Seeing the whole situation, I think that may be the best approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants