bug: make logout request need add signature logic if sp.SignatureMethod is not empty.

[ronething]

no signature when make logout request. If IDP enables Client signature required, it will cause signature verification to fail.

saml/service_provider.go

Lines 1235 to 1262 in a32b643

	// Redirect returns a URL suitable for using the redirect binding with the request
	func (r *LogoutRequest) Redirect(relayState string) *url.URL {
	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)
	}
	if err := w2.Close(); err != nil {
	panic(err)
	}
	if err := w1.Close(); err != nil {
	panic(err)
	}
	rv, _ := url.Parse(r.Destination)
	query := rv.Query()
	query.Set("SAMLRequest", w.String())
	if relayState != "" {
	query.Set("RelayState", relayState)
	}
	rv.RawQuery = query.Encode()
	return rv
	}

ref: make authn request.

saml/service_provider.go

Lines 248 to 295 in a32b643

	// Redirect returns a URL suitable for using the redirect binding with the request
	func (r *AuthnRequest) 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)
	}
	if err := w2.Close(); err != nil {
	panic(err)
	}
	if err := w1.Close(); err != nil {
	panic(err)
	}
	rv, _ := url.Parse(r.Destination)
	// We can't depend on Query().set() as order matters for signing
	query := rv.RawQuery
	if len(query) > 0 {
	query += "&SAMLRequest=" + url.QueryEscape(w.String())
	} else {
	query += "SAMLRequest=" + url.QueryEscape(w.String())
	}
	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
	}
	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
	}

[gravgaard]

Im currently having issues with a logout request with a national IdP but it seems to be that the idp does not understand the signature generated. I believe the logout request is signed in

saml/service_provider.go

Line 1161 in a32b643

