search

openshift / oauth-proxy

A reverse proxy that provides authentication with OpenShift via OAuth and Kubernetes service accounts
MIT License
261 stars 136 forks source link

Bug 2026573: use rd for redirect #234

Closed clyang82 closed 2 years ago

clyang82 commented 2 years ago

fixes: https://github.com/openshift/oauth-proxy/issues/232

req.RequestURI is oauth/start so there is a dead loop, because the request is from xxx/oauth/start. It can be reproduced by entering the url xxx/oauth/sign_in or xxx/oauth/start into the browsers.

Signed-off-by: clyang82 chuyang@redhat.com

clyang82 commented 2 years ago

/retest

openshift-ci[bot] commented 2 years ago

@clyang82: This pull request references Bugzilla bug 2026573, which is invalid:

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to [this](https://github.com/openshift/oauth-proxy/pull/234): >Bug 2026573: use rd for redirect Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
s-urbaniak commented 2 years ago

/bugzilla refresh

openshift-ci[bot] commented 2 years ago

@s-urbaniak: This pull request references Bugzilla bug 2026573, which is invalid:

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to [this](https://github.com/openshift/oauth-proxy/pull/234#issuecomment-980084184): >/bugzilla refresh Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
clyang82 commented 2 years ago

/retest

clyang82 commented 2 years ago

/test e2e-component

clyang82 commented 2 years ago

Thanks @s-urbaniak for sharing the history for skip-provider-button fix. From this issue https://github.com/openshift/oauth-proxy/issues/18, we can see the problem is

When I set -proxy-prefix to , the authentication process works correctly. However, when I also introduce -skip-provider-button, after I perform the login (which seems to work just fine), I am redirected to a URL that omits the / portion, which fails.

For my case, I do not set -proxy-prefix so that the req.RequestURI is oauth/start it leads a dead loop. so the fix is to add fine-grained check.

stlaz commented 2 years ago

/lgtm cancel Pre-review: there should not be 3 commits for a single line change, especially Merge branch 'openshift:master' into redirect does not look correct.

openshift-ci[bot] commented 2 years ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: clyang82, slaskawi To complete the pull request process, please ask for approval from stlaz after the PR has been reviewed.

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

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/openshift/oauth-proxy/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
clyang82 commented 2 years ago

Could you please provide an example of a request that would loop? Also, the explanation of this change should be included in a commit message.

Does the example here help? Let me know if it is unclear. Thanks.

clyang82 commented 2 years ago

/test e2e-component

clyang82 commented 2 years ago

/test e2e-component

clyang82 commented 2 years ago

/retest

clyang82 commented 2 years ago

/retest

stlaz commented 2 years ago

The tests are failing with the proxy returning 500s, please check what's the issue and address it.

clyang82 commented 2 years ago

/retest

clyang82 commented 2 years ago

/test e2e-component

clyang82 commented 2 years ago

/retest

clyang82 commented 2 years ago

The tests are failing with the proxy returning 500s, please check what's the issue and address it.

run make test-e2e --warn-undefined-variables in local OCP 4.8, always pass.

clyang82 commented 2 years ago

/retest-required

clyang82 commented 2 years ago

/bugzilla refresh

openshift-ci[bot] commented 2 years ago

@clyang82: This pull request references Bugzilla bug 2026573, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug * bug is open, matching expected state (open) * bug target release (4.10.0) matches configured target release for branch (4.10.0) * bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (ytripath@redhat.com), skipping review request.

In response to [this](https://github.com/openshift/oauth-proxy/pull/234#issuecomment-998764824): >/bugzilla refresh Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
openshift-bot commented 2 years ago

/bugzilla refresh

The requirements for Bugzilla bugs have changed (BZs linked to PRs on master branch need to target OCP 4.11), recalculating validity.

openshift-ci[bot] commented 2 years ago

@openshift-bot: This pull request references Bugzilla bug 2026573, which is invalid:

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to [this](https://github.com/openshift/oauth-proxy/pull/234#issuecomment-1024507447): >/bugzilla refresh > >The requirements for Bugzilla bugs have changed (BZs linked to PRs on master branch need to target OCP 4.11), recalculating validity. Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
clyang82 commented 2 years ago

/bugzilla refresh

openshift-ci[bot] commented 2 years ago

@clyang82: This pull request references Bugzilla bug 2026573, which is invalid:

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to [this](https://github.com/openshift/oauth-proxy/pull/234#issuecomment-1074820538): >/bugzilla refresh Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
clyang82 commented 2 years ago

/retest

openshift-ci[bot] commented 2 years ago

@clyang82: 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/e2e-component e105658cb1a883263095dee88f67e77da1920132 link true /test e2e-component

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
openshift-bot commented 2 years ago

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close. Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

ibihim commented 2 years ago

@clyang82, as mentioned in the issue, it is a misconfiguration, please check my response on your issue: https://github.com/openshift/oauth-proxy/issues/232.

We can't just add a custom path to make your solution work in particular. We would need to add a feature flag (which defaults to turned off) and it would allow relative paths (set by rd or conservation of the relative path in the first place) on skip-provider-button=true, which seems to be the issue.

openshift-ci[bot] commented 2 years ago

@clyang82: This pull request references Bugzilla bug 2026573. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state. Warning: Failed to comment on Bugzilla bug with reason for changed state.

In response to [this](https://github.com/openshift/oauth-proxy/pull/234): >Bug 2026573: use rd for redirect Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.