Skip to content

Fix downgrade logic of KubernetesResources to Role v6#35991

Merged
tigrato merged 1 commit intomasterfrom
tigrato/properly-handle-downgrades
Dec 22, 2023
Merged

Fix downgrade logic of KubernetesResources to Role v6#35991
tigrato merged 1 commit intomasterfrom
tigrato/properly-handle-downgrades

Conversation

@tigrato
Copy link
Copy Markdown
Contributor

@tigrato tigrato commented Dec 21, 2023

KubernetesResources were improperly downgraded when they grant access to all resources.

In that case, the role was downgraded to something that can't be used to access Kubernetes clusters but they could have been downgraded to a Role v6 with the same permissions as the KubernetesResources.

This commit fixes the downgrade logic to downgrade to a Role v6 with the same permissions as the KubernetesResources.

A role v7 with

kubenretes_labels:
 '*': '*'
kubernetes_resources:
- kind: pod
  name: '*'
  namespace: '*'
  verbs:
  - '*'

Is downgraded to a role v6 with

kubenretes_labels:
 '*': '*'
kubernetes_resources:
- kind: pod
  name: '*'
  namespace: '*'

Changelog: Fix downgrade logic of KubernetesResources to Role v6

@github-actions
Copy link
Copy Markdown
Contributor

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

Comment thread lib/auth/grpcserver.go Outdated
Comment thread lib/auth/grpcserver.go Outdated
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.

I'd make it more explicit here

Suggested change
!slices.Equal(resource.Verbs, []string{types.Wildcard}) {
(len(resource.Verbs) != 1 || resource.Verbs[0] != types.Wildcard) {

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.

(what if there's two verbs and they're both "*" tho?)

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.

You can't. CheckAndSetDefaults prevents you from adding * and any other verb

`KubernetesResources` were improperly downgraded when they grant access
to all resources.

In that case, the role was downgraded to something that can't be used to
access Kubernetes clusters but they could have been downgraded to a
`Role` v6 with the same permissions as the `KubernetesResources`.

This commit fixes the downgrade logic to downgrade to a `Role` v6 with
the same permissions as the `KubernetesResources`.

A role v7 with

```json
kubenretes_labels:
 '*': '*'
kubernetes_resources:
- kind: pod
  name: '*'
  namespace: '*'
  verbs:
  - '*'
```
Is downgraded to a role v6 with

```json
kubenretes_labels:
 '*': '*'
kubernetes_resources:
- kind: pod
  name: '*'
  namespace: '*'
```

Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>
@tigrato tigrato force-pushed the tigrato/properly-handle-downgrades branch from 5c038a2 to 34a3942 Compare December 22, 2023 12:45
@tigrato tigrato enabled auto-merge December 22, 2023 12:45
@tigrato tigrato added this pull request to the merge queue Dec 22, 2023
Merged via the queue into master with commit df0adf0 Dec 22, 2023
@tigrato tigrato deleted the tigrato/properly-handle-downgrades branch December 22, 2023 13:21
@public-teleport-github-review-bot
Copy link
Copy Markdown

@tigrato See the table below for backport results.

Branch Result
branch/v14 Failed

tigrato added a commit that referenced this pull request Dec 22, 2023
`KubernetesResources` were improperly downgraded when they grant access
to all resources.

In that case, the role was downgraded to something that can't be used to
access Kubernetes clusters but they could have been downgraded to a
`Role` v6 with the same permissions as the `KubernetesResources`.

This commit fixes the downgrade logic to downgrade to a `Role` v6 with
the same permissions as the `KubernetesResources`.

A role v7 with

```json
kubenretes_labels:
 '*': '*'
kubernetes_resources:
- kind: pod
  name: '*'
  namespace: '*'
  verbs:
  - '*'
```
Is downgraded to a role v6 with

```json
kubenretes_labels:
 '*': '*'
kubernetes_resources:
- kind: pod
  name: '*'
  namespace: '*'
```

Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>
github-merge-queue Bot pushed a commit that referenced this pull request Dec 22, 2023
…36009)

`KubernetesResources` were improperly downgraded when they grant access
to all resources.

In that case, the role was downgraded to something that can't be used to
access Kubernetes clusters but they could have been downgraded to a
`Role` v6 with the same permissions as the `KubernetesResources`.

This commit fixes the downgrade logic to downgrade to a `Role` v6 with
the same permissions as the `KubernetesResources`.

A role v7 with

```json
kubenretes_labels:
 '*': '*'
kubernetes_resources:
- kind: pod
  name: '*'
  namespace: '*'
  verbs:
  - '*'
```
Is downgraded to a role v6 with

```json
kubenretes_labels:
 '*': '*'
kubernetes_resources:
- kind: pod
  name: '*'
  namespace: '*'
```

Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants