Sig-Auth Pre-Acceptance 2nd Review

[ibihim]

What

Issues collected during the second round of review with @enj.

Why

kube-rbac-proxy should become a project owned by Kubernetes.

Issues

cmd/kube-rbac-proxy/app/kube-rbac-proxy.go:

• (1) Log deprecation warnings link.
• (2) Rename to ApplyTo to conform naming convention link.
• (3) Replace func with cobra.Args link.
• (4) Run should run with a final config version. Complete should finish the necessary changes and those shouldn't happen anymore in Run. link.
• (5) Use context from Signals. link.
• (6) Wrapper to highlight tokens being audience agnostic link.
• (7) Don't use deprecated Director func link.
• (8) Verify that custom Connection headers can't drop headers link.
• (9) H2C is fine as plaintext if listening to 127.0.0.1 / ::1 or UDS link.
• (10) Describe what an empty request info factory means link.
• (11) Use safe wait group from k/k instead of run.Group link
• (12) Avoid nested code blocks link
• (13) Comment why we have two listeners on different ports link
• (14) Validate if a simple response by the health endpoint is enough to proof healthiness link
• (15) Add serverconfig.SetupSignalContext lin.
• (16) Log error of serverCtxCancel link.
• (17) Authorizers should only be added, when used link.
• (18) Verify that Rewrite Attributes is definitely turned off if not configured link.
• (19) Discuss in comment, why regular path k/k authorizer can't be used link.
• (20) What should happen if an Authorizer errs? link.
  • Maybe fail? link.

cmd/kube-rbac-proxy/app/options/legacyoptions.go:

• (21) Add whitespace link.
• (22) Remove validation or add validation link.
• (23) Rename to ApplyTo due to naming conventions link.
• (24) Same: why do we have empty functions? link.

cmd/kube-rbac-proxy/app/options/proxyoptions.go:

• (25) Rename config file name to authz config filename as it is specific to authz link.
• (26) Add whitespace link.
• (27) Reject tokens targeting api server at least link.
• (28) Force a manual opt-in for legacy SA token? link
• (29) Doesn't make sense with OIDC link.

pkg/authn/config.go:

• (30) What does this do? link.

pkg/authn/identityheaders/identityheaders.go:

• (31) Allow authHeaders only with mTLS or 127.0.0.1/::1 link.
• (32) If !ok err-out link.
• (33) Several header entries for groups, such that group separator can be used in group name link.

pkg/authorization/path/path.go:

• (34) Better NewDenyPathAuthorizerUnlessAllowed name? link
• (35) Just use authorizationpath.NewAuthorizer link

pkg/authorization/rewrite/rewrite.go:

• (36) Use dedicated type link.
• (37) Better link.
• (38) Emphasize AND logic link.
• (39) Better link.
• (40) Not used? link
• (41) Is a massive mutation of the authorizer.Attributes intended? Why? link.
• (42) Describe that this turns dynamic path authz checks into static resource checks link.
• (43) Verify and parse templates on process startup link.
• (44) Use sync.Pool instead? link
• (45) Noop on nil link.
• (46) Comment why we re-write untrusted (pre-authn/authz) requests link.
• (47) Collapse logic into a custom request info parser, if possible link.
• (48) If only one header is supported, ensure to fail on more than one link.
• (49) Quit early on no params link.

pkg/authorization/static/static.go:

• (50) Seems to be unused link.
• (51) Return interface instead of unexported struct link.

pkg/server/config.go:

• (52) Check conditional statement on ForceAttemptHTTP2 link.

Epic

• (53) Should we have rewrites in general link OR
• (54) several SAR requests in particular per incoming request OR
• (55) support only header based config link OR
• (56) have a dedicated promtheus solution link OR
• (57) highlight as a very scary config link.

[liouk]

@ibihim the code related to (2) appears to have changed since the pre-acceptance review as it's no longer on brancz:sig-auth-acceptance. Could you double-check whether it still applies please?

[liouk]

@ibihim (12) seems to not apply anymore; nesting has been simplified in the latest version in brancz:sig-auth-acceptance. Could you please verify?

[ibihim]

@ibihim the code related to (2) appears to have changed since the pre-acceptance review as it's no longer on brancz:sig-auth-acceptance. Could you double-check whether it still applies please?

If there are no options, that have ConvertToNewOptions insterad of ApplyTo, you can check it off.

[ibihim]

@ibihim (12) seems to not apply anymore; nesting has been simplified in the latest version in brancz:sig-auth-acceptance. Could you please verify?

Yes, the nesting seems to have been gone. We should keep an eye open, in case we stumble into it somewhere else / try to avoid doing it.

Feel free to check it off.

[liouk]

@ibihim link for 17 points to the task/link for 16 -- does 17 maybe refer to this block instead?

[liouk]

FYI I do not have permissions to edit the issue description therefore I can't tick off any checkboxes :/

[ibihim]

@ibihim link for 17 points to the task/link for 16 -- does 17 maybe refer to this block instead?

Yes, I think so. Sorry. The markdown with all the hash-links makes it super hard to read.

[ibihim]

FYI I do not have permissions to edit the issue description therefore I can't tick off any checkboxes :/

You can enumerate the tasks you've done in a comment.
I assigned the issue to both of us, but I assume that won't suffice.
I have limited permission on this repository too, so I can't edit permissions.

[liouk]

List of issues I'm working on and their status:

cmd/kube-rbac-proxy/app/kube-rbac-proxy.go:

• (2) Rename to ApplyTo to conform naming convention link.
  • not applicable anymore; has already been done
• (3) Replace func with cobra.Args link.
• (4) Run should run with a final config version. Complete should finish the necessary changes and those shouldn't happen anymore in Run. link.
• (5) Use context from Signals. link.
• (10) Describe what an empty request info factory means link.
• (11) Use safe wait group from k/k instead of run.Group link
• (12) Avoid nested code blocks link
  • not applicable anymore; nesting has been already removed
• (13) Comment why we have two listeners on different ports link
• (15) Add serverconfig.SetupSignalContext link.
• (17) Authorizers should only be added, when used link.
• (18) Verify that Rewrite Attributes is definitely turned off if not configured link.

I'll keep this comment up-to-date to help coordination.

The issues in this comment are addressed in the following PR:

• Minor improvements in cmd/kube-rbac-proxy #243

[liouk]

Also working on the following issues:

• (1) Log deprecation warnings link.
• (7) Don't use deprecated Director func link.
• (14) Validate if a simple response by the health endpoint is enough to proof healthiness link
• (19) Discuss in comment, why regular path k/k authorizer can't be used link.