support SAML HTTP redirect binding #1045

easeway · 2017-08-23T00:04:13Z

Solve issue: #1042

easeway · 2017-08-23T00:05:52Z

@ericchiang Please take a look...

ericchiang

Need to change the response handler too.

Almost feel like this should surface as a different interface value https://godoc.org/github.com/coreos/dex/connector#SAMLConnector

ericchiang · 2017-08-23T00:06:48Z

server/handlers.go

+				  </body>
+				  </html>`, ssoURL, value, authReqID)
+			} else {
+				// use HTTP redirect binding


We should just return an actual redirect here, not render a page. E.g.

http.Redirect(w, r, ...)

ericchiang · 2017-08-23T00:07:48Z

Documentation/saml-connector.md

+    #
+    # Some identity provider only supports HTTP redirect binding
+    #
+    # redirectBinding: true


this should probably be something like

binding: redirect # defaults to post

in case we want to support more bindings in the future.

ericchiang · 2017-08-23T00:08:41Z

server/handlers.go

@@ -254,30 +254,47 @@ func (s *Server) handleConnectorLogin(w http.ResponseWriter, r *http.Request) {
 				s.logger.Errorf("Server template error: %v", err)
 			}
 		case connector.SAMLConnector:
-			action, value, err := conn.POSTData(scopes, authReqID)
+			ssoURL, value, err := conn.POSTData(scopes, authReqID)


Need to rename POSTData or break it into a separate method.

I think it's better to pass http.ResponseWriter and http.Request into provider to reduce provider specific code in server/handlers.go. Eventually we can eliminate this switch conn := conn.Connector.(type)

My proposal is to define a common Connector interface including a method:

HandleLogin(w http.ResponseWriter, r *http.Request)

And leave to the Connector implementation for all provider specific logic.

In the mean time, I will do the following change:

Rename PostData to AuthnRequest(scopes, authId, w http.ResponseWriter, r *http.Request) and perform the logic there.

I'm actually against that. The server package controls things like html error responses and I'd rather all of that logic continue to live there. Not have any connectors refer to the net/http package directly.

ericchiang · 2017-08-23T00:11:06Z

connector/saml/saml.go

+	if p.redirectBinding {
+		var buf bytes.Buffer
+		zw, e := flate.NewWriter(&buf, flate.DefaultCompression)
+		if e == nil {


This block shouldn't keep going if err != nil

here e == nil and err != nil is handled in one place below.

I use e == nil because the sequence will go through 3 steps: NewWriter, Write and Flush. Each may fail and should bail out immediately. So use if e == nil { do...} will make the code simpler, and handle the error in one place.

ericchiang · 2017-08-23T00:12:01Z

connector/saml/saml.go

+			return "", "", fmt.Errorf("deflate request: %v", e)
+		}
+
+		redirectURL := fmt.Sprintf("%s?SAMLRequest=%s&RelayState=%s",


use url.URL and an actual url.Values object https://golang.org/pkg/net/url/#Values

OK. I will construct a URL.

ericchiang · 2017-08-23T00:14:34Z

connector/saml/saml.go

+			p.ssoURL,
+			url.QueryEscape(base64.StdEncoding.EncodeToString(buf.Bytes())),
+			url.QueryEscape(id))
+		return redirectURL, "", nil


This method now returns completely different kind of values depending on how it's configured. The URL creation and HTML creation should be done in the same place.

I will update the interface according to your comments below.

easeway · 2017-08-23T18:45:43Z

@ericchiang PR updated. Please take a look when you have time.

ericchiang · 2017-08-23T21:34:19Z

server/handlers.go

+				  </html>`, ssoURL, value, authReqID)
+			case connector.SAMLBindingRedirect:
+				query := make(url.Values)
+				query["SAMLRequest"] = []string{value}


nit: this is just query.Set("SAMLRequest", value)

ericchiang

That worked out nicely. Thanks for the refactors.

When the provider redirects back to dex, that'll now be different though, right? E.g. this logic needs to change https://github.com/coreos/dex/blob/master/server/handlers.go#L327

ericchiang · 2017-08-23T21:37:45Z

connector/connector.go

@@ -65,18 +65,24 @@ type CallbackConnector interface {
 	HandleCallback(s Scopes, r *http.Request) (identity Identity, err error)
 }

