None of my SingleLogoutSerivce elements in metadata xml files seem to be getting parsed

[dougwebb]

I'm testing with a few Idps, which are configured using metadata xml files, like the one below. During configuration loading, Init() is called, which calls IdentityProviderCollection.Refresh(), which calls ParseFile() for each file, which calls LoadFileAsXmlDocument(). Finally, the doc object is passed to Saml20MetadataDocument's constructor which calls DeserializeFromXmlString.

At that point the deserialized object contains the logout endpoints, but when control returns back to the file loop in IdentityProviderCollection, the metadataDoc objects have IDPSLOEndpoints objects whose Type is set to SignOn instead of Logout. They seem to be uninitialized rather than explicitly set incorrectly, but I can't figure out where that should be fixed.

<?xml version="1.0"?>
<md:EntityDescriptor xmlns:md="urn:oasis:names:tc:SAML:2.0:metadata" entityID="https://capriza.github.io/samling/samling.html">
  <md:IDPSSODescriptor WantAuthnRequestsSigned="false" protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol">
    <md:KeyDescriptor use="signing">
      <ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
        <ds:X509Data>
          <ds:X509Certificate>MIICpzCCAhACCQDuFX0Db5iljDANBgkqhkiG9w0BAQsFADCBlzELMAkGA1UEBhMCVVMxEzARBgNVBAgMCkNhbGlmb3JuaWExEjAQBgNVBAcMCVBhbG8gQWx0bzEQMA4GA1UECgwHU2FtbGluZzEPMA0GA1UECwwGU2FsaW5nMRQwEgYDVQQDDAtjYXByaXphLmNvbTEmMCQGCSqGSIb3DQEJARYXZW5naW5lZXJpbmdAY2Fwcml6YS5jb20wHhcNMTgwNTE1MTgxMTEwWhcNMjgwNTEyMTgxMTEwWjCBlzELMAkGA1UEBhMCVVMxEzARBgNVBAgMCkNhbGlmb3JuaWExEjAQBgNVBAcMCVBhbG8gQWx0bzEQMA4GA1UECgwHU2FtbGluZzEPMA0GA1UECwwGU2FsaW5nMRQwEgYDVQQDDAtjYXByaXphLmNvbTEmMCQGCSqGSIb3DQEJARYXZW5naW5lZXJpbmdAY2Fwcml6YS5jb20wgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBAJEBNDJKH5nXr0hZKcSNIY1l4HeYLPBEKJLXyAnoFTdgGrvi40YyIx9lHh0LbDVWCgxJp21BmKll0CkgmeKidvGlr3FUwtETro44L+SgmjiJNbftvFxhNkgA26O2GDQuBoQwgSiagVadWXwJKkodH8tx4ojBPYK1pBO8fHf3wOnxAgMBAAEwDQYJKoZIhvcNAQELBQADgYEACIylhvh6T758hcZjAQJiV7rMRg+Omb68iJI4L9f0cyBcJENR+1LQNgUGyFDMm9Wm9o81CuIKBnfpEE2Jfcs76YVWRJy5xJ11GFKJJ5T0NEB7txbUQPoJOeNoE736lF5vYw6YKp8fJqPW0L2PLWe9qTn8hxpdnjo3k6r5gXyl8tk=</ds:X509Certificate>
        </ds:X509Data>
      </ds:KeyInfo>
    </md:KeyDescriptor>
    <md:NameIDFormat>urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress</md:NameIDFormat>
    <md:SingleSignOnService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect" Location="https://capriza.github.io/samling/samling.html"/>
    <md:SingleLogoutService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect" Location="https://capriza.github.io/samling/samling.html"/>
  </md:IDPSSODescriptor>
</md:EntityDescriptor>

[dougwebb]

Actually, for THAT metadata xml, the SingleLogoutService elements are NOT deserialized at all. There seems to be two problems here; sometimes the elements aren't deserialized, and then when they are they have the wrong Type property values.

[i8beef]

IdentityProviderEndpoint is part of the configuration namespace. The "Type" only has meaning when used as part of configuration parsing, when it has to split up the identity provider endpoints by type. Here, the type is provided explicitly by the XML element name, so when it splits the endpoints it doesn't actually set Type. Arguably, it would be a good idea if it DID set it here just to avoid confusion, though it isn't used.

[dougwebb]

Ok. How about the problem with the logout element not being deserialized at all? Any thoughts on that? It might be specific to this xml file, because some others I've tried do seem to work. Could there be an issue with the Location being the same for both signon and logout?

[i8beef]

Nope, that looks right to me. I don't see a reason it wouldn't deserialize.

[dougwebb]

I figured out the problem. I had to fix the xml by moving the SingleLogoutService element to before the NameIDFormat element. (This xml was provided by the identity provider; it wasn't hand-coded.)

In SAML2.Schema.Metadata, the SsoDescriptor and IdpSsoDescriptor classes use Order arguments in the XmlElement attributes on their properties. This enforces a strict ordering on the elements in the xml, and the error I was getting was caused by having the wrong order. Also, it appears that the elements expected by SsoDescriptor, which is the base class, have to appear first, followed by the derived class IdpSsoDescriptor's elements.

To help debug this, I added an UnknownElement event handler to SAML2.Utils.Serialization.Deserialize. You might want to add logging for this situation, or even throw exceptions. I just had a breakpoint in the handler so I could look at the event object.

public static T Deserialize<T>(XmlReader reader)
{
    var serializer = new XmlSerializer(typeof(T));
    serializer.UnknownElement += Serializer_UnknownElement;
    var item = (T)serializer.Deserialize(reader);

    return item;
}

private static void Serializer_UnknownElement(object sender, XmlElementEventArgs e)
{
}

[i8beef]

Been a while since I read the spec... https://www.oasis-open.org/committees/download.php/51890/SAML%20MD%20simplified%20overview.pdf

If you go down to about page 8 for IDPSSODescriptor, it says this:

The order of all this information is significant, which you can refer to the schema for, but the
most common elements included would be present in the following order:
• <Extension> (optional IdP discovery and algorithm support)
• <KeyDescriptor> (can be omitted, but rarely)
• <ArtifactResolutionService> (only needed if supporting response by artifact)
• <SingleLogoutService> (if any)
• <NameIDFormat> (if any)
• <SingleSignOnService> (always at least one)

I considered removing the Order declarations for a minute, but it appears they are actually important...

Your suggestion is probably a good one as I can't think of a better way to indicate that you have an invalid metadata document if it is gonna silently ignore things like this. If you feel like submitting a PR to log a warning in this case, Ill review it. You can grab a current logger instance and send a Warning as below. Just mention this ticket in the commit.

Logging.LoggerProvider.LoggerFor(GetType()).Warn("Warning message");