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

Add System.Security.Cryptography.Xml.SignedXml class #4278

Closed
ghost opened this Issue Nov 1, 2015 · 230 comments

Comments

@ghost

ghost commented Nov 1, 2015

Execution plan

Goal: Provide APIs fully compatible with full/Desktop .NET Framework (no changes as part of this work - straight port only)

Plan:

  • 1. Add the source code on GH sanitized, with licenses, etc. (it won't build)
  • 2. Make the source code build (still excluded from overall repo build)
  • 3. Remove Desktop registry-compat code paths
    • Remove methods that have [RegistryPermission] (Utils and SignedXml classes) along with any owned methods
    • Address any other registry-related compile errors such as those using Registry, RegistryPermission, RegistryKey, etc (delete code as necessary)
  • 4. Add tests (We have to agree on expected code coverage and how much of the spec we have to cover)
    • Compare test results between Desktop and .NET Core implementations
  • 5. Make it part of overall repo build & ship it!

Code changes rules: Only code changes absolutely necessary to make it build and compatible with (full) .NET Framework are allowed, no additional bug fixes or changing architecture - such PRs will be rejected now.
Changes like that can be considered after the initial port is done, when we have a good test bed and when we are able to validate such change.

If larger code/architecture changes are needed from some reason, we should discuss them first here, before doing the work / submitting PR.

If you work on some parts of the plan, please say so in the discussion to avoid duplicated work. We (@steveharter @karelz) will co-assign the issue to you.


Original

The class to digitally sign XML needs to be added.

@rschiefer

This comment has been minimized.

rschiefer commented Feb 12, 2016

As referenced by Tratcher, this is a blocker for adding support for WsFederation/ADFS in ASP.NET 5. We use ADFS extensively for many enterprise ASP.NET 4 applications. We are very interested in migrating to ASP.NET 5 and using WsFederation.

@bartonjs

This comment has been minimized.

Member

bartonjs commented Feb 12, 2016

@rschiefer @Tratcher Well, it's... complicated.

  • ADFS doesn't use System.Security.Cryptography.Xml.SignedXml; but instead its own implementation.
    • (That's largely from dusting off mental cobwebs and remembering back to being on that team for version 1 of ADFS)
  • System.IdentityModel definitely doesn't use System.Security.Cryptography.Xml
    • (That's 'cuz their source code on referencesource says so 😄)
  • People don't really like System.Security.Cryptography.Xml.SignedXml because it's based on XmlDocument, which leads to some perf issues.
    • The ADFS version was based on XmlReader, IIRC.
  • So, likely what people want is CoreFX to create a new implementation of SignedXml.
  • But a new version of SignedXml won't compatible with the old version of SignedXml
  • So some people might want us to keep the old version, too.
  • So in all, people really want two versions.
  • Except they don't want the old version.
  • Except for when they do.

So we're left with the conundrum of:

  • We can port that class that no one really wants
  • Or, we can stop and design a brand new thing that probably no one will use, since they can't be compatible with anything else
  • Or, to use the most effort and still not benefit anyone, do both 😄.

@terrajobst - fyi

@bartonjs

This comment has been minimized.

Member

bartonjs commented Feb 12, 2016

Apparently I was a bit rambly this morning. Sorry :).

We've definitely identified that there's work to be done here, but we don't think the right answer is to bring forward the existing System.Security.Cryptography.Xml code. Instead, this represents the item on our backlog to engineer a general-purposed implementation for XmlDSig which is fast and not tied to legacy object models (e.g. XmlDocument).

That effort isn't something that we're expecting to do for .NET Core 1.0, simply because we're focused on other things.

But, letting us know that you're impacted by the missing functionality does help with ongoing prioritization.

@AndersAbel

This comment has been minimized.

AndersAbel commented Apr 1, 2016

I'm looking at creating an ASP.NET Core SAML2 Middleware, based on https://github.com/KentorIT/authservices. Without a SignedXml port it won't be able to run on the Core framework.

I definitely do agree that not carrying over the existing one is a good idea. Would it be possible to do anything based on an XmlReader API? That way both XDocument and XmlDocument can be supported. Also providing an implementation of the enveloped reader used in System.IdentityModel would be nice (if it was improved to support XML files with whitespace...)

@bartonjs

This comment has been minimized.

Member

bartonjs commented Apr 1, 2016

@AndersAbel XmlReader is where I'd start, yeah (unless the XML guys tell me there's something better).