+// SAMLConnector bindings
+const (
+	SAMLBindingPOST     = "post"


nit: this should probably be httpPost and httpRedirect, right? Since the other ones are SOAP based.

Do you mean the const name should be "SAMLHTTPPOST" or the value should be "httpPost"? I'm using the same value from config option. I'm OK if we decide to use "http-post", "http-redirect" in values of config option "binding". But I'd like to use the same value internally in code.

I meant the "post" and "redirect" values.

Actually, I think the way it currently is is fine. Let's keep it as is.

easeway · 2017-08-23T21:56:27Z

The handler doesn't need any change. When specifying "binding: redirect", it only specifies how dex opens login page on identity provider. The way identity provider calls back to dex can use a different http method, which is defined by the trusted service callback mechanism in identity provider (the pre-registered trusted service provider definition).

If identity provider respect AssertionConsumerServiceURL in AuthnRequest, it will use ProtocolBinding to send back response. Currently in dex, the ProtocolBinding is always POST.

I have verified the implement against vSphere 6.5

ericchiang · 2017-08-23T23:42:21Z

@easeway ah I see that you can actually mix bindings.

It's good that vSphere works with this setup, but what about providers that don't support the POST binding for the SAMLResponse? Do we need explicit requestBinding and responseBinding values?

easeway · 2017-08-23T23:48:48Z

@ericchiang For providers don't support POST binding in response, we need to do further fixes. We should make it configurable, that when constructing AuthnRequest, we can specify ProtocolBinding from the configuration. Then we will have to fix the handler too.

What about making a separate PR for that?

easeway · 2017-08-24T18:56:42Z

@ericchiang I created a separate PR #1048 to support response redirect binding. However vSphere doesn't support redirect binding on response, so I can't verify that, and that's why I make it a separate PR.

Would you approve this PR if you think the change is good here?

ericchiang · 2017-08-24T19:00:57Z

Would you approve this PR if you think the change is good here?

No we should just move to #1048. Since it's all the same feature I'd rather we design the full thing instead of doing it piecemeal.

easeway · 2017-08-24T19:03:37Z

@ericchiang All right. I will collapse the changes in #1048 into this PR and close #1048. And I make sure the features except response redirect binding is verified.

easeway · 2017-08-24T19:10:04Z

@ericchiang Updated. I separate the config for requestBinding and responseBinding and updated the handler. Please take a look.

ericchiang

Thanks, this is getting close!

Going to try to test this patch against another SAML provider that implements the HTTP redirect binding.

ericchiang · 2017-08-25T23:33:05Z

connector/saml/saml.go

@@ -72,6 +75,18 @@ func init() {
 	}
 }

