Fix AuthN Request signing for HTTP-Redirect binding (crewjam#339)

* Fix signing for HTTP-Redirect binding

The currently implemented behavior for signing AuthN Requests where
an enveloped signature is added in the XML Document, is appropriate
only when the HTTP-POST binding is used.
Signing for authentication requests when the HTTP-Redirect binding
is in use, is described in
http://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf
section 3.4.4.1 and involves generating a signature of the deflated
form of the AuthN request along with some other URL parameters, mainly
because of URL length considerations.

This commit implements proper AuthNRequest signing support according
to the specification.

* Add comment for function

* linter is picky :)

identity_provider_test.go

@@ -308,7 +308,7 @@ func TestIDPCanHandlePostRequestWithExistingSession(t *testing.T) {
 
 	w := httptest.NewRecorder()
 
-	authRequest, err := test.SP.MakeAuthenticationRequest(test.SP.GetSSOBindingLocation(HTTPRedirectBinding))
+	authRequest, err := test.SP.MakeAuthenticationRequest(test.SP.GetSSOBindingLocation(HTTPRedirectBinding), HTTPRedirectBinding)
 	assert.Check(t, err)
 	authRequestBuf, err := xml.Marshal(authRequest)
 	assert.Check(t, err)

samlsp/middleware.go

@@ -140,7 +140,7 @@ func (m *Middleware) HandleStartAuthFlow(w http.ResponseWriter, r *http.Request)
 		}
 	}
 
-	authReq, err := m.ServiceProvider.MakeAuthenticationRequest(bindingLocation)
+	authReq, err := m.ServiceProvider.MakeAuthenticationRequest(bindingLocation, binding)
 	if err != nil {
 		http.Error(w, err.Error(), http.StatusInternalServerError)
 		return
@@ -157,7 +157,11 @@ func (m *Middleware) HandleStartAuthFlow(w http.ResponseWriter, r *http.Request)
 	}
 
 	if binding == saml.HTTPRedirectBinding {
-		redirectURL := authReq.Redirect(relayState)
+		redirectURL, err := authReq.Redirect(relayState, &m.ServiceProvider)
+		if err != nil {
+			http.Error(w, err.Error(), http.StatusInternalServerError)
+			return
+		}
 		w.Header().Add("Location", redirectURL.String())
 		w.WriteHeader(http.StatusFound)
 		return

service_provider.go

@@ -206,15 +206,15 @@ func (sp *ServiceProvider) Metadata() *EntityDescriptor {
 // the HTTP-Redirect binding. It returns a URL that we will redirect the user to
 // in order to start the auth process.
 func (sp *ServiceProvider) MakeRedirectAuthenticationRequest(relayState string) (*url.URL, error) {
-	req, err := sp.MakeAuthenticationRequest(sp.GetSSOBindingLocation(HTTPRedirectBinding))
+	req, err := sp.MakeAuthenticationRequest(sp.GetSSOBindingLocation(HTTPRedirectBinding), HTTPRedirectBinding)
 	if err != nil {
 		return nil, err
 	}
-	return req.Redirect(relayState), nil
+	return req.Redirect(relayState, sp)
 }
 
 // Redirect returns a URL suitable for using the redirect binding with the request
-func (req *AuthnRequest) Redirect(relayState string) *url.URL {
+func (req *AuthnRequest) Redirect(relayState string, sp *ServiceProvider) (*url.URL, error) {
 	w := &bytes.Buffer{}
 	w1 := base64.NewEncoder(base64.StdEncoding, w)
 	w2, _ := flate.NewWriter(w1, 9)
@@ -227,15 +227,35 @@ func (req *AuthnRequest) Redirect(relayState string) *url.URL {
 	w1.Close()
 
 	rv, _ := url.Parse(req.Destination)
+	// We can't depend on Query().set() as order matters for signing
+	query := rv.RawQuery
+	if len(query) > 0 {
+		query += "&SAMLRequest=" + url.QueryEscape(string(w.Bytes()))
+	} else {
+		query += "SAMLRequest=" + url.QueryEscape(string(w.Bytes()))
+	}
 
-	query := rv.Query()
-	query.Set("SAMLRequest", string(w.Bytes()))
 	if relayState != "" {
-		query.Set("RelayState", relayState)
+		query += "&RelayState=" + relayState
 	}
-	rv.RawQuery = query.Encode()
+	if len(sp.SignatureMethod) > 0 {
+		query += "&SigAlg=" + url.QueryEscape(sp.SignatureMethod)
+		signingContext, err := GetSigningContext(sp)
 
-	return rv
+		if err != nil {
+			return nil, err
+		}
+
+		sig, err := signingContext.SignString(query)
+		if err != nil {
+			return nil, err
+		}
+		query += "&Signature=" + url.QueryEscape(base64.StdEncoding.EncodeToString(sig))
+	}
+
+	rv.RawQuery = query
+
+	return rv, nil
 }
 
 // GetSSOBindingLocation returns URL for the IDP's Single Sign On Service binding
@@ -314,8 +334,9 @@ func (sp *ServiceProvider) getIDPSigningCerts() ([]*x509.Certificate, error) {
 	return certs, nil
 }
 
-// MakeAuthenticationRequest produces a new AuthnRequest object for idpURL.
-func (sp *ServiceProvider) MakeAuthenticationRequest(idpURL string) (*AuthnRequest, error) {
+// MakeAuthenticationRequest produces a new AuthnRequest object to send to the idpURL
+// that uses the specified binding (HTTPRedirectBinding or HTTPPostBinding)
+func (sp *ServiceProvider) MakeAuthenticationRequest(idpURL string, binding string) (*AuthnRequest, error) {
 
 	allowCreate := true
 	nameIDFormat := sp.nameIDFormat()
@@ -339,16 +360,17 @@ func (sp *ServiceProvider) MakeAuthenticationRequest(idpURL string) (*AuthnReque
 		},
 		ForceAuthn: sp.ForceAuthn,
 	}
-	if len(sp.SignatureMethod) > 0 {
+	// We don't need to sign the XML document if the IDP uses HTTP-Redirect binding
+	if len(sp.SignatureMethod) > 0 && binding == HTTPPostBinding {
 		if err := sp.SignAuthnRequest(&req); err != nil {
 			return nil, err
 		}
 	}
 	return &req, nil
 }
 
-// SignAuthnRequest adds the `Signature` element to the `AuthnRequest`.
-func (sp *ServiceProvider) SignAuthnRequest(req *AuthnRequest) error {
+// GetSigningContext returns a dsig.SigningContext initialized based on the Service Provider's configuration
+func GetSigningContext(sp *ServiceProvider) (*dsig.SigningContext, error) {
 	keyPair := tls.Certificate{
 		Certificate: [][]byte{sp.Certificate.Raw},
 		PrivateKey:  sp.Key,
@@ -363,15 +385,25 @@ func (sp *ServiceProvider) SignAuthnRequest(req *AuthnRequest) error {
 	if sp.SignatureMethod != dsig.RSASHA1SignatureMethod &&
 		sp.SignatureMethod != dsig.RSASHA256SignatureMethod &&
 		sp.SignatureMethod != dsig.RSASHA512SignatureMethod {
-		return fmt.Errorf("invalid signing method %s", sp.SignatureMethod)
+		return nil, fmt.Errorf("invalid signing method %s", sp.SignatureMethod)
 	}
 	signatureMethod := sp.SignatureMethod
 	signingContext := dsig.NewDefaultSigningContext(keyStore)
 	signingContext.Canonicalizer = dsig.MakeC14N10ExclusiveCanonicalizerWithPrefixList(canonicalizerPrefixList)
 	if err := signingContext.SetSignatureMethod(signatureMethod); err != nil {
-		return err
+		return nil, err
 	}
 
+	return signingContext, nil
+}
+
+// SignAuthnRequest adds the `Signature` element to the `AuthnRequest`.
+func (sp *ServiceProvider) SignAuthnRequest(req *AuthnRequest) error {
+
+	signingContext, err := GetSigningContext(sp)
+	if err != nil {
+		return err
+	}
 	assertionEl := req.Element()
 
 	signedRequestEl, err := signingContext.SignEnveloped(assertionEl)
@@ -388,7 +420,7 @@ func (sp *ServiceProvider) SignAuthnRequest(req *AuthnRequest) error {
 // the HTTP-POST binding. It returns HTML text representing an HTML form that
 // can be sent presented to a browser to initiate the login process.
 func (sp *ServiceProvider) MakePostAuthenticationRequest(relayState string) ([]byte, error) {
-	req, err := sp.MakeAuthenticationRequest(sp.GetSSOBindingLocation(HTTPPostBinding))
+	req, err := sp.MakeAuthenticationRequest(sp.GetSSOBindingLocation(HTTPPostBinding), HTTPPostBinding)
 	if err != nil {
 		return nil, err
 	}

service_provider_test.go

@@ -6,8 +6,10 @@ import (
 	"crypto/x509"
 	"encoding/base64"
 	"encoding/xml"
+	"html"
 	"net/http"
 	"net/url"
+	"regexp"
 	"strings"
 	"testing"
 	"time"
@@ -79,25 +81,25 @@ func TestSPCanSetAuthenticationNameIDFormat(t *testing.T) {
 	}
 
 	// defaults to "transient"
-	req, err := s.MakeAuthenticationRequest("")
+	req, err := s.MakeAuthenticationRequest("", HTTPRedirectBinding)
 	assert.Check(t, err)
 	assert.Check(t, is.Equal(string(TransientNameIDFormat), *req.NameIDPolicy.Format))
 
 	// explicitly set to "transient"
 	s.AuthnNameIDFormat = TransientNameIDFormat
-	req, err = s.MakeAuthenticationRequest("")
+	req, err = s.MakeAuthenticationRequest("", HTTPRedirectBinding)
 	assert.Check(t, err)
 	assert.Check(t, is.Equal(string(TransientNameIDFormat), *req.NameIDPolicy.Format))
 
 	// explicitly set to "unspecified"
 	s.AuthnNameIDFormat = UnspecifiedNameIDFormat
-	req, err = s.MakeAuthenticationRequest("")
+	req, err = s.MakeAuthenticationRequest("", HTTPRedirectBinding)
 	assert.Check(t, err)
 	assert.Check(t, is.Equal("", *req.NameIDPolicy.Format))
 
 	// explicitly set to "emailAddress"
 	s.AuthnNameIDFormat = EmailAddressNameIDFormat
-	req, err = s.MakeAuthenticationRequest("")
+	req, err = s.MakeAuthenticationRequest("", HTTPRedirectBinding)
 	assert.Check(t, err)
 	assert.Check(t, is.Equal(string(EmailAddressNameIDFormat), *req.NameIDPolicy.Format))
 }
@@ -221,7 +223,7 @@ func TestSPCanProducePostRequest(t *testing.T) {
 	golden.Assert(t, string(form), t.Name()+"_form")
 }
 
-func TestSPCanProduceSignedRequest(t *testing.T) {
+func TestSPCanProduceSignedRequestRedirectBinding(t *testing.T) {
 	test := NewServiceProviderTest(t)
 	TimeNow = func() time.Time {
 		rv, _ := time.Parse("Mon Jan 2 15:04:05.999999999 UTC 2006", "Mon Dec 1 01:31:21.123456789 UTC 2015")
@@ -241,13 +243,48 @@ func TestSPCanProduceSignedRequest(t *testing.T) {
 
 	redirectURL, err := s.MakeRedirectAuthenticationRequest("relayState")
 	assert.Check(t, err)
+	// Signature we check against in the query string was validated with
+	// https://www.samltool.com/validate_authn_req.php . Once we add
+	// support for validating signed AuthN requests in the IDP implementation
+	// we can switch to testing using that.
+	golden.Assert(t, redirectURL.RawQuery, t.Name()+"_queryString")
 
 	decodedRequest, err := testsaml.ParseRedirectRequest(redirectURL)
 	assert.Check(t, err)
 	assert.Check(t, is.Equal("idp.testshib.org",
 		redirectURL.Host))
 	assert.Check(t, is.Equal("/idp/profile/SAML2/Redirect/SSO",
 		redirectURL.Path))
+	// Contains no enveloped signature
+	golden.Assert(t, string(decodedRequest), t.Name()+"_decodedRequest")
+}
+
+func TestSPCanProduceSignedRequestPostBinding(t *testing.T) {
+	test := NewServiceProviderTest(t)
+	TimeNow = func() time.Time {
+		rv, _ := time.Parse("Mon Jan 2 15:04:05.999999999 UTC 2006", "Mon Dec 1 01:31:21.123456789 UTC 2015")
+		return rv
+	}
+	Clock = dsig.NewFakeClockAt(TimeNow())
+	s := ServiceProvider{
+		Key:             test.Key,
+		Certificate:     test.Certificate,
+		MetadataURL:     mustParseURL("https://15661444.ngrok.io/saml2/metadata"),
+		AcsURL:          mustParseURL("https://15661444.ngrok.io/saml2/acs"),
+		IDPMetadata:     &EntityDescriptor{},
+		SignatureMethod: dsig.RSASHA1SignatureMethod,
+	}
+	err := xml.Unmarshal(test.IDPMetadata, &s.IDPMetadata)
+	assert.Check(t, err)
+
+	htmlForm, err := s.MakePostAuthenticationRequest("relayState")
+	assert.Check(t, err)
+	rgx := regexp.MustCompile(`\"SAMLRequest\" value=\"(.*?)\" /><input`)
+	rs := rgx.FindStringSubmatch(string(htmlForm))
+	assert.Check(t, len(rs) == 2)
+
+	decodedRequest, err := base64.StdEncoding.DecodeString(html.UnescapeString(rs[1]))
+	assert.Check(t, err)
 	golden.Assert(t, string(decodedRequest), t.Name()+"_decodedRequest")
 }
 

testdata/TestSPCanProduceSignedRequestPostBinding_decodedRequest

@@ -0,0 +1 @@
+<samlp:AuthnRequest xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" ID="id-00020406080a0c0e10121416181a1c1e20222426" Version="2.0" IssueInstant="2015-12-01T01:31:21.123Z" Destination="https://idp.testshib.org/idp/profile/SAML2/POST/SSO" AssertionConsumerServiceURL="https://15661444.ngrok.io/saml2/acs" ProtocolBinding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST"><saml:Issuer Format="urn:oasis:names:tc:SAML:2.0:nameid-format:entity">https://15661444.ngrok.io/saml2/metadata</saml:Issuer><ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#"><ds:SignedInfo><ds:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/><ds:SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/><ds:Reference URI="#id-00020406080a0c0e10121416181a1c1e20222426"><ds:Transforms><ds:Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature"/><ds:Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/></ds:Transforms><ds:DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/><ds:DigestValue>hJD/5Zu9pV3hk8yrz7PF3aOAeb4=</ds:DigestValue></ds:Reference></ds:SignedInfo><ds:SignatureValue>bAem8RP7VOdIl3m7TlVuh1mq2pymKHLzXRsrlrqum8tkYsXnvSDcUsn5/gCNUd+hbuA2mjp4xVAJbSDoXd6ePQDyCCsEwvY6tnb5aThIFI+FaM2h9UeoENn9cCuFZIp25tqUnvqg45S8kzJw3HZ17o2Qgu9Zsy89yU5kxwCOkEk=</ds:SignatureValue><ds:KeyInfo><ds:X509Data><ds:X509Certificate>MIIB7zCCAVgCCQDFzbKIp7b3MTANBgkqhkiG9w0BAQUFADA8MQswCQYDVQQGEwJVUzELMAkGA1UECAwCR0ExDDAKBgNVBAoMA2ZvbzESMBAGA1UEAwwJbG9jYWxob3N0MB4XDTEzMTAwMjAwMDg1MVoXDTE0MTAwMjAwMDg1MVowPDELMAkGA1UEBhMCVVMxCzAJBgNVBAgMAkdBMQwwCgYDVQQKDANmb28xEjAQBgNVBAMMCWxvY2FsaG9zdDCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEA1PMHYmhZj308kWLhZVT4vOulqx/9ibm5B86fPWwUKKQ2i12MYtz07tzukPymisTDhQaqyJ8Kqb/6JjhmeMnEOdTvSPmHO8m1ZVveJU6NoKRn/mP/BD7FW52WhbrUXLSeHVSKfWkNk6S4hk9MV9TswTvyRIKvRsw0X/gfnqkroJcCAwEAATANBgkqhkiG9w0BAQUFAAOBgQCMMlIO+GNcGekevKgkakpMdAqJfs24maGb90DvTLbRZRD7Xvn1MnVBBS9hzlXiFLYOInXACMW5gcoRFfeTQLSouMM8o57h0uKjfTmuoWHLQLi6hnF+cvCsEFiJZ4AbF+DgmO6TarJ8O05t8zvnOwJlNCASPZRH/JmF8tX0hoHuAQ==</ds:X509Certificate></ds:X509Data></ds:KeyInfo></ds:Signature><samlp:NameIDPolicy Format="urn:oasis:names:tc:SAML:2.0:nameid-format:transient" AllowCreate="true"/></samlp:AuthnRequest>

testdata/TestSPCanProduceSignedRequestRedirectBinding_decodedRequest

@@ -0,0 +1 @@
+<samlp:AuthnRequest xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" ID="id-00020406080a0c0e10121416181a1c1e20222426" Version="2.0" IssueInstant="2015-12-01T01:31:21.123Z" Destination="https://idp.testshib.org/idp/profile/SAML2/Redirect/SSO" AssertionConsumerServiceURL="https://15661444.ngrok.io/saml2/acs" ProtocolBinding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST"><saml:Issuer Format="urn:oasis:names:tc:SAML:2.0:nameid-format:entity">https://15661444.ngrok.io/saml2/metadata</saml:Issuer><samlp:NameIDPolicy Format="urn:oasis:names:tc:SAML:2.0:nameid-format:transient" AllowCreate="true"/></samlp:AuthnRequest>

testdata/TestSPCanProduceSignedRequestRedirectBinding_queryString

@@ -0,0 +1 @@
+SAMLRequest=nFJNb9swDP0rhu62RNU1CqE2kDUYFqBbgzjbYTfVZhNituSJ9Lb%2B%2B8Fph2WXDOiV4uP70LtlPw6TW81yDDv8PiNL9mscArvloVZzCi56JnbBj8hOOteuPt47WxjnmTEJxaDOINNlzJSixC4OKtusa0V9boyxpjSVuTHedAbBgIUSKrgBDx2gNdba0lYq%2B4KJKYZa2cKobMM84yaw%2BCC1sgauc7C5gb0BdwXOQgH26qvK1shCwcsJeRSZ2GlN%2FVQIsvCRHouYDstATyk%2B0YB60Wr1DntK2Ilu2weVrf5YvYuB5xFTi%2BkHdfh5d%2F%2F3KlxXFZRlWYRDit8KinoJxGrfscq2r8bfUegpHC6n9PiyxO7Dfr%2FNtw%2FtXjWnn3In2yl7H9Po5fKRZUJ9%2FnRadRiE5Fk1%2FxM7ovjei7%2FVZ3zNa00%2B%2BRE3620cqHt%2BgwZJPjBhEJWthiH%2BvEvoBWslaUalmxfKf8vY%2FA4AAP%2F%2F&RelayState=relayState&SigAlg=http%3A%2F%2Fwww.w3.org%2F2000%2F09%2Fxmldsig%23rsa-sha1&Signature=WqMc7vKRJVNXwNHJmTemdfw5OML2XkLntYw%2FzwKoLMfavV%2FYy6fBP0GeGYlJVMweZBvbpjwoe%2BgpRkUCHKDUgixCG7hPi41p6MpQC%2Fp7ExTW5plvlS97iVAOvaF5V1MjvQCgBNKYnKNnvwAuxK%2Bu3N4rZjwGM%2F4JGgjJ5pannFQ%3D