feat(oauth-proxy): configuration enhancements for k8s config #1400

karatkep · 2022-06-10T13:05:20Z

What does this PR do?

ouath-proxy configuration enhancements for kubernetes config

Adding whitelist_domains to whitelist che domain (ie: .yourcompany.com)
Adding cookie_domains to set cookie domain to force cookies to (ie: .yourcompany.com)
Adding IdentityToken to Che Resource to configure identity token (id_token or access_token) to be passed to upstream.

spec:
  auth:
    IdentityToken: access_token

Adding OAuthScope to Che Resource to support custom scopes. Example:

spec:
  auth:
    oAuthScope: 'scope1 scope2 scope3'

Screenshot/screencast of this PR

What issues does this PR fix or reference?

It needs for eclipse-che/che#21450

How to test this PR?

cat EOF << > /tmp/patch.yaml
<PATCH_CONTENT>
EOF

chectl server:deploy \
     --installer operator \
     --platform <PLATFORM_TO_DEPLOY> \
     --che-operator-image <CUSTOM_OPERATOR_IMAGE> \
     --che-operator-cr-patch-yaml /tmp/patch.yaml

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

openshift-ci · 2022-06-10T13:05:31Z

Hi @karatkep. Thanks for your PR.

I'm waiting for a eclipse-che member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

codecov · 2022-06-13T06:07:49Z

Codecov Report

Merging #1400 (b1ed036) into main (ad1c7a8) will increase coverage by 0.87%.
The diff coverage is 100.00%.

❗ Current head b1ed036 differs from pull request most recent head 3ade2ab. Consider uploading reports for the commit 3ade2ab to get more accurate results

@@            Coverage Diff             @@
##             main    #1400      +/-   ##
==========================================
+ Coverage   63.04%   63.91%   +0.87%     
==========================================
  Files          70       70              
  Lines        5812     5853      +41     
==========================================
+ Hits         3664     3741      +77     
+ Misses       1785     1749      -36     
  Partials      363      363

