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

.NET Standard 2.0 port #939

Merged
merged 19 commits into from Sep 13, 2018

Conversation

Projects
None yet
@blushingpenguin
Copy link
Contributor

blushingpenguin commented Mar 30, 2018

In this fork, I've ported the library so that it only relies on .NET standard and not the full framework. I have all the tests passing, but code coverage will be incomplete (I need to add tests for the new code).

Some of the code that was in WIF is provided by Microsoft.IdentityModel:
https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet
This is incomplete, notably the metadata serialization, so I re-implemented the metadata serialization that was in WIF. (I haven't added any tests for the new code yet). I think there is a bug in the signing code in Microsoft.IdentityModel.Xml.EnvelopedSignatureWriter:
AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#900
I've worked around this temporarily with some cut and paste. (I also fixed SignedXml so that it could cope with Reference Id=)

Xml signatures are provided by the package from corefx:
https://github.com/dotnet/corefx/tree/master/src/System.Security.Cryptography.Xml
SignedXml in the corefx package supports sha256/384/512 without needing to patch CryptoConfig's algorithm list so I've simply removed the code that deals with that.
Note that System.Security.Cryptography.Xml is .NET standard 2.0, which sets the minimum version for Sustainsys.Saml2 to 2.0, which implies that the revised library needs .NET framework 4.6.1 or .NET core 2.0

Please let me know what you think!

Thanks,

Mark

@AndersAbel

This comment has been minimized.

Copy link
Member

AndersAbel commented Mar 31, 2018

This looks fantastic! I will do my best to review it after the weekend.

blushingpenguin added some commits Mar 31, 2018

@MrSmoke

This comment has been minimized.

Copy link

MrSmoke commented Apr 20, 2018

🔥 Has any progress been made with this MR and the current netstandard branch? Would love to see .netstandard2.0 support soon 👍

@sandorfr

This comment has been minimized.

Copy link
Contributor

sandorfr commented Apr 24, 2018

This is awesome, I was actually doing duplicated work. Looking forward to this being merged.

blushingpenguin added some commits Apr 24, 2018

removed asp.net identity from SampleIdentityServer4 and added a sampl…
…e with working IdentityServer4 + ASP.NET Identity
@devharis

This comment has been minimized.

Copy link

devharis commented Apr 30, 2018

Whats the status on this one? Is there anything I could help with to speed up the process? 🔨 @AndersAbel

@lol768

This comment has been minimized.

Copy link

lol768 commented May 4, 2018

Awesome work here @mdw211, hope it gets merged.

@ChronosWS

This comment has been minimized.

Copy link

ChronosWS commented May 28, 2018

Definitely would love to see this get merged asap. Much needed, very appreciate!

HashAlgorithm hashAlg = signatureDescription.CreateDigest();
hashAlg.ComputeHash(Encoding.UTF8.GetBytes(queryString));
AsymmetricSignatureFormatter asymmetricSignatureFormatter =
signatureDescription.CreateFormatter(
((RSACryptoServiceProvider)message.SigningCertificate.PrivateKey)
EnvironmentHelpers.IsNetCore ? message.SigningCertificate.PrivateKey :

This comment has been minimized.

@nbarbettini

nbarbettini May 31, 2018

IANA crypto expert, but according to this Stack Overflow discussion I believe certificate.GetRSAPrivateKey() is the future-proof way of doing this: https://stackoverflow.com/a/49777672/3191599

This comment has been minimized.

@AndersAbel

AndersAbel Jun 1, 2018

Member

@nbarbettini Thanks for pointing this out. I've still not had time to review this PR (stuck in a full time project), so help like this is very valuable.

This comment has been minimized.

@blushingpenguin

blushingpenguin Jun 5, 2018

Contributor

Thanks, I put fixing that on my todo list.

@AndersAbel

This comment has been minimized.

Copy link
Member

AndersAbel commented Jun 1, 2018

So, I think I'd better update on the progress of this. My plan is to merge this, but I have to get a few days of focused work to do that. Unfortunately I'm currently stuck in another full time project.

@scottbrady91

This comment has been minimized.

Copy link

scottbrady91 commented Jun 4, 2018

@mdw211 After talking to Anders about this, I was interested to know how you ported System.IdentityModel.Metadata. Did you decompile the .NET Framework code? Did the Microsoft license permit this?

Anyone know if Microsoft has plans to port this library?

@blushingpenguin

This comment has been minimized.

Copy link
Contributor

blushingpenguin commented Jun 5, 2018

@scottbrady91

@mdw211 https://github.com/mdw211 After talking to Anders about this, I was interested to know how you ported System.IdentityModel.Metadata. Did you decompile the .NET Framework code? Did the Microsoft license permit this?

It's done from the interface of the metadata parser, which was my starting point, and then the SAML specification documents. If you look at the code, you'll see I've pasted in the BNF productions from the specs and then basically turned them into parser code. (Keeping the interface gave me a relatively isolated section of code to work on which given I'm not a SAML expert by any means and didn't know the Sustainsys.Saml2 library seemed to be the easiest approach. After having worked through this I think the interface is too flexible if anything -- there are many hook points into the parser interface which I don't see as being that useful -- it would be interesting to know if anybody has used them).

Anyone know if Microsoft has plans to port this library?

They have ported parts of WIF to .NET core (which my port uses) under the System.IdentityModel namespace. I don't know if they have any plans to port the rest of it, it's for some sort of azure AD/saml integration so I guess they've just done what they needed:

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet

@scottbrady91

This comment has been minimized.

Copy link

scottbrady91 commented Jun 5, 2018

@mdw211 Okay, so the work you've done is new and in no way breaks the license of the original System.IdentityModel.Metadata. In that case, would you and @AndersAbel be interested in collaborating with me to pull this work out into a separate repository and library?

My employer and I have a vested interest in this since our IdentityServer 4 SAML component has a requirement on these libraries and looking at your PR most of the work seems to be done.

You guys up for this?

@blushingpenguin

This comment has been minimized.

Copy link
Contributor

blushingpenguin commented Jun 6, 2018

Anders told me that he was planning to put out a 1.0 release of the current library, then merge my fork (after code review/fixes) and put that out as a 2.0 release. I don't know what his timescales are on that, but it seems like a good plan to me (and one I'd prefer to renaming and putting out a separate nuget package). For my own current use of the fork we've just got an internal build that pushes a re-versioned package to an internal nuget server so timescales aren't an issue to me.

