Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
68 changes: 59 additions & 9 deletions integration/proxy/teleterm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,8 @@ type gatewayCertRenewalParams struct {
webauthnLogin libclient.WebauthnLoginFunc
generateAndSetupUserCreds generateAndSetupUserCredsFunc
wantPromptMFACallCount int
customCertsExpireFunc func(gateway.Gateway)
expectNoRelogin bool
}

func testGatewayCertRenewal(ctx context.Context, t *testing.T, params gatewayCertRenewalParams) {
Expand Down Expand Up @@ -280,11 +282,15 @@ func testGatewayCertRenewal(ctx context.Context, t *testing.T, params gatewayCer

params.testGatewayConnection(ctx, t, daemonService, gateway)

// Advance the fake clock to simulate the db cert expiry inside the middleware.
fakeClock.Advance(time.Hour * 48)
if params.customCertsExpireFunc != nil {
params.customCertsExpireFunc(gateway)
} else {
// Advance the fake clock to simulate the db cert expiry inside the middleware.
fakeClock.Advance(time.Hour * 48)

// Overwrite user certs with expired ones to simulate the user cert expiry.
params.generateAndSetupUserCreds(t, tc, -time.Hour)
// Overwrite user certs with expired ones to simulate the user cert expiry.
params.generateAndSetupUserCreds(t, tc, -time.Hour)
}

// Open a new connection.
// This should trigger the relogin flow. The middleware will notice that the cert has expired
Expand All @@ -293,7 +299,11 @@ func testGatewayCertRenewal(ctx context.Context, t *testing.T, params gatewayCer
// will let the connection through.
params.testGatewayConnection(ctx, t, daemonService, gateway)

require.Equal(t, uint32(1), tshdEventsService.reloginCallCount.Load(),
expectedReloginCalls := uint32(1)
if params.expectNoRelogin {
expectedReloginCalls = uint32(0)
}
require.Equal(t, expectedReloginCalls, tshdEventsService.reloginCallCount.Load(),
"Unexpected number of calls to TSHDEventsClient.Relogin")
require.Equal(t, uint32(0), tshdEventsService.sendNotificationCallCount.Load(),
"Unexpected number of calls to TSHDEventsClient.SendNotification")
Expand Down Expand Up @@ -495,13 +505,51 @@ func TestTeletermKubeGateway(t *testing.T) {
webauthnLogin: webauthnLogin,
})
})
t.Run("reissue cert after clearing it for root kube", func(t *testing.T) {
profileName := mustGetProfileName(t, suite.root.Web)
kubeURI := uri.NewClusterURI(profileName).AppendKube(kubeClusterName)
// The test can potentially hang forever if something is wrong with the MFA prompt, add a timeout.
ctx, cancel := context.WithTimeout(ctx, 10*time.Second)
t.Cleanup(cancel)
testKubeGatewayCertRenewal(ctx, t, kubeGatewayCertRenewalParams{
suite: suite,
kubeURI: kubeURI,
webauthnLogin: webauthnLogin,
customCertsExpireFunc: func(gw gateway.Gateway) {
kubeGw, err := gateway.AsKube(gw)
require.NoError(t, err)
kubeGw.ClearCerts()
},
expectNoRelogin: true,
})
})
t.Run("reissue cert after clearing it for leaf kube", func(t *testing.T) {
profileName := mustGetProfileName(t, suite.root.Web)
kubeURI := uri.NewClusterURI(profileName).AppendLeafCluster(suite.leaf.Secrets.SiteName).AppendKube(kubeClusterName)
// The test can potentially hang forever if something is wrong with the MFA prompt, add a timeout.
ctx, cancel := context.WithTimeout(ctx, 10*time.Second)
t.Cleanup(cancel)
testKubeGatewayCertRenewal(ctx, t, kubeGatewayCertRenewalParams{
suite: suite,
kubeURI: kubeURI,
webauthnLogin: webauthnLogin,
customCertsExpireFunc: func(gw gateway.Gateway) {
kubeGw, err := gateway.AsKube(gw)
require.NoError(t, err)
kubeGw.ClearCerts()
},
expectNoRelogin: true,
})
})
}

type kubeGatewayCertRenewalParams struct {
suite *Suite
kubeURI uri.ResourceURI
albAddr string
webauthnLogin libclient.WebauthnLoginFunc
suite *Suite
kubeURI uri.ResourceURI
albAddr string
webauthnLogin libclient.WebauthnLoginFunc
customCertsExpireFunc func(gateway.Gateway)
expectNoRelogin bool
}