func (sp *ServiceProvider) SignLogoutRequest(req *LogoutRequest) error {

called from

saml/service_provider.go

Line 1216 in a32b643

if len(sp.SignatureMethod) > 0 {

[ronething]

@gravgaard

I think SignLogoutRequest only signs the LogoutRequest, which is SAMLRequest argument in the URL, but the Signature and SigAlg parameters are required in the URL so that the IDP does not report an error.

you can ref this link for how to make a SAML logout request: https://stackoverflow.com/a/8215470

[gravgaard]

Your are right @ronething, the signature and algorithm needs to be included in the request, but i I think they are?!

Here i copied an example of my logout request.

<samlp:LogoutRequest xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion"
                     xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol"
                     ID="id-de5eb5b0-32d4-4bbd-9221-c523fc02a94d" Version="2.0"
                     IssueInstant="2024-01-29T09:23:32.487Z"
                     Destination="https://t-seb.dkseb.dk/runtime/saml2/issue.idp">
    <saml:Issuer>https://saml.samblik-diabetes.localhost/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/2001/04/xmldsig-more#rsa-sha256"/>
            <ds:Reference URI="#id-de5eb5b0-32d4-4bbd-9221-c523fc02a94d">
                <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/2001/04/xmlenc#sha256"/>
                <ds:DigestValue>ON+JylDXz//Nbck08Bk/XAunNICTUYDbzwq3tREdE6I=</ds:DigestValue>
            </ds:Reference>
        </ds:SignedInfo>
        <ds:SignatureValue>
            hWhdSuGoQWxDBlrRPEVRlk3QNXVHUrGYI8JZO8kAIcOmkLBd2qcPIH44X06gUgTkvjmrunJM0prVEwGsaK2+DvMBhBVyp1dGyxZp2xhEJrOza1wdzQE1lodeeIZuiIGNVy/uZ9jNbdGRAXErCzqBIpWqVfSRo4Ov8EbjH/2QzRC0zh0JSSn14lzjdKjOyx1/AW4n2NHbipE5ulbM9+4KO5r3Q/5HrJ85O5PxQt840jqX6aKEmqvTu1mTbfMchqWOprfs4pxv470fkbH6niC/iTz6IsUoXRq4y5Vd6kOEUQlHUCszf9xlw1vvlkpPGDryiN5tI5voygH3qyvEGlg3R0TRvm+hXNE6e2EvappZvnm9HnQMSPbRPUCb3+N7UHdHi0I+VoNPkSLWJDXNayoKfkqw2qxJl67dEfvhiF/JeGz+LZB/XRmHFYDud7yYjGZv/zjLOxH3o+mu8Izfz8VLU4cChYR+cALne4sBUg9RmacHwxU8D71zJZsC9I7Yc4n4
        </ds:SignatureValue>
        <ds:KeyInfo>
            <ds:X509Data>
                <ds:X509Certificate>
                    MIIGhTCCBLmgAwIBAgIUUnCoHO0eqF5DMFu5WOA3QpoRpHIwQQYJKoZIhvcNAQEKMDSgDzANBglghkgBZQMEAgEFAKEcMBoGCSqGSIb3DQEBCDANBglghkgBZQMEAgEFAKIDAgEgMGsxLTArBgNVBAMMJERlbiBEYW5za2UgU3RhdCBPQ0VTIHVkc3RlZGVuZGUtQ0EgMTETMBEGA1UECwwKVGVzdCAtIGN0aTEYMBYGA1UECgwPRGVuIERhbnNrZSBTdGF0MQswCQYDVQQGEwJESzAeFw0yMzA4MjkxMDA2NTNaFw0yNjA4MjgxMDA2NTJaMIGbMR0wGwYDVQQDDBRTYW1ibGlrIERpYWJldGVzIERldjE3MDUGA1UEBRMuVUk6REstTzpHOjgzMTQ4NDJiLWMxMTgtNDA2ZC1hZWNlLTg5OGNjNmJmMTEzMjEbMBkGA1UECgwSVHJpZm9yayBQdWJsaWMgQS9TMRcwFQYDVQRhDA5OVFJESy0yNTUyMDA0MTELMAkGA1UEBhMCREswggGiMA0GCSqGSIb3DQEBAQUAA4IBjwAwggGKAoIBgQDdx0DUm4x5fDUS0ijqw0rRXdZblfUQWtOi1PjLKsTzpNRrPWlVJvVcXiUpVz9FSKO7cxxq/jKBQrxD0XGqmSTMubhCH08pm3X1NRnbwvKCVha1VPUknOdZQ4j8GvLTv8bTFkdvGsa6TgtBiWFI+x1WXzg/KgrSPa8E2J9ti9GO3z7rveghNO9wK99Kuu/LnCq2jr0xceBhRfynxWUWS54Xj3lF75WMuV7ZtFwjzC2skE/DEcn4TWSISkUpdLZZhArLORBNw+uPeSV7I7JIw9a4WjVGapnLiiKkiO0uDH9Oq22VhFWyb6vwyPtRJEtXh5o/3aZv3DV6SuiJa8mipJnZwMJ1hLcVikvlkSlQq9Y7Eu9S86fnz2/pm37CA+jJNsSREMx6VnybNl5fOJI9Y1n6ycn4DFJaVMLFQpOt9bMeO+l+XIFBIXUDLK3rE9+VWkKCS5cY6KpqEhIaEmlHM2umnygqtr5k/sJsANIlopf4B+RhEAVpfqhM+7Xiq+dNndMCAwEAAaOCAYYwggGCMAwGA1UdEwEB/wQCMAAwHwYDVR0jBBgwFoAUfyif2XGZQuJ159c1di5NCCVtdl4wewYIKwYBBQUHAQEEbzBtMEMGCCsGAQUFBzAChjdodHRwOi8vY2ExLmN0aS1nb3YuZGsvb2Nlcy9pc3N1aW5nLzEvY2FjZXJ0L2lzc3VpbmcuY2VyMCYGCCsGAQUFBzABhhpodHRwOi8vY2ExLmN0aS1nb3YuZGsvb2NzcDAhBgNVHSAEGjAYMAgGBgQAj3oBATAMBgoqgVCBKQEBAQMHMDsGCCsGAQUFBwEDBC8wLTArBggrBgEFBQcLAjAfBgcEAIvsSQECMBSGEmh0dHBzOi8vdWlkLmdvdi5kazBFBgNVHR8EPjA8MDqgOKA2hjRodHRwOi8vY2ExLmN0aS1nb3YuZGsvb2Nlcy9pc3N1aW5nLzEvY3JsL2lzc3VpbmcuY3JsMB0GA1UdDgQWBBSGGinO9asnKFkncMw2F8UXPos94jAOBgNVHQ8BAf8EBAMCBaAwQQYJKoZIhvcNAQEKMDSgDzANBglghkgBZQMEAgEFAKEcMBoGCSqGSIb3DQEBCDANBglghkgBZQMEAgEFAKIDAgEgA4IBgQBkjFqBFV18zv2oc2/lNclIaK0Pe3TcKIBOYtdHo+PVB5ug+91vaMJK+Zsr7ouGvxYD5FXUFeuQ4XbCoO1ngB5lnYy0k+djQI2Zp33cK2ww9XIGRSYcCdE2wg9/wywWZWtpASg/hQ4Yb289ERbQlFQqfxSgdAnTOtq0vAv+9/H+BVCJQnqDAWRW+YYkPaHIrnw+SgLUhqb33xmTg/G931c4dd83g6o3Vfo63fVfTOAdXcWi2ceKgs3s7wNKhsIHR5cDhDLe07lRd5fzhcTJHSpJfnwVDdEPj0L09QKzCZ0PHV5o/S9C/bs/iTdOHkM/8o9VicfM8zzw2u1Iq07RyFEM/D64fHItUht5jgBDiSEQpFjVWDd/yNaiOuUK6YqeHLCTYPrb1c9Rk1Xg5N57iRFylD/rPBu4IDyWgQ9co94ZlrxQD8HNtrlEsh7t/unEQMr9HuYfpFF2KUb5L3AZCMyu6Kx3Hk8G8GjO0+XUjC4j28+ETsnKqP1qLugHfs9tVdA=
                </ds:X509Certificate>
            </ds:X509Data>
        </ds:KeyInfo>
    </ds:Signature>
    <saml:NameID Format="urn:oasis:names:tc:SAML:2.0:nameid-format:persistent">
        https://data.gov.dk/model/core/eid/professional/uuid/8f80a49e-7924-407b-86ff-969991ae4b43
    </saml:NameID>
    <samlp:SessionIndex xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol">
        a4EVB26cAautLYcHWX9eiQ==
    </samlp:SessionIndex>
</samlp:LogoutRequest>

I did modify the request slightly as I added the session Index paramter and changed the NameId to be the Principal and not the SP (I believe that these are required by the saml standard but should be reported as seperat bugs). anyway the custom codesnippet i have is here:

func MakeRedirectLogoutRequest(
	m *samlsp.Middleware,
	logger *zap.Logger,
	relayState string,
	assertion *saml.Assertion,
) (*url.URL, error) {

	location := m.ServiceProvider.GetSSOBindingLocation(saml.HTTPRedirectBinding)
	req := saml.LogoutRequest{
		ID:           "id-" + uuid.NewString(),
		IssueInstant: time.Now().UTC(),
		Version:      "2.0",
		Destination:  location,
		Issuer: &saml.Issuer{
			// This is custom (Excluded the format specification)
			Value: m.ServiceProvider.EntityID,
		},

		// This is custom SPNameQualifier and NameQualifier.
		// Also format and value is taken from the principal and not the SP
		NameID: &saml.NameID{
			Format: assertion.Subject.NameID.Format,
			Value:  assertion.Subject.NameID.Value,
		},
	}

	// This is the custom - include the session index from the assertion!
	if len(assertion.AuthnStatements) > 0 {
		req.SessionIndex = &saml.SessionIndex{Value: assertion.AuthnStatements[0].SessionIndex}
	}

	if len(m.ServiceProvider.SignatureMethod) > 0 {
		if err := m.ServiceProvider.SignLogoutRequest(&req); err != nil {
			return nil, err
		}
	}

	// This is custom - log the document
	logRequstXmlDoc := etree.NewDocument()
	logRequstXmlDoc.SetRoot(req.Element())
	s, _ := logRequstXmlDoc.WriteToString()
	logger.Debug("Logout Request", zap.String(" xml", s))

	return req.Redirect(relayState), nil

}

I can recommend using the chrome extension for http://rcfed.com/ to record your saml request for debugging ☺️

[ronething]

but i I think they are?!

@gravgaard maybe you can try this patch on the this repo, or implement Redirect function on your own project.

diff --git a/service_provider.go b/service_provider.go
index 30b3567..6a2b6b6 100644
--- a/service_provider.go
+++ b/service_provider.go
@@ -1229,11 +1229,12 @@ 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)
@@ -1251,14 +1252,35 @@ func (r *LogoutRequest) Redirect(relayState string) *url.URL {
 
 	rv, _ := url.Parse(r.Destination)
 
-	query := rv.Query()
-	query.Set("SAMLRequest", w.String())
+	query := rv.RawQuery
+	if len(query) > 0 {
+		query += "&SAMLRequest=" + url.QueryEscape(w.String())
+	} else {
+		query += "SAMLRequest=" + url.QueryEscape(w.String())
+	}
+
 	if relayState != "" {
-		query.Set("RelayState", relayState)
+		query += "&RelayState=" + relayState
 	}
-	rv.RawQuery = query.Encode()
 
-	return rv
+	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
+		}
+		query += "&Signature=" + url.QueryEscape(base64.StdEncoding.EncodeToString(sig))
+	}
+
+	rv.RawQuery = query
+
+	return rv, nil
 }
 
 // MakePostLogoutRequest creates a SAML authentication request using

