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

Cannot configure both SOAP and Redirect bindings for SingleLogoutService in metadata #28

Open
HDenBreejen opened this issue Mar 26, 2018 · 10 comments

Comments

@HDenBreejen
Copy link

I have a problem getting IDP-initiated logoff configured with SAML2.
The Dutch DigiD IDP is specified to send my SP a SOAP call for an IDP-initiated logoff.
This is a fragment of sample SP metadata, taken from the DigiD spec.

<md:SingleLogoutService  Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect" Location="http://test.local/saml/sp/logged_out"/>
<md:SingleLogoutService  Binding="urn:oasis:names:tc:SAML:2.0:bindings:SOAP" Location="http://test.local/saml/sp/logout"/>
<md:AssertionConsumerService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Artifact" Location="http://test.local/saml/sp/artifact_resolution" index="0"/>

There does not seem to a way to achieve this using the SAML2 package..

The config can have just the one endpoint with type "Logout".
When I specify SOAP I get two identical SOAP entries in the metadata.
And when I specify Redirect, I get entries for REDIRECT and POST, but no SOAP.

I believe the code that handles this is in Saml20MetadataDocument.cs:

            else if (endpoint.Type == EndpointType.Logout)
            {
                var logoutEndpoint = new Endpoint
                {
                    Location = new Uri(baseUrl, endpoint.LocalPath).ToString()
                };

                logoutEndpoint.ResponseLocation = logoutEndpoint.Location;
                logoutEndpoint.Binding = GetBinding(endpoint.Binding, Saml20Constants.ProtocolBindings.HttpPost);
                logoutServiceEndpoints.Add(logoutEndpoint);

                // TODO: Look at this...
                logoutEndpoint = new Endpoint
                {
                    Location = new Uri(baseUrl, endpoint.LocalPath).ToString()
                };

                logoutEndpoint.ResponseLocation = logoutEndpoint.Location;
                logoutEndpoint.Binding = GetBinding(endpoint.Binding, Saml20Constants.ProtocolBindings.HttpRedirect);
                logoutServiceEndpoints.Add(logoutEndpoint);

                var artifactLogoutEndpoint = new IndexedEndpoint
                {
                    Binding = Saml20Constants.ProtocolBindings.HttpSoap,
                    Index = endpoint.Index,
                    Location = logoutEndpoint.Location
                };

                artifactResolutionEndpoints.Add(artifactLogoutEndpoint);
            }

When a binding is specified in the config, both calls to GetBinding() produce SingleLogoutService entries with this binding in the metadata, resulting in duplication.

The above code does produce an ArtifactResolutionService element with SOAP binding, but that is not what the DigiD spec requires.

Perhaps the simplest, and most versatile solution would be for SAML2 to allow configuration of the exact metadata required.

For the record.. I have changed the above code to produce the metadata DigiD requires. But still, DigiD does not send me the expected SOAP call. I assume that to be a problem with DigiD, but it could be an error in their spec.
It puzzles me that SAML2 unconditionally adds a ArtifactResolutionService element for logout that binds to SOAP. Is this to support some common practise?
Why would DigiD divert from this?

@i8beef
Copy link
Owner

i8beef commented Apr 11, 2018

Keep in mind that the original library this forked from, OIOSAML, was specifically written to support the Danish gov't implementation. While it has been mostly genericized to support most regular SAML implementations, it is totally possible that there are things that remain simply because no one was exercising those code paths. It looks like you found one that I questioned early on when converting it (see the TODO), but didn't feel comfortable changing without more research.

You actually can define as many endpoints as you want in that config element of each type, but the FIRST one of each type will be treated as default. But this issue would in fact need to be corrected to eliminate the doubles. I'll put a PR together.

@i8beef
Copy link
Owner

i8beef commented Apr 11, 2018

Also, you don't define the AttributeConsumingService directly. It gets generated several lines below that IF there have RequestedAttributes.

@i8beef
Copy link
Owner

i8beef commented Apr 11, 2018

Please see #29

You should be able to define multiple logout endpoints just fine as is. This should fix the duplicate endpoints issue. And your issue with the AttributeConsumingService should be fixed by defining requested attributes in your config.

@jbreuer
Copy link

jbreuer commented Apr 11, 2018

Thanks for fixing this @i8beef! I'm currently also looking at this library for DigiD so I'll probably also need this fix.

@i8beef
Copy link
Owner

i8beef commented Apr 11, 2018

Cool. If one of you can confirm what I said above (and that this branch generates what you are expecting) then I will merge it.

@HDenBreejen
Copy link
Author

Thanks! I will test this and report back.

@HDenBreejen
Copy link
Author

HDenBreejen commented Apr 16, 2018

I have tested #29, but that does not completely fix this issue.
It does prevent duplicate entries in the metadata, but I still cannot have both a SOAP and a Redirect binding.

I need the Redirect-binding for SP-initiated logoff, and the SOAP-binding for IDP-initiated logoff.

You write that it is possible to have as many endpoints as required in the metadata, but that confuses me. The doc states that you cannot configure multiple endpoints with the same 'type' attribute, and indeed, doing this results in a ConfigurationErrorsException.

BindingType has a [Flags] attribute, suggesting that bindingtypes can be combined.
It would by nice to be able to configure the required Soap and Redirect bindings like this:

<endpoint localPath="Logout.ashx" type="Logout" binding="Soap,Redirect" redirectUrl="https://...." />

But that results in a ConfigurationErrorsException too.

@i8beef
Copy link
Owner

i8beef commented Apr 16, 2018

Where are you getting a ConfigurationErrorsException? I don't see any of the code in the library specifically throwing one of those that would be related to SP bindings.

@HDenBreejen
Copy link
Author

The ConfigurationErrorsException exception is not thrown by SAML2 code, but by the .NET runtime while parsing the saml2 config section.

I would like to configure something like this. Note that I have two endpoints of type 'logout'.

<endpoint localPath="Login.ashx" type="SignOn" redirectUrl="https://desktop-hildo.zorgned.local/UsingSaml2Package/default.aspx" binding="Post" />
<endpoint localPath="Logout.ashx" type="Logout" binding="Redirect" redirectUrl="https://desktop-hildo.zorgned.local/UsingSaml2Package/homepage.aspx" />
<endpoint localPath="Logout.ashx" type="Logout" binding="Soap"  />
<endpoint localPath="Metadata.ashx" type="Metadata" />

The ServiceProviderEndpointElement class in SAML2, however, requires the type attribute to be unique.

[ConfigurationProperty("type", IsKey = true, IsRequired = true)]
public EndpointType Type
{
    get { return (EndpointType)base["type"]; }
    set { base["type"] = value; }
}

@i8beef
Copy link
Owner

i8beef commented Apr 24, 2018

I might be able to lift that restriction but I have to check. There are several utility methods there that get the .First(x => Type = sometype) that are used in various places that could introduce bugs, so Id have to look at that pretty thoroughly first.

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