kube-rbac-proxy icon indicating copy to clipboard operation
kube-rbac-proxy copied to clipboard

Published 20 hours ago • verified publisher icon brancz

Sig-Auth Pre-Acceptance 2nd Review

Open ibihim opened this issue 1 year ago • 10 comments

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.
  • [x] (2) Rename to ApplyTo to conform naming convention link.
  • [x] (3) Replace func with cobra.Args link.
  • [x] (4) Run should run with a final config version. Complete should finish the necessary changes and those shouldn't happen anymore in Run. link.
  • [x] (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.
  • [x] (10) Describe what an empty request info factory means link.
  • [x] (11) Use safe wait group from k/k instead of run.Group link
  • [x] (12) Avoid nested code blocks link
  • [x] (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
  • [x] (15) Add serverconfig.SetupSignalContext lin.
  • [ ] (16) Log error of serverCtxCancel link.
  • [x] (17) Authorizers should only be added, when used link.
  • [x] (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:

  • [x] (31) Allow authHeaders only with mTLS or 127.0.0.1/::1 link.
  • [x] (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:

  • [x] (36) Use dedicated type link.
  • [x] (37) Better link.
  • [x] (38) Emphasize AND logic link.
  • [x] (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.
  • [x] (43) Verify and parse templates on process startup link.
  • [ ] (44) Use sync.Pool instead? link
  • [x] (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.
  • [x] (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.

ibihim avatar May 30 '23 13:05 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?

liouk avatar May 31 '23 14:05 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?

liouk avatar Jun 01 '23 08:06 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?

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

ibihim avatar Jun 01 '23 08:06 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.

ibihim avatar Jun 01 '23 08:06 ibihim

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

liouk avatar Jun 01 '23 08:06 liouk

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

liouk avatar Jun 01 '23 08:06 liouk

@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 avatar Jun 01 '23 09:06 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.

ibihim avatar Jun 01 '23 09:06 ibihim

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

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

  • [x] (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
  • [x] (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:

  • #243

liouk avatar Jun 01 '23 09:06 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.

liouk avatar Jun 08 '23 11:06 liouk