Redirect to relative subpath for approval, relative parent path on success #13569

liggitt · 2017-03-29T14:53:13Z

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1434983

Moves grant approval endpoint from /oauth/approve to /oauth/authorize/approve
Changes the redirect to the grant approval flow to be a relative path redirect (Location: authorize/approve)
Changes the grant approval form action to be relative (action="approve")
Changes the redirect from the approval flow back to the authorize endpoint to be relative (Location: ../authorize)
Adds validation to RequestHeaderIdentityProvider loginURL to catch two scenarios that would prevent relative redirects from working (trailing / and the proxy path not ending in /authorize)

Tasks:

deads2k · 2017-03-29T19:29:54Z

pkg/auth/oauth/handlers/grant.go

+	reqURL := &(*req.URL)
+	reqURL.Host = ""
+	reqURL.Scheme = ""
+	reqURL.Path = path.Join("..", lastSegment)


Why go up a level and back down to the same level?

gets rid of trailing slash

Use path.Base or strings.TrimRight? Regardless, probably needs a comment.

you misunderstand, it gets rid of the trailing slash in the browser... if you just redirect to ., you'd get /oauth/authorize/

deads2k · 2017-03-29T19:32:17Z

pkg/cmd/server/api/validation/oauth.go

+					),
+				)
+			}
+			if strings.HasSuffix(url.Path, "/") {


These didn't work before either?

they didn't work for grant approval flows (which realistically, no one hit before jenkins OAuth), they could have worked for /oauth/authorize proxying

liggitt · 2017-04-03T03:57:51Z

[test]

…ccess

enj · 2017-04-04T14:35:53Z

LGTM 👍

liggitt · 2017-04-04T14:54:10Z

[merge]

openshift-bot · 2017-04-04T15:05:39Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/556/) (Base Commit: f0670df) (Image: devenv-rhel7_6115)

openshift-bot · 2017-04-04T15:05:40Z

Evaluated for origin merge up to f42b363

liggitt · 2017-04-04T18:12:04Z

[test]

openshift-bot · 2017-04-04T18:13:23Z

Evaluated for origin test up to f42b363

openshift-bot · 2017-04-04T19:37:41Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/556/) (Base Commit: f0670df)

This change moves the docs away from the frontend based RequestHeaderIdentityProvider to the backend based BasicAuthPasswordIdentityProvider. This is because the LDAP docs assume a password based flow which requires the RequestHeaderIDP to support form based login. In 3.3 and earlier (before the grant approval flow existed), this form based login occurred in a single step that did not require a session. The grant flow changes this flow to require more than one step, necessitating the need for a session based approach. Apache does not have a form module that supports sessions, thus we cannot use the RequestHeaderIDP. By moving to the BasicAuthPasswordIDP, we leverage OpenShift's ability to manage sessions directly. This changes the network flow from: RequestHeaderIDP: User -> Apache -> OpenShift to: BasicAuthPasswordIDP: User -> OpenShift -> Apache From the perspective of Apache, the entire flow occurs in one step and thus Apache does not need to maintain a session. This allows users to continue to use their investment in SSSD and PAM to have a robust LDAP setup that supports automatic failover. There is no specific requirement on which PAM provider is used; these docs simply present one specific approach. This does have the negative side effect of making OpenShift see user's passwords. However, any individual that is concerned by this should instead use the RequestHeaderIdentityProvider with SAML and GSSAPI to eliminate the need for users to type passwords at all. This is required for OCP 3.4+ Fixes bug: Bug 1545548 Caused by fixing: Bug 1421629 Bug 1434983 Bug 1439221 Bug 1439222 via openshift/origin#13569 Signed-off-by: Monis Khan <mkhan@redhat.com>

…dentityProvider This change moves the docs away from the frontend based RequestHeaderIdentityProvider to the backend based BasicAuthPasswordIdentityProvider. This is because the LDAP docs assume a password based flow which requires the RequestHeaderIDP to support form based login. In 3.3 and earlier (before the grant approval flow existed), this form based login occurred in a single step that did not require a session. The grant flow changes this flow to require more than one step, necessitating the need for a session based approach. Apache does not have a form module that supports sessions, thus we cannot use the RequestHeaderIDP. By moving to the BasicAuthPasswordIDP, we leverage OpenShift's ability to manage sessions directly. This changes the network flow from: RequestHeaderIDP: User -> Apache -> OpenShift to: BasicAuthPasswordIDP: User -> OpenShift -> Apache From the perspective of Apache, the entire flow occurs in one step and thus Apache does not need to maintain a session. This allows users to continue to use their investment in SSSD and PAM to have a robust LDAP setup that supports automatic failover. There is no specific requirement on which PAM provider is used; these docs simply present one specific approach. This does have the negative side effect of making OpenShift see user's passwords. However, any individual that is concerned by this should instead use the RequestHeaderIdentityProvider with SAML and GSSAPI to eliminate the need for users to type passwords at all. This is required for OCP 3.4+ Fixes bug: Bug 1545548 Caused by fixing: Bug 1421629 Bug 1434983 Bug 1439221 Bug 1439222 via openshift/origin#13569 Signed-off-by: Monis Khan <mkhan@redhat.com>

…ntityProvider This change moves the docs away from the frontend based RequestHeaderIdentityProvider to the backend based BasicAuthPasswordIdentityProvider. This is because the LDAP docs assume a password based flow which requires the RequestHeaderIDP to support form based login. In 3.3 and earlier (before the grant approval flow existed), this form based login occurred in a single step that did not require a session. The grant flow changes this flow to require more than one step, necessitating the need for a session based approach. Apache does not have a form module that supports sessions, thus we cannot use the RequestHeaderIDP. By moving to the BasicAuthPasswordIDP, we leverage OpenShift's ability to manage sessions directly. This changes the network flow from: RequestHeaderIDP: User -> Apache -> OpenShift to: BasicAuthPasswordIDP: User -> OpenShift -> Apache From the perspective of Apache, the entire flow occurs in one step and thus Apache does not need to maintain a session. This allows users to continue to use their investment in SSSD and PAM to have a robust LDAP setup that supports automatic failover. There is no specific requirement on which PAM provider is used; these docs simply present one specific approach. This does have the negative side effect of making OpenShift see user's passwords. However, any individual that is concerned by this should instead use the RequestHeaderIdentityProvider with SAML and GSSAPI to eliminate the need for users to type passwords at all. This is required for OCP 3.4+ Fixes bug: Bug 1545548 Caused by fixing: Bug 1421629 Bug 1434983 Bug 1439221 Bug 1439222 via openshift/origin#13569 Signed-off-by: Monis Khan <mkhan@redhat.com>

…entityProvider This change moves the docs away from the frontend based RequestHeaderIdentityProvider to the backend based BasicAuthPasswordIdentityProvider. This is because the LDAP docs assume a password based flow which requires the RequestHeaderIDP to support form based login. In 3.3 and earlier (before the grant approval flow existed), this form based login occurred in a single step that did not require a session. The grant flow changes this flow to require more than one step, necessitating the need for a session based approach. Apache does not have a form module that supports sessions, thus we cannot use the RequestHeaderIDP. By moving to the BasicAuthPasswordIDP, we leverage OpenShift's ability to manage sessions directly. This changes the network flow from: RequestHeaderIDP: User -> Apache -> OpenShift to: BasicAuthPasswordIDP: User -> OpenShift -> Apache From the perspective of Apache, the entire flow occurs in one step and thus Apache does not need to maintain a session. This allows users to continue to use their investment in SSSD and PAM to have a robust LDAP setup that supports automatic failover. There is no specific requirement on which PAM provider is used; these docs simply present one specific approach. This does have the negative side effect of making OpenShift see user's passwords. However, any individual that is concerned by this should instead use the RequestHeaderIdentityProvider with SAML and GSSAPI to eliminate the need for users to type passwords at all. This is required for OCP 3.4+ Fixes bug: Bug 1545548 Caused by fixing: Bug 1421629 Bug 1434983 Bug 1439221 Bug 1439222 via openshift/origin#13569 Signed-off-by: Monis Khan <mkhan@redhat.com>

liggitt changed the title ~~Redirect to relative subpath for approval, relative parent path on success~~ WIP - redirect to relative subpath for approval, relative parent path on success Mar 29, 2017

liggitt assigned enj and deads2k Mar 29, 2017

deads2k reviewed Mar 29, 2017

View reviewed changes

liggitt added 2 commits April 3, 2017 17:05

Redirect to relative subpath for approval, relative parent path on su…

c80d3ba

…ccess

Update OAuth grant flow tests

f42b363

liggitt changed the title ~~WIP - redirect to relative subpath for approval, relative parent path on success~~ Redirect to relative subpath for approval, relative parent path on success Apr 3, 2017

liggitt mentioned this pull request Apr 4, 2017

Redirect to relative subpath for approval, relative parent path on success #13626

Merged

openshift-bot merged commit 6a175fa into openshift:master Apr 4, 2017

liggitt mentioned this pull request Apr 5, 2017

Update authentication proxy requirements openshift/openshift-docs#4079

Closed

liggitt deleted the relative-approval-redirect branch April 7, 2017 14:55

enj mentioned this pull request May 3, 2018

Update advanced LDAP docs to use BasicAuthPasswordIdentityProvider openshift/openshift-docs#9025

Closed

Redirect to relative subpath for approval, relative parent path on success #13569

Redirect to relative subpath for approval, relative parent path on success #13569

liggitt commented Mar 29, 2017 •

edited

deads2k Mar 29, 2017

liggitt Mar 29, 2017

enj Mar 29, 2017

liggitt Apr 3, 2017

liggitt Apr 3, 2017

deads2k Mar 29, 2017

liggitt Mar 29, 2017 •

edited

liggitt commented Apr 3, 2017

enj commented Apr 4, 2017

liggitt commented Apr 4, 2017

openshift-bot commented Apr 4, 2017 •

edited

openshift-bot commented Apr 4, 2017

liggitt commented Apr 4, 2017

openshift-bot commented Apr 4, 2017

openshift-bot commented Apr 4, 2017

Redirect to relative subpath for approval, relative parent path on success #13569

Redirect to relative subpath for approval, relative parent path on success #13569

Conversation

liggitt commented Mar 29, 2017 • edited

deads2k Mar 29, 2017

Choose a reason for hiding this comment

liggitt Mar 29, 2017

Choose a reason for hiding this comment

enj Mar 29, 2017

Choose a reason for hiding this comment

liggitt Apr 3, 2017

Choose a reason for hiding this comment

liggitt Apr 3, 2017

Choose a reason for hiding this comment

deads2k Mar 29, 2017

Choose a reason for hiding this comment

liggitt Mar 29, 2017 • edited

Choose a reason for hiding this comment

liggitt commented Apr 3, 2017

enj commented Apr 4, 2017

liggitt commented Apr 4, 2017

openshift-bot commented Apr 4, 2017 • edited

openshift-bot commented Apr 4, 2017

liggitt commented Apr 4, 2017

openshift-bot commented Apr 4, 2017

openshift-bot commented Apr 4, 2017

liggitt commented Mar 29, 2017 •

edited

liggitt Mar 29, 2017 •

edited

openshift-bot commented Apr 4, 2017 •

edited