+// normalizeBinding verifies binding from config and normalize the string
+func normalizeBinding(binding string) (string, error) {
+	binding = strings.ToLower(strings.TrimSpace(binding))


I'd avoid any normalization. Keeps the configuration explicit.

OK. I will remove this ToLower line.

ericchiang · 2017-08-25T23:33:25Z

connector/saml/saml.go

+	if binding == "" {
+		return connector.SAMLBindingPOST, nil
+	}
+	if binding != connector.SAMLBindingPOST && binding != connector.SAMLBindingRedirect {


probably just use a globally defined map + a lookup

only two string const comparisons. I'm trying to avoid a global var. Anyway, I will add a global map.

ericchiang · 2017-08-25T23:33:45Z

connector/saml/saml.go

+	if p.requestBinding, err = normalizeBinding(c.RequestBinding); err != nil {
+		return nil, err
+	}
+	if p.responseBinding, err = normalizeBinding(c.ResponseBinding); err != nil {


the response binding should probably default to whatever the request binding is, right?

not sure. In most cases, responseBinding will be POST. Even requestBinding is redirect. Would it be simpler to define fixed default values in config?

Would it be simpler to define fixed default values in config?

Works for me.

ericchiang · 2017-08-25T23:34:57Z

connector/saml/saml.go

@@ -252,6 +281,11 @@ func (p *provider) POSTData(s connector.Scopes, id string) (action, value string
 		},
 		AssertionConsumerServiceURL: p.redirectURI,
 	}
+
+	if p.responseBinding == connector.SAMLBindingRedirect {
+		r.ProtocolBinding = bindingRedirect


Do we not set this if it's a POST?

It's already set to POST in a few lines above when constructing the authnRequest.

ericchiang · 2017-08-25T23:35:37Z

connector/saml/saml.go

@@ -598,3 +647,34 @@ func before(now, notBefore time.Time) bool {
 func after(now, notOnOrAfter time.Time) bool {
 	return now.After(notOnOrAfter.Add(allowedClockDrift))
 }
+
+// deflate compresses the data
+func deflate(data []byte) ([]byte, error) {


nit: just call this "compressRequest" and "decompressRequest"

ericchiang · 2017-08-25T23:38:26Z

server/handlers.go

 			s.renderError(w, http.StatusBadRequest, "Invalid request")
 			return
 		}
-		identity, err = conn.HandlePOST(parseScopes(authReq.Scopes), r.PostFormValue("SAMLResponse"), authReq.ID)
+		identity, err = conn.HandleResponse(parseScopes(authReq.Scopes), response, authReq.ID)


So in this case I could request responseBinding: post, get an HTTP redirect response, and we'd still process it even though we've explicitly stated that we don't.

I think we need to enforce that only the binding that the connector is configured with is actually used.

You are right. I will fix this.

The problem is at this layer, it doesn't have the knowledge about bindings. Anyway, let me figure it out...

ericchiang · 2017-08-25T23:40:19Z

server/handlers.go

+		var response string
+		switch r.Method {
+		case http.MethodGet:
+			response = r.URL.Query().Get("SAMLResponse")


With this logic I could use "state" to look up the authID and never actually inspect the "RelayState" even though it's required for this response. This seems wrong.

handleConnectorCallback is shared by multiple providers. OAuth2 uses "state" while SAML uses "RelayState". So if it's SAML, there's no "state", and we should use "RelayState".

ericchiang · 2017-10-23T18:40:16Z

server/handlers.go

-		if authID = r.URL.Query().Get("state"); authID == "" {
+	case http.MethodGet: // OAuth2 or SAML HTTP-Redirect callback
+		// try OAuth2
+		authID = r.URL.Query().Get("state")


The SAML connector can still use the value set by "state" here. We need to enforce:

var usedRelayState bool authID = r.URL.Query().Get("state") if authID == "" { usedRelayState = true authID = r.URL.Query.Get("RelayState") }

Then verify this value later.

case connector.CallbackConnector: if usedRelayState { return nil, fmt.Errorf("no state parameter provided") } case connector.SAMLConnector: if !userRelayState { return nil, fmt.Errorf("invalid url query value 'state' provided") }

understand!

ericchiang · 2017-10-23T18:41:47Z

connector/saml/saml.go

@@ -72,6 +79,17 @@ func init() {
 	}
 }

+// verifyBinding verifies binding from config
+func verifyBinding(binding string) (string, error) {
+	if binding == "" {


nit: just set defaults in openConnector

ericchiang · 2017-10-23T18:43:29Z

I need to grab an open source SAML provider and try to write up some instructions for how to test this before merging. We want to make sure we can debug future changes to this binding.

ericchiang · 2017-10-26T16:25:00Z

For all my efforts I'm having trouble finding a SAML provider to test this with. @easeway any suggestions?

easeway · 2017-10-26T16:40:45Z

I've verified with vSphere with both POST and redirect binding for request, and POST binding for response. I believe okta works with POST binding for both request and response (as in original code). So we are good with bindings for requests. Unfortunately, I didn't find a provider which supports redirect binding for response. I can continue looking for a provider which allows a services to be registered with a redirect binding for response.

ericchiang reviewed Aug 23, 2017

View reviewed changes

easeway force-pushed the redirect-binding branch from 0cd5141 to 3d9fb4d Compare August 23, 2017 18:43

ericchiang reviewed Aug 23, 2017

View reviewed changes

easeway force-pushed the redirect-binding branch from 3d9fb4d to dd18a33 Compare August 23, 2017 22:02

easeway force-pushed the redirect-binding branch from dd18a33 to 3cfdf6a Compare August 24, 2017 19:08

easeway mentioned this pull request Aug 24, 2017

Resp redirect binding #1048

Closed

ericchiang reviewed Aug 25, 2017

View reviewed changes

easeway force-pushed the redirect-binding branch from 3cfdf6a to b107d43 Compare October 9, 2017 18:41

ericchiang reviewed Oct 23, 2017

View reviewed changes

support SAML HTTP redirect binding

0a77593

easeway force-pushed the redirect-binding branch from b107d43 to 0a77593 Compare October 23, 2017 22:54

amdonov mentioned this pull request Sep 2, 2018

SAML Artifact Binding Support #1282

Closed

srenatus mentioned this pull request Sep 10, 2018

SAML test scaffolding #1295

Open

support SAML HTTP redirect binding #1045

Are you sure you want to change the base?

support SAML HTTP redirect binding #1045

Conversation

easeway commented Aug 23, 2017

easeway commented Aug 23, 2017

ericchiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easeway Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easeway commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easeway commented Aug 23, 2017

ericchiang commented Aug 23, 2017

easeway commented Aug 23, 2017 • edited Loading

easeway commented Aug 24, 2017

ericchiang commented Aug 24, 2017

easeway commented Aug 24, 2017

easeway commented Aug 24, 2017

ericchiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang commented Oct 23, 2017

ericchiang commented Oct 26, 2017

easeway commented Oct 26, 2017

easeway Aug 23, 2017 •

edited

Loading

easeway commented Aug 23, 2017 •

edited

Loading