func testKubeGatewayCertRenewal(ctx context.Context, t *testing.T, params kubeGatewayCertRenewalParams) {
Expand Down Expand Up @@ -550,6 +598,8 @@ func testKubeGatewayCertRenewal(ctx context.Context, t *testing.T, params kubeGa
},
testGatewayConnection: testKubeConnection,
webauthnLogin: params.webauthnLogin,
customCertsExpireFunc: params.customCertsExpireFunc,
expectNoRelogin: params.expectNoRelogin,
generateAndSetupUserCreds: func(t *testing.T, tc *libclient.TeleportClient, ttl time.Duration) {
creds, err := helpers.GenerateUserCreds(helpers.UserCredsRequest{
Process: params.suite.root.Process,
Expand Down
12 changes: 12 additions & 0 deletions lib/srv/alpnproxy/common/kube.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import (
"encoding/hex"
"fmt"
"strings"

"github.com/gravitational/trace"
)

// KubeLocalProxySNI generates the SNI used for Kube local proxy.
Expand All @@ -37,6 +39,16 @@ func TeleportClusterFromKubeLocalProxySNI(serverName string) string {
return teleportCluster
}

// KubeClusterFromKubeLocalProxySNI returns Kubernetes cluster name from SNI.
func KubeClusterFromKubeLocalProxySNI(serverName string) (string, error) {
kubeCluster, _, _ := strings.Cut(serverName, ".")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we care about the teleport cluster?
It's possible you have the same cluster name in two different teleport clusters

Copy link
Copy Markdown
Contributor Author

@gzdunek gzdunek Jan 13, 2025

Choose a reason for hiding this comment

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

I'm reading the teleport cluster using TeleportClusterFromKubeLocalProxySNI() before reading the kube cluster.
Then m.certReissuer() is called with that pair of teleport + kube cluster.

Do you mean something else?

str, err := hex.DecodeString(kubeCluster)
if err != nil {
return "", trace.Wrap(err)
}
return string(str), nil
}

// KubeLocalProxyWildcardDomain returns the wildcard domain used to generate
// local self-signed CA for provided Teleport cluster.
func KubeLocalProxyWildcardDomain(teleportCluster string) string {
Expand Down
52 changes: 37 additions & 15 deletions lib/srv/alpnproxy/kube.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,12 +157,23 @@ func writeKubeError(ctx context.Context, rw http.ResponseWriter, kubeError *apie
}
}

// ClearCerts clears the middleware certs.
// It will try to reissue them when a new request comes in.
func (m *KubeMiddleware) ClearCerts() {
m.certsMu.Lock()
defer m.certsMu.Unlock()
clear(m.certs)
}

// HandleRequest checks if middleware has valid certificate for this request and
// reissues it if needed. In case of reissuing error we write directly to the response and return true,
// so caller won't continue processing the request.
func (m *KubeMiddleware) HandleRequest(rw http.ResponseWriter, req *http.Request) bool {
cert, err := m.getCertForRequest(req)
if err != nil {
// If the cert is cleared using m.ClearCerts(), it won't be found.
// This forces the middleware to issue a new cert on a new request.
// This is used in access requests in Connect where we want to refresh certs without closing the proxy.
if err != nil && !trace.IsNotFound(err) {
Comment thread
gzdunek marked this conversation as resolved.
return false
}

Expand Down Expand Up @@ -220,23 +231,38 @@ func (m *KubeMiddleware) OverwriteClientCerts(req *http.Request) ([]tls.Certific
var ErrUserInputRequired = errors.New("user input required")

// reissueCertIfExpired checks if provided certificate has expired and reissues it if needed and replaces in the middleware certs.
// serverName has a form of <hex-encoded-kube-cluster>.<teleport-cluster>.
Comment thread
gzdunek marked this conversation as resolved.
func (m *KubeMiddleware) reissueCertIfExpired(ctx context.Context, cert tls.Certificate, serverName string) error {
x509Cert, err := utils.TLSCertLeaf(cert)
if err != nil {
return trace.Wrap(err)
needsReissue := false
if len(cert.Certificate) == 0 {
m.logger.InfoContext(ctx, "missing TLS certificate, attempting to reissue a new one")
needsReissue = true
} else {
x509Cert, err := utils.TLSCertLeaf(cert)
if err != nil {
return trace.Wrap(err)
}
if err := utils.VerifyCertificateExpiry(x509Cert, m.clock); err != nil {
needsReissue = true
}
}
if err := utils.VerifyCertificateExpiry(x509Cert, m.clock); err == nil {
if !needsReissue {
return nil
}

if m.certReissuer == nil {
return trace.BadParameter("can't reissue expired proxy certificate - reissuer is not available")
return trace.BadParameter("can't reissue proxy certificate - reissuer is not available")
}

// If certificate has expired we try to reissue it.
identity, err := tlsca.FromSubject(x509Cert.Subject, x509Cert.NotAfter)
teleportCluster := common.TeleportClusterFromKubeLocalProxySNI(serverName)
if teleportCluster == "" {
return trace.BadParameter("can't reissue proxy certificate - teleport cluster is empty")
}
kubeCluster, err := common.KubeClusterFromKubeLocalProxySNI(serverName)
if err != nil {
return trace.Wrap(err)
return trace.Wrap(err, "can't reissue proxy certificate - kube cluster name is invalid")
}
if kubeCluster == "" {
return trace.BadParameter("can't reissue proxy certificate - kube cluster is empty")
Comment thread
gzdunek marked this conversation as resolved.
}

errCh := make(chan error, 1)
Expand All @@ -247,11 +273,7 @@ func (m *KubeMiddleware) reissueCertIfExpired(ctx context.Context, cert tls.Cert
go func() {
defer m.isCertReissuingRunning.Store(false)

cluster := identity.TeleportCluster
if identity.RouteToCluster != "" {
cluster = identity.RouteToCluster
}
newCert, err := m.certReissuer(m.closeContext, cluster, identity.KubernetesCluster)
newCert, err := m.certReissuer(m.closeContext, teleportCluster, kubeCluster)
if err == nil {
m.certsMu.Lock()
m.certs[serverName] = newCert
Expand Down
5 changes: 5 additions & 0 deletions lib/srv/alpnproxy/local_proxy_http_middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ type LocalProxyHTTPMiddleware interface {

// OverwriteClientCerts overwrites the client certs used for upstream connection.
OverwriteClientCerts(req *http.Request) ([]tls.Certificate, error)

// ClearCerts clears the middleware certs.
// It will try to reissue them when a new request comes in.
ClearCerts()
}

// DefaultLocalProxyHTTPMiddleware provides default implementations for LocalProxyHTTPMiddleware.
Expand All @@ -56,3 +60,4 @@ func (m *DefaultLocalProxyHTTPMiddleware) HandleResponse(resp *http.Response) er
func (m *DefaultLocalProxyHTTPMiddleware) OverwriteClientCerts(req *http.Request) ([]tls.Certificate, error) {
return nil, trace.NotImplemented("not implemented")
}
func (m *DefaultLocalProxyHTTPMiddleware) ClearCerts() {}
43 changes: 27 additions & 16 deletions lib/srv/alpnproxy/local_proxy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import (
"github.com/gravitational/teleport/lib/kube/proxy/responsewriters"
"github.com/gravitational/teleport/lib/srv/alpnproxy/common"
"github.com/gravitational/teleport/lib/tlsca"
"github.com/gravitational/teleport/lib/utils"
)

// TestHandleAWSAccessSigVerification tests if LocalProxy verifies the AWS SigV4 signature of incoming request.
Expand Down Expand Up @@ -471,7 +472,7 @@ func TestKubeMiddleware(t *testing.T) {

now := time.Now()
clock := clockwork.NewFakeClockAt(now)
var certReissuer KubeCertReissuer
teleportCluster := "localhost"

ca := mustGenSelfSignedCert(t)
kube1Cert := mustGenCertSignedWithCA(t, ca,
Expand Down Expand Up @@ -499,7 +500,7 @@ func TestKubeMiddleware(t *testing.T) {
withClock(clock),
)

certReissuer = func(ctx context.Context, teleportCluster, kubeCluster string) (tls.Certificate, error) {
certReissuer := func(ctx context.Context, teleportCluster, kubeCluster string) (tls.Certificate, error) {
select {
case <-ctx.Done():
return tls.Certificate{}, ctx.Err()
Expand All @@ -511,7 +512,7 @@ func TestKubeMiddleware(t *testing.T) {
t.Run("expired certificate is still reissued if request context expires", func(t *testing.T) {
req := &http.Request{
TLS: &tls.ConnectionState{
ServerName: "kube1",
ServerName: common.KubeLocalProxySNI(teleportCluster, "kube1"),
},
}
// we set request context to a context that is already canceled, so handler function will start reissuing
Expand All @@ -520,8 +521,10 @@ func TestKubeMiddleware(t *testing.T) {
cancel()
req = req.WithContext(reqCtx)

startCerts := KubeClientCerts{}
startCerts.Add(teleportCluster, "kube1", kube1Cert)
km := NewKubeMiddleware(KubeMiddlewareConfig{
Certs: KubeClientCerts{"kube1": kube1Cert},
Certs: startCerts,
CertReissuer: certReissuer,
Clock: clockwork.NewFakeClockAt(now.Add(time.Hour * 2)),
CloseContext: context.Background(),
Expand Down Expand Up @@ -553,6 +556,12 @@ func TestKubeMiddleware(t *testing.T) {
require.Equal(t, newCert, certs[0], "certificate was not reissued")
})

getStartCerts := func() KubeClientCerts {
certs := KubeClientCerts{}
certs.Add(teleportCluster, "kube1", kube1Cert)
certs.Add(teleportCluster, "kube2", kube2Cert)
return certs
}
testCases := []struct {
name string
reqClusterName string
Expand All @@ -562,32 +571,33 @@ func TestKubeMiddleware(t *testing.T) {
wantErr string
}{
{
name: "kube cluster not found",
reqClusterName: "kube3",
startCerts: KubeClientCerts{"kube1": kube1Cert, "kube2": kube2Cert},
clock: clockwork.NewFakeClockAt(now),
wantErr: "no client cert found for kube3",
name: "reissue cert when not found",
reqClusterName: "kube3",
startCerts: getStartCerts(),
clock: clockwork.NewFakeClockAt(now),
overwrittenCert: newCert,
wantErr: "",
},
{
name: "expired cert reissued",
name: "expired cert is reissued",
reqClusterName: "kube1",
startCerts: KubeClientCerts{"kube1": kube1Cert, "kube2": kube2Cert},
startCerts: getStartCerts(),
clock: clockwork.NewFakeClockAt(now.Add(time.Hour * 2)),
overwrittenCert: newCert,
wantErr: "",
},
{
name: "success kube1",
name: "valid cert for kube1 is returned",
reqClusterName: "kube1",
startCerts: KubeClientCerts{"kube1": kube1Cert, "kube2": kube2Cert},
startCerts: getStartCerts(),
clock: clockwork.NewFakeClockAt(now),
overwrittenCert: kube1Cert,
wantErr: "",
},
{
name: "success kube2",
name: "valid cert for kube2 is returned",
reqClusterName: "kube2",
startCerts: KubeClientCerts{"kube1": kube1Cert, "kube2": kube2Cert},
startCerts: getStartCerts(),
clock: clockwork.NewFakeClockAt(now),
overwrittenCert: kube2Cert,
wantErr: "",
Expand All @@ -598,12 +608,13 @@ func TestKubeMiddleware(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
req := http.Request{
TLS: &tls.ConnectionState{
ServerName: tt.reqClusterName,
ServerName: common.KubeLocalProxySNI(teleportCluster, tt.reqClusterName),
},
}
km := NewKubeMiddleware(KubeMiddlewareConfig{
Certs: tt.startCerts,
CertReissuer: certReissuer,
Logger: utils.NewSlogLoggerForTests(),
Clock: tt.clock,
CloseContext: context.Background(),
})
Expand Down
22 changes: 22 additions & 0 deletions lib/teleterm/daemon/daemon.go
Original file line number Diff line number Diff line change
Expand Up @@ -811,6 +811,28 @@ func (s *Service) AssumeRole(ctx context.Context, req *api.AssumeRoleRequest) er
return trace.Wrap(err)
}

// Clear certs in kube gateways.
// Access requests may grant elevated permissions for accessing a kube cluster.
// To allow the user to use these permissions, we clear the existing certs.
// When a kube proxy receives a new request, it will issue new certs,
// similarly to the process when certs expire.
//
// We don't know which gateways are affected by the access request,
// so we need to clear certs for all of them.
s.mu.RLock()
defer s.mu.RUnlock()
for _, gw := range s.gateways {
Comment thread
gzdunek marked this conversation as resolved.
targetURI := gw.TargetURI()
if !(targetURI.IsKube() && targetURI.GetRootClusterURI() == cluster.URI) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this correct? a gateway from a leaf cluster can be modified when you assume a root cluster role

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think so. As Grzegorz wrote in the PR description:

There's one downside to this approach: we can't determine which local proxies will be affected by the access request, so we must invalidate all of them.

If you assume a root cluster role, AFAIK we cannot easily tell if it affects leaf cluster resources.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, Rafał's explanation is correct.

continue
}
kubeGw, err := gateway.AsKube(gw)
if err != nil {
s.cfg.Logger.ErrorContext(ctx, "Could not clear certs for kube when assuming request", "error", err, "target_uri", targetURI)
}
kubeGw.ClearCerts()
}

// We have to reconnect using the updated cert.
return trace.Wrap(s.ClearCachedClientsForRoot(cluster.URI))
}
Expand Down
Loading