From ea6eee2a0b0a8ed3c3d2f60653dc2f958ad355ab Mon Sep 17 00:00:00 2001 From: jguer Date: Wed, 20 Jul 2022 17:32:57 +0200 Subject: [PATCH 1/4] implement query signature verification add support for sha1 & sha512 add tests use query sign in redirect implement review feedback - Return error if signature is unsupported - wrap errors Co-authored-by: Ieva Co-authored-by: Orgad Shaneh --- service_provider.go | 35 ++++---- service_provider_signed.go | 141 ++++++++++++++++++++++++++++++++ service_provider_signed_test.go | 117 ++++++++++++++++++++++++++ testdata/SP_IDPMetadata_signing | 96 ++++++++++++++++++++++ 4 files changed, 374 insertions(+), 15 deletions(-) create mode 100644 service_provider_signed.go create mode 100644 service_provider_signed_test.go create mode 100644 testdata/SP_IDPMetadata_signing diff --git a/service_provider.go b/service_provider.go index 3eac33f7..415b90e0 100644 --- a/service_provider.go +++ b/service_provider.go @@ -259,29 +259,24 @@ func (req *AuthnRequest) Redirect(relayState string, sp *ServiceProvider) (*url. rv, _ := url.Parse(req.Destination) // We can't depend on Query().set() as order matters for signing + reqString := string(w.Bytes()) query := rv.RawQuery if len(query) > 0 { - query += "&SAMLRequest=" + url.QueryEscape(string(w.Bytes())) + query += "&" + string(samlRequest) + "=" + url.QueryEscape(reqString) } else { - query += "SAMLRequest=" + url.QueryEscape(string(w.Bytes())) + query += string(samlRequest) + "=" + url.QueryEscape(reqString) } if relayState != "" { query += "&RelayState=" + relayState } if len(sp.SignatureMethod) > 0 { - query += "&SigAlg=" + url.QueryEscape(sp.SignatureMethod) - signingContext, err := GetSigningContext(sp) - - if err != nil { - return nil, err + var errSig error + query, errSig = sp.signQuery(samlRequest, query, reqString, relayState) + if errSig != nil { + return nil, errSig } - sig, err := signingContext.SignString(query) - if err != nil { - return nil, err - } - query += "&Signature=" + url.QueryEscape(base64.StdEncoding.EncodeToString(sig)) } rv.RawQuery = query @@ -1456,8 +1451,9 @@ func (sp *ServiceProvider) nameIDFormat() string { // ValidateLogoutResponseRequest validates the LogoutResponse content from the request func (sp *ServiceProvider) ValidateLogoutResponseRequest(req *http.Request) error { - if data := req.URL.Query().Get("SAMLResponse"); data != "" { - return sp.ValidateLogoutResponseRedirect(data) + query := req.URL.Query() + if data := query.Get("SAMLResponse"); data != "" { + return sp.ValidateLogoutResponseRedirect(query) } err := req.ParseForm() @@ -1512,7 +1508,8 @@ func (sp *ServiceProvider) ValidateLogoutResponseForm(postFormData string) error // // URL Binding appears to be gzip / flate encoded // See https://www.oasis-open.org/committees/download.php/20645/sstc-saml-tech-overview-2%200-draft-10.pdf 6.6 -func (sp *ServiceProvider) ValidateLogoutResponseRedirect(queryParameterData string) error { +func (sp *ServiceProvider) ValidateLogoutResponseRedirect(query url.Values) error { + queryParameterData := query.Get("SAMLResponse") retErr := &InvalidResponseError{ Now: TimeNow(), } @@ -1534,6 +1531,13 @@ func (sp *ServiceProvider) ValidateLogoutResponseRedirect(queryParameterData str return err } + if query.Get("Signature") != "" && query.Get("SigAlg") != "" { + if err := sp.validateQuerySig(query); err != nil { + retErr.PrivateErr = err + return retErr + } + } + doc := etree.NewDocument() if err := doc.ReadFromBytes(rawResponseBuf); err != nil { retErr.PrivateErr = err @@ -1553,6 +1557,7 @@ func (sp *ServiceProvider) ValidateLogoutResponseRedirect(queryParameterData str if err := sp.validateLogoutResponse(&resp); err != nil { return err } + return nil } diff --git a/service_provider_signed.go b/service_provider_signed.go new file mode 100644 index 00000000..d8b02f06 --- /dev/null +++ b/service_provider_signed.go @@ -0,0 +1,141 @@ +package saml + +import ( + "crypto" + "crypto/rsa" + "crypto/sha1" // #nosec G505 + "crypto/sha256" + "crypto/sha512" + "crypto/x509" + "encoding/base64" + "errors" + "fmt" + "net/url" + + dsig "github.com/russellhaering/goxmldsig" +) + +type reqType string + +const ( + samlRequest reqType = "SAMLRequest" + samlResponse reqType = "SAMLResponse" +) + +var ( + // ErrInvalidQuerySignature is returned when the query signature is invalid + ErrInvalidQuerySignature = errors.New("invalid query signature") + // ErrNoQuerySignature is returned when the query does not contain a signature + ErrNoQuerySignature = errors.New("query Signature or SigAlg not found") +) + +// Sign Query with the SP private key. +// Returns provided query with the SigAlg and Signature parameters added. +func (sp *ServiceProvider) signQuery(reqT reqType, query, body, relayState string) (string, error) { + signingContext, err := GetSigningContext(sp) + + // Encode Query as standard demands. query.Encode() is not standard compliant + toHash := string(reqT) + "=" + url.QueryEscape(body) + if relayState != "" { + toHash += "&RelayState=" + url.QueryEscape(relayState) + } + + toHash += "&SigAlg=" + url.QueryEscape(sp.SignatureMethod) + + if err != nil { + return "", err + } + + sig, err := signingContext.SignString(toHash) + if err != nil { + return "", err + } + + query += "&SigAlg=" + url.QueryEscape(sp.SignatureMethod) + query += "&Signature=" + url.QueryEscape(base64.StdEncoding.EncodeToString(sig)) + + return query, nil +} + +// validateSig validation of the signature of the Redirect Binding in query values +// Query is valid if return is nil +func (sp *ServiceProvider) validateQuerySig(query url.Values) error { + sig := query.Get("Signature") + alg := query.Get("SigAlg") + if sig == "" || alg == "" { + return ErrNoQuerySignature + } + + certs, err := sp.getIDPSigningCerts() + if err != nil { + return err + } + + respType := "" + if query.Get("SAMLResponse") != "" { + respType = "SAMLResponse" + } else if query.Get("SAMLRequest") != "" { + respType = "SAMLRequest" + } else { + return fmt.Errorf("No SAMLResponse or SAMLRequest found in query") + } + + // Encode Query as standard demands. + // query.Encode() is not standard compliant + // as query encoding order matters + res := respType + "=" + url.QueryEscape(query.Get(respType)) + + relayState := query.Get("RelayState") + if relayState != "" { + res += "&RelayState=" + url.QueryEscape(relayState) + } + + res += "&SigAlg=" + url.QueryEscape(alg) + + // Signature is base64 encoded + sigBytes, err := base64.StdEncoding.DecodeString(sig) + if err != nil { + return fmt.Errorf("failed to decode signature: %w", err) + } + + var ( + hashed []byte + hashAlg crypto.Hash + sigAlg x509.SignatureAlgorithm + ) + + // Hashed Query + switch alg { + case dsig.RSASHA256SignatureMethod: + hashed256 := sha256.Sum256([]byte(res)) + hashed = hashed256[:] + hashAlg = crypto.SHA256 + sigAlg = x509.SHA256WithRSA + case dsig.RSASHA512SignatureMethod: + hashed512 := sha512.Sum512([]byte(res)) + hashed = hashed512[:] + hashAlg = crypto.SHA512 + sigAlg = x509.SHA512WithRSA + case dsig.RSASHA1SignatureMethod: + hashed1 := sha1.Sum([]byte(res)) // #nosec G401 + hashed = hashed1[:] + hashAlg = crypto.SHA1 + sigAlg = x509.SHA1WithRSA + default: + return fmt.Errorf("unsupported signature algorithm: %s", alg) + } + + // validate signature + for _, cert := range certs { + // verify cert is RSA + if cert.SignatureAlgorithm != sigAlg { + continue + } + + if err := rsa.VerifyPKCS1v15(cert.PublicKey.(*rsa.PublicKey), hashAlg, hashed, sigBytes); err == nil { + return nil + } + } + + return ErrInvalidQuerySignature +} diff --git a/service_provider_signed_test.go b/service_provider_signed_test.go new file mode 100644 index 00000000..a45b8cbd --- /dev/null +++ b/service_provider_signed_test.go @@ -0,0 +1,117 @@ +package saml + +import ( + "crypto/rsa" + "encoding/base64" + "encoding/xml" + "net/url" + "testing" + + dsig "github.com/russellhaering/goxmldsig" + "gotest.tools/assert" + "gotest.tools/golden" +) + +// Given a SAMLRequest query string, sign the query and validate signature +// Using same Cert for SP and IdP in order to test +func TestSigningAndValidation(t *testing.T) { + type testCase struct { + desc string + relayState string + requestType reqType + wantErr bool + wantRawQuery string + } + + testCases := []testCase{ + { + desc: "validate signature of SAMLRequest with relayState", + relayState: "AAAAAAAAAAAA", + requestType: samlRequest, + wantRawQuery: "SAMLRequest=PHNhbWxwOkF1dGhuUmVxdWVzdCB4bWxuczpzYW1sPSJ1cm46b2FzaXM6bmFtZXM6dGM6U0FNTDoyLjA6YXNzZXJ0aW9uIiB4bWxuczpzYW1scD0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOnByb3RvY29sIiBJRD0iaWQtMDAwMjA0MDYwODBhMGMwZTEwMTIxNDE2MTgxYTFjMWUyMDIyMjQyNiIgVmVyc2lvbj0iMi4wIiBJc3N1ZUluc3RhbnQ9IjIwMTUtMTItMDFUMDE6NTc6MDlaIiBEZXN0aW5hdGlvbj0iaHR0cHM6Ly9pZHAuZXhhbXBsZS5jb20vc2FtbC9zc28iIEFzc2VydGlvbkNvbnN1bWVyU2VydmljZVVSTD0iaHR0cHM6Ly9zcC5leGFtcGxlLmNvbS9zYW1sMi9hY3MiIFByb3RvY29sQmluZGluZz0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOmJpbmRpbmdzOkhUVFAtUE9TVCI%2BPHNhbWw6SXNzdWVyIEZvcm1hdD0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOm5hbWVpZC1mb3JtYXQ6ZW50aXR5Ij5odHRwczovL3NwLmV4YW1wbGUuY29tL3NhbWwyL21ldGFkYXRhPC9zYW1sOklzc3Vlcj48c2FtbHA6TmFtZUlEUG9saWN5IEZvcm1hdD0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOm5hbWVpZC1mb3JtYXQ6dHJhbnNpZW50IiBBbGxvd0NyZWF0ZT0idHJ1ZSIvPjwvc2FtbHA6QXV0aG5SZXF1ZXN0Pg%3D%3D&RelayState=AAAAAAAAAAAA&SigAlg=http%3A%2F%2Fwww.w3.org%2F2000%2F09%2Fxmldsig%23rsa-sha1&Signature=zWAF4S%2FIs7tfmEriOsT5Fm8EFOGS3iCq6OxP5i7hM%2BMPwAoXwdDz6fKH8euS1gQ3sGOZBdHD588FZLvnO1OeCxLaEsxHMVKsAZSZFLBmPPwqB6e%2B84cCwX2szOeoMROaR%2B36mdoBDRQz36JIvyBBG%2FND9x41k%2FGQuAuwk%2B9fkuE%3D", + }, + { + desc: "validate signature of SAML request without relay state", + relayState: "", + requestType: samlRequest, + wantRawQuery: "SAMLRequest=PHNhbWxwOkF1dGhuUmVxdWVzdCB4bWxuczpzYW1sPSJ1cm46b2FzaXM6bmFtZXM6dGM6U0FNTDoyLjA6YXNzZXJ0aW9uIiB4bWxuczpzYW1scD0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOnByb3RvY29sIiBJRD0iaWQtMDAwMjA0MDYwODBhMGMwZTEwMTIxNDE2MTgxYTFjMWUyMDIyMjQyNiIgVmVyc2lvbj0iMi4wIiBJc3N1ZUluc3RhbnQ9IjIwMTUtMTItMDFUMDE6NTc6MDlaIiBEZXN0aW5hdGlvbj0iaHR0cHM6Ly9pZHAuZXhhbXBsZS5jb20vc2FtbC9zc28iIEFzc2VydGlvbkNvbnN1bWVyU2VydmljZVVSTD0iaHR0cHM6Ly9zcC5leGFtcGxlLmNvbS9zYW1sMi9hY3MiIFByb3RvY29sQmluZGluZz0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOmJpbmRpbmdzOkhUVFAtUE9TVCI%2BPHNhbWw6SXNzdWVyIEZvcm1hdD0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOm5hbWVpZC1mb3JtYXQ6ZW50aXR5Ij5odHRwczovL3NwLmV4YW1wbGUuY29tL3NhbWwyL21ldGFkYXRhPC9zYW1sOklzc3Vlcj48c2FtbHA6TmFtZUlEUG9saWN5IEZvcm1hdD0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOm5hbWVpZC1mb3JtYXQ6dHJhbnNpZW50IiBBbGxvd0NyZWF0ZT0idHJ1ZSIvPjwvc2FtbHA6QXV0aG5SZXF1ZXN0Pg%3D%3D&SigAlg=http%3A%2F%2Fwww.w3.org%2F2000%2F09%2Fxmldsig%23rsa-sha1&Signature=HDdoHJSdkYh9%2BmE7RZ1LXcsAWIMJ6LuzKJgwLxH%2BQ4sKFlh8b5moFuQ%2B7rPEwoTcg9SjgCGV5rW9v8PrSU7WGKcLfAbeVwXWyU94ghjFZHEj%2BFCDpsfTD750ZPAPVnhVr0GogFZZ7c%2BEWX4NAqL4CYxDvsg56o%2BpOjw62G%2FyPDc%3D", + }, + { + desc: "validate signature of SAML response with relay state", + relayState: "AAAAAAAAAAAA", + requestType: samlResponse, + wantRawQuery: "SAMLResponse=PHNhbWxwOkF1dGhuUmVxdWVzdCB4bWxuczpzYW1sPSJ1cm46b2FzaXM6bmFtZXM6dGM6U0FNTDoyLjA6YXNzZXJ0aW9uIiB4bWxuczpzYW1scD0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOnByb3RvY29sIiBJRD0iaWQtMDAwMjA0MDYwODBhMGMwZTEwMTIxNDE2MTgxYTFjMWUyMDIyMjQyNiIgVmVyc2lvbj0iMi4wIiBJc3N1ZUluc3RhbnQ9IjIwMTUtMTItMDFUMDE6NTc6MDlaIiBEZXN0aW5hdGlvbj0iaHR0cHM6Ly9pZHAuZXhhbXBsZS5jb20vc2FtbC9zc28iIEFzc2VydGlvbkNvbnN1bWVyU2VydmljZVVSTD0iaHR0cHM6Ly9zcC5leGFtcGxlLmNvbS9zYW1sMi9hY3MiIFByb3RvY29sQmluZGluZz0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOmJpbmRpbmdzOkhUVFAtUE9TVCI%2BPHNhbWw6SXNzdWVyIEZvcm1hdD0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOm5hbWVpZC1mb3JtYXQ6ZW50aXR5Ij5odHRwczovL3NwLmV4YW1wbGUuY29tL3NhbWwyL21ldGFkYXRhPC9zYW1sOklzc3Vlcj48c2FtbHA6TmFtZUlEUG9saWN5IEZvcm1hdD0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOm5hbWVpZC1mb3JtYXQ6dHJhbnNpZW50IiBBbGxvd0NyZWF0ZT0idHJ1ZSIvPjwvc2FtbHA6QXV0aG5SZXF1ZXN0Pg%3D%3D&RelayState=AAAAAAAAAAAA&SigAlg=http%3A%2F%2Fwww.w3.org%2F2000%2F09%2Fxmldsig%23rsa-sha1&Signature=JDeiWfLgV7SZqgqU64wgtAHS%2FqtF2c3c%2B9g1vdfRHn03tm5jrgsvJtIYg1BD8HoejCoyruH3xgDz1i2qqecVcUiAdaVgVvhn0JWJ%2BzeN9YpUFTEQ4Ah1pwezlSArzuz5esgYzSkemViox313HePWZ%2Fd0FAmtdXuGHA8O0Lp%2F4Ws%3D", + }, + } + + idpMetadata := golden.Get(t, "SP_IDPMetadata_signing") + s := ServiceProvider{ + Key: mustParsePrivateKey(golden.Get(t, "idp_key.pem")).(*rsa.PrivateKey), + Certificate: mustParseCertificate(golden.Get(t, "idp_cert.pem")), + MetadataURL: mustParseURL("https://15661444.ngrok.io/saml2/metadata"), + AcsURL: mustParseURL("https://15661444.ngrok.io/saml2/acs"), + SignatureMethod: dsig.RSASHA1SignatureMethod, + } + + err := xml.Unmarshal(idpMetadata, &s.IDPMetadata) + idpCert, err := s.getIDPSigningCerts() + + assert.Check(t, err == nil) + assert.Check(t, + s.Certificate.Issuer.CommonName == idpCert[0].Issuer.CommonName, "expected %s, got %s", + s.Certificate.Issuer.CommonName, idpCert[0].Issuer.CommonName) + + req := golden.Get(t, "idp_authn_request.xml") + reqString := base64.StdEncoding.EncodeToString(req) + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + relayState := tc.relayState + + rawQuery := string(tc.requestType) + "=" + url.QueryEscape(reqString) + + if relayState != "" { + rawQuery += "&RelayState=" + relayState + } + + rawQuery, err = s.signQuery(tc.requestType, rawQuery, reqString, relayState) + assert.NilError(t, err, "error signing query: %s", err) + + assert.Equal(t, tc.wantRawQuery, rawQuery) + + query, err := url.ParseQuery(rawQuery) + assert.NilError(t, err, "error parsing query: %s", err) + + err = s.validateQuerySig(query) + assert.NilError(t, err, "error validating query: %s", err) + }) + } +} + +// Given a raw query with an unsupported signature method, the signature should be rejected. +func TestInvalidSignatureAlgorithm(t *testing.T) { + rawQuery := "SAMLRequest=PHNhbWxwOkF1dGhuUmVxdWVzdCB4bWxuczpzYW1sPSJ1cm46b2FzaXM6bmFtZXM6dGM6U0FNTDoyLjA6YXNzZXJ0aW9uIiB4bWxuczpzYW1scD0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOnByb3RvY29sIiBJRD0iaWQtMDAwMjA0MDYwODBhMGMwZTEwMTIxNDE2MTgxYTFjMWUyMDIyMjQyNiIgVmVyc2lvbj0iMi4wIiBJc3N1ZUluc3RhbnQ9IjIwMTUtMTItMDFUMDE6NTc6MDlaIiBEZXN0aW5hdGlvbj0iaHR0cHM6Ly9pZHAuZXhhbXBsZS5jb20vc2FtbC9zc28iIEFzc2VydGlvbkNvbnN1bWVyU2VydmljZVVSTD0iaHR0cHM6Ly9zcC5leGFtcGxlLmNvbS9zYW1sMi9hY3MiIFByb3RvY29sQmluZGluZz0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOmJpbmRpbmdzOkhUVFAtUE9TVCI%2BPHNhbWw6SXNzdWVyIEZvcm1hdD0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOm5hbWVpZC1mb3JtYXQ6ZW50aXR5Ij5odHRwczovL3NwLmV4YW1wbGUuY29tL3NhbWwyL21ldGFkYXRhPC9zYW1sOklzc3Vlcj48c2FtbHA6TmFtZUlEUG9saWN5IEZvcm1hdD0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOm5hbWVpZC1mb3JtYXQ6dHJhbnNpZW50IiBBbGxvd0NyZWF0ZT0idHJ1ZSIvPjwvc2FtbHA6QXV0aG5SZXF1ZXN0Pg%3D%3D&RelayState=AAAAAAAAAAAA&SigAlg=http%3A%2F%2Fwww.w3.org%2F2000%2F09%2Fxmldsig%23rsa-sha384&Signature=zWAF4S%2FIs7tfmEriOsT5Fm8EFOGS3iCq6OxP5i7hM%2BMPwAoXwdDz6fKH8euS1gQ3sGOZBdHD588FZLvnO1OeCxLaEsxHMVKsAZSZFLBmPPwqB6e%2B84cCwX2szOeoMROaR%2B36mdoBDRQz36JIvyBBG%2FND9x41k%2FGQuAuwk%2B9fkuE%3D" + + idpMetadata := golden.Get(t, "SP_IDPMetadata_signing") + s := ServiceProvider{ + Key: mustParsePrivateKey(golden.Get(t, "idp_key.pem")).(*rsa.PrivateKey), + Certificate: mustParseCertificate(golden.Get(t, "idp_cert.pem")), + MetadataURL: mustParseURL("https://15661444.ngrok.io/saml2/metadata"), + AcsURL: mustParseURL("https://15661444.ngrok.io/saml2/acs"), + SignatureMethod: dsig.RSASHA1SignatureMethod, + } + + err := xml.Unmarshal(idpMetadata, &s.IDPMetadata) + idpCert, err := s.getIDPSigningCerts() + + assert.Check(t, err == nil) + assert.Check(t, + s.Certificate.Issuer.CommonName == idpCert[0].Issuer.CommonName, "expected %s, got %s", + s.Certificate.Issuer.CommonName, idpCert[0].Issuer.CommonName) + + query, err := url.ParseQuery(rawQuery) + assert.NilError(t, err, "error parsing query: %s", err) + + err = s.validateQuerySig(query) + assert.Error(t, err, "unsupported signature algorithm: http://www.w3.org/2000/09/xmldsig#rsa-sha384") +} diff --git a/testdata/SP_IDPMetadata_signing b/testdata/SP_IDPMetadata_signing new file mode 100644 index 00000000..58793ea9 --- /dev/null +++ b/testdata/SP_IDPMetadata_signing @@ -0,0 +1,96 @@ + + + + + + + + + + + + + + + testshib.org + + TestShib Test IdP + TestShib IdP. Use this as a source of attributes + for your test SP. + https://www.testshib.org/testshibtwo.jpg + + + + + + MIIB7zCCAVgCCQDFzbKIp7b3MTANBgkqhkiG9w0BAQUFADA8MQswCQYDVQQGEwJV + UzELMAkGA1UECAwCR0ExDDAKBgNVBAoMA2ZvbzESMBAGA1UEAwwJbG9jYWxob3N0 + MB4XDTEzMTAwMjAwMDg1MVoXDTE0MTAwMjAwMDg1MVowPDELMAkGA1UEBhMCVVMx + CzAJBgNVBAgMAkdBMQwwCgYDVQQKDANmb28xEjAQBgNVBAMMCWxvY2FsaG9zdDCB + nzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEA1PMHYmhZj308kWLhZVT4vOulqx/9 + ibm5B86fPWwUKKQ2i12MYtz07tzukPymisTDhQaqyJ8Kqb/6JjhmeMnEOdTvSPmH + O8m1ZVveJU6NoKRn/mP/BD7FW52WhbrUXLSeHVSKfWkNk6S4hk9MV9TswTvyRIKv + Rsw0X/gfnqkroJcCAwEAATANBgkqhkiG9w0BAQUFAAOBgQCMMlIO+GNcGekevKgk + akpMdAqJfs24maGb90DvTLbRZRD7Xvn1MnVBBS9hzlXiFLYOInXACMW5gcoRFfeT + QLSouMM8o57h0uKjfTmuoWHLQLi6hnF+cvCsEFiJZ4AbF+DgmO6TarJ8O05t8zvn + OwJlNCASPZRH/JmF8tX0hoHuAQ== + + + + + + + + + + + + urn:mace:shibboleth:1.0:nameIdentifier + urn:oasis:names:tc:SAML:2.0:nameid-format:transient + + + + + + + + + + + + MIIB7zCCAVgCCQDFzbKIp7b3MTANBgkqhkiG9w0BAQUFADA8MQswCQYDVQQGEwJV + UzELMAkGA1UECAwCR0ExDDAKBgNVBAoMA2ZvbzESMBAGA1UEAwwJbG9jYWxob3N0 + MB4XDTEzMTAwMjAwMDg1MVoXDTE0MTAwMjAwMDg1MVowPDELMAkGA1UEBhMCVVMx + CzAJBgNVBAgMAkdBMQwwCgYDVQQKDANmb28xEjAQBgNVBAMMCWxvY2FsaG9zdDCB + nzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEA1PMHYmhZj308kWLhZVT4vOulqx/9 + ibm5B86fPWwUKKQ2i12MYtz07tzukPymisTDhQaqyJ8Kqb/6JjhmeMnEOdTvSPmH + O8m1ZVveJU6NoKRn/mP/BD7FW52WhbrUXLSeHVSKfWkNk6S4hk9MV9TswTvyRIKv + Rsw0X/gfnqkroJcCAwEAATANBgkqhkiG9w0BAQUFAAOBgQCMMlIO+GNcGekevKgk + akpMdAqJfs24maGb90DvTLbRZRD7Xvn1MnVBBS9hzlXiFLYOInXACMW5gcoRFfeT + QLSouMM8o57h0uKjfTmuoWHLQLi6hnF+cvCsEFiJZ4AbF+DgmO6TarJ8O05t8zvn + OwJlNCASPZRH/JmF8tX0hoHuAQ== + + + + + + + + + + + + urn:mace:shibboleth:1.0:nameIdentifier + urn:oasis:names:tc:SAML:2.0:nameid-format:transient + + + TestShib Two Identity Provider + TestShib Two + http://www.testshib.org/testshib-two/ + + + Nate + Klingenstein + ndk@internet2.edu + + \ No newline at end of file From f920bb2429f583574b35ccdaa43633cd9e98300e Mon Sep 17 00:00:00 2001 From: Jo Garnier Date: Tue, 14 Apr 2026 11:14:52 +0200 Subject: [PATCH 2/4] Fix query signature CI issues --- service_provider.go | 6 +++--- service_provider_signed.go | 9 +++++---- service_provider_signed_test.go | 3 ++- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/service_provider.go b/service_provider.go index a3494aa8..db082c6b 100644 --- a/service_provider.go +++ b/service_provider.go @@ -304,12 +304,12 @@ func (r *AuthnRequest) Redirect(relayState string, sp *ServiceProvider) (*url.UR } // We can't depend on Query().set() as order matters for signing - reqString := string(w.Bytes()) + reqString := requestStr.String() query := rv.RawQuery if len(query) > 0 { - query += "&" + string(samlRequest) + "=" + url.QueryEscape(requestStr.String()) + query += "&" + string(samlRequest) + "=" + url.QueryEscape(reqString) } else { - query += string(samlRequest) + "=" + url.QueryEscape(requestStr.String()) + query += string(samlRequest) + "=" + url.QueryEscape(reqString) } if relayState != "" { diff --git a/service_provider_signed.go b/service_provider_signed.go index d8b02f06..0514746a 100644 --- a/service_provider_signed.go +++ b/service_provider_signed.go @@ -72,12 +72,13 @@ func (sp *ServiceProvider) validateQuerySig(query url.Values) error { } respType := "" - if query.Get("SAMLResponse") != "" { + switch { + case query.Get("SAMLResponse") != "": respType = "SAMLResponse" - } else if query.Get("SAMLRequest") != "" { + case query.Get("SAMLRequest") != "": respType = "SAMLRequest" - } else { - return fmt.Errorf("No SAMLResponse or SAMLRequest found in query") + default: + return fmt.Errorf("no SAMLResponse or SAMLRequest found in query") } // Encode Query as standard demands. diff --git a/service_provider_signed_test.go b/service_provider_signed_test.go index a45b8cbd..cc6e8a5f 100644 --- a/service_provider_signed_test.go +++ b/service_provider_signed_test.go @@ -19,7 +19,6 @@ func TestSigningAndValidation(t *testing.T) { desc string relayState string requestType reqType - wantErr bool wantRawQuery string } @@ -54,6 +53,7 @@ func TestSigningAndValidation(t *testing.T) { } err := xml.Unmarshal(idpMetadata, &s.IDPMetadata) + assert.NilError(t, err, "error unmarshalling metadata: %s", err) idpCert, err := s.getIDPSigningCerts() assert.Check(t, err == nil) @@ -102,6 +102,7 @@ func TestInvalidSignatureAlgorithm(t *testing.T) { } err := xml.Unmarshal(idpMetadata, &s.IDPMetadata) + assert.NilError(t, err, "error unmarshalling metadata: %s", err) idpCert, err := s.getIDPSigningCerts() assert.Check(t, err == nil) From 234f4359c3f93fc3463d463bbcf4fd42f6461046 Mon Sep 17 00:00:00 2001 From: Jo Garnier Date: Tue, 14 Apr 2026 11:27:38 +0200 Subject: [PATCH 3/4] Fix lint CI Go version mismatch --- .github/workflows/lint.yml | 2 +- samlidp/samlidp.go | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 688090cb..63d085a0 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -13,7 +13,7 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-go@v5 with: - go-version: stable + go-version: '1.24.x' - name: golangci-lint uses: golangci/golangci-lint-action@v7 with: diff --git a/samlidp/samlidp.go b/samlidp/samlidp.go index c97c1926..2c3bc78a 100644 --- a/samlidp/samlidp.go +++ b/samlidp/samlidp.go @@ -127,3 +127,12 @@ func (s *Server) InitializeHTTP() { mux.HandleFunc("PUT /shortcuts/{id}", s.HandlePutShortcut) mux.HandleFunc("DELETE /shortcuts/{id}", s.HandleDeleteShortcut) } + +// ServeHTTP delegates to the initialized handler. +func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if s.Handler == nil { + http.NotFound(w, r) + return + } + s.Handler.ServeHTTP(w, r) +} From 21da9e32eab8150bcb2c66d8301845aa36037082 Mon Sep 17 00:00:00 2001 From: Jo Garnier Date: Tue, 14 Apr 2026 11:39:04 +0200 Subject: [PATCH 4/4] Add detached signature validation fixes --- service_provider.go | 49 ++++++++++++------ service_provider_signed_test.go | 92 +++++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 15 deletions(-) diff --git a/service_provider.go b/service_provider.go index db082c6b..d6fdbdfc 100644 --- a/service_provider.go +++ b/service_provider.go @@ -313,7 +313,7 @@ func (r *AuthnRequest) Redirect(relayState string, sp *ServiceProvider) (*url.UR } if relayState != "" { - query += "&RelayState=" + relayState + query += "&RelayState=" + url.QueryEscape(relayState) } if len(sp.SignatureMethod) > 0 { var errSig error @@ -1392,36 +1392,51 @@ func (sp *ServiceProvider) MakeRedirectLogoutRequest(nameID, relayState string) 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 (r *LogoutRequest) Redirect(relayState string) *url.URL { +func (r *LogoutRequest) Redirect(relayState string, sp *ServiceProvider) (*url.URL, error) { w := &bytes.Buffer{} w1 := base64.NewEncoder(base64.StdEncoding, w) w2, _ := flate.NewWriter(w1, 9) doc := etree.NewDocument() doc.SetRoot(r.Element()) if _, err := doc.WriteTo(w2); err != nil { - panic(err) + return nil, err } if err := w2.Close(); err != nil { - panic(err) + return nil, err } if err := w1.Close(); err != nil { - panic(err) + return nil, err + } + rv, err := url.Parse(r.Destination) + if err != nil { + return nil, err } - rv, _ := url.Parse(r.Destination) - - query := rv.Query() - query.Set("SAMLRequest", w.String()) + // We can't depend on Query().Set() as order matters for signing + reqString := w.String() + query := rv.RawQuery + if len(query) > 0 { + query += "&" + string(samlRequest) + "=" + url.QueryEscape(reqString) + } else { + query += string(samlRequest) + "=" + url.QueryEscape(reqString) + } if relayState != "" { - query.Set("RelayState", relayState) + query += "&RelayState=" + url.QueryEscape(relayState) } - rv.RawQuery = query.Encode() + if len(sp.SignatureMethod) > 0 { + var errSig error + query, errSig = sp.signQuery(samlRequest, query, reqString, relayState) + if errSig != nil { + return nil, errSig + } + } + rv.RawQuery = query - return rv + return rv, nil } // MakePostLogoutRequest creates a SAML authentication request using @@ -1695,12 +1710,14 @@ func (sp *ServiceProvider) ValidateLogoutResponseRedirect(query url.Values) erro if err := xrv.Validate(bytes.NewReader(gr)); err != nil { return err } + querySig := false if query.Get("Signature") != "" && query.Get("SigAlg") != "" { if err := sp.validateQuerySig(query); err != nil { retErr.PrivateErr = err return retErr } + querySig = true } doc := etree.NewDocument() @@ -1710,8 +1727,10 @@ func (sp *ServiceProvider) ValidateLogoutResponseRedirect(query url.Values) erro } if err := sp.validateSignature(doc.Root()); err != nil { - retErr.PrivateErr = err - return retErr + if err != errSignatureElementNotPresent || !querySig { + retErr.PrivateErr = err + return retErr + } } var resp LogoutResponse diff --git a/service_provider_signed_test.go b/service_provider_signed_test.go index cc6e8a5f..52696bcd 100644 --- a/service_provider_signed_test.go +++ b/service_provider_signed_test.go @@ -1,11 +1,17 @@ package saml import ( + "bytes" + "compress/flate" "crypto/rsa" "encoding/base64" "encoding/xml" "net/url" + "strings" "testing" + "time" + + "github.com/beevik/etree" dsig "github.com/russellhaering/goxmldsig" "gotest.tools/assert" @@ -116,3 +122,89 @@ func TestInvalidSignatureAlgorithm(t *testing.T) { err = s.validateQuerySig(query) assert.Error(t, err, "unsupported signature algorithm: http://www.w3.org/2000/09/xmldsig#rsa-sha384") } + +func TestRedirectRequestsEscapeRelayStateAndValidateDetachedSignature(t *testing.T) { + idpMetadata := golden.Get(t, "SP_IDPMetadata_signing") + s := ServiceProvider{ + Key: mustParsePrivateKey(golden.Get(t, "idp_key.pem")).(*rsa.PrivateKey), + Certificate: mustParseCertificate(golden.Get(t, "idp_cert.pem")), + MetadataURL: mustParseURL("https://15661444.ngrok.io/saml2/metadata"), + AcsURL: mustParseURL("https://15661444.ngrok.io/saml2/acs"), + SignatureMethod: dsig.RSASHA1SignatureMethod, + } + + err := xml.Unmarshal(idpMetadata, &s.IDPMetadata) + assert.NilError(t, err, "error unmarshalling metadata: %s", err) + + relayState := "relay state+plus/slash" + + authRedirectURL, err := s.MakeRedirectAuthenticationRequest(relayState) + assert.NilError(t, err, "error creating auth redirect request: %s", err) + assert.Assert(t, strings.Contains(authRedirectURL.RawQuery, "RelayState="+url.QueryEscape(relayState))) + authQuery, err := url.ParseQuery(authRedirectURL.RawQuery) + assert.NilError(t, err, "error parsing auth query: %s", err) + assert.NilError(t, s.validateQuerySig(authQuery), "error validating auth query: %s") + + logoutRedirectURL, err := s.MakeRedirectLogoutRequest("ross@octolabs.io", relayState) + assert.NilError(t, err, "error creating logout redirect request: %s", err) + assert.Assert(t, strings.Contains(logoutRedirectURL.RawQuery, "RelayState="+url.QueryEscape(relayState))) + logoutQuery, err := url.ParseQuery(logoutRedirectURL.RawQuery) + assert.NilError(t, err, "error parsing logout query: %s", err) + assert.NilError(t, s.validateQuerySig(logoutQuery), "error validating logout query: %s") +} + +func TestValidateLogoutResponseRedirectAcceptsDetachedSignatureWithoutEmbeddedSignature(t *testing.T) { + idpMetadata := golden.Get(t, "SP_IDPMetadata_signing") + s := ServiceProvider{ + Key: mustParsePrivateKey(golden.Get(t, "idp_key.pem")).(*rsa.PrivateKey), + Certificate: mustParseCertificate(golden.Get(t, "idp_cert.pem")), + MetadataURL: mustParseURL("https://15661444.ngrok.io/saml2/metadata"), + SloURL: mustParseURL("https://15661444.ngrok.io/saml2/slo"), + IDPMetadata: &EntityDescriptor{}, + SignatureMethod: dsig.RSASHA1SignatureMethod, + } + + err := xml.Unmarshal(idpMetadata, &s.IDPMetadata) + assert.NilError(t, err, "error unmarshalling metadata: %s", err) + + resp := LogoutResponse{ + ID: "id-logout-response", + InResponseTo: "id-logout-request", + Version: "2.0", + IssueInstant: time.Now(), + Destination: s.SloURL.String(), + Issuer: &Issuer{ + Format: "urn:oasis:names:tc:SAML:2.0:nameid-format:entity", + Value: s.IDPMetadata.EntityID, + }, + Status: Status{ + StatusCode: StatusCode{ + Value: StatusSuccess, + }, + }, + } + + encodedResponse := deflatedBase64(t, resp.Element()) + rawQuery := "SAMLResponse=" + url.QueryEscape(encodedResponse) + rawQuery, err = s.signQuery(samlResponse, rawQuery, encodedResponse, "") + assert.NilError(t, err, "error signing logout response query: %s", err) + + query, err := url.ParseQuery(rawQuery) + assert.NilError(t, err, "error parsing query: %s", err) + assert.NilError(t, s.ValidateLogoutResponseRedirect(query), "detached-signature validation should succeed without embedded signature") +} + +func deflatedBase64(t *testing.T, el *etree.Element) string { + t.Helper() + + var buf bytes.Buffer + encoder := base64.NewEncoder(base64.StdEncoding, &buf) + compressor, _ := flate.NewWriter(encoder, 9) + doc := etree.NewDocument() + doc.SetRoot(el) + _, err := doc.WriteTo(compressor) + assert.NilError(t, err, "error serializing XML: %s", err) + assert.NilError(t, compressor.Close(), "error closing flate writer: %s") + assert.NilError(t, encoder.Close(), "error closing base64 encoder: %s") + return buf.String() +}