Sustainsys / Saml2

Saml2 Authentication services for ASP.NET
Other
960 stars 603 forks source link

Support for EphemeralKeySet #1307

Closed microalps closed 2 years ago

microalps commented 3 years ago

Resolves #1306, Possibly resolves #1298, Resolves #1252 Incorporates PR #1255

Changes library builds of .NET 4.7 to 4.7.2 - 4.7 will not be excluded and instead be covered by 4.6.1 build. Changes test builds to .NET 4.7.2 and .NET Core 2.1 - "Standard" doesn't include Ephemeral Key Sets and is no longer being developed.

Question 1 - should we build libraries for .NET Core App 2.1+ instead of .NET standard? Question 2 - since we need this fixed on the v2 branch - should I be basing fix on v2 rather than develop?

microalps commented 3 years ago

This includes the suggestion from @AndersAbel

The certificate handling is something that really should be reworked - accessing the private key and repackaging the key in a new cert was a workaround to support SHA256 several years ago. But it also creates all sorts of other issues. -- Gitter

The access of certificate.PrivateKey was introduced back when it was common for certificates to be generated with a crypto service provider that didn't support SHA256. That magic should all be removed now. It's almost 8 years since I started this project now, and the .NET echo system has evolved a lot during that time. -- Issue 1298

samisq commented 2 years ago

@microalps @AndersAbel What's the status of this PR and this project as a whole? Is the project no longer maintained?

microalps commented 2 years ago

Status of this PR I can speak for. It is waiting for a maintainer to approve even running tests/CI since I am a first-time contributor. @AndersAbel said on gitter they would accept a PR for v2+ but that never panned out. However, as I gave up on this getting merged in a timely fashion, I made public my commits for ALL versions for whoever needs it. See my note https://github.com/Sustainsys/Saml2/issues/1306#issuecomment-954241677

microalps commented 2 years ago

@AndersAbel I've pushed a rebased copy of this after your merge of #1313. Locally all tests are passing, need your approval for CI/Tests again. Once this is accepted to develop, I will create PR for v2 (all the code is checked in to my repo and does not include #1255 as you wanted)

AndersAbel commented 2 years ago

Build is broken, but I think that is the build server - develop doesn't build clean either. I'll build locally later today and check.

microalps commented 2 years ago

Yes, same errors. https://stackoverflow.com/questions/60371951/rzc-generate-exited-with-code-2147450730 seems to imply it is missing an SDK. https://github.com/Sustainsys/Saml2/blame/develop/.github/workflows/dotnetcore.yml says it has 3.1 but some projects refer to netcoreapp2.1.

AndersAbel commented 2 years ago

While the functionality is something I would like, the PR is from before the rewrite of the XML signature handling, so merging this won't be possible.

microalps commented 2 years ago

Instead of closing - you could welcome contributions by requesting a merge/rebase. However, it seems like having an empty issue list is your priority at the cost of losing potential contributions. It is unfortunate state of affairs. Go ahead, close #1306 as well and wish the problem away.

AndersAbel commented 2 years ago

The project unfortunately has come to a state where I simply cannot handle all the open PRs and issues. As the sole maintainer of the library I had to make the decision to make a cut to be able to move forward. What I am working on now is a complete rewrite, with a design that solves many of the problems that were hard/impossible to solve. If you have a look at the current contents of the develop branch, you'll see that I've just completely rewritten the XML processing and signature validation code. In doing that, I have removed a lot of the old patterns and quirks that caused modern scenarios to break. One is to no longer use the PrivateKey property directly and try to be a lot let invasive with the keys and cryptographic providers supplied. If I have gotten things right, i think that a lot of the key usage issues are already handled by the new code. If not, I would be happy to fix it - but any PRs written against the old code are simply not possible to use. It's not just to rebase - the target code is completely rewritten.