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 cd687a4 commit b0f81d2
Show file tree
Hide file tree
Showing 7 changed files with 262 additions and 46 deletions.
2 changes: 1 addition & 1 deletion lib/auth/permissions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -682,7 +682,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
59 changes: 33 additions & 26 deletions lib/kube/proxy/forwarder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -308,8 +308,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 @@ -340,6 +351,9 @@ type authContext struct {
certExpires time.Time
// sessionTTL specifies the duration of the user's session
sessionTTL time.Duration
// 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 @@ -598,7 +612,9 @@ func (f *Forwarder) setupContext(authCtx auth.Context, req *http.Request, isRemo
}

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 @@ -615,14 +631,20 @@ func (f *Forwarder) setupContext(authCtx auth.Context, req *http.Request, isRemo
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)
kubeAccessDetails, err := f.getKubeAccessDetails(kubeServers, roles, kubeCluster, sessionTTL)
if err != nil && !trace.IsNotFound(err) {
return nil, trace.Wrap(err)
// roles.CheckKubeGroupsAndUsers returns trace.NotFound if the user does
Expand Down Expand Up @@ -738,6 +760,7 @@ func (f *Forwarder) setupContext(authCtx auth.Context, req *http.Request, isRemo
isRemote: isRemoteCluster,
isRemoteClosed: isRemoteClosed,
},
kubeServers: kubeServers,
}, nil
}

Expand All @@ -753,15 +776,11 @@ 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,
) (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 @@ -804,23 +823,20 @@ 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)
}
ap, err := f.cfg.CachingAuthClient.GetAuthPreference(ctx)

authPref, err := f.cfg.CachingAuthClient.GetAuthPreference(ctx)
if err != nil {
return trace.Wrap(err)
}

mfaParams := actx.MFAParams(ap.GetRequireMFAType())
mfaParams := actx.MFAParams(authPref.GetRequireMFAType())

// Check authz against the first match.
//
// We assume that users won't register two identically-named clusters with
// mis-matched labels. If they do, expect weirdness.
clusterNotFound := trace.AccessDenied("kubernetes cluster %q not found", actx.kubeCluster)
for _, s := range servers {
for _, s := range actx.kubeServers {
ks := s.GetCluster()
if ks.GetName() != actx.kubeCluster {
continue
Expand All @@ -830,7 +846,6 @@ func (f *Forwarder) authorize(ctx context.Context, actx *authContext) error {
return clusterNotFound
}
return nil

}
if actx.kubeCluster == f.cfg.ClusterName {
f.log.WithField("auth_context", actx.String()).Debug("Skipping authorization for proxy-based kubernetes cluster,")
Expand Down Expand Up @@ -1865,11 +1880,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.kubeCluster == ctx.teleportCluster.name {
return nil, trace.Wrap(localErr)
}
Expand Down Expand Up @@ -1898,12 +1909,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.kubeCluster]
if !ok {
details, err := f.findKubeDetailsByClusterName(ctx.kubeCluster)
if err != nil {
return nil, trace.NotFound("kubernetes cluster %q not found", ctx.kubeCluster)
}

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 @@ -157,6 +157,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 @@ -214,6 +227,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 @@ -237,6 +265,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 @@ -251,6 +303,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 @@ -283,6 +350,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 @@ -314,6 +393,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 @@ -346,6 +438,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 @@ -447,6 +551,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 @@ -495,6 +611,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 @@ -553,6 +681,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 @@ -952,6 +1095,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 b0f81d2

Please sign in to comment.