Skip to content

Commit

Permalink
Prevent unauthorized access to kube clusters by upserting kube_servers (
Browse files Browse the repository at this point in the history 
#470)

This PR changes the behavior of the kubernetes_service when validating access
to kubernetes clusters. Previously, the kubernetes_service would use the first
kubernetes cluster it found in the Auth server backend to validate access. This was
problematic because if the first kubernetes cluster was upserted with a
the same name as a kubernetes cluster the user was trying to access but
with different labels, the user would be able to access the cluster even
though they shouldn't be able to.

This PR changes the behavior of the kubernetes_service to use the
in memory kubernetes cluster representation used for heartbeats
instead of relying on the information received from the auth server. This would
block the user from accessing the cluster if the cluster was upserted
with a different set of labels since the kubernetes_service would not
have the updated labels in memory and would deny access.

Fixes #469
  • Loading branch information
@tigrato @r0mant
tigrato authored and r0mant committed Mar 30, 2023
1 parent 4857bf5 commit c79d79a
Show file tree
Hide file tree
Showing 8 changed files with 264 additions and 47 deletions.
2 changes: 1 addition & 1 deletion lib/authz/permissions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -753,7 +753,7 @@ func definitionForBuiltinRole(clusterName string, recConfig types.SessionRecordi
types.NewRule(types.KindRole, services.RO()),
types.NewRule(types.KindNamespace, services.RO()),
types.NewRule(types.KindLock, services.RO()),
types.NewRule(types.KindKubernetesCluster, services.RW()),
types.NewRule(types.KindKubernetesCluster, services.RO()),
types.NewRule(types.KindSemaphore, services.RW()),
},
},
Expand Down
56 changes: 32 additions & 24 deletions lib/kube/proxy/forwarder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -325,8 +325,19 @@ type Forwarder struct {
sessions map[uuid.UUID]*session
// upgrades connections to websockets
upgrader websocket.Upgrader
// getKubernetesServersForKubeCluster is a function that returns a list of
// kubernetes servers for a given kube cluster but uses different methods
// depending on the service type.
// For example, if the service type is KubeService, it will use the
// local kubernetes clusters. If the service type is Proxy, it will
// use the heartbeat clusters.
getKubernetesServersForKubeCluster getKubeServersByNameFunc
}

// getKubeServersByNameFunc is a function that returns a list of
// kubernetes servers for a given kube cluster.
type getKubeServersByNameFunc = func(ctx context.Context, name string) ([]types.KubeServer, error)

