From b119dc5e780eb6abe5fb7da87dcb3923659b9bc2 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Thu, 25 Mar 2021 15:05:21 +0200 Subject: [PATCH] Fix AuthN Request signing for HTTP-Redirect binding (#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 | 2 +- samlsp/middleware.go | 8 ++- service_provider.go | 64 ++++++++++++++----- service_provider_test.go | 47 ++++++++++++-- ...uceSignedRequestPostBinding_decodedRequest | 1 + ...ignedRequestRedirectBinding_decodedRequest | 1 + ...ceSignedRequestRedirectBinding_queryString | 1 + ...stSPCanProduceSignedRequest_decodedRequest | 1 - 8 files changed, 100 insertions(+), 25 deletions(-) create mode 100644 testdata/TestSPCanProduceSignedRequestPostBinding_decodedRequest create mode 100644 testdata/TestSPCanProduceSignedRequestRedirectBinding_decodedRequest create mode 100644 testdata/TestSPCanProduceSignedRequestRedirectBinding_queryString delete mode 100644 testdata/TestSPCanProduceSignedRequest_decodedRequest diff --git a/identity_provider_test.go b/identity_provider_test.go index b403bdbe..907a5e42 100644 --- a/identity_provider_test.go +++ b/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) diff --git a/samlsp/middleware.go b/samlsp/middleware.go index a84cc9dc..e01bf89e 100644 --- a/samlsp/middleware.go +++ b/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 diff --git a/service_provider.go b/service_provider.go index b59481a3..f76140ab 100644 --- a/service_provider.go +++ b/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,7 +360,8 @@ 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 } @@ -347,8 +369,8 @@ func (sp *ServiceProvider) MakeAuthenticationRequest(idpURL string) (*AuthnReque 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 } diff --git a/service_provider_test.go b/service_provider_test.go index b2d0252e..f26c5388 100644 --- a/service_provider_test.go +++ b/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,6 +243,11 @@ 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) @@ -248,6 +255,36 @@ func TestSPCanProduceSignedRequest(t *testing.T) { 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=\"(.*?)\" />https://15661444.ngrok.io/saml2/metadatahJD/5Zu9pV3hk8yrz7PF3aOAeb4=bAem8RP7VOdIl3m7TlVuh1mq2pymKHLzXRsrlrqum8tkYsXnvSDcUsn5/gCNUd+hbuA2mjp4xVAJbSDoXd6ePQDyCCsEwvY6tnb5aThIFI+FaM2h9UeoENn9cCuFZIp25tqUnvqg45S8kzJw3HZ17o2Qgu9Zsy89yU5kxwCOkEk=MIIB7zCCAVgCCQDFzbKIp7b3MTANBgkqhkiG9w0BAQUFADA8MQswCQYDVQQGEwJVUzELMAkGA1UECAwCR0ExDDAKBgNVBAoMA2ZvbzESMBAGA1UEAwwJbG9jYWxob3N0MB4XDTEzMTAwMjAwMDg1MVoXDTE0MTAwMjAwMDg1MVowPDELMAkGA1UEBhMCVVMxCzAJBgNVBAgMAkdBMQwwCgYDVQQKDANmb28xEjAQBgNVBAMMCWxvY2FsaG9zdDCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEA1PMHYmhZj308kWLhZVT4vOulqx/9ibm5B86fPWwUKKQ2i12MYtz07tzukPymisTDhQaqyJ8Kqb/6JjhmeMnEOdTvSPmHO8m1ZVveJU6NoKRn/mP/BD7FW52WhbrUXLSeHVSKfWkNk6S4hk9MV9TswTvyRIKvRsw0X/gfnqkroJcCAwEAATANBgkqhkiG9w0BAQUFAAOBgQCMMlIO+GNcGekevKgkakpMdAqJfs24maGb90DvTLbRZRD7Xvn1MnVBBS9hzlXiFLYOInXACMW5gcoRFfeTQLSouMM8o57h0uKjfTmuoWHLQLi6hnF+cvCsEFiJZ4AbF+DgmO6TarJ8O05t8zvnOwJlNCASPZRH/JmF8tX0hoHuAQ== \ No newline at end of file diff --git a/testdata/TestSPCanProduceSignedRequestRedirectBinding_decodedRequest b/testdata/TestSPCanProduceSignedRequestRedirectBinding_decodedRequest new file mode 100644 index 00000000..6cae3525 --- /dev/null +++ b/testdata/TestSPCanProduceSignedRequestRedirectBinding_decodedRequest @@ -0,0 +1 @@ +https://15661444.ngrok.io/saml2/metadata \ No newline at end of file diff --git a/testdata/TestSPCanProduceSignedRequestRedirectBinding_queryString b/testdata/TestSPCanProduceSignedRequestRedirectBinding_queryString new file mode 100644 index 00000000..41ddd26c --- /dev/null +++ b/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 \ No newline at end of file diff --git a/testdata/TestSPCanProduceSignedRequest_decodedRequest b/testdata/TestSPCanProduceSignedRequest_decodedRequest deleted file mode 100644 index f5db91ad..00000000 --- a/testdata/TestSPCanProduceSignedRequest_decodedRequest +++ /dev/null @@ -1 +0,0 @@ -https://15661444.ngrok.io/saml2/metadataXQ5+kdgOf34vpAemZRFalLlzjr0=Wtomi/PiWx0bMFlImy5soCrrDbdY4BR2Qb8woGqc8KsVtXAwvl6lfYE2tuoT0YS5ipPLMMsFG8dB1TmLcA+0lnUcqfBiTiiHEwTIo3193RIsoH3STlOmXqBQf9Ax2nRdX+/4HwIYF58lgUzOb+nur+zGL6mYw2xjQBw6YGaX9Cc=MIIB7zCCAVgCCQDFzbKIp7b3MTANBgkqhkiG9w0BAQUFADA8MQswCQYDVQQGEwJVUzELMAkGA1UECAwCR0ExDDAKBgNVBAoMA2ZvbzESMBAGA1UEAwwJbG9jYWxob3N0MB4XDTEzMTAwMjAwMDg1MVoXDTE0MTAwMjAwMDg1MVowPDELMAkGA1UEBhMCVVMxCzAJBgNVBAgMAkdBMQwwCgYDVQQKDANmb28xEjAQBgNVBAMMCWxvY2FsaG9zdDCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEA1PMHYmhZj308kWLhZVT4vOulqx/9ibm5B86fPWwUKKQ2i12MYtz07tzukPymisTDhQaqyJ8Kqb/6JjhmeMnEOdTvSPmHO8m1ZVveJU6NoKRn/mP/BD7FW52WhbrUXLSeHVSKfWkNk6S4hk9MV9TswTvyRIKvRsw0X/gfnqkroJcCAwEAATANBgkqhkiG9w0BAQUFAAOBgQCMMlIO+GNcGekevKgkakpMdAqJfs24maGb90DvTLbRZRD7Xvn1MnVBBS9hzlXiFLYOInXACMW5gcoRFfeTQLSouMM8o57h0uKjfTmuoWHLQLi6hnF+cvCsEFiJZ4AbF+DgmO6TarJ8O05t8zvnOwJlNCASPZRH/JmF8tX0hoHuAQ== \ No newline at end of file