Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ronething opened this issue Jan 31, 2024 · 7 comments

Comments

@ronething
Copy link

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
Copy link

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

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

called from

if len(sp.SignatureMethod) > 0 {

@ronething
Copy link
Author

@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
Copy link

gravgaard commented Jan 31, 2024

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
Copy link
Author

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
Copy link

@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:

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

@ronething
Copy link
Author

ronething commented Jan 31, 2024

@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
Copy link

omerkarj commented Jun 9, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants