From 8a9be3fece42b9ce414b0c23b9c49b8c07b7cc3c Mon Sep 17 00:00:00 2001 From: Tiago Silva Date: Mon, 3 Apr 2023 14:51:05 +0100 Subject: [PATCH] Fix relaxed moderator joining for Kube Access (#23674) This PR fixes the relaxed mode for moderator joining moderated sessions. Previously, when the moderator didn't had access to the Kubernetes cluster where the session he tried to join was active, Teleport would panic because the user didn't had at least one `kubernetes_user` or `kubernetes_group`. This PR moves the check of kubernetes principals into the autorization step instead of the authentication step in order to prevent the failure of authentication followed by a panic. --- lib/kube/proxy/forwarder.go | 91 ++++++++++++----------- lib/kube/proxy/forwarder_test.go | 11 +-- lib/kube/proxy/moderated_sessions_test.go | 8 +- lib/kube/proxy/sess.go | 6 +- lib/web/apiserver_test.go | 1 - 5 files changed, 57 insertions(+), 60 deletions(-) diff --git a/lib/kube/proxy/forwarder.go b/lib/kube/proxy/forwarder.go index 8c75a9da4e143..8ad54685ba557 100644 --- a/lib/kube/proxy/forwarder.go +++ b/lib/kube/proxy/forwarder.go @@ -447,10 +447,11 @@ type handlerWithAuthFunc func(ctx *authContext, w http.ResponseWriter, r *http.R // handlerWithAuthFuncStd is http handler with passed auth context type handlerWithAuthFuncStd func(ctx *authContext, w http.ResponseWriter, r *http.Request) (any, error) +// accessDeniedMsg is a message returned to the client when access is denied. +const accessDeniedMsg = "[00] access denied" + // authenticate function authenticates request func (f *Forwarder) authenticate(req *http.Request) (*authContext, error) { - const accessDeniedMsg = "[00] access denied" - var isRemoteUser bool userTypeI, err := authz.UserFromContext(req.Context()) if err != nil { @@ -585,9 +586,7 @@ func (f *Forwarder) withAuthPassthrough(handler handlerWithAuthFunc) httprouter. return httplib.MakeHandlerWithErrorWriter(func(w http.ResponseWriter, req *http.Request, p httprouter.Params) (any, error) { authContext, err := f.authenticate(req) if err != nil { - if !trace.IsAccessDenied(err) && !trace.IsNotFound(err) { - return nil, trace.Wrap(err) - } + return nil, trace.Wrap(err) } err = f.acquireConnectionLockWithIdentity(req.Context(), authContext) if err != nil { @@ -673,10 +672,8 @@ func (f *Forwarder) setupContext(authCtx authz.Context, req *http.Request, isRem } var ( - kubeUsers, kubeGroups []string - kubeLabels map[string]string - kubeServers []types.KubeServer - err error + kubeServers []types.KubeServer + err error ) // Only check k8s principals for local clusters. // @@ -687,27 +684,8 @@ func (f *Forwarder) setupContext(authCtx authz.Context, req *http.Request, isRem if err != nil || len(kubeServers) == 0 { return nil, trace.NotFound("cluster %q not found", kubeCluster) } - // check signing TTL and return a list of allowed logins for local cluster based on Kubernetes service labels. - kubeAccessDetails, err := f.getKubeAccessDetails(kubeServers, roles, kubeCluster, sessionTTL, kubeResource) - if err != nil && !trace.IsNotFound(err) { - return nil, trace.Wrap(err) - // roles.CheckKubeGroupsAndUsers returns trace.NotFound if the user does - // does not have at least one configured kubernetes_users or kubernetes_groups. - } else if trace.IsNotFound(err) { - const errMsg = "Your user's Teleport role does not allow Kubernetes access." + - " Please ask cluster administrator to ensure your role has appropriate kubernetes_groups and kubernetes_users set." - return nil, trace.NotFound(errMsg) - } - - kubeUsers = kubeAccessDetails.kubeUsers - kubeGroups = kubeAccessDetails.kubeGroups - kubeLabels = kubeAccessDetails.clusterLabels } - // fillDefaultKubePrincipalDetails fills the default details in order to keep - // the correct behavior when forwarding the request to the Kubernetes API. - kubeUsers, kubeGroups = fillDefaultKubePrincipalDetails(kubeUsers, kubeGroups, authCtx.User.GetName()) - clientSrc, clientDst := utils.ClientAddrFromContext(req.Context()) forwarderType := f.cfg.KubeServiceType @@ -750,7 +728,6 @@ func (f *Forwarder) setupContext(authCtx authz.Context, req *http.Request, isRem } dialFn = func(ctx context.Context, network string, endpoint kubeClusterEndpoint) (net.Conn, error) { - // Make sure we will only send signed PROXY headers to teleport kube service node, not real kube cluster if forwarderType != ProxyService && endpoint.serverID == "" { clientDst = nil @@ -811,9 +788,6 @@ func (f *Forwarder) setupContext(authCtx authz.Context, req *http.Request, isRem clientIdleTimeout: roles.AdjustClientIdleTimeout(netConfig.GetClientIdleTimeout()), sessionTTL: sessionTTL, Context: authCtx, - kubeGroups: utils.StringsSet(kubeGroups), - kubeUsers: utils.StringsSet(kubeUsers), - kubeClusterLabels: kubeLabels, recordingConfig: recordingConfig, kubeClusterName: kubeCluster, kubeResource: kubeResource, @@ -1022,6 +996,38 @@ func (f *Forwarder) authorize(ctx context.Context, actx *authContext) error { services.NewKubernetesResourceMatcher(*actx.kubeResource), } } + var kubeUsers, kubeGroups []string + // Only check k8s principals for local clusters. + // + // For remote clusters, everything will be remapped to new roles on the + // leaf and checked there. + if !actx.teleportCluster.isRemote { + // check signing TTL and return a list of allowed logins for local cluster based on Kubernetes service labels. + kubeAccessDetails, err := f.getKubeAccessDetails(actx.kubeServers, actx.Checker, actx.kubeClusterName, actx.sessionTTL, actx.kubeResource) + if err != nil && !trace.IsNotFound(err) { + if actx.kubeResource != nil { + return trace.AccessDenied(notFoundMessage) + } + // TODO (tigrato): should return another message here. + return trace.AccessDenied(accessDeniedMsg) + // roles.CheckKubeGroupsAndUsers returns trace.NotFound if the user does + // does not have at least one configured kubernetes_users or kubernetes_groups. + } else if trace.IsNotFound(err) { + const errMsg = "Your user's Teleport role does not allow Kubernetes access." + + " Please ask cluster administrator to ensure your role has appropriate kubernetes_groups and kubernetes_users set." + return trace.NotFound(errMsg) + } + + kubeUsers = kubeAccessDetails.kubeUsers + kubeGroups = kubeAccessDetails.kubeGroups + actx.kubeClusterLabels = kubeAccessDetails.clusterLabels + } + + // fillDefaultKubePrincipalDetails fills the default details in order to keep + // the correct behavior when forwarding the request to the Kubernetes API. + kubeUsers, kubeGroups = fillDefaultKubePrincipalDetails(kubeUsers, kubeGroups, actx.User.GetName()) + actx.kubeUsers = utils.StringsSet(kubeUsers) + actx.kubeGroups = utils.StringsSet(kubeGroups) // Check authz against the first match. // @@ -1127,7 +1133,7 @@ func (f *Forwarder) join(ctx *authContext, w http.ResponseWriter, req *http.Requ // the resources as soon as we know the session is no longer active. defer sess.close() - if err := f.setupForwardingHeaders(sess, req); err != nil { + if err := f.setupForwardingHeaders(sess, req, false /* withImpersonationHeaders */); err != nil { return nil, trace.Wrap(err) } @@ -1560,7 +1566,7 @@ func (f *Forwarder) exec(ctx *authContext, w http.ResponseWriter, req *http.Requ onResize: func(remotecommand.TerminalSize) {}, } - if err := f.setupForwardingHeaders(sess, req); err != nil { + if err := f.setupForwardingHeaders(sess, req, true /* withImpersonationHeaders */); err != nil { return nil, trace.Wrap(err) } @@ -1648,7 +1654,7 @@ func (f *Forwarder) portForward(ctx *authContext, w http.ResponseWriter, req *ht return nil, trace.Wrap(err) } - if err := f.setupForwardingHeaders(sess, req); err != nil { + if err := f.setupForwardingHeaders(sess, req, true /* withImpersonationHeaders */); err != nil { f.log.Debugf("DENIED Port forward: %v.", req.URL.String()) return nil, trace.Wrap(err) } @@ -1730,11 +1736,12 @@ const ( ImpersonationRequestDeniedMessage = "impersonation request has been denied" ) -func (f *Forwarder) setupForwardingHeaders(sess *clusterSession, req *http.Request) error { - if err := setupImpersonationHeaders(f.log, sess.authContext, req.Header); err != nil { - return trace.Wrap(err) +func (f *Forwarder) setupForwardingHeaders(sess *clusterSession, req *http.Request, withImpersonationHeaders bool) error { + if withImpersonationHeaders { + if err := setupImpersonationHeaders(f.log, sess.authContext, req.Header); err != nil { + return trace.Wrap(err) + } } - // Setup scheme, override target URL to the destination address req.URL.Scheme = "https" req.RequestURI = req.URL.Path + "?" + req.URL.RawQuery @@ -1905,7 +1912,7 @@ func (f *Forwarder) catchAll(ctx *authContext, w http.ResponseWriter, req *http. return nil, trace.Wrap(err) } - if err := f.setupForwardingHeaders(sess, req); err != nil { + if err := f.setupForwardingHeaders(sess, req, true /* withImpersonationHeaders */); err != nil { // This error goes to kubernetes client and is not visible in the logs // of the teleport server if not logged here. f.log.Errorf("Failed to set up forwarding headers: %v.", err) @@ -2472,7 +2479,7 @@ func (f *Forwarder) listPods(ctx *authContext, w http.ResponseWriter, req *http. return nil, trace.Wrap(err) } - if err := f.setupForwardingHeaders(sess, req); err != nil { + if err := f.setupForwardingHeaders(sess, req, true /* withImpersonationHeaders */); err != nil { // This error goes to kubernetes client and is not visible in the logs // of the teleport server if not logged here. f.log.Errorf("Failed to set up forwarding headers: %v.", err) @@ -2578,7 +2585,7 @@ func (f *Forwarder) deletePodsCollection(ctx *authContext, w http.ResponseWriter return nil, trace.Wrap(err) } - if err := f.setupForwardingHeaders(sess, req); err != nil { + if err := f.setupForwardingHeaders(sess, req, true /* withImpersonationHeaders */); err != nil { // This error goes to kubernetes client and is not visible in the logs // of the teleport server if not logged here. f.log.Errorf("Failed to set up forwarding headers: %v.", err) diff --git a/lib/kube/proxy/forwarder_test.go b/lib/kube/proxy/forwarder_test.go index 9a55eb3e647bf..4b88c0591358f 100644 --- a/lib/kube/proxy/forwarder_test.go +++ b/lib/kube/proxy/forwarder_test.go @@ -462,8 +462,6 @@ func TestAuthenticate(t *testing.T) { tunnel: tun, wantCtx: &authContext{ - kubeUsers: utils.StringsSet([]string{"user-a"}), - kubeGroups: utils.StringsSet([]string{teleport.KubeSystemAuthenticated}), certExpires: certExpiration, teleportCluster: teleportClusterClient{ name: "remote", @@ -481,8 +479,6 @@ func TestAuthenticate(t *testing.T) { tunnel: tun, wantCtx: &authContext{ - kubeUsers: utils.StringsSet([]string{"user-a"}), - kubeGroups: utils.StringsSet([]string{teleport.KubeSystemAuthenticated}), certExpires: certExpiration, teleportCluster: teleportClusterClient{ name: "remote", @@ -500,8 +496,6 @@ func TestAuthenticate(t *testing.T) { tunnel: tun, wantCtx: &authContext{ - kubeUsers: utils.StringsSet([]string{"user-a"}), - kubeGroups: utils.StringsSet([]string{teleport.KubeSystemAuthenticated}), certExpires: certExpiration, teleportCluster: teleportClusterClient{ name: "remote", @@ -709,8 +703,6 @@ func TestAuthenticate(t *testing.T) { tunnel: tun, wantCtx: &authContext{ - kubeUsers: utils.StringsSet([]string{"user-a"}), - kubeGroups: utils.StringsSet([]string{teleport.KubeSystemAuthenticated}), kubeClusterName: "foo", certExpires: certExpiration, teleportCluster: teleportClusterClient{ @@ -780,11 +772,12 @@ func TestAuthenticate(t *testing.T) { require.Equal(t, trace.IsAccessDenied(err), tt.wantAuthErr) return } + err = f.authorize(context.Background(), gotCtx) require.NoError(t, err) require.Empty(t, cmp.Diff(gotCtx, tt.wantCtx, cmp.AllowUnexported(authContext{}, teleportClusterClient{}), - cmpopts.IgnoreFields(authContext{}, "clientIdleTimeout", "sessionTTL", "Context", "recordingConfig", "disconnectExpiredCert"), + cmpopts.IgnoreFields(authContext{}, "clientIdleTimeout", "sessionTTL", "Context", "recordingConfig", "disconnectExpiredCert", "kubeCluster"), cmpopts.IgnoreFields(teleportClusterClient{}, "dial", "isRemoteClosed"), )) diff --git a/lib/kube/proxy/moderated_sessions_test.go b/lib/kube/proxy/moderated_sessions_test.go index 534f4f2365078..cbb05e67090fe 100644 --- a/lib/kube/proxy/moderated_sessions_test.go +++ b/lib/kube/proxy/moderated_sessions_test.go @@ -118,9 +118,7 @@ func TestModeratedSessions(t *testing.T) { t, moderatorUsername, RoleSpec{ - Name: moderatorRoleName, - KubeUsers: roleKubeUsers, - KubeGroups: roleKubeGroups, + Name: moderatorRoleName, // sessionJoin: SessionJoin: []*types.SessionJoinPolicy{ { @@ -130,6 +128,10 @@ func TestModeratedSessions(t *testing.T) { Modes: []string{string(types.SessionModeratorMode)}, }, }, + SetupRoleFunc: func(r types.Role) { + // set kubernetes labels to empty to test relaxed join rules + r.SetKubernetesLabels(types.Allow, types.Labels{}) + }, }) // create a userRequiringModerator with access to kubernetes thar requires diff --git a/lib/kube/proxy/sess.go b/lib/kube/proxy/sess.go index 5c62525b3d896..91fd1c475810b 100644 --- a/lib/kube/proxy/sess.go +++ b/lib/kube/proxy/sess.go @@ -828,11 +828,7 @@ func (s *session) lockedSetupLaunch(request *remoteCommandRequest, q url.Values, // join attempts to connect a party to the session. func (s *session) join(p *party) error { if p.Ctx.User.GetName() != s.ctx.User.GetName() { - roleNames := p.Ctx.Identity.GetIdentity().Groups - roles, err := getRolesByName(s.forwarder, roleNames) - if err != nil { - return trace.Wrap(err) - } + roles := p.Ctx.Checker.Roles() accessContext := auth.SessionAccessContext{ Username: p.Ctx.User.GetName(), diff --git a/lib/web/apiserver_test.go b/lib/web/apiserver_test.go index 4bfad461f18f6..d5713dacf1567 100644 --- a/lib/web/apiserver_test.go +++ b/lib/web/apiserver_test.go @@ -2314,7 +2314,6 @@ func TestMotD(t *testing.T) { // TestPingAutomaticUpgrades ensures /webapi/ping returns whether AutomaticUpgrades are enabled. func TestPingAutomaticUpgrades(t *testing.T) { - t.Run("Automatic Upgrades are enabled", func(t *testing.T) { // Enable Automatic Upgrades modules.SetTestModules(t, &modules.TestModules{TestFeatures: modules.Features{