Impacted Files	Coverage Δ
controllers/devworkspace/solver/che_routing.go	`79.57% <100.00%> (+0.61%)`	⬆️
pkg/common/operator-defaults/defaults.go	`20.23% <100.00%> (+4.26%)`	⬆️
pkg/common/utils/utils.go	`30.76% <100.00%> (+2.45%)`	⬆️
pkg/deploy/dashboard/dashboard.go	`50.00% <100.00%> (ø)`
pkg/deploy/gateway/gateway.go	`83.63% <100.00%> (+0.04%)`	⬆️
pkg/deploy/gateway/oauth_proxy.go	`96.25% <100.00%> (+19.53%)`	⬆️
pkg/deploy/gateway/traefik_config_util.go	`91.80% <0.00%> (+2.00%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad1c7a8...3ade2ab. Read the comment docs.

tolusha · 2022-06-13T06:36:54Z

/cc @sparkoo

tolusha · 2022-06-14T06:52:41Z

/retest

sparkoo

code looks fine. Can you please explain your motivation behind pass_access_token=true ?

api/v1/checluster_types.go

karatkep · 2022-06-14T10:20:25Z

code looks fine. Can you please explain your motivation behind pass_access_token=true ?

Hi @sparkoo,
Thank you for your question.

The motivation is to pass OAuth access_token to upstreams via X-Forwarded-Access-Token header.
This is a first step to manage cases when upstreams require access_token and don't support ID token.
The next step is to work of PR to update Traefik component to configure access_token usage instead of ID token.

The long story short: the idea is to improve serviceability and to allow users to configure che-operator to pass OAuth access_token to upstreams.
Of course the default behaviour will stay as is (ID token will be passed). But if user will need access_token (for example, in current version che-operator is hardcoded to use access_token for OpenShift) it will be as simple as to add 2-4 strings into Che Resource file.

sparkoo · 2022-06-14T11:58:28Z

@karatkep do you have some particular use-case where you need access token? We use it with openshift, because we don't have ID token there.

karatkep · 2022-06-14T12:43:39Z

@karatkep do you have some particular use-case where you need access token? We use it with openshift, because we don't have ID token there.

@sparkoo we need access token in case AKS cluster security integrated with Azure AD. We use approach described on https://docs.microsoft.com/en-us/azure/aks/concepts-identity#webhook-and-api-server. API Server requires access token to perform validation.

karatkep · 2022-06-14T14:10:19Z

/retest

sparkoo · 2022-06-14T18:27:21Z

@karatkep do you have some particular use-case where you need access token? We use it with openshift, because we don't have ID token there.

@sparkoo we need access token in case AKS cluster security integrated with Azure AD. We use approach described on https://docs.microsoft.com/en-us/azure/aks/concepts-identity#webhook-and-api-server. API Server requires access token to perform validation.

And how do you use that token? How do you get it from X-Forwarded-Access-Token header? Is that something that azure can do?

karatkep · 2022-06-14T19:07:59Z

And how do you use that token? How do you get it from X-Forwarded-Access-Token header? Is that something that azure can do?

Unf, no magic here :( I used the same way as for OpenShift. This is my POC https://github.com/karatkep/che-operator/pull/1/files which working fine for AKS case. I hardcoded a lot due to POC reason.

And of course I will not follow hardcoded version for original repository. As was mentioned above, my plan is to improve che-operator serviceability to allow users to manage this use case via configuration:
Step 1 (current): extend oauth-proxy component to pass access token
Step 2 (next): extend traefik component to manage use cases when access token need to be passed to upstream.

sparkoo · 2022-06-14T19:48:00Z

Thank you. I didn't get this part that you want to use header rewrite traefik plugin to do this. It make sense now.

I'm not sure that pass_access_token should be enabled all the time. I don't like that access token will be in http header even when we don't need it. So I would probably make it configurable. Is there a case when you need both pass_authorization_header pass_access_token enabled? Or is it rather one xor another? Maybe we could make a switch what token to use and configure both oauth2-proxy and header rewrite traefik middleware based to that switch. wdyt?

karatkep · 2022-06-14T20:39:10Z

We need only pass_access_token enabled for AKS case... But I am not sure about others...

One option to consider is to try ouath2-proxy Alpha configuration. Looks like we no need Traefik changes in this way.

I already asked about it in oauth2-proxy/oauth2-proxy#843 (comment)

tolusha · 2022-06-15T06:22:35Z

I am a bit afraid of having these hardcoded. What about configuring them?

pass_access_token = true
cookie_refresh = "1h0m0s"

karatkep · 2022-06-15T13:26:09Z

Hi @sparkoo , @tolusha
I updated PR according to your recommendations. Could you please review one more time?

karatkep · 2022-06-20T05:27:44Z

/retest

tolusha · 2022-06-20T11:37:24Z

I just have some minor remarks:

Could you move GetDefaultIdentityToken() to defaults.go ?
Could you move IsAccessTokenToPass (rename to IsAccessTokenConfigured) and GetIdentityToken to v2/checluster_types.go ?

In general I don't like utils package to depend on v2.

@sparkoo
pls approve one more time

karatkep · 2022-06-21T08:47:50Z

@tolusha code has been updated according to your recommendations. Please review and approve one more time

karatkep · 2022-06-21T10:23:01Z

/retest

Signed-off-by: Anatolii Bazko <abazko@redhat.com>

tolusha · 2022-06-21T13:44:17Z

@karatkep
Pls accept karatkep#5

Code restructuring

karatkep · 2022-06-21T14:00:35Z

Thank you @tolusha . I just slightly refactored unit tests - just to move to the correct folder.

Thank you for you help!

karatkep · 2022-06-21T15:24:38Z

/retest

openshift-ci · 2022-06-21T16:09:08Z

@karatkep: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/v9-upgrade-stable-to-next	`3ade2ab`	link	true	`/test v9-upgrade-stable-to-next`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

tolusha · 2022-06-21T17:08:45Z

Let me fix v9-upgrade-stable-to-next. it seems my mistake.

tolusha · 2022-06-21T18:47:39Z

Me can merge it. I will fix the test in a separate PR.

karatkep · 2022-06-21T19:09:47Z

Thank you @tolusha

sparkoo

very nice contribution, thank you

openshift-ci · 2022-06-21T19:22:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: karatkep, sparkoo, tolusha

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

feat(oauth-proxy): configuration enhancements for k8s config

d041208

karatkep requested review from AndrienkoAleksandr, flacatus, nickboldt and tolusha as code owners June 10, 2022 13:05

openshift-ci bot added the needs-ok-to-test label Jun 10, 2022

openshift-ci bot requested a review from sparkoo June 13, 2022 06:36

karatkep added 3 commits June 13, 2022 12:37

unit tests for oauth-proxy

7d7bb83

fix resources validation

31fc85f

resources validation fix

d653f3e

tolusha added the ok-to-test label Jun 14, 2022

openshift-ci bot removed the needs-ok-to-test label Jun 14, 2022

sparkoo reviewed Jun 14, 2022

View reviewed changes

api/v1/checluster_types.go Show resolved Hide resolved

identity token support: id_token and access_token

db7c1d5

resources validation fix

9f30c5a

openshift-ci bot removed the lgtm label Jun 18, 2022

resource validation fix plus code refactoring according to comments

306183e

tolusha requested a review from sparkoo June 20, 2022 11:32

tolusha approved these changes Jun 20, 2022

View reviewed changes

openshift-ci bot assigned tolusha Jun 20, 2022

openshift-ci bot added the lgtm label Jun 20, 2022

code structure update

b1ed036

openshift-ci bot removed the lgtm label Jun 21, 2022

karatkep requested a review from tolusha June 21, 2022 08:48

Refactoring

a9a6ea8

Signed-off-by: Anatolii Bazko <abazko@redhat.com>

karatkep and others added 2 commits June 21, 2022 16:52

Merge pull request #5 from tolusha/oauth-proxy-k8s-config

89b51d3

Code restructuring

unit tests refactoring

3ade2ab

tolusha approved these changes Jun 21, 2022

View reviewed changes

openshift-ci bot added the lgtm label Jun 21, 2022

sparkoo approved these changes Jun 21, 2022

View reviewed changes

tolusha merged commit c364ba4 into eclipse-che:main Jun 22, 2022

che-bot added this to the 7.50 milestone Jun 22, 2022

karatkep deleted the oauth-proxy-k8s-config branch June 22, 2022 07:00

feat(oauth-proxy): configuration enhancements for k8s config #1400

feat(oauth-proxy): configuration enhancements for k8s config #1400

Uh oh!

Conversation

karatkep commented Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Screenshot/screencast of this PR

What issues does this PR fix or reference?

How to test this PR?

PR Checklist

Reviewers

Uh oh!

openshift-ci bot commented Jun 10, 2022

Uh oh!

codecov bot commented Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tolusha commented Jun 13, 2022

Uh oh!

tolusha commented Jun 14, 2022

Uh oh!

sparkoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

karatkep commented Jun 14, 2022

Uh oh!

sparkoo commented Jun 14, 2022

Uh oh!

karatkep commented Jun 14, 2022

Uh oh!

karatkep commented Jun 14, 2022

Uh oh!

sparkoo commented Jun 14, 2022

Uh oh!

karatkep commented Jun 14, 2022

Uh oh!

sparkoo commented Jun 14, 2022

Uh oh!

karatkep commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tolusha commented Jun 15, 2022

Uh oh!

karatkep commented Jun 15, 2022

Uh oh!

karatkep commented Jun 20, 2022

Uh oh!

tolusha commented Jun 20, 2022

Uh oh!

karatkep commented Jun 21, 2022

Uh oh!

karatkep commented Jun 21, 2022

Uh oh!

tolusha commented Jun 21, 2022

Uh oh!

karatkep commented Jun 21, 2022

Uh oh!

karatkep commented Jun 21, 2022

Uh oh!

openshift-ci bot commented Jun 21, 2022

Uh oh!

tolusha commented Jun 21, 2022

Uh oh!

tolusha commented Jun 21, 2022

Uh oh!

karatkep commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sparkoo left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Jun 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

karatkep commented Jun 10, 2022 •

edited

Loading

codecov bot commented Jun 13, 2022 •

edited

Loading

karatkep commented Jun 14, 2022 •

edited

Loading

karatkep commented Jun 21, 2022 •

edited

Loading