feat(kuma-cp) verify ServiceAccountToken bound to a Pod #2745
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #2745 +/- ##
==========================================
- Coverage 51.99% 51.98% -0.01%
==========================================
Files 877 877
Lines 51142 51162 +20
==========================================
+ Hits 26589 26596 +7
- Misses 22437 22449 +12
- Partials 2116 2117 +1
Continue to review full report at Codecov.
|
jpeach
approved these changes
Sep 8, 2021
| @@ -57,6 +58,12 @@ func WithStatefulSet(apply bool) DeploymentOptsFn { | |||
| } | |||
| } | |||
|
|
|||
| func WithServiceAccount(sa string) DeploymentOptsFn { | |||
There was a problem hiding this comment.
nit:
Suggested change
| func WithServiceAccount(sa string) DeploymentOptsFn { | |
| func WithServiceAccount(accountName string) DeploymentOptsFn { |
pkg/xds/auth/k8s/authenticator.go
Outdated
| Namespace: namespace, | ||
| Name: name, | ||
| }, pod); err != nil { | ||
| return "", errors.Wrap(err, "could not retrieve Pod to verify identity of a dataplane proxy") |
There was a problem hiding this comment.
Nit: might be useful to include the namespaced name in the error
pkg/xds/auth/k8s/authenticator.go
Outdated
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (k *kubeAuthenticator) podServiceAccountName(ctx context.Context, name, namespace string) (string, error) { |
There was a problem hiding this comment.
Nit:
Suggested change
| func (k *kubeAuthenticator) podServiceAccountName(ctx context.Context, name, namespace string) (string, error) { | |
| func (k *kubeAuthenticator) podServiceAccountName(ctx context.Context, podName types.NamespacedName) (string, error) { |
pkg/xds/auth/k8s/authenticator.go
Outdated
| } | ||
| namespace := userInfo[2] | ||
| if namespace != proxyNamespace { | ||
| return errors.Errorf("authentication failed: token belongs to a namespace (%q) different from proxyId (%q)", namespace, proxyNamespace) | ||
| return errors.Errorf("token belongs to a namespace (%q) different from proxyId (%q)", namespace, proxyNamespace) |
There was a problem hiding this comment.
Nit: the parentheses in this error seem a bit out of place since no other error format uses them
pkg/xds/auth/k8s/authenticator.go
Outdated
| _, proxyNamespace, err := util_k8s.CoreNameToK8sName(dataplane.Meta.GetName()) | ||
| if err != nil { | ||
| return err | ||
| return errors.Errorf("token must belong to a k8s system account, got %q", tokenReview.Status.User.Username) |
There was a problem hiding this comment.
Suggested change
| return errors.Errorf("token must belong to a k8s system account, got %q", tokenReview.Status.User.Username) | |
| return errors.Errorf("user %q is not a service account", tokenReview.Status.User.Username) |
pkg/xds/auth/k8s/authenticator.go
Outdated
| } | ||
| serviceAccountName, err := k.podServiceAccountName(ctx, proxyName, proxyNamespace) | ||
| if err != nil { | ||
| return errors.Wrap(err, "could not retrieve Pods Service Account to verify identity of a dataplane proxy") |
There was a problem hiding this comment.
This error message substantially repeats the one in podServiceAccountName. Maybe trim or or the other?
Same in authZoneIngress.
…dual-satoken Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
mergify bot
pushed a commit
that referenced
this pull request
Sep 13, 2021
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com> (cherry picked from commit 0b2a7bf)
jakubdyszkiewicz
pushed a commit
that referenced
this pull request
Sep 13, 2021
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
nikita15p
pushed a commit
to nikita15p/kuma
that referenced
this pull request
Sep 28, 2021
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
nikita15p
pushed a commit
to nikita15p/kuma
that referenced
this pull request
Sep 28, 2021
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
When authenticating kuma-dp on Kubernetes, we are carrying ServiceAccountToken and verify it in Kuma CP.
So far, we were only verifying the namespace of this Service Account Token but not the name of the token itself.
That means that any kuma-dp in one namespace could impersonate others in the same namespace.
I think this was done this way because if as a team member you manage Deployment and ServiceAccount objects in one namespace. There is no built-in mechanism in Kubernetes to prevent a user from using another ServiceAccount in a namespace for your Deployment.
However, there are cases where users are not responsible for managing Deployment objects but some high-level provisioner does that for them.
Alternatively, you could have a controller or OPA for Kubernetes that would restrict ServiceAccountToken to given Deployment.
Therefore Kuma should be more strict about verifying the token to not only verify a namespace of a token but check the Pod for the
serviceAccountTokenNameand also verify a token name.Issues resolved
No issues.
Documentation
Testing
Backwards compatibility
backport-to-stablelabel if the code is backwards compatible. Otherwise, list breaking changes.