Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🌱 proxy: Optionally enable token auth #2178

Merged
merged 6 commits into from
Oct 25, 2022

Conversation

hardys
Copy link

@hardys hardys commented Oct 12, 2022

Summary

If we want to use the front-proxy as the gateway to shard APIs, we need to enable more auth methods beyond client cert, e.g for dev/CI we probably want token auth, and for production deployments we probably want to enable OIDC.

This will allow us to reject requests which can't be authenticated before they hit any shard apiserver, and may enable future features such as rate-limiting at the proxy where we need information from authentication.

For testing the first step is to enable token auth, then we can test multiple users etc in the same way existing e2e tests do, e.g for development/CI we can now do this:

go run ./cmd/sharded-test-server --proxy-token-auth-file=$PWD/test/e2e/framework/auth-tokens.csv

In future we can easily add e.g OIDC and any other auth methods supported by the kubeapiserver code

pkg/proxy/filters/filters.go Outdated Show resolved Hide resolved
@hardys
Copy link
Author

hardys commented Oct 12, 2022

/cc @sttts we discussed this recently and I added some questions, feedback welcome!

This is just a first step to enable multi-user dev-testing via the proxy, we can easily add other proxy auth methods e.g OIDC now this is refactored to use NewBuiltInAuthenticationOptions

@hardys hardys requested a review from sttts October 12, 2022 15:37
@hardys
Copy link
Author

hardys commented Oct 12, 2022

/hold pending discussion re questions and e2e-sharded is not passing

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 12, 2022
@hardys
Copy link
Author

hardys commented Oct 13, 2022

The e2e-sharded failure is real - it's because the e2e test enables token auth at the shard, but with this PR we'll need to also enable the same token file at the front proxy (since Authorization: Bearer requests are now authenticated at the proxy, not passed through to the shard unauthenticated)

@hardys hardys force-pushed the proxy_auth_methods2 branch 2 times, most recently from 8c63d50 to 11d93ae Compare October 13, 2022 18:24
@hardys
Copy link
Author

hardys commented Oct 13, 2022

/retitle 🌱 proxy: Optionally enable token auth

@openshift-ci openshift-ci bot changed the title 🌱 proxy: Enable token auth 🌱 proxy: Optionally enable token auth Oct 13, 2022
@hardys
Copy link
Author

hardys commented Oct 14, 2022

/retitle [WIP] seedling proxy: Optionally enable token auth

Marking this WIP - the e2e-sharded job is still failing, but for a different reason.

The syncer in the e2e tests uses a bearer token that can't be authenticated via the token-auth-file - I don't yet fully understand where that token is generated, or how the shard API consumes it (pointers welcome!) so this is WIP while I figure that out.

@openshift-ci openshift-ci bot changed the title 🌱 proxy: Optionally enable token auth [WIP] seedling proxy: Optionally enable token auth Oct 14, 2022
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 14, 2022
@hardys hardys changed the title [WIP] seedling proxy: Optionally enable token auth [WIP] 🌱 proxy: Optionally enable token auth Oct 14, 2022
@stevekuznetsov
Copy link
Contributor

/assign @s-urbaniak

@hardys
Copy link
Author

hardys commented Oct 20, 2022

There was some discussion on slack about the e2e-sharded failure, summary:

I think there are two possible solutions:

  1. Pass any missing UserInfo for the authorizer (in this case the ClusterName) via headers similar to the existing approach for user/groups
  2. Authenticate at the proxy, but then continue to forward the request to the shard (so the request gets re-authenticated at the shard)

Currently I'm thinking (1) is preferable so we can avoid the duplicated authentication, but this is probably only workable if there are a small number of additional headers required - any opinions on this welcome! :)

@hardys
Copy link
Author

hardys commented Oct 21, 2022

Turns out there is already support for passing userInfo.Extra via headers in the requestheader authenticator (which the shard already enables, but we need a new arg to enable parsing the extra prefix)

That fixes the subset of tests I've been iterating on locally, but will leave this WIP until we get confirmation e2e-sharded is passing

Steven Hardy added 4 commits October 24, 2022 18:22
Currently this still only enables ClientCert authentication but will
enable other auth methods to be added more easily in future
For dev/CI testing token auth is a useful option, this enables
proxy auth to use this method
Since the proxy now supports token auth, we need this enabled at
the proxy API
This is needed for the e2e tests wherer the syncer uses the proxy
endpoint and authenticates via a ServiceAccount bearer token
This is required for some e2e tests, specifically the syncer uses
a service account token to connect via the proxy
The requestheader authenticator enabled at the shard can interpret
extra headers when configured to parse headers with a specific
prefix, so add these headers and configure the sharded test server
to consume them
@hardys
Copy link
Author

hardys commented Oct 25, 2022

/retitle :seedling proxy: Optionally enable token auth

The e2e-sharded job is now passing, so removing WIP and this is ready for review.

We'll also want to add OIDC, but given this PR already contains several commits I'm planning to propose that as a follow-up (with some details on how to test with a local test OIDC server)

One thing to mention - if any additional auth methods are enabled, we require auth at the proxy (but if neither token or service-account auth args are passed, the existing behavior is maintained).

I discussed previously that with @sttts and I'm not certain if we view that as acceptable (opt-in to the new "require auth" behavior by specifying auth methods in addition to client-cert), or if we want an explicit "require auth" flag, thoughts on that are welcome!

@hardys
Copy link
Author

hardys commented Oct 25, 2022

/retitle 🌱 proxy: Optionally enable token auth

@openshift-ci openshift-ci bot changed the title [WIP] 🌱 proxy: Optionally enable token auth 🌱 proxy: Optionally enable token auth Oct 25, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 25, 2022
@hardys
Copy link
Author

hardys commented Oct 25, 2022

/hold cancel

@openshift-ci openshift-ci bot changed the title 🌱 proxy: Optionally enable token auth :seedling proxy: Optionally enable token auth Oct 25, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 25, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 25, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stevekuznetsov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 25, 2022
@hardys
Copy link
Author

hardys commented Oct 25, 2022

/retitle 🌱 proxy: Optionally enable token auth

@openshift-ci openshift-ci bot changed the title :seedling proxy: Optionally enable token auth 🌱 proxy: Optionally enable token auth Oct 25, 2022
@openshift-merge-robot openshift-merge-robot merged commit 46a2d32 into kcp-dev:main Oct 25, 2022
hardys pushed a commit to hardys/kcp that referenced this pull request Oct 25, 2022
This is a partial revert of kcp-dev#2178 - which passed e2e-sharded,
but there are reports of that job now taking twice as long
with these new auth methods enabled.

I'm reverting the Makefile changes to restore the CI job to
the previous behavior, and will investigate locally why
performance is impacted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants