sso-proxy: adding an optional PROXY_PROVIDER_URL for split dns deployments #88

danbf · 2018-10-08T21:24:10Z

sso-proxy: adding an optional PROXY_PROVIDER_URL for split dns deployments

addressing: #26

danbf · 2018-10-08T21:55:59Z

working on this error maybe on the sso_auth side:

sso-proxy_1    | 2018/10/08 21:52:16 hardcode.RedeemURL! http://host.docker.internal/redeem
sso-proxy_1    | 2018/10/08 21:52:16 p.RedeemURL! http://sso-auth.localtest.me/redeem
sso-auth_1     | {"action":"redeem","http_status":404,"level":"info","msg":"","proxy_host":"httpbin.sso.localtest.me","remote_address":"172.22.0.1","request_duration":0.078322,"request_method":"POST","request_uri":"/redeem","service":"sso-authenticator","time":"2018-10-08 21:52:16.1089","user":"","user_agent":"sso_proxy/HEAD"}
nginx-proxy_1  | nginx.1    | host.docker.internal 172.22.0.1 - - [08/Oct/2018:21:52:16 +0000] "POST /redeem HTTP/1.1" 404 19 "-" "sso_proxy/HEAD"
sso-proxy_1    | {"error":"got 404 from \"http://sso-auth.localtest.me/redeem\" 404 page not found\n","level":"error","msg":"error redeeming authorization code","remote_address":"172.22.0.1","service":"sso-proxy","time":"2018-10-08 21:52:16.1089"}
sso-proxy_1    | {"http_status":500,"level":"info","msg":"error page","page_message":"Internal Error","page_title":"Internal Error","remote_address":"172.22.0.1","service":"sso-proxy","time":"2018-10-08 21:52:16.1089"}

danbf · 2018-10-09T15:21:33Z

ok, so this is working now. now need to actually pass the values from the config to the function.

danbf · 2018-10-09T22:00:24Z

ok, got values making their way into the functions, just got to fixup tests.

internal/proxy/providers/sso_test.go

shrayolacrayon · 2018-10-15T20:26:10Z

internal/proxy/providers/sso_test.go

 				p.ProfileURL, profileServer = newTestServer(http.StatusOK, body)
 			} else {
 				p.RedeemURL, profileServer = newCodeTestServer(400)
+				p.ProxyRedeemURL, redeemServer = newCodeTestServer(400)


might need to defer this server being closed as well

shrayolacrayon · 2018-10-15T21:52:39Z

internal/proxy/providers/sso_test.go

+				RefreshToken: "refresh12345",
+				Email:        "michael.bland@gsa.gov",
+			},
+			ProxyProviderURL: &url.URL{


I think what you'd want to do to test this behavior is create a ProxyProviderResponse similar to the Redeem Response and Profile Response, as well as a ProxyRedeemResponse, and assign the server urls to those provider fields. Then you can test that the endpoints are being hit based on the response.

benjsto · 2018-10-16T18:56:40Z

internal/proxy/providers/sso_test.go

+				p.ProxyRedeemURL, redeemServer = newTestServer(http.StatusOK, body)
 			} else {
 				p.RedeemURL, redeemServer = newCodeTestServer(400)
+				p.ProxyRedeemURL, redeemServer = newCodeTestServer(400)


similar to @shrayolacrayon 's comment above, why do we need to create the test server twice here?

internal/proxy/providers/sso_test.go

shrayolacrayon · 2018-10-23T18:31:26Z

internal/proxy/options_test.go

+		{
+			name:                   "redeem string based on proxyProviderURL",
+			providerURLString:      "https://provider.example.com",
+			proxyProviderURLString: "https://provider-internal.example.com",


is this field being used anywhere?

@shrayolacrayon it's the string that we use to generate the proxyProviderURL url

sso/internal/proxy/options.go

Lines 223 to 226 in dec5586

proxyProviderURL, err := url.Parse(o.ProxyProviderURLString)

if err != nil {

return err

}

which is used to generate the redeem url:

sso/internal/proxy/providers/sso.go

Line 63 in dec5586

p.ProxyRedeemURL = p.ProxyProviderURL.ResolveReference(&url.URL{Path: "/redeem"})

shrayolacrayon · 2018-10-23T20:33:32Z

internal/proxy/options_test.go

 	o.EmailDomains = []string{"*"}
 	o.ProviderURLString = "https://www.example.com"
-	o.ProxyProviderURLString = "https://internal.example.com"
+	o.ProxyProviderURLString = ""


you don't need to set this to be empty, it'll default to an empty string

internal/proxy/options_test.go

benjsto · 2018-10-23T21:54:25Z

internal/proxy/options.go

 // Options are configuration options that can be set by Environment Variables
 // Port - int -  port to listen on for HTTP clients
 // ProviderURLString - the URL for the provider in this environment: "https://sso-auth.example.com"
+// ProxyProviderURLString - the internal URL for the provider in this environment: "https://sso-auth-int.example.com"


If this field is truly intended as an internal URL, would it be clearer if the name reflected that? I struggle a little bit with the names ProviderURLString and ProxyProviderURLString being not meaningfully different as far as I can tell.

thought here is that the sso_proxy service is using this one, the other is for the client. but then naming is not my strong suit

shrayolacrayon · 2018-10-23T22:25:33Z

internal/proxy/providers/sso_test.go

+			var redeemServer *httptest.Server
+			var redeemServerInternal *httptest.Server
+			// set up redemption resource
+			if tc.RedeemResponseInternal != nil {


why aren't we marshaling the redeem response if RedeemResponseInternal is nil?

this is copied from the existing tests so i had not put much thought into it:

sso/internal/proxy/providers/sso_test.go

Lines 260 to 266 in 7898834

if tc.RedeemResponse != nil {

testutil.Equal(t, nil, err)

testutil.NotEqual(t, session, nil)

testutil.Equal(t, tc.RedeemResponse.Email, session.Email)

testutil.Equal(t, tc.RedeemResponse.AccessToken, session.AccessToken)

testutil.Equal(t, tc.RedeemResponse.RefreshToken, session.RefreshToken)

}

from what i can see it looks like the test cases are divided into two classes, those that don't produce a redeem response, and ones that should produce a valid redeem response which then gets checked for validity. for this particular test, i'm testing to see that the code is handling the case where both the RedeemURL and ProxyRedeemURL both exist. in that case the internal ProxyRedeemURL should be used for the redeem.

Oh got it, I didn't see that we're still testing RedeemResponse above.

internal/proxy/options.go

shrayolacrayon

added one more error nit, but other than that this lgtm!

…ments

danbf self-assigned this Oct 8, 2018

loganmeetsworld added the in progress label Oct 8, 2018

danbf mentioned this pull request Oct 8, 2018

sso-proxy: adding an optional PROXY_PROVIDER_URL for split dns deployments {WIP} #31

Closed

danbf force-pushed the provides-PROXY_PROVIDER_URL branch from 0362612 to 2178477 Compare October 8, 2018 21:26

danbf force-pushed the provides-PROXY_PROVIDER_URL branch from 5762ed9 to 8fbf65b Compare October 9, 2018 15:14

danbf force-pushed the provides-PROXY_PROVIDER_URL branch 12 times, most recently from 435de59 to 6fe9d49 Compare October 9, 2018 21:59

danbf force-pushed the provides-PROXY_PROVIDER_URL branch 10 times, most recently from 774d874 to a9e50a2 Compare October 10, 2018 14:37

danbf force-pushed the provides-PROXY_PROVIDER_URL branch from 49a0add to d1008ef Compare October 15, 2018 18:29

shrayolacrayon reviewed Oct 15, 2018

View reviewed changes

internal/proxy/providers/sso_test.go Outdated Show resolved Hide resolved

shrayolacrayon reviewed Oct 15, 2018

View reviewed changes

internal/proxy/providers/sso_test.go Show resolved Hide resolved

shrayolacrayon reviewed Oct 15, 2018

View reviewed changes

benjsto reviewed Oct 16, 2018

View reviewed changes

danbf force-pushed the provides-PROXY_PROVIDER_URL branch 2 times, most recently from ac9ac27 to ffbc8a9 Compare October 22, 2018 23:16

danbf commented Oct 22, 2018

View reviewed changes

internal/proxy/providers/sso_test.go Show resolved Hide resolved

danbf force-pushed the provides-PROXY_PROVIDER_URL branch 2 times, most recently from 5e2d6a5 to dec5586 Compare October 23, 2018 18:18

shrayolacrayon reviewed Oct 23, 2018

View reviewed changes

danbf force-pushed the provides-PROXY_PROVIDER_URL branch from 7b880b0 to b1ce2a6 Compare October 23, 2018 20:35

shrayolacrayon reviewed Oct 23, 2018

View reviewed changes

internal/proxy/options_test.go Show resolved Hide resolved

danbf force-pushed the provides-PROXY_PROVIDER_URL branch from 39acd6b to f33791d Compare October 23, 2018 21:08

benjsto reviewed Oct 23, 2018

View reviewed changes

shrayolacrayon reviewed Oct 23, 2018

View reviewed changes

shrayolacrayon reviewed Oct 24, 2018

View reviewed changes

internal/proxy/options.go Outdated Show resolved Hide resolved

shrayolacrayon reviewed Oct 24, 2018

View reviewed changes

sso-proxy: adding an optional PROXY_PROVIDER_URL for split dns deploy…

af20521

…ments

danbf force-pushed the provides-PROXY_PROVIDER_URL branch from 1d04389 to af20521 Compare October 24, 2018 21:03

shrayolacrayon approved these changes Oct 24, 2018

View reviewed changes

danbf merged commit 1662f7c into master Oct 24, 2018

danbf deleted the provides-PROXY_PROVIDER_URL branch October 24, 2018 21:09

danbf added enhancement New feature or request and removed in review labels Oct 25, 2018

This was referenced Oct 25, 2018

add a new parameter which overrides the PROVIDER_URL for the sso-proxy->sso-auth connection for split dns environments #26

Closed

Error redeeming authorization code #102

Closed

benjsto mentioned this pull request Nov 26, 2018

sso-proxy: internal host should apply to redeem, /refresh, /validate, /profile #123

Merged

	proxyProviderURL, err := url.Parse(o.ProxyProviderURLString)
	if err != nil {
	return err
	}

	if tc.RedeemResponse != nil {
	testutil.Equal(t, nil, err)
	testutil.NotEqual(t, session, nil)
	testutil.Equal(t, tc.RedeemResponse.Email, session.Email)
	testutil.Equal(t, tc.RedeemResponse.AccessToken, session.AccessToken)
	testutil.Equal(t, tc.RedeemResponse.RefreshToken, session.RefreshToken)
	}

sso-proxy: adding an optional PROXY_PROVIDER_URL for split dns deployments #88

sso-proxy: adding an optional PROXY_PROVIDER_URL for split dns deployments #88

Uh oh!

Conversation

danbf commented Oct 8, 2018

Uh oh!

danbf commented Oct 8, 2018

Uh oh!

danbf commented Oct 9, 2018

Uh oh!

danbf commented Oct 9, 2018

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shrayolacrayon Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbf Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shrayolacrayon left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shrayolacrayon Oct 23, 2018 •

edited

Loading

danbf Oct 24, 2018 •

edited

Loading