Since XmlReader what the System.IdentityModel variant is based on, it should be doable :).

@AndersAbel

This comment has been minimized.

AndersAbel commented Apr 1, 2016

@bartonjs The System.IdentityModel variant is quite limited though in what transforms it can handle. For SAML2/WS-Fed work it wouldn't be a problem, but as a general API it must be considered how to deal with non-enveloped signatures and xml containing nested signatures (such as a signed saml response containing signed assertions). Also I think that the System.IdentityModel.EnvelopedSignatureReader is copying the data when doing the validation. There's a lot of fun to be done there. If I had the time I'd love to work on it.

@Icestorm0141

This comment has been minimized.

Icestorm0141 commented Apr 18, 2016

I appreciate the rambling @bartonjs, helps to see whats going on behind the scenes.

Currently my company is impacted by this. We have some legacy code that we're trying to port over to .NET Core that generates signed XML license files and without this set of classes we're stuck. We're open to diverging from XML files as the base for the licenses but as of this moment we haven't found a good solution that fits our needs.

Looking forward to seeing this get added in the future.

@joshfree joshfree added the Feature label May 2, 2016

@sandersaares

This comment has been minimized.

sandersaares commented May 26, 2016

I can attest that this (and also the slightly related XML Encryption) is relevant for us. The existing form in .NET Framework would be just fine - no need for innovation here, from my perspective. Copy & paste implementation would be very welcome!

Is there currently any workaround available?

@af0l

This comment has been minimized.

af0l commented Jun 5, 2016

Would like to know what @sandersaares asked, too. There's no built in way to sign xml in the CoreFX now?

@bartonjs

This comment has been minimized.

Member

bartonjs commented Jun 6, 2016

@sandersaares / @af0l : There is not, for .NET Core 1.0, any inbuilt implementation of SignedXml/XmlDSig.

Based off of comments here (and others) we'll probably just bring over the old API, but we didn't have time to make it happen for 1.0.

@af0l

This comment has been minimized.

af0l commented Jun 6, 2016

Thank you @bartonjs, that must be the reason I could not get our project to work on Core. :) It's also a great shame, because I'd love to move ahead and can't until it's done. We have to report all payments to our tax authority using signed xml's, so it's a showstopper really.

@emailtowalter

This comment has been minimized.

emailtowalter commented Jul 11, 2016

Any progress on this? I am stuck with the SAML token validation which requires this feature. Thanks

@af0l

This comment has been minimized.

af0l commented Jul 11, 2016

Yeah, would liek to know that too. We ended up extracting the features needing the signature and putting them into a separate web API solution...

@john182

This comment has been minimized.

john182 commented Jul 24, 2016

Already have idea on which version will be implemented or solution

@bartonjs bartonjs modified the milestones: 1.1.0, Future Jul 28, 2016

@bartonjs

This comment has been minimized.

Member

bartonjs commented Jul 28, 2016

At this point it seems that the most straightforward answer is to port the existing .NET Framework implementation to .NET Core. So we're bucketing this in with other "making it hard to port" API omissions.

@sandersaares

This comment has been minimized.

sandersaares commented Aug 2, 2016

Potentially relevant to the topic: https://connect.microsoft.com/VisualStudio/feedback/details/3002812/xmldsigc14ntransform-incorrectly-strips-whitespace-and-does-it-inconsistently and https://github.com/sandersaares/xml-c14n-whitespace-defect. It would seem to me that the .NET Framework implementation of Canonical XML 1.0 is incorrect. I hope I am wrong but if so, this might introduce some hairy compatibility questions.

@AndersAbel

This comment has been minimized.

AndersAbel commented Aug 2, 2016

@sandersaares Looked at your sample and you need to set XmlDocument.PreserveWhiteSpace = true when reading the Xml if it contains whitespace.

@JaedsonBarbosa

This comment has been minimized.

JaedsonBarbosa commented Apr 11, 2017

@krwq I just put that command:
Install-Package System.Security.Cryptography.Xml -Version 4.4.0-preview1-25210-01

That's the error:

O pacote System.Security.Cryptography.Xml 4.4.0-preview1-25210-01 não é compatível com uap10.0 (UAP,Version=v10.0). O pacote System.Security.Cryptography.Xml
4.4.0-preview1-25210-01 dá suporte a:

  • monoandroid10 (MonoAndroid,Version=v1.0)
  • monotouch10 (MonoTouch,Version=v1.0)
  • netcoreapp2.0 (.NETCoreApp,Version=v2.0)
  • uap10.1 (UAP,Version=v10.1)
  • xamarinios10 (Xamarin.iOS,Version=v1.0)
  • xamarinmac20 (Xamarin.Mac,Version=v2.0)
  • xamarintvos10 (Xamarin.TVOS,Version=v1.0)
  • xamarinwatchos10 (Xamarin.WatchOS,Version=v1.0)
@karelz

This comment has been minimized.

Member

karelz commented Apr 11, 2017

Just a reminder of what I said earlier: #4278 (comment)
I don't think you will get UWP support with the package. The package depends on .NET Standard 2.0 AFAIK and UWP does not yet have .NET Standard 2.0 support -- it is something we will work on for .NET Core 2.1 (some bits are done early to unblock larger team to work on it post 5/10, but it is not fully functional).
IMO to get UWP support for the package you will have to wait on 2.1.

@JaedsonBarbosa

This comment has been minimized.

JaedsonBarbosa commented Apr 11, 2017

@karelz So do you think I will need to wait until when?
Can I create an uap10.1 project?

@karelz

This comment has been minimized.

Member

karelz commented Apr 11, 2017

Yes, I think you will need to wait until then.
Unless you are super-motivated and can make it through all these speed bumps. As I said, until 5/10 our ability to help you with anything UWP will be very limited, so you will be mostly on your own 😦. Just setting the expectations here ...

@JaedsonBarbosa

This comment has been minimized.

JaedsonBarbosa commented Apr 11, 2017

So I will wait because I can't build the corefx-master with Visual Studio 2017 😞

@JaedsonBarbosa

This comment has been minimized.

JaedsonBarbosa commented Apr 12, 2017

@karelz @krwq I can't build the solution because I give an error, you can see the CMakeError here:
https://1drv.ms/u/s!AjDoJtNk3vvWjKIAYajeMPkWkI-m6Q
Please give me a help 🆘
PS: It's in portuguese, but you can use a translator to read that 😄

I think that failed isn't good:
-- The C compiler identification is MSVC 19.0.24218.2
-- The CXX compiler identification is MSVC 19.0.24218.2
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/Shared/14.0/VC/bin/amd64/cl.exe
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/Shared/14.0/VC/bin/amd64/cl.exe -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/Shared/14.0/VC/bin/amd64/cl.exe
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/Shared/14.0/VC/bin/amd64/cl.exe -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Performing Test COMPILER_HAS_DEPRECATED_ATTR
-- Performing Test COMPILER_HAS_DEPRECATED_ATTR - Failed
-- Performing Test COMPILER_HAS_DEPRECATED
-- Performing Test COMPILER_HAS_DEPRECATED - Success
-- Configuring done
-- Generating done

@JaedsonBarbosa

This comment has been minimized.

JaedsonBarbosa commented Apr 12, 2017

Anybody now what can I do to make it build?

@krwq

This comment has been minimized.

Member

krwq commented Apr 12, 2017

@JaedsonBarbosa how do you build? Regular process for me is:

  1. Open cmd.exe
  2. pushd corefx\repo\path
  3. git pull
  4. git clean -fdx - this will clean your repository of any leftover files (be careful if you're not sure what it does)
  5. build or ./build.sh if you are on non-windows

For building native components you need to install CMake 2.8.12 or higher

This has always just worked for me.

@karelz

This comment has been minimized.

Member

karelz commented Apr 12, 2017

@Jaedson33 you can also check & help us improve our new-contributor docs (linked from CoreFX main page)

@JaedsonBarbosa

This comment has been minimized.

JaedsonBarbosa commented Apr 12, 2017

I'm using the CMake 3.8.
@krwq I did what you said, and I'm waiting about 1 hour and there is just Installing dotnet cli..., how many hours do you think I will need to wait?

@krwq

This comment has been minimized.

Member

krwq commented Apr 12, 2017

@JaedsonBarbosa it should take up to few minutes, try restarting - it could be some connection issues, if doesn't work please file a separate issue in this repo, someone should be able to track down what has happened

@JaedsonBarbosa

This comment has been minimized.

JaedsonBarbosa commented Apr 13, 2017

@krwq I get this error:

C:\Users\jaeds\Source\Repos\corefx>call "C:\Users\jaeds\Source\Repos\corefx\Tools\dotnetcli\dotnet.exe" restore "C:\Users\jaeds\Source\Repos\corefx\packages\microsoft.dotnet.buildtools\1.0.27-prerelease-01512-01\lib\\tool-runtime\project.csproj" --source https://dotnet.myget.org/F/dotnet-core/api/v3/index.json --source https://dotnet.myget.org/F/dotnet-buildtools/api/v3/index.json --source https://api.nuget.org/v3/index.json  
C:\Users\jaeds\Source\Repos\corefx\packages\microsoft.dotnet.buildtools\1.0.27-prerelease-01512-01\lib\\tool-runtime\project.csproj --source https://dotnet.myget.org/F/dotnet-core/api/v3/index.json --source https://dotnet.myget.org/F/dotnet-buildtools/api/v3/index.json --source https://api.nuget.org/v3/index.json
  Restoring packages for C:\Users\jaeds\Source\Repos\corefx\packages\microsoft.dotnet.buildtools\1.0.27-prerelease-01512-01\lib\tool-runtime\project.csproj...
  Retrying 'FindPackagesByIdAsync' for source 'https://dotnetmyget.blob.core.windows.net/artifacts/dotnet-buildtools/nuget/v3/flatcontainer/system.diagnostics.debug/index.json'.
  An error occurred while sending the request.
    A conexÆo com o servidor foi interrompida de modo anormal
  Retrying 'FindPackagesByIdAsync' for source 'https://dotnetmyget.blob.core.windows.net/artifacts/dotnet-buildtools/nuget/v3/flatcontainer/system.runtime/index.json'.
  An error occurred while sending the request.
    A conexÆo com o servidor foi interrompida de modo anormal
  Retrying 'FindPackagesByIdAsync' for source 'https://dotnetmyget.blob.core.windows.net/artifacts/dotnet-buildtools/nuget/v3/flatcontainer/system.text.encoding/index.json'.
  An error occurred while sending the request.
    A conexÆo com o servidor foi interrompida de modo anormal
  Retrying 'FindPackagesByIdAsync' for source 'https://dotnetmyget.blob.core.windows.net/artifacts/dotnet-buildtools/nuget/v3/flatcontainer/system.runtime/index.json'.
  An error occurred while sending the request.
    A conexÆo com o servidor foi interrompida de modo anormal
  Retrying 'FindPackagesByIdAsync' for source 'https://dotnetmyget.blob.core.windows.net/artifacts/dotnet-buildtools/nuget/v3/flatcontainer/system.text.encoding/index.json'.
  An error occurred while sending the request.
    A conexÆo com o servidor foi interrompida de modo anormal
C:\Users\jaeds\Source\Repos\corefx\Tools\dotnetcli\sdk\2.0.0-preview1-005724\NuGet.targets(97,5): error : Failed to retrieve information about 'System.Text.Encoding' from remote source 'https://dotnetmyget.blob.core.windows.net/artifacts/dotnet-buildtools/nuget/v3/flatcontainer/system.text.encoding/index.json'. [C:\Users\jaeds\Source\Repos\corefx\packages\microsoft.dotnet.buildtools\1.0.27-prerelease-01512-01\lib\tool-runtime\project.csproj]
C:\Users\jaeds\Source\Repos\corefx\Tools\dotnetcli\sdk\2.0.0-preview1-005724\NuGet.targets(97,5): error :   An error occurred while sending the request. [C:\Users\jaeds\Source\Repos\corefx\packages\microsoft.dotnet.buildtools\1.0.27-prerelease-01512-01\lib\tool-runtime\project.csproj]
C:\Users\jaeds\Source\Repos\corefx\Tools\dotnetcli\sdk\2.0.0-preview1-005724\NuGet.targets(97,5): error :   A conexÆo com o servidor foi interrompida de modo anormal [C:\Users\jaeds\Source\Repos\corefx\packages\microsoft.dotnet.buildtools\1.0.27-prerelease-01512-01\lib\tool-runtime\project.csproj]

C:\Users\jaeds\Source\Repos\corefx>set RESTORE_ERROR_LEVEL=1 
ERROR: An error occured when running: '"C:\Users\jaeds\Source\Repos\corefx\Tools\dotnetcli\dotnet.exe" restore "C:\Users\jaeds\Source\Repos\corefx\packages\microsoft.dotnet.buildtools\1.0.27-prerelease-01512-01\lib\\tool-runtime\project.csproj"'. Please check above for more details.
@karelz

This comment has been minimized.

Member

karelz commented Apr 13, 2017

@JaedsonBarbosa please file a new issue as suggested above. Just a reminder that we may not be able to provide as much troubleshooting support to you before 5/10 as we would like to.

@JaedsonBarbosa

This comment has been minimized.

JaedsonBarbosa commented Apr 13, 2017

@karelz It's now working 😄
The only think I did to make it build was move the files from C:\Users\jaeds\Source\Repos\corefx-master to C:\Users\jaeds\Source.
I think it should be in the README

@JaedsonBarbosa

This comment has been minimized.

JaedsonBarbosa commented Apr 24, 2017

@karelz

This comment has been minimized.

Member

karelz commented Apr 24, 2017

@JaedsonBarbosa great! Is there anything from your work which would be valuable to flow back into CoreFX? (i.e. things which are not temporary hacks)

@JaedsonBarbosa

This comment has been minimized.

JaedsonBarbosa commented Apr 24, 2017

@karelz Well, I think you can use my project to see what can you simplify (or remove) in the CoreFX 😄

@leopignataro

This comment has been minimized.

leopignataro commented May 16, 2017

Hello everybody, great effort on porting this!

May I ask why is the Nuget package referencing .NET Core 2.0, instead of .NET Standard 2.0? Wouldn't that be preferred?

@danmosemsft

This comment has been minimized.

Member

danmosemsft commented May 17, 2017

@leopignataro

This comment has been minimized.

leopignataro commented May 17, 2017

Yes, that's were I was looking. Thank you, @danmosemsft!

@danmosemsft

This comment has been minimized.

Member

danmosemsft commented May 17, 2017

@leopignataro no problem, I encourage you to try bits from "head" -- you can get them from the homepage here https://github.com/dotnet/cli ... you can just download the zip if you want. Let us know if you find issues -- we have only a little time left to fix them before release.

@karelz

This comment has been minimized.

Member

karelz commented May 17, 2017

FYI: Here's info about timelines: #17619 (comment)

@leopignataro

This comment has been minimized.

leopignataro commented May 25, 2017

Since you mention it, @danmosemsft, there's this one issue:

#19198

I've suggested a fix on the thread but my message may have been missed in all the buzz.

@karelz

This comment has been minimized.

Member

karelz commented May 25, 2017

@leopignataro fix for what where? If it is fix for #19198, then it should be tracked in the bug. If it is something else, I would prefer to see separate issue for it.
If you think your fix suggestion was overlooked somewhere, raise it again on that thread and ask for feedback.

@fletchsod-developer

This comment has been minimized.

fletchsod-developer commented Sep 6, 2017

I'm confused now. I thought System.Security.Cryptography.Xml NuGet package is for .NET Framework and it is already included in Dot Net Core v2. It's not recognizing this namespace in Dot Net Core v2. Did I hear this incorrectly? Thanks.

@bartonjs

This comment has been minimized.

Member

bartonjs commented Sep 6, 2017

@fletchsod-developer The package is mainly for .NET Core. But if you reference it from a library targeting .NET Standard it will unify with the .NET Framework version on .NET Framework, and run the code from within the package on .NET Core.

We don't have any plans to put SignedXml into the default install experience for .NET Core; it's niche enough that being a separate package on NuGet seems the best distribution model.

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