Skip to content

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

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

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

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 13 additions & 0 deletions lib/launcher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,14 @@ def https_plugins
plugins.select { |p| p[:https_enabled] }.map { |p| p[:class] }
end

# Imitate OpenSSL::X509::Certificate.load_file for openssl < 3.0.0
def load_fullchain(bundle_pem)
File.read(bundle_pem)
.lines
.slice_after(/END CERTIFICATE/)
.map { |pem| OpenSSL::X509::Certificate.new(pem.join) }
end

def http_app(http_port, plugins = http_plugins)
return nil unless http_enabled?
app = Rack::Builder.new do
Expand All @@ -62,6 +70,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

@ekohl ekohl Sep 20, 2023

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

@jpasqualetto jpasqualetto Sep 20, 2023

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

@ekohl ekohl Sep 20, 2023

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

@ekohl ekohl Sep 20, 2023

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

@jpasqualetto jpasqualetto Sep 20, 2023

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

@ekohl ekohl Sep 20, 2023

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

@ekohl ekohl Sep 20, 2023

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.

logger.error "Unable to read #{settings.foreman_ssl_ca}. Are the values correct in settings.yml and do permissions allow reading?"
end

app = Rack::Builder.new do
plugins.each { |p| instance_eval(p.https_rackup) }
end
Expand Down Expand Up @@ -95,6 +107,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

@ekohl ekohl Sep 20, 2023

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

:SSLCACertificateFile => settings.ssl_ca_file,
:SSLOptions => ssl_options,
:SSLCiphers => CIPHERS - Proxy::SETTINGS.ssl_disabled_ciphers,
Expand Down