Skip to content

Commit

Permalink
Fix relaxed moderator joining for Kube Access (#23674)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@tigrato
tigrato committed Apr 3, 2023
1 parent 68ef47d commit a9ca0de
Show file tree
Hide file tree
Showing 5 changed files with 57 additions and 59 deletions.
90 changes: 49 additions & 41 deletions lib/kube/proxy/forwarder.go
View file
Expand Up @@ -441,10 +441,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 {
Expand Down Expand Up @@ -579,9 +580,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 {
Expand Down Expand Up @@ -667,10 +666,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.
//
Expand All @@ -681,27 +678,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())

// Get a dialer for either a k8s endpoint in current cluster or a tunneled
// endpoint for a leaf teleport cluster.
var dialFn dialFunc
Expand Down Expand Up @@ -774,9 +752,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,
Expand Down Expand Up @@ -980,6 +955,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.
//
Expand Down Expand Up @@ -1085,7 +1092,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)
}

Expand Down Expand Up @@ -1514,7 +1521,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)
}

Expand Down Expand Up @@ -1599,7 +1606,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)
}
Expand Down Expand Up @@ -1681,11 +1688,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
Expand Down Expand Up @@ -1856,7 +1864,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)
Expand Down Expand Up @@ -2423,7 +2431,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)
Expand Down Expand Up @@ -2529,7 +2537,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)
Expand Down
11 changes: 2 additions & 9 deletions lib/kube/proxy/forwarder_test.go
View file
Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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"),
))

Expand Down
8 changes: 5 additions & 3 deletions lib/kube/proxy/moderated_sessions_test.go
View file
Expand Up @@ -118,9 +118,7 @@ func TestModeratedSessions(t *testing.T) {
t,
moderatorUsername,
RoleSpec{
Name: moderatorRoleName,
KubeUsers: roleKubeUsers,
KubeGroups: roleKubeGroups,
Name: moderatorRoleName,
// sessionJoin:
SessionJoin: []*types.SessionJoinPolicy{
{
Expand All @@ -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
Expand Down
6 changes: 1 addition & 5 deletions lib/kube/proxy/sess.go
View file
Expand Up @@ -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(),
Expand Down
1 change: 0 additions & 1 deletion lib/web/apiserver_test.go
View file
Expand Up @@ -2301,7 +2301,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{
Expand Down

0 comments on commit a9ca0de

Please sign in to comment.