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

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

Fixes #336

Signed-off-by: kramaranya <kramaranya15@gmail.com>

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.

stlaz · 2025-02-18T14:41:10Z

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

@@ -17,6 +17,7 @@ limitations under the License.
 package options

 import (
+	"github.com/brancz/kube-rbac-proxy/pkg/authn/identityheaders"


this belongs in the last import group below

stlaz · 2025-02-18T14:54:07Z

README.md

      --allow-paths strings                         Comma-separated list of paths against which kube-rbac-proxy pattern-matches the incoming request. If the request doesn't match, kube-rbac-proxy responds with a 404 status code. If omitted, the incoming request path isn't checked. Cannot be used with --ignore-paths.
      --auth-header-groups-field-name string        The name of the field inside a http(2) request header to tell the upstream server about the user's groups (default "x-remote-groups")
      --auth-header-groups-field-separator string   The separator string used for concatenating multiple group names in a groups header field's value (default "|")
      --auth-header-user-field-name string          The name of the field inside a http(2) request header to tell the upstream server about the user's name (default "x-remote-user")
-      --auth-token-audiences strings                Comma-separated list of token audiences to accept. By default a token does not have to have any specific audience. It is recommended to set a specific audience.
+      --auth-token-audiences strings                Comma-separated list of token audiences to accept. Tokens must have at least one audience from this list. If omitted, the token is considered legacy.


Suggested change

--auth-token-audiences strings Comma-separated list of token audiences to accept. Tokens must have at least one audience from this list. If omitted, the token is considered legacy.

--auth-token-audiences strings Comma-separated list of token audiences to accept. Tokens must have at least one audience from this list. Must be set unless --allow-legacy-serviceaccount-tokens is true.

stlaz · 2025-02-18T14:58:10Z

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

+	userKey := "User"
+	groupKey := "Group"


should be const

stlaz · 2025-02-18T14:59:05Z

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

+		Upstream                        string
+		UpstreamForceH2C                bool
+		UpstreamCAFile                  string
+		UpstreamClientCertFile          string
+		UpstreamClientKeyFile           string
+		UpstreamHeader                  *identityheaders.AuthnHeaderConfig
+		AuthzConfigFileName             string
+		AllowPaths                      []string
+		IgnorePaths                     []string
+		ProxyEndpointsPort              int
+		TokenAudiences                  []string
+		AllowLegacyServiceAccountTokens bool
+		DisableHTTP2Serving             bool


Remove fields that are currently untested. Or, preferrably, write tests for the rest of the fields 🙂

Sure, I've added additional test cases to cover those fields :)

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

stlaz · 2025-02-27T11:24:32Z

test/e2e/allowpaths/deployment.yaml

@@ -22,6 +22,7 @@ spec:
            - "--proxy-endpoints-port=8643"
            - "--upstream=http://127.0.0.1:8081/"
            - "--allow-paths=/metrics,/api/v1/label/*"
+            - "--allow-legacy-serviceaccount-tokens=true"


Please also add e2e tests for scenarios that actually use the legacy token according to #357 (review)

oops completely forgot about this comment. will do, thanks for the reminder!

Signed-off-by: kramaranya <kramaranya15@gmail.com>

kramaranya added 2 commits February 10, 2025 13:48

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

57f326b

Signed-off-by: kramaranya <kramaranya15@gmail.com>

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

a9c2ea9

Signed-off-by: kramaranya <kramaranya15@gmail.com>

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

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

a8e9da6

Signed-off-by: kramaranya <kramaranya15@gmail.com>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kramaranya commented Feb 4, 2025

kramaranya commented Feb 10, 2025

ibihim left a comment •

edited

Loading

ibihim commented Feb 13, 2025

ibihim commented Feb 14, 2025

stlaz left a comment

stlaz Feb 18, 2025

stlaz Feb 18, 2025

stlaz Feb 18, 2025

stlaz Feb 18, 2025

kramaranya Feb 26, 2025

kramaranya commented Feb 26, 2025

stlaz Feb 27, 2025 •

edited

Loading

kramaranya Feb 27, 2025

	--auth-token-audiences strings Comma-separated list of token audiences to accept. Tokens must have at least one audience from this list. If omitted, the token is considered legacy.
	--auth-token-audiences strings Comma-separated list of token audiences to accept. Tokens must have at least one audience from this list. Must be set unless --allow-legacy-serviceaccount-tokens is true.

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

Conversation

kramaranya commented Feb 4, 2025

kramaranya commented Feb 10, 2025

ibihim left a comment • edited Loading

Choose a reason for hiding this comment

ibihim commented Feb 13, 2025

ibihim commented Feb 14, 2025

stlaz left a comment

Choose a reason for hiding this comment

stlaz Feb 18, 2025

Choose a reason for hiding this comment

stlaz Feb 18, 2025

Choose a reason for hiding this comment

stlaz Feb 18, 2025

Choose a reason for hiding this comment

stlaz Feb 18, 2025

Choose a reason for hiding this comment

kramaranya Feb 26, 2025

Choose a reason for hiding this comment

kramaranya commented Feb 26, 2025

stlaz Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

kramaranya Feb 27, 2025

Choose a reason for hiding this comment

ibihim left a comment •

edited

Loading

stlaz Feb 27, 2025 •

edited

Loading