Problem configuring clientCertificate in artifactResolution

[HDenBreejen]

This is a fragment of my configuration:

<artifactResolution>
    <clientCertificate findValue="53dc8af28a949dcac7991da0a56fa0b0a2e90a6f" storeLocation="LocalMachine" storeName="My" x509FindType="FindByThumbprint">
    </clientCertificate>
</artifactResolution>

Using this configuration I get an Exception from

public Stream GetResponse(string endpoint, string message, HttpAuthElement auth)
 {
      if (auth != null && auth.ClientCertificate != null && auth.Credentials != null)
      {
            throw new Saml20Exception(string.Format("Artifact resolution cannot specify both client certificate and basic credentials for endpoint {0}", endpoint));
      }

This code fragment checks for a clientcertificate and credentials for basic authentication both being present. Although my config has a clientcertificate only, during execution of this code auth.Credentials is not null, as expected.

I have changed this fragement to

 public Stream GetResponse(string endpoint, string message, HttpAuthElement auth)
  {
      if (auth != null &&
                auth.ClientCertificate != null && 
                auth.Credentials != null &&
                auth.Credentials.Username != null &&
                auth.Credentials.Username != string.Empty)
      {
           throw new Saml20Exception(string.Format("Artifact resolution cannot specify both client certificate and basic credentials for endpoint {0}", endpoint));
      }

This solves my immediate problem, but I feel that the original code should have worked fine.
What am I doing wrong?

[i8beef]

Can you try changing the original check to this?

if (auth.ElementInformation.IsPresent && auth.ClientCertificate.ElementInformation.IsPresent && auth.Credentials.ElementInformation.IsPresent)
{
    throw new Saml20Exception(string.Format("Artifact resolution cannot specify both client certificate and basic credentials for endpoint {0}", endpoint));
}

I suspect this is just a bug in the config reading stuff. It look like a custom config section will read the whole object graph of the config and put in default implementations of all sub objects except collections or something. That would imply all null checks here would need to be done like this... or I pass the whole config through a utility method before caching it to remove any elements that are "IsPresent" false possibly....

If the above works for you, it'll verify my belief, I can fix it, and you might give me an excuse to enable code-based configuration as well by splitting internal config representation away from web.config structure.

[HDenBreejen]

Thanks for looking in to this!

I tried the code you suggest, and this works fine. So the config support in .NET does not map absent elements to null... To me that is quite unexpected.

So, using ElementInformation properties in SAML2 code works, but things would look cleaner and more compact if the representation for absent elements would be null.

If we could have code-based config as an added bonus, that would be awesome. But doing just the ElementInformation.Ispresent would me much appreciated too.

Thanks!

[i8beef]

Yeah that was a little piece of trivia I wasn't aware of actually. There are other locations in the app where it does checks for this...

I have another branch up now: https://github.com/i8beef/SAML2/tree/5-CodeBaseConfig

This is a first pass at implementing a code-based configuration approach. It still needs some work and testing, but it standardizes the internal config needs on my own configuration settings, allows for a code-based approach with a config based fall back, and SHOULD translate the web.config stuff over automatically (though I finished this up at 3 AM so it probably needs another once over before I merge it).

If you wanna try that branch out just to see if anything immediately breaks for you, that'd be helpful.

[HDenBreejen]

The code looks promising, but unfortunately, it does break immediately.. I use the default stateservice, and the line var stateServiceClass = Saml2Config.Current.State.StateServiceFactory; in the static constructor of StateServiceProvider errors on Saml2Config.Current.State being null.

[i8beef]

Update pushed to that branch that should fix that. There might be other places like that which rely on the weird configuration manager behavior though, so I'll have to go through and look for that, thanks.

[HDenBreejen]

Ahh.. Same problem in Saml20SignonHandler.HandleAssertion(), line 381. In my situation, Saml2Config.Current.AssertionProfile == null...

Perhaps Saml2Config could handle defaults more actively, and always populate the entire config graph. That way, the rest of the code would not have to check for nulls and supply defaults.

[i8beef]

Ok, just went through the config and usages and I think it should be good now. Try that branch now.

https://github.com/i8beef/SAML2/tree/5-CodeBaseConfig

[HDenBreejen]

Thanks for all the work on SAML2!
The config seems to work fine now. I have just tested with Azure-AD SSO. That does not require artifact resolution though. I'll test artifact resolution soon, using DigiD, the Dutch governmental ISP for citizen access to all kinds of official stuff. Can't do that from home. I'll let you know soon. Will the new config code make it to the master branch and the Nuget package?

[i8beef]

Yes it will. I'm cautious on this nowadays as I'm more of a "code owner" than a maintainer anymore. I don't actually have SAML2 implementations around anymore to even test with, so I tend to rely on community members like yourself to help push through some of these changes. This is sort of a pure abstraction job here, so I'm ok with the scope of this change where we aren't touching deep pieces of SAML functionality as long as we can get some quick smoke testing from community members.

This ALSO enables unit testing significantly, as we wouldn't be reliant on actual config files to configure the instance. Now we can configure the library per-test, which should allow for much better unit testing for a lot of areas that don't lend themselves well to it right now.

I do want to introduce one more piece before that though, but I want to make sure the config is solid first. I want to add a fluent configuration builder to allow for code-based configuration. It will be easier than trying to document how to build a Saml2Config object structure from scratch. That would all go in one big 3.0 release.

I'd also like to roll in #10 and maybe #24 in a 3.0 if we can get the testing all wrapped up on them.

[HDenBreejen]

I have tested branch 5 with DigiD now, so artifact-resolution looks fine too. Great! Adding a fluent config builder and more comprehensive unit testing would help increase quality further. I will have to accept that my app may temporarily will have work with the branch, instead of the nuget-package.
Would you consider just merging branch 5 into the main branch only and do a point-release of the package? This would resolve the bug this issue started with. Anyway, this is my first active involvement with open source. I can do cursory tests occasionally, like I did this week.

BTW: branche 5 has a breaking change to the public API. In my custom action, I did Saml2Config.GetConfig(), which is now .Current. Is deprecating GetConfig() an option?

[i8beef]

I'll have this done this weekend. The fluent builder is already built, I just need to write a configuration validator real quick, and some unit tests around all of it. Expect that I'll have a 3.x by Monday. Saml2Config.GetConfig() I'll consider... generally I shy away from deprecated flags when I'm already bumping a major version for other breaking changes, but as I'm in the code updating it, and as I go through and check the other logging libraries, etc., I'll see if I think its a good idea or not.

[i8beef]

Ok, latest is up in that branch with the config builder, config validator, and a test placeholder for adding more testing around that. If you'd like to pull that down and make sure that the validator passes your configuration correctly, I'll merge this into a 3.x release

[HDenBreejen]

Works fine with my configuration.

[i8beef]

should be pushing up now, might take it a few minutes to show up on NuGet

[HDenBreejen]

Great! Thanks a lot for the work you've put into this.

[jbreuer]

Hi @HDenBreejen. I would also like to use this library to implement DigiD. Could you share an example how you've implemented this?

[HDenBreejen]

Hello @jbreuer, I 'm quite happy with the SAML2 package. I've had some small issues in the config and metadata. The implementation of SAMLP seems solid so far.
My project has not gone into production yet, but I'm confident that I will be able to stick with SAML2. I have written little code. I have spent most of my time learning how ASP handles authentication, and how to integrate the SAML2 package into this. The code I did write was to eliminate the dependency on the ASP session state during log-off. I have moved this state into the ASP authentication cookie, and I'm happy with that. There is quite a lot going on in SAMLP, and consequently in the package. Getting to grasps with that while authentication against DigiD is not ideal. DigiD does not return any useful diagnostic, and the Logius helpdesk has limited knowledge of what is actually going on between your app and DigiD.

That's why I started out using Azure AD as a SAMLP IDP. Azure AD has useful diagnostics, and you will probably need that to gain some knowledge and confidence. I would like to share the lab solution I'have built, but it is not mine to share.. I have worked on this for my employer.

I do understand the need for some reference material, since there is very little to be found on the web. I wish I would have had access to a working solution early on. That would have saved me a lot of time. On the other hand, I do feel that working on this project, gathering fragments of knowledge for weeks has taught me quite a bit about ASP security, and about SAMLP as well.

Good Luck. Contact me again if necessary. Perhaps we can work something out.

[jbreuer]

Thanks for the feedback @HDenBreejen. Did you use Azure AD for testing purpose only or can you somehow also use it with DigiD?

I'm still pretty new to DigiD and SAML and like you mentioned there is a lot going on. An example of the saml2 config section for DigiD could already help a lot. I understand you can't just share your lab solution. However maybe if you ask your employer he doesn't mind. I've released a couple of solutions myself which can be used as examples: https://github.com/jbreuer/Hybrid-Framework-for-Umbraco-v7-Best-Practises https://github.com/jbreuer/1-1-multilingual-example By open sourcing these projects I even got some great feedback which I used to improve my code :-).

[jbreuer]

Hi @HDenBreejen. Since you are using DigiD didn't you run into this issue? https://github.com/i8beef/SAML2/issues/20

I think DigiD only supports the sha256 SignatureMethod.

[HDenBreejen]

SAML2 supports sha256 now, so there's no problem there. I did, however, have a problem with the signature of SAML token. This signature was rejected by SAML2. I have configured SAML2 not to validate this signature for the time being. I feel that the token is sufficiently protected anyway, but I may need to spend some time on this problem, since DigiD requires all signatures to be checked by the SP.
Theoretically, the problem could be in the signature DigiD provides.. that would also imply that no existing SP-implementation checks this signature.
So it's probably my fault, but I haven't a clue as yet.

[i8beef]

Not unless you added it yourself, in which case you should consider putting in a PR for it. SHA256 based singing in this library is still an open item.

[HDenBreejen]

There's SHA256-support in XmlSignatureUtil.cs. Is this incomplete perhaps?

[jbreuer]

See https://github.com/i8beef/SAML2/issues/20 what is needed to make it work.

I've also run into the issue with the signature of the SAML token. Might be related to only sha1 being supported right now. That probably needs to be fixed before this lib can work with DigiD.

[HDenBreejen]

SAML2 correctly parses DigiD's metadata, which is signed with SHA256. Perhaps the metadata signature was not validated.. Then what is the SHA256 stuff in XmlSignatureUtil.cs doing there?

[i8beef]

That allows for SHA256 validation of metadata and messages coming in you are correct (point 2 of the referenced ticket). But as I look at the actual code that would handle signing of metadata going out (Saml20MetadataDocument.SignDocument) and the HttpRedirectBindingBuilder class, I think it's all hard coded to SHA1. Part of the reason for that is that prior to .NET 4,0, there wasn't even a way to sign XML with anything BUT SHA1, and this library had its root in much older .NET versions. You'll note that even that piece only works in .NET 4.0+

If your IDP disallows signing by anything but SHA256, I believe it will in fact require modification to allow (a) specifying the signing algorithm in config, (b) using that in Saml20Metadata generation, (c) applying that in HttpRedirectBindingBuilder and (d) modifying XmlSignatureUtils.SignDocument to use the specified signing algorithm.

[HDenBreejen]

Thanks for this clarification. The support for SHA256 for incoming messages is sufficient for DigiD. My app's metadata is signed with SHA1, hadn't noticed that before, but DigiD does not require SHA256 there. The only problem I have encountered in this area, is that SAML2 rejects the SHA256-signature of the authentication token that I receive from DigiD as response to artifact resolution.
The artifact resolution response has another signature, at the outer level, which is validated ok though. Any ideas? How can I be certain that the signature is actually correct?

For the time being, I have used omitAssertionSignatureCheck in the config to ignore this signature alltogether, but I don't want to do this in production.