From 08d2ceaeda576d3fcb8d7ef9fc770d6f9aaec751 Mon Sep 17 00:00:00 2001 From: Andreas Kupries Date: Wed, 26 Jun 2024 12:36:13 +0200 Subject: [PATCH 1/4] fix missing verification of detached signatures in responses Applied long-open PR https://github.com/crewjam/saml/pull/449 plus https://github.com/crewjam/saml/pull/449/files#r1654477177 to make embedded signature optional when a good detached signature is seen. --- service_provider.go | 46 ++++++----- service_provider_signed.go | 141 ++++++++++++++++++++++++++++++++ service_provider_signed_test.go | 117 ++++++++++++++++++++++++++ testdata/SP_IDPMetadata_signing | 96 ++++++++++++++++++++++ 4 files changed, 381 insertions(+), 19 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 c97886d0..6feea94b 100644 --- a/service_provider.go +++ b/service_provider.go @@ -304,29 +304,23 @@ 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()) query := rv.RawQuery if len(query) > 0 { - query += "&SAMLRequest=" + url.QueryEscape(requestStr.String()) + query += "&" + string(samlRequest) + "=" + url.QueryEscape(reqString) } else { - query += "SAMLRequest=" + url.QueryEscape(requestStr.String()) + query += string(samlRequest) + "=" + url.QueryEscape(reqString) } if relayState != "" { - query += "&RelayState=" + relayState + query += "&RelayState=" + url.QueryEscape(relayState) } if len(sp.SignatureMethod) > 0 { - query += "&SigAlg=" + url.QueryEscape(sp.SignatureMethod) - signingContext, err := GetSigningContext(sp) - - if err != nil { - return nil, err - } - - sig, err := signingContext.SignString(query) - if err != nil { - return nil, err + var errSig error + query, errSig = sp.signQuery(samlRequest, query, reqString, relayState) + if errSig != nil { + return nil, errSig } - query += "&Signature=" + url.QueryEscape(base64.StdEncoding.EncodeToString(sig)) } rv.RawQuery = query @@ -1624,8 +1618,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() @@ -1677,7 +1672,9 @@ 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(), } @@ -1699,6 +1696,15 @@ func (sp *ServiceProvider) ValidateLogoutResponseRedirect(queryParameterData str 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() if err := doc.ReadFromBytes(gr); err != nil { retErr.PrivateErr = err @@ -1706,8 +1712,10 @@ func (sp *ServiceProvider) ValidateLogoutResponseRedirect(queryParameterData str } 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.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 991d77ba45c9ffc40abb0f3ad452a3e51454f563 Mon Sep 17 00:00:00 2001 From: Andreas Kupries Date: Wed, 26 Jun 2024 13:12:01 +0200 Subject: [PATCH 2/4] Extended logout redirect to create detached signature too, analogous to the 449 tweaks in auth. fix of golangci report. --- service_provider.go | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/service_provider.go b/service_provider.go index 6feea94b..4a265cdd 100644 --- a/service_provider.go +++ b/service_provider.go @@ -304,7 +304,7 @@ 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 := w.String() query := rv.RawQuery if len(query) > 0 { query += "&" + string(samlRequest) + "=" + url.QueryEscape(reqString) @@ -1391,11 +1391,11 @@ 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) @@ -1413,14 +1413,29 @@ func (r *LogoutRequest) Redirect(relayState string) *url.URL { 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) + } + 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.Encode() - return rv + rv.RawQuery = query + + return rv, nil } // MakePostLogoutRequest creates a SAML authentication request using From 4a125c17f8af31c2fdf773c21f119ca962225b1b Mon Sep 17 00:00:00 2001 From: Andreas Kupries Date: Mon, 1 Jul 2024 09:32:33 +0200 Subject: [PATCH 3/4] updated readme with fork information. --- README.md | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/README.md b/README.md index 1a435834..dc60e4bc 100644 --- a/README.md +++ b/README.md @@ -1,3 +1,37 @@ + +# FORK information + +This fork of the https://github.com/crewjam/saml repository is rooted in commit +a32b643a25a46182499b1278293e265150056d89 of the origin repository. + +This is the head of the `main` branch at the time of writing this document, containing version +`v0.4.14` of the package, i.e. commit b07b16cf83c4171d16da4d85608cb827f183cd79, plus a number of +version bumps for dependent modules, via dependabot. + +Compared to the root the fork contains the following changes + + 1. Application of pull request https://github.com/crewjam/saml/pull/491 + + This fixes the mishandling of compressed data. The XML parser always operated on the input data + even if it was compressed. While decompression was performed the resulting data was ignored and + not used. + + 1. Application of pull request https://github.com/crewjam/saml/pull/449 + + This adds the missing verification of detached signatures to the handling of logout responses. + + 1. Beyond the above the work of item 2 was modified/extended to allow an embedded signature to be + missing when a good, i.e. verified, detached signature is found. + + This means that the code now accepts the case of detached signature without embedded signature + as valid. This is what Keycloak provides. + + 1. Going further the work of item 2 was used as template to add the generation of detached + signatures to logout requests generated by the package. + + Such a missing detached signature was the cause of OKTA not accepting our logout requests. The + embedded signature had no meaning. + # SAML [![](https://godoc.org/github.com/crewjam/saml?status.svg)](http://godoc.org/github.com/crewjam/saml) From 65e83aac413513d49a44086d4709e396fbf8345c Mon Sep 17 00:00:00 2001 From: Kevin McDermott Date: Mon, 2 Mar 2026 13:20:12 +0000 Subject: [PATCH 4/4] Clarify the changes from upstream. Bump dependencies. Bump Go to newest versions. --- .github/workflows/lint.yml | 4 ++-- .github/workflows/test.yml | 2 +- README.md | 10 ++-------- go.mod | 12 ++++++------ go.sum | 35 ++++++++++------------------------- 5 files changed, 21 insertions(+), 42 deletions(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 688090cb..9f97be8a 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -15,6 +15,6 @@ jobs: with: go-version: stable - name: golangci-lint - uses: golangci/golangci-lint-action@v7 + uses: golangci/golangci-lint-action@v9 with: - version: v2.0 + version: v2.10 diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index bca5ba69..527d2e11 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -10,7 +10,7 @@ jobs: runs-on: ubuntu-latest strategy: matrix: - go: [ '1.22.x', '1.23.x', '1.24.x'] + go: [ '1.24.x', '1.25.x', '1.26.x'] steps: - uses: actions/checkout@v4 - uses: actions/setup-go@v4 diff --git a/README.md b/README.md index dc60e4bc..59449a3f 100644 --- a/README.md +++ b/README.md @@ -10,23 +10,17 @@ version bumps for dependent modules, via dependabot. Compared to the root the fork contains the following changes - 1. Application of pull request https://github.com/crewjam/saml/pull/491 - - This fixes the mishandling of compressed data. The XML parser always operated on the input data - even if it was compressed. While decompression was performed the resulting data was ignored and - not used. - 1. Application of pull request https://github.com/crewjam/saml/pull/449 This adds the missing verification of detached signatures to the handling of logout responses. - 1. Beyond the above the work of item 2 was modified/extended to allow an embedded signature to be + 1. Beyond the above the work of item 1 was modified/extended to allow an embedded signature to be missing when a good, i.e. verified, detached signature is found. This means that the code now accepts the case of detached signature without embedded signature as valid. This is what Keycloak provides. - 1. Going further the work of item 2 was used as template to add the generation of detached + 1. Going further the work of item 1 was used as template to add the generation of detached signatures to logout requests generated by the package. Such a missing detached signature was the cause of OKTA not accepting our logout requests. The diff --git a/go.mod b/go.mod index 44c05136..2c7a064d 100644 --- a/go.mod +++ b/go.mod @@ -1,19 +1,19 @@ module github.com/crewjam/saml -go 1.22 +go 1.25 require ( - github.com/golang-jwt/jwt/v5 v5.2.2 - github.com/beevik/etree v1.5.0 + github.com/beevik/etree v1.6.0 + github.com/golang-jwt/jwt/v5 v5.3.1 github.com/google/go-cmp v0.7.0 github.com/mattermost/xml-roundtrip-validator v0.1.0 - github.com/russellhaering/goxmldsig v1.4.0 - golang.org/x/crypto v0.33.0 + github.com/russellhaering/goxmldsig v1.5.0 + golang.org/x/crypto v0.48.0 gotest.tools v2.2.0+incompatible ) require ( - github.com/jonboulle/clockwork v0.2.2 // indirect + github.com/jonboulle/clockwork v0.5.0 // indirect github.com/pkg/errors v0.9.1 // indirect github.com/stretchr/testify v1.10.0 // indirect ) diff --git a/go.sum b/go.sum index d09e9615..0c271b0e 100644 --- a/go.sum +++ b/go.sum @@ -1,45 +1,30 @@ -github.com/beevik/etree v1.1.0/go.mod h1:r8Aw8JqVegEf0w2fDnATrX9VpkMcyFeM0FhwO62wh+A= -github.com/beevik/etree v1.5.0 h1:iaQZFSDS+3kYZiGoc9uKeOkUY3nYMXOKLl6KIJxiJWs= -github.com/beevik/etree v1.5.0/go.mod h1:gPNJNaBGVZ9AwsidazFZyygnd+0pAU38N4D+WemwKNs= -github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= +github.com/beevik/etree v1.6.0 h1:u8Kwy8pp9D9XeITj2Z0XtA5qqZEmtJtuXZRQi+j03eE= +github.com/beevik/etree v1.6.0/go.mod h1:bh4zJxiIr62SOf9pRzN7UUYaEDa9HEKafK25+sLc0Gc= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/golang-jwt/jwt/v5 v5.2.2 h1:Rl4B7itRWVtYIHFrSNd7vhTiz9UpLdi6gZhZ3wEeDy8= -github.com/golang-jwt/jwt/v5 v5.2.2/go.mod h1:pqrtFR0X4osieyHYxtmOUWsAWrfe1Q5UVIyoH402zdk= +github.com/golang-jwt/jwt/v5 v5.3.1 h1:kYf81DTWFe7t+1VvL7eS+jKFVWaUnK9cB1qbwn63YCY= +github.com/golang-jwt/jwt/v5 v5.3.1/go.mod h1:fxCRLWMO43lRc8nhHWY6LGqRcf+1gQWArsqaEUEa5bE= github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= -github.com/jonboulle/clockwork v0.2.2 h1:UOGuzwb1PwsrDAObMuhUnj0p5ULPj8V/xJ7Kx9qUBdQ= -github.com/jonboulle/clockwork v0.2.2/go.mod h1:Pkfl5aHPm1nk2H9h0bjmnJD/BcgbGXUBGnn1kMkgxc8= -github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo= -github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= -github.com/kr/pretty v0.3.0/go.mod h1:640gp4NfQd8pI5XOwp5fnNeVWj67G7CFk/SaSQn7NBk= -github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= -github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= -github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= +github.com/jonboulle/clockwork v0.5.0 h1:Hyh9A8u51kptdkR+cqRpT1EebBwTn1oK9YfGYbdFz6I= +github.com/jonboulle/clockwork v0.5.0/go.mod h1:3mZlmanh0g2NDKO5TWZVJAfofYk64M7XN3SzBPjZF60= github.com/mattermost/xml-roundtrip-validator v0.1.0 h1:RXbVD2UAl7A7nOTR4u7E3ILa4IbtvKBHw64LDsmu9hU= github.com/mattermost/xml-roundtrip-validator v0.1.0/go.mod h1:qccnGMcpgwcNaBnxqpJpWWUiPNr5H3O8eDgGV9gT5To= -github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA= github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= -github.com/rogpeppe/go-internal v1.6.1/go.mod h1:xXDCJY+GAPziupqXw64V24skbSoqbTEfhy4qGm1nDQc= -github.com/rogpeppe/go-internal v1.8.0/go.mod h1:WmiCO8CzOY8rg0OYDC4/i/2WRWAB6poM+XZ2dLUbcbE= -github.com/russellhaering/goxmldsig v1.4.0 h1:8UcDh/xGyQiyrW+Fq5t8f+l2DLB1+zlhYzkPUJ7Qhys= -github.com/russellhaering/goxmldsig v1.4.0/go.mod h1:gM4MDENBQf7M+V824SGfyIUVFWydB7n0KkEubVJl+Tw= +github.com/russellhaering/goxmldsig v1.5.0 h1:AU2UkkYIUOTyZRbe08XMThaOCelArgvNfYapcmSjBNw= +github.com/russellhaering/goxmldsig v1.5.0/go.mod h1:x98CjQNFJcWfMxeOrMnMKg70lvDP6tE0nTaeUnjXDmk= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.10.0 h1:Xv5erBjTwe/5IxqUQTdXv5kgmIvbHo3QQyRwhJsOfJA= github.com/stretchr/testify v1.10.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= -golang.org/x/crypto v0.33.0 h1:IOBPskki6Lysi0lo9qQvbxiQ+FvsCC/YWOecCHAixus= -golang.org/x/crypto v0.33.0/go.mod h1:bVdXmD7IV/4GdElGPozy6U7lWdRXA4qyRVGJV57uQ5M= +golang.org/x/crypto v0.48.0 h1:/VRzVqiRSggnhY7gNRxPauEQ5Drw9haKdM0jqfcCFts= +golang.org/x/crypto v0.48.0/go.mod h1:r0kV5h3qnFPlQnBSrULhlsRfryS2pmewsg+XfMgkVos= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= -gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= -gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q= -gopkg.in/errgo.v2 v2.1.0/go.mod h1:hNsd1EY+bozCKY1Ytp96fpM3vjJbqLJn88ws8XvfDNI= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= -gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gotest.tools v2.2.0+incompatible h1:VsBPFP1AI068pPrMxtb/S8Zkgf9xEmTLJjfM+P5UIEo=