Add manual opt-in for legacy service account tokens #357

kramaranya · 2025-02-04T12:09:05Z

Fixes #336

kramaranya · 2025-02-10T13:54:39Z

@stlaz @ibihim could you please review this pr?

ibihim

The unit tests might not have been necessary, but fixing the e2e-tests would be crucial :)

You also need to regenerate the README.md with the make generate.

ibihim · 2025-02-13T16:51:04Z

/lgtm

ibihim · 2025-02-14T13:46:41Z

@kramaranya, I think we have an audience token test, right? we might be able to drop it, if we do not have any tests without audience set, right? Maybe we could have a negative test case then, that the server fails if it isn't set?

stlaz

Will legacy SA tokens be denied when the new flag is unset but audiences are set?
Will they be accepted if both are set?

Please add e2e tests for all possible scenarios, I don't think I've found legacy SA tokens creation in the e2e tests.

cmd/kube-rbac-proxy/app/options/proxyoptions_test.go

README.md

cmd/kube-rbac-proxy/app/options/proxyoptions_test.go

kramaranya · 2025-02-26T13:07:08Z

@kramaranya, I think we have an audience token test, right? we might be able to drop it, if we do not have any tests without audience set, right? Maybe we could have a negative test case then, that the server fails if it isn't set?

@ibihim If I understood you correctly, I've already added a negative test for this scenario (empty audiences when legacy tokens are not allowed) -- https://github.com/brancz/kube-rbac-proxy/pull/357/files#diff-770d985644314c26e2d1c0e8fb70e4714408a9888a4e2749a633144697201bacR191-R203

test/e2e/allowpaths/deployment.yaml

stlaz

We'll have to create the legacy SA token separately

stlaz · 2025-03-10T11:59:42Z

test/e2e/basics.go

