Skip to content

Commit

Permalink
[v14] Fix DynamicIdentityFileCreds being incompatible with L7 Loadbal…
Browse files Browse the repository at this point in the history
…ancers (#36469)

* DynamicIdentityFileCreds should correctly provide TLS root CAs to ALPN handshake

* Fix bug involving multiple credentials

* Add test case covering CA Cert Pool

* Update api/client/credentials_test.go

Co-authored-by: Alan Parra <alan.parra@goteleport.com>

---------

Co-authored-by: Alan Parra <alan.parra@goteleport.com>
  • Loading branch information
strideynet and codingllama committed Jan 9, 2024
1 parent 860125c commit c03e2d4
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 7 deletions.
7 changes: 2 additions & 5 deletions api/client/contextdialer.go
Original file line number Diff line number Diff line change
Expand Up @@ -364,11 +364,8 @@ func newTLSRoutingWithConnUpgradeDialer(ssh ssh.ClientConfig, params connectPara
},
ALPNConnUpgradeRequired: IsALPNConnUpgradeRequired(ctx, params.addr, insecure),
GetClusterCAs: func(_ context.Context) (*x509.CertPool, error) {
tlsConfig, err := params.cfg.Credentials[0].TLSConfig()
if err != nil {
return nil, trace.Wrap(err)
}
return tlsConfig.RootCAs, nil
// Uses the Root CAs from the TLS Config of the Credentials.
return params.tlsConfig.RootCAs, nil
},
})
if err != nil {
Expand Down
9 changes: 7 additions & 2 deletions api/client/credentials.go
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,8 @@ func (d *DynamicIdentityFileCreds) Dialer(

// TLSConfig returns TLS configuration. Implementing the Credentials interface.
func (d *DynamicIdentityFileCreds) TLSConfig() (*tls.Config, error) {
d.mu.RLock()
defer d.mu.RUnlock()
// Build a "dynamic" tls.Config which can support a changing cert and root
// CA pool.
cfg := &tls.Config{
Expand All @@ -506,7 +508,10 @@ func (d *DynamicIdentityFileCreds) TLSConfig() (*tls.Config, error) {
},

// VerifyConnection is used instead of the static RootCAs field.
RootCAs: nil,
// However, there's some client code which relies on the static RootCAs
// field. So we set it to a copy of the current root CAs pool to support
// those - e.g ALPNDialerConfig.GetClusterCAs
RootCAs: d.tlsRootCAs.Clone(),
// InsecureSkipVerify is forced true to ensure that only our
// VerifyConnection callback is used to verify the server's presented
// certificate.
Expand All @@ -521,7 +526,7 @@ func (d *DynamicIdentityFileCreds) TLSConfig() (*tls.Config, error) {
opts := x509.VerifyOptions{
DNSName: state.ServerName,
Intermediates: x509.NewCertPool(),
Roots: d.tlsRootCAs,
Roots: d.tlsRootCAs.Clone(),
}
for _, cert := range state.PeerCertificates[1:] {
// Whilst we don't currently use intermediate certs at
Expand Down
7 changes: 7 additions & 0 deletions api/client/credentials_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,13 @@ func TestDynamicIdentityFileCreds(t *testing.T) {
require.NoError(t, err)
require.Equal(t, wantTLSCert, *gotTLSCert)

tlsCACertPEM, _ := pem.Decode(tlsCACert)
tlsCACertDER, err := x509.ParseCertificate(tlsCACertPEM.Bytes)
require.NoError(t, err)
wantCertPool := x509.NewCertPool()
wantCertPool.AddCert(tlsCACertDER)
require.True(t, wantCertPool.Equal(tlsConfig.RootCAs), "tlsconfig.RootCAs mismatch")

// Generate a new TLS certificate that contains the same private key as
// the original.
template := &x509.Certificate{
Expand Down

0 comments on commit c03e2d4

Please sign in to comment.