[gravgaard]

@ronething holy moly you are right. It totally worked 🍻. So you are right:

If IDP enables Client signature required, it will cause signature verification to fail.

As matter of fact your suggestion is the exact same way the authn request are made:

saml/service_provider.go

Line 405 in a32b643

func (sp *ServiceProvider) MakeAuthenticationRequest(idpURL string, binding string, resultBinding string) (*AuthnRequest, error) {

[ronething]

@gravgaard

As matter of fact your suggestion is the exact same way the authn request are made:

Yeah, as mentioned in this issue's description.

[omerkarj]

Note that when using urn:oasis:names:tc:SAML:2.0:bindings:URL-Encoding:DEFLATE, which is the default unless a different value is provided with SAMLEncoding query param, is that the SAML message itself must not include any signature data.

https://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf

Any signature on the SAML protocol message, including the ds:Signature XML element itself,
MUST be removed. Note that if the content of the message includes another signature, such as a
signed SAML assertion, this embedded signature is not removed. However, the length of such a
message after encoding essentially precludes using this mechanism. Thus SAML protocol
messages that contain signed content SHOULD NOT be encoded using this mechanism.

Since the request data is constructed with

func (sp *ServiceProvider) MakeLogoutRequest(idpURL, nameID string) (*LogoutRequest, error) {
	req := LogoutRequest{
		...
	}
	if len(sp.SignatureMethod) > 0 {
		if err := sp.SignLogoutRequest(&req); err != nil {
			return nil, err
		}
	}
	return &req, nil
}

There needs to be additional handling for removing the signature before constructing the SAMLRequest data.