// Close signals close to all outstanding or background operations
// to complete
func (f *Forwarder) Close() error {
Expand Down Expand Up @@ -369,6 +380,9 @@ type authContext struct {
kubeResource *types.KubernetesResource
// httpMethod is the request HTTP Method.
httpMethod string
// kubeServers are the registered agents for the kubernetes cluster the request
// is targeted to.
kubeServers []types.KubeServer
}

func (c authContext) String() string {
Expand Down Expand Up @@ -636,7 +650,9 @@ func (f *Forwarder) setupContext(authCtx authz.Context, req *http.Request, isRem
}

kubeCluster := identity.KubernetesCluster
if !isRemoteCluster {
// Only set a default kube cluster if the user is not accessing a specific cluster.
// The check for kubeCluster != "" is happens in the next code section.
if !isRemoteCluster && kubeCluster == "" {
kc, err := kubeutils.CheckOrSetKubeCluster(req.Context(), f.cfg.CachingAuthClient, identity.KubernetesCluster, teleportClusterName)
if err != nil {
if !trace.IsNotFound(err) {
Expand All @@ -653,14 +669,20 @@ func (f *Forwarder) setupContext(authCtx authz.Context, req *http.Request, isRem
var (
kubeUsers, kubeGroups []string
kubeLabels map[string]string
kubeServers []types.KubeServer
err error
)
// Only check k8s principals for local clusters.
//
// For remote clusters, everything will be remapped to new roles on the
// leaf and checked there.
if !isRemoteCluster {
kubeServers, err = f.getKubernetesServersForKubeCluster(req.Context(), kubeCluster)
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(roles, kubeCluster, sessionTTL, kubeResource)
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
Expand Down Expand Up @@ -767,7 +789,8 @@ func (f *Forwarder) setupContext(authCtx authz.Context, req *http.Request, isRem
isRemote: isRemoteCluster,
isRemoteClosed: isRemoteClosed,
},
httpMethod: req.Method,
httpMethod: req.Method,
kubeServers: kubeServers,
}, nil
}

Expand Down Expand Up @@ -840,16 +863,12 @@ type kubeAccessDetails struct {

// getKubeAccessDetails returns the allowed kube groups/users names and the cluster labels for a local kube cluster.
func (f *Forwarder) getKubeAccessDetails(
kubeServers []types.KubeServer,
roles services.AccessChecker,
kubeClusterName string,
sessionTTL time.Duration,
kubeResource *types.KubernetesResource,
) (kubeAccessDetails, error) {
kubeServers, err := f.cfg.CachingAuthClient.GetKubernetesServers(f.ctx)
if err != nil {
return kubeAccessDetails{}, trace.Wrap(err)
}

// Find requested kubernetes cluster name and get allowed kube users/groups names.
for _, s := range kubeServers {
c := s.GetCluster()
Expand Down Expand Up @@ -939,10 +958,7 @@ func (f *Forwarder) authorize(ctx context.Context, actx *authContext) error {
f.log.WithField("auth_context", actx.String()).Debug("Skipping authorization due to unknown kubernetes cluster name")
return nil
}
servers, err := f.cfg.CachingAuthClient.GetKubernetesServers(ctx)
if err != nil {
return trace.Wrap(err)
}

authPref, err := f.cfg.CachingAuthClient.GetAuthPreference(ctx)
if err != nil {
return trace.Wrap(err)
Expand All @@ -969,7 +985,7 @@ func (f *Forwarder) authorize(ctx context.Context, actx *authContext) error {
//
// We assume that users won't register two identically-named clusters with
// mis-matched labels. If they do, expect weirdness.
for _, s := range servers {
for _, s := range actx.kubeServers {
ks := s.GetCluster()
if ks.GetName() != actx.kubeClusterName {
continue
Expand Down Expand Up @@ -2049,11 +2065,7 @@ func (f *Forwarder) newClusterSessionSameCluster(ctx authContext) (*clusterSessi
return sess, nil
}

kubeServers, err := f.cfg.CachingAuthClient.GetKubernetesServers(f.ctx)
if err != nil && !trace.IsNotFound(err) {
return nil, trace.Wrap(err)
}

kubeServers := ctx.kubeServers
if len(kubeServers) == 0 && ctx.kubeClusterName == ctx.teleportCluster.name {
return nil, trace.Wrap(localErr)
}
Expand Down Expand Up @@ -2082,12 +2094,8 @@ func (f *Forwarder) newClusterSessionSameCluster(ctx authContext) (*clusterSessi
}

func (f *Forwarder) newClusterSessionLocal(ctx authContext) (*clusterSession, error) {
if len(f.clusterDetails) == 0 {
return nil, trace.NotFound("this Teleport process is not configured for direct Kubernetes access; you likely need to 'tsh login' into a leaf cluster or 'tsh kube login' into a different kubernetes cluster")
}

details, ok := f.clusterDetails[ctx.kubeClusterName]
if !ok {
details, err := f.findKubeDetailsByClusterName(ctx.kubeClusterName)
if err != nil {
return nil, trace.NotFound("kubernetes cluster %q not found", ctx.kubeClusterName)
}

Expand Down
146 changes: 146 additions & 0 deletions lib/kube/proxy/forwarder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,19 @@ func TestAuthenticate(t *testing.T) {
ClusterName: "local",
CachingAuthClient: ap,
},
getKubernetesServersForKubeCluster: func(ctx context.Context, name string) ([]types.KubeServer, error) {
servers, err := ap.GetKubernetesServers(ctx)
if err != nil {
return nil, err
}
var filtered []types.KubeServer
for _, server := range servers {
if server.GetCluster().GetName() == name {
filtered = append(filtered, server)
}
}
return filtered, nil
},
}

const remoteAddr = "user.example.com"
Expand Down Expand Up @@ -215,6 +228,21 @@ func TestAuthenticate(t *testing.T) {
name: "local",
remoteAddr: *utils.MustParseAddr(remoteAddr),
},
kubeServers: newKubeServersFromKubeClusters(
t,
&types.KubernetesClusterV3{
Metadata: types.Metadata{
Name: "local",
Labels: map[string]string{
"static_label1": "static_value1",
"static_label2": "static_value2",
},
},
Spec: types.KubernetesClusterSpecV3{
DynamicLabels: map[string]types.CommandLabelV2{},
},
},
),
},
},
{
Expand All @@ -238,6 +266,30 @@ func TestAuthenticate(t *testing.T) {
DynamicLabels: map[string]types.CommandLabelV2{},
},
},
&types.KubernetesClusterV3{
Metadata: types.Metadata{
Name: "foo",
Labels: map[string]string{
"static_label1": "static_value1",
"static_label2": "static_value2",
},
},
Spec: types.KubernetesClusterSpecV3{
DynamicLabels: map[string]types.CommandLabelV2{},
},
},
&types.KubernetesClusterV3{
Metadata: types.Metadata{
Name: "bar",
Labels: map[string]string{
"static_label1": "static_value1",
"static_label2": "static_value2",
},
},
Spec: types.KubernetesClusterSpecV3{
DynamicLabels: map[string]types.CommandLabelV2{},
},
},
),
wantCtx: &authContext{
kubeUsers: utils.StringsSet([]string{"user-a"}),
Expand All @@ -252,6 +304,21 @@ func TestAuthenticate(t *testing.T) {
name: "local",
remoteAddr: *utils.MustParseAddr(remoteAddr),
},
kubeServers: newKubeServersFromKubeClusters(
t,
&types.KubernetesClusterV3{
Metadata: types.Metadata{
Name: "local",
Labels: map[string]string{
"static_label1": "static_value1",
"static_label2": "static_value2",
},
},
Spec: types.KubernetesClusterSpecV3{
DynamicLabels: map[string]types.CommandLabelV2{},
},
},
),
},
},
{
Expand Down Expand Up @@ -284,6 +351,18 @@ func TestAuthenticate(t *testing.T) {
name: "local",
remoteAddr: *utils.MustParseAddr(remoteAddr),
},
kubeServers: newKubeServersFromKubeClusters(
t,
&types.KubernetesClusterV3{
Metadata: types.Metadata{
Name: "local",
Labels: map[string]string{},
},
Spec: types.KubernetesClusterSpecV3{
DynamicLabels: map[string]types.CommandLabelV2{},
},
},
),
},
},
{
Expand Down Expand Up @@ -315,6 +394,19 @@ func TestAuthenticate(t *testing.T) {
name: "local",
remoteAddr: *utils.MustParseAddr(remoteAddr),
},

kubeServers: newKubeServersFromKubeClusters(
t,
&types.KubernetesClusterV3{
Metadata: types.Metadata{
Name: "local",
Labels: map[string]string{},
},
Spec: types.KubernetesClusterSpecV3{
DynamicLabels: map[string]types.CommandLabelV2{},
},
},
),
},
},
{
Expand Down Expand Up @@ -347,6 +439,18 @@ func TestAuthenticate(t *testing.T) {
name: "local",
remoteAddr: *utils.MustParseAddr(remoteAddr),
},
kubeServers: newKubeServersFromKubeClusters(
t,
&types.KubernetesClusterV3{
Metadata: types.Metadata{
Name: "local",
Labels: map[string]string{},
},
Spec: types.KubernetesClusterSpecV3{
DynamicLabels: map[string]types.CommandLabelV2{},
},
},
),
},
},
{
Expand Down Expand Up @@ -448,6 +552,18 @@ func TestAuthenticate(t *testing.T) {
name: "local",
remoteAddr: *utils.MustParseAddr(remoteAddr),
},
kubeServers: newKubeServersFromKubeClusters(
t,
&types.KubernetesClusterV3{
Metadata: types.Metadata{
Name: "local",
Labels: map[string]string{},
},
Spec: types.KubernetesClusterSpecV3{
DynamicLabels: map[string]types.CommandLabelV2{},
},
},
),
},
},
{
Expand Down Expand Up @@ -496,6 +612,18 @@ func TestAuthenticate(t *testing.T) {
name: "local",
remoteAddr: *utils.MustParseAddr(remoteAddr),
},
kubeServers: newKubeServersFromKubeClusters(
t,
&types.KubernetesClusterV3{
Metadata: types.Metadata{
Name: "local",
Labels: map[string]string{},
},
Spec: types.KubernetesClusterSpecV3{
DynamicLabels: map[string]types.CommandLabelV2{},
},
},
),
},
},
{
Expand Down Expand Up @@ -554,6 +682,21 @@ func TestAuthenticate(t *testing.T) {
name: "local",
remoteAddr: *utils.MustParseAddr(remoteAddr),
},
kubeServers: newKubeServersFromKubeClusters(
t,
&types.KubernetesClusterV3{
Metadata: types.Metadata{
Name: "foo",
Labels: map[string]string{
"static_label1": "static_value1",
"static_label2": "static_value2",
},
},
Spec: types.KubernetesClusterSpecV3{
DynamicLabels: map[string]types.CommandLabelV2{},
},
},
),
},
},
{
Expand Down Expand Up @@ -953,6 +1096,9 @@ func TestNewClusterSessionDirect(t *testing.T) {
f.cfg.CachingAuthClient = mockAccessPoint{
kubeServers: []types.KubeServer{publicKubeService, otherKubeService, tunnelKubeService, otherKubeService},
}

authCtx.kubeServers, err = f.cfg.CachingAuthClient.GetKubernetesServers(context.Background())
require.NoError(t, err)
sess, err := f.newClusterSession(authCtx)
require.NoError(t, err)
require.Equal(t, []kubeClusterEndpoint{publicEndpoint, tunnelEndpoint}, sess.kubeClusterEndpoints)
Expand Down
Loading

0 comments on commit c79d79a

Please sign in to comment.