+				),
+			),
+			Then: kubetest.Actions(
+				kubetest.ClientSucceeds(


You're attempting to use a legacy token but the proxy does is not supposed to accept these in this case. This should fail.

stlaz · 2025-03-10T12:01:46Z

test/e2e/basics.go

+				kubetest.ClientSucceeds(
+					client,
+					legacyCommand,
+					&kubetest.RunOptions{TokenAudience: "kube-rbac-proxy"},


we don't need the options in this case, right?

stlaz · 2025-03-10T12:14:51Z

test/e2e/basics.go

+				kubetest.ClientSucceeds(
+					client,
+					command,
+					&kubetest.RunOptions{TokenAudience: ""},


This is equal to it being nil, right?

stlaz · 2025-03-10T12:23:18Z

test/e2e/basics.go

+
+func testLegacyTokenFlag(client kubernetes.Interface) kubetest.TestSuite {
+	return func(t *testing.T) {
+		legacyCommand := `curl --connect-timeout 5 -v -s -k --fail -H "Authorization: Bearer $(cat /var/run/secrets/tokens/requestedtoken)" https://kube-rbac-proxy.default.svc.cluster.local:8443/metrics`


/var/run/secrets/tokens/requestedtoken does not actually house the legacy tokens. The below code appears to be creating such tokens:

kube-rbac-proxy/test/kubetest/kubernetes.go

Lines 524 to 544 in 670377f

if opts != nil && opts.TokenAudience != "" {

pod.Spec.Volumes = append(pod.Spec.Volumes, corev1.Volume{

Name: "requestedtoken",

VolumeSource: corev1.VolumeSource{

Projected: &corev1.ProjectedVolumeSource{

Sources: []corev1.VolumeProjection{{

ServiceAccountToken: &corev1.ServiceAccountTokenProjection{

Audience: opts.TokenAudience,

Path: "requestedtoken",

},

}},

},

},

})

pod.Spec.Containers[0].VolumeMounts = append(pod.Spec.Containers[0].VolumeMounts,

corev1.VolumeMount{

Name: "requestedtoken",

MountPath: "/var/run/secrets/tokens",

},

)

}

We may actually want to rename the directory from /var/run/secrets/tokens to something else, like /var/run/projected/tokens, I personally find it a bit confusing.

You'll have to follow https://kubernetes.io/docs/concepts/configuration/secret/#serviceaccount-token-secrets to create a token secret along with your other manifests. Use a different directory than for the short-lived SA tokens so that we can distinguish these two in tests.

stlaz · 2025-03-10T12:28:20Z

test/e2e/basics.go

+func testLegacyTokenFlag(client kubernetes.Interface) kubetest.TestSuite {
+	return func(t *testing.T) {
+		legacyCommand := `curl --connect-timeout 5 -v -s -k --fail -H "Authorization: Bearer $(cat /var/run/secrets/tokens/requestedtoken)" https://kube-rbac-proxy.default.svc.cluster.local:8443/metrics`
+		command := `curl --connect-timeout 5 -v -s -k --fail -H "Authorization: Bearer $(cat /var/run/secrets/kubernetes.io/serviceaccount/token)" https://kube-rbac-proxy.default.svc.cluster.local:8443/metrics`


I think this command should actually have /var/run/secrets/tokens/requestedtoken as an input for the token

The LegacyAllowedNoAudience test checks a scenario with no audience set. But /var/run/secrets/tokens/requestedtoken sets an audience, so it wouldn't match our no-audience case.

kube-rbac-proxy/test/kubetest/kubernetes.go

Lines 524 to 532 in 670377f

if opts != nil && opts.TokenAudience != "" {

pod.Spec.Volumes = append(pod.Spec.Volumes, corev1.Volume{

Name: "requestedtoken",

VolumeSource: corev1.VolumeSource{

Projected: &corev1.ProjectedVolumeSource{

Sources: []corev1.VolumeProjection{{

ServiceAccountToken: &corev1.ServiceAccountTokenProjection{

Audience: opts.TokenAudience,

Path: "requestedtoken",

That's why I use the default token /var/run/secrets/kubernetes.io/serviceaccount/token instead.

The /var/run/secrets/kubernetes.io/serviceaccount/token will also contain an audience.

Signed-off-by: kramaranya <[email protected]>

stlaz · 2025-05-26T12:14:34Z

test/e2e/legacy/audience/deployment-allowed.yaml

+          args:
+            - "--secure-port=8443"
+            - "--upstream=http://127.0.0.1:8081/"
+            - "--auth-token-audiences=kube-rbac-proxy,https://kubernetes.default.svc.cluster.local"


I see, I was wondering why the test was passing. This and the below configs were supposed to be the same, the difference should only be in "--allow-legacy-serviceaccount-tokens=true", not in audiences.

https://kubernetes.default.svc.cluster.local is an "implicit" audience, so when you use the legacy SA token, IIRC this is returned when there are no other audiences present. However, this implicit audience can be different depending on kube-apiserver configuration, and so we couldn't make a simple guidance on how to config the audiences.

@ibihim Should we treat the --allow-legacy-serviceaccount-tokens that allows empty --auth-token-audiences but will fail if there are audiences set? Or would we actually allow both legacy and bound SA tokens, the latter retains the audience check if audiences are set? Doing that might get unnecessarily complicated.

kramaranya force-pushed the legacy-sa-opt-in branch from ffdc6c1 to a9c2ea9 Compare February 10, 2025 13:52

kramaranya changed the title ~~WIP: Add manual opt-in for legacy service account tokens~~ Add manual opt-in for legacy service account tokens Feb 10, 2025

ibihim reviewed Feb 13, 2025

View reviewed changes

stlaz requested changes Feb 18, 2025

View reviewed changes

kramaranya force-pushed the legacy-sa-opt-in branch from b11a610 to f44889c Compare February 26, 2025 12:54

stlaz reviewed Feb 27, 2025

View reviewed changes

test/e2e/allowpaths/deployment.yaml Show resolved Hide resolved

kramaranya force-pushed the legacy-sa-opt-in branch from f44889c to a8e9da6 Compare March 3, 2025 12:53

stlaz requested changes Mar 10, 2025

View reviewed changes

kramaranya force-pushed the legacy-sa-opt-in branch 2 times, most recently from dcf81c3 to 37debda Compare March 27, 2025 12:35

kramaranya added 3 commits March 27, 2025 12:39

c/k/a/opts: add --allow-legacy-serviceaccount-tokens flag and validation

29fb7fc

Signed-off-by: kramaranya <[email protected]>

c/k/a/opts: add test for legacy serviceaccount token validation

e94690d

Signed-off-by: kramaranya <[email protected]>

README, test/e2e: add --allow-legacy-serviceaccount-tokens flag

6d1f5e5

Signed-off-by: kramaranya <[email protected]>

kramaranya force-pushed the legacy-sa-opt-in branch from 37debda to 6d1f5e5 Compare March 27, 2025 12:41

stlaz added the sig-auth-acceptance issues created during review for sig-auth-acceptance label Apr 8, 2025

stlaz reviewed May 26, 2025

View reviewed changes

	if opts != nil && opts.TokenAudience != "" {
	pod.Spec.Volumes = append(pod.Spec.Volumes, corev1.Volume{
	Name: "requestedtoken",
	VolumeSource: corev1.VolumeSource{
	Projected: &corev1.ProjectedVolumeSource{
	Sources: []corev1.VolumeProjection{{
	ServiceAccountToken: &corev1.ServiceAccountTokenProjection{
	Audience: opts.TokenAudience,
	Path: "requestedtoken",
	},
	}},
	},
	},
	})
	pod.Spec.Containers[0].VolumeMounts = append(pod.Spec.Containers[0].VolumeMounts,
	corev1.VolumeMount{
	Name: "requestedtoken",
	MountPath: "/var/run/secrets/tokens",
	},
	)
	}

Add manual opt-in for legacy service account tokens #357

Are you sure you want to change the base?

Add manual opt-in for legacy service account tokens #357

Uh oh!

Conversation

kramaranya commented Feb 4, 2025

Uh oh!

kramaranya commented Feb 10, 2025

Uh oh!

ibihim left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibihim commented Feb 13, 2025

Uh oh!

ibihim commented Feb 14, 2025

Uh oh!

stlaz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kramaranya commented Feb 26, 2025

Uh oh!

Uh oh!

stlaz left a comment

Choose a reason for hiding this comment

Uh oh!

stlaz Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

stlaz Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

stlaz Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

stlaz Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stlaz Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

kramaranya Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

stlaz Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

stlaz May 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ibihim left a comment •

edited

Loading

stlaz Mar 10, 2025 •

edited

Loading