Skip to content

Extend kubectl auth can-i support for kubernetes_resources RBAC rules#26425

Merged
tigrato merged 1 commit intomasterfrom
tigrato/change-rolev6-kuberesources-defaults
May 19, 2023
Merged

Extend kubectl auth can-i support for kubernetes_resources RBAC rules#26425
tigrato merged 1 commit intomasterfrom
tigrato/change-rolev6-kuberesources-defaults

Conversation

@tigrato
Copy link
Copy Markdown
Contributor

@tigrato tigrato commented May 17, 2023

This PR extends the Teleport capabilities of kubectl auth can-i to support
evaluation of kubernetes_resources.

It also changes the error message to be clear to the users what's the
cause of the error and adds a warning when users try to edit invalid
roles.

@tigrato tigrato force-pushed the tigrato/change-rolev6-kuberesources-defaults branch 5 times, most recently from 6266007 to 363db21 Compare May 17, 2023 12:56
@tigrato tigrato marked this pull request as ready for review May 17, 2023 13:14
@tigrato tigrato requested a review from r0mant May 17, 2023 13:14
@github-actions github-actions Bot requested review from hugoShaka and strideynet May 17, 2023 13:15
@hugoShaka
Copy link
Copy Markdown
Contributor

hugoShaka commented May 17, 2023

Edit: the rolev5 was fixed, I removed the part about it.

If I understand correctly, before the change:

  • rolev5:
    • if resource restrictions empty -> wildcard in resource restrictions
  • rolev6:
    • if resource restrictions are set we honour them
    • we don't touch resource restrictions -> by default no access

after the change:

  • rolev5:
    • if label set -> wildcard in resource restrictions
  • rolev6:
    • if resource restrictions are set we honour them
    • if no resource restrictions but labels set -> wildcard in resource definition

The following rolev6 which was not providing any access will start to work.

kind: role
metadata:
  name: kube-access
version: v6
spec:
  allow:
    kubernetes_labels:
      'region': '*'
      'platform': 'minikube'
    kubernetes_groups:
    - developers
    kubernetes_users:
    - minikube
  deny: {}

I understand this is intended and provides a better user experience, but this was not a bug and might be a surprise if users who had no access suddenly can hit any resource after a Teleport update.

I think we should not change the v5 behaviour at all, for the v6 behaviour change I would be more comfortable with introducing a v7. It's awkward to have to bump the version again, but as a security company I think we must to strictly follow versioning constraints to avoid exposing our user's infrastructure.

@tigrato
Copy link
Copy Markdown
Contributor Author

tigrato commented May 17, 2023

@hugoShaka the rolev5 was a typo.
The change should only affect rolev6 and by default it will fill the kubernetes_resources in a similar way as rolev5 and rolev4

@tigrato tigrato force-pushed the tigrato/change-rolev6-kuberesources-defaults branch from 4fd16a1 to 4edcb24 Compare May 18, 2023 18:58
@tigrato tigrato changed the title Change defaults for RoleV6's kubernetes_resources Extend kubectl auth can-i support for kubernetes_resources RBAC rules May 18, 2023
@tigrato tigrato force-pushed the tigrato/change-rolev6-kuberesources-defaults branch from 4edcb24 to c453d03 Compare May 18, 2023 19:09
Comment thread lib/kube/proxy/forwarder.go Outdated
Comment thread lib/kube/proxy/self_subject_reviews.go Outdated
Comment thread lib/kube/proxy/self_subject_reviews.go Outdated
Comment thread tool/tctl/common/resource_command.go Outdated
@tigrato tigrato force-pushed the tigrato/change-rolev6-kuberesources-defaults branch 2 times, most recently from 8181658 to f534bfc Compare May 19, 2023 09:15
Comment thread api/types/role.go Outdated
Comment thread api/types/role.go Outdated
Comment thread tool/tctl/common/resource_command.go Outdated
Comment thread integrations/operator/controllers/resources/role_controller_test.go Outdated
Comment thread lib/kube/proxy/forwarder.go Outdated
Comment thread lib/kube/proxy/self_subject_reviews.go Outdated
Comment on lines 206 to 208
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'm not sure this heuristic is robust enough: depluralizeResource("ingress") will have a weird behaviour.

If you don't think we need to fix this now, can you add a warning in types.KubernetesResourcesKinds to ensure we don't introduce support for a resource ending with an s without changing the logic here?

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.

Right now we only support pods and once we support other options I will have a conversion table

@tigrato tigrato force-pushed the tigrato/change-rolev6-kuberesources-defaults branch from f534bfc to 7f67a72 Compare May 19, 2023 09:39
@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from strideynet May 19, 2023 10:14
@hugoShaka
Copy link
Copy Markdown
Contributor

Would you mind adding kubectl auth can-i at the end of the Kubernetes access guide in a subsequent PR (or this one but we'd need to pull docs folks manually)?

…ules

This PR extends Teleport capabilities of `kubectl auth can-i` to support
evaluation of `kubernetes_resources`.

It also changes the error message to be clear to the users what's the
cause of the error and adds a warning when users try to edit invalid
roles.
@tigrato tigrato force-pushed the tigrato/change-rolev6-kuberesources-defaults branch from 7f67a72 to 05e931e Compare May 19, 2023 10:18
@tigrato tigrato enabled auto-merge May 19, 2023 10:18
@tigrato tigrato added this pull request to the merge queue May 19, 2023
Merged via the queue into master with commit df3f5b3 May 19, 2023
@tigrato tigrato deleted the tigrato/change-rolev6-kuberesources-defaults branch May 19, 2023 10:53
@public-teleport-github-review-bot
Copy link
Copy Markdown

@tigrato See the table below for backport results.

Branch Result
branch/v12 Failed
branch/v13 Create PR

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