If you have additional work that you want to do around my fork then I'm happy to collaborate with you (you can either fork the fork and send me PRs or I can add you as a collaborator -- whichever you prefer). Out of interest, what work remains to be done from your perspective?

@scottbrady91

This comment has been minimized.

Copy link

scottbrady91 commented Jun 6, 2018

Just to confirm, I'm only talking about the code you've ported from the System.IdentityModel.Metadata namespace, not about releasing a fork of the Sustainsys package.

Since the metadata stuff is a big chunk of new code not currently maintained by this repository and something that I think others will want to use outside of just SAML, I think it would work well as a new library which could then be used in your PR.

If you're opposed either way, are you happy for me to use some of the metadata code you've ported? Have you licensed the code in the PR in a particular way?

@blushingpenguin

This comment has been minimized.

Copy link
Contributor

blushingpenguin commented Jun 7, 2018

Oh I see, in that case I'd very much got the wrong end of the stick. I'm certainly not averse to housing that code in a separate library, if you think it is useful. (I don't see it has any use outside of SAML though -- it just parses the SAML federation metadata XML into C# structures so I can't think what else you'd do with it). In license terms, I think it will be MIT (Anders told me he was planning to change the license of Sustainsys.Saml2 to MIT, but I don't think that has happened yet) or as it currently is, LGPLv3.

@devharis

This comment has been minimized.

Copy link

devharis commented Jun 7, 2018

Isn't RockSolidKnowledge selling this as a solution to customers but currently only supporting full .net framework so this would solve .Net Core support?

@scottbrady91

This comment has been minimized.

Copy link

scottbrady91 commented Jun 8, 2018

@devharis Yeah, Rock Solid Knowledge has products and commercial support for enabling IdentityServer4 to behave as a SAML or WS-Federation IdP, and limited support for as an SP.

@mdw211 Cool, I thought we were talking about different things :p This metadata library would be handy for both our SAML & WS-Fed components.

Okay, since you're okay with this, I'm going to make an example repo of the library I'm proposing. I'll license it as MIT. We would then be interested in discussing with @AndersAbel and yourself as to how Rock Solid Knowledge can contribute back to the Sustainsys SAML project.

@ckouroupis

This comment has been minimized.

Copy link

ckouroupis commented Jun 14, 2018

Hi @mdw211,

I managed to integrate my Identity Server 4 .Net Core project targetting netcoreapp2.1 with Sustainsys.Saml2 using your fork: https://github.com/mdw211/Saml2

Thanks VERY much!!!

I ran into these two minor issues:
a) In MetadataLoader.cs, I replaced WebClient reference with HttpClient to avoid PlatformNotSupportedException.
b) My ADFS metadata was failing to parse due to reader.GetAttribute("xsi", XsiNs); returning null

@blushingpenguin blushingpenguin referenced this pull request Jun 20, 2018

Closed

Netstandard #981

@ChronosWS

This comment has been minimized.

Copy link

ChronosWS commented Jun 22, 2018

What is the status of this work? I pulled this PR and applied it for our use, but it looks like there is some refactoring into a separate library for the identity metadata. Has that happened? Can I test building with it if so? Much appreciated!

@blushingpenguin

This comment has been minimized.

Copy link
Contributor

blushingpenguin commented Jun 22, 2018

From my point of view the changes are self-contained and working -- merging into the main repository is just waiting on Anders getting some free time.

The proposed refactoring is to allow separate use of the SAML federation metadata parser by scottbrady91 (personally I don't have any need for this, but equally I don't mind it having it split out and having the PR use the factored out code). I don't know where he's up to with it though.

@scottbrady91

This comment has been minimized.

Copy link

scottbrady91 commented Jul 6, 2018

I've made a start on the metadata stuff: https://github.com/scottbrady91/ScottBrady91.IdentityModel.Metadata

After getting hands-on with the code I think I'm seeing some additions specific to this repo. I'm currently focussing on keeping it as close as possible to the original metadata libraries, so I'm leaving new features out for now. I'll see how that goes, but it may turn into something not using by this repo. Not sure yet.

@toddlucas

This comment has been minimized.

Copy link

toddlucas commented Jul 31, 2018

Do we need to wait for the metadata library to be integrated into this branch prior to merging the PR? Or is that something that can be refactored on a different branch afterwards? We would really like to start making use of this on other platforms as soon as possible. Thanks!

@AndersAbel AndersAbel referenced this pull request Aug 2, 2018

Closed

Abandoned? #996

@AndersAbel AndersAbel merged commit 8e282b4 into Sustainsys:master Sep 13, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
WIP work in progress – do not merge!
Details

AndersAbel added a commit that referenced this pull request Sep 13, 2018

.Net Standard port
- Closes #939
@chris5287

This comment has been minimized.

Copy link

chris5287 commented Sep 19, 2018

@AndersAbel just wondering if there will be a new release/NuGet package soon? Or maybe there is a MyGet feed that I could use?

@AndersAbel

This comment has been minimized.

Copy link
Member

AndersAbel commented Sep 19, 2018

@chris5287 Yes, I'm currently working on reviewing this. A new release is planned for next week.

@AndersAbel AndersAbel added this to the v2.0.0 .NET Standard milestone Sep 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment