AES-GCM support

[jnivala]

We are facing a requirement to support receiving SAML2 responses with https://www.w3.org/2008/xmlsec/namespaces.html#aes128-gcm encrypted assertions on .NET 4.7.2 and .NET Core 3.1.

NET 4.7.2 does not contain the algorithm which leaves us with BouncyCastle implementation.

.NET Core 3.1 has it, but EncryptedXml, that handles decrypting, seems to be tightly coupled with SymmetricAlgorithm class that the AES-GCM implementation does not inherit from. So it seems you can not just plug in AES-GCM support even if framework contained it.

Any thoughts about this?

Regards.

[jnivala]

Ok, for the application (runs under Net 4.7.2) where we instantiate Saml2Response ourselves it seems we would need:

• Handle decrypting XML element inside Saml2Response rather than in extension class.
• Add a method with "public virtual EncryptedXml GetEncryptedXml(AsymmetricAlgorithm key, XmlDocument xmlDoc)" signature to Saml2Response to allow inheriting class to decide the EncryptedXml implementation.
• Open RSAEncryptedXml as public.

Then we can extend RSAEncryptedXml with our own decryption code and tell our Saml2Response implementation to use it.

For another application where we use the ASP NET Core pipeline, it would be the previous plus:

• To Saml2Handler add "public virtual ICommand GetCommand(string commandName)" to allow override command instantiation.
• To AcsCommand add method "public virtual Saml2Response GetSaml2Response(HttpRequestData request, IOptions options, UnbindResult unbindResult)" to allow instantiate custom Saml2Response instance.

Unrelated to the previous but to be able to extend AcsCommand behavior:

• Change AcsCommand.ProcessResponse from "private static" to "public virtual".

=> It seems we need to fork. Btw if there are any chances to get these to v2, I am willing to contribute. Also, for v3 it would be great to see more class member as virtual public for the sake of extensibility.

[JukkaM]

We are facing the same requirement as well. It would be great if we could have proper converstation here to decide how to proceed. @jnivala pointed out some refactorings which would allow us to extend the library so that we could build AES-GCM support on our own but is there plans to add AES-GCM support inside the library? Which is the best practice you suggest?

Naturally extensibility is great thing to support but adding more features to the library itself is good thing as well.

[AndersAbel]

The idea is to make things more extensible i v3 - yes.

And if the tight coupling with SymmetricAlgorithm can be removed it might be an idea. It is also worth checking out what features are available in Microsoft.IdentityModel. They have their own XML parser that might support these scenarios.

[scleikas]

Just dropped in to say that we're also interested in resolving this issue. I'm personally not intimately familiar with the library, but maybe there's something I can help with.

[tapiokulmala]

This is very important because 15.10 suomi.fi authentication switched into AES-GCM in test-environment and they are rolling it out into production in January.

[scleikas]

@tapiokulmala Indeed. I'm currently evaluating if it would be better to just switch libraries, or to try to modify this one. To make a PR on my own would probably require too much ramping up, though...

[JukkaM]

This seems to be very popular issue due to incoming Suomi.fi authentication change. We are also evaluating other libraries but it is a tough decision to replace already fully tested solution. I'm not very familiar with this library either but it seems like @jnivala has the most detailed technical point of view about required changes to this library. Do we have any estimate how much time those changes would take? Are we talking about days or weeks?

Anyway, if anyone knows a good alternative to this library we are also interested to know about it.

[AndersAbel]

I've had a look at what updates are needed. The bad news is that the .NET Framework's EncryptedXml implementation does not support AES-CGM. So this is not a problem unique to this library.

The good news is that AES-CGM in itself is supported in the .NET Framework and it doesn't look too hard to make an implementation that bypasses EncryptedXml and uses the algorithm directly.

Since multiple organizations appears to be affected I would suggest a solution where you together sponsor me to do the work and make it part of the official release. And with a release in time before the production switch over. If you are interested in taking part of a joint sponsorship, please mail me at anders@sustainsys.com.

[laurihelkkula]

Here's my attempt at implementing this to the v2 branch with minimal changes to the existing projects https://github.com/Sustainsys/Saml2/compare/v2...laurihelkkula:feature/v2-aesgcm?expand=1

The main points:

• RSAEncryptedXml needed to be inherited, so it is now public (but still in the Internal namespace)
• Instantiation of RSAEncryptedXml in CryptographyExtensions.Decrypt() is done via a static factory
• The new package Sustainsys.Saml2.AesGcmExtension targets .Net Standard 2.1, so it can use AesGcm from .Net Standard 2.1
• AesGcmAlgorithm & AesGcmDecryptor are very simple wrappers around AesGcm (and only for this specific purpose)
• Depending projects would need to call AesGcmRegistration.RegisterAesGcmSupport() in their Startup

I have verified that this works in our project's dev environment, which uses the current suomi.fi test environment (which now uses AES-GCM), but I haven't yet looked at contribution guidelines, naming conventions, unit tests, code formatting, how to integrate this to the develop branch, etc.

Obviously the way of extending RSAEncryptedXml is very crude and probably not suitable for the upcoming v3. Support for .Net Standard 2.0 or .Net framework should be possible with similar approach, but the AES-GCM implementation would need to come from somewhere else (BouncyCastle most likely).

I'm willing to continue working on this, so please let me know if this kind of approach could merged into this repository and what more should be done for that to happen. The AesGcmExtension package can also live outside this repo, but the extension possibility of RSAEncryptedXml is mandatory.

[AndersAbel]

@laurihelkkula Thank you for sharing. I looked quickly and have a few questions:

• Why do you put it in a separate package? I would prefer to have it in the main package. Dependency updates might be ok depending on what they are.
• Do you validate the integrity of the data as part of decryption operation? As I understand it this is the main feature advantage of AES-GCM.

[laurihelkkula]

@AndersAbel Thanks for taking a look.

I put the AES-GCM related code in a new package, because it requires targeting .Net Standard 2.1 and the current packages only target up to .Net Standard 2.0. I did try adding .Net Standard 2.1 target for the core and AspNetCore2 packages. It seemed to be fairly straightforward thanks to the good test coverage, but it seemed simpler and less risky to isolate the code that requires .Net Standard 2.1 to its own package. The required code changes were adjusting the few .Net Standard 2.0 references in the code and conditional package references. In the unit tests, a new Tests.NetCore31 project was simple to add thanks to the test code being already in a shared project, but I didn't yet look at how to run AspNetCore2.Tests in both .Net Core 2.1 and 3.1. If you think adding .Net Standard 2.1 target to the core and AspNetCore2 packages is ok, I can do those changes and put the AES-GCM related code in the core package.

The authentication tag of AES-GCM is validated by the method AesGcm.Decrypt. I'm planning to do a unit test to verify this.

[AndersAbel]

What would an upgrade to .Net Standard 2.1 mean for compat? I guess .Net Core is fine for the versions actually used. But what about .Net Framework?

[laurihelkkula]

Sorry for the delay.

I've been pondering on this and here are some options that I have thought about:

1. Use AES-GCM from .Net Core. Requires targeting .Net Standard 2.1, no built-in support for other targets (users could still provide the implementation they choose).
2. Use AES-GCM from BouncyCastle.Portable. Does not require targeting 2.1, can support all current targets.
3. Don't decide on the AES-GCM implementation in this repo, and leave it to the users. Just provide the necessary extension possibilities.

I feel like option 3 would be the best choice. If the static factory pattern for the RSAEncryptedXml class should be avoided, the necessary change could be included in the existing RSAEncryptedXml class. This part of the implementation is based on the https://www.w3.org/2008/xmlsec/namespaces.html#aes128-gcm spec, and is not the actual implementation of AES-GCM, so it should be safe to include. This way, there would actually be no new configurations. Users would simply provide the chosen AES-GCM implementation by registering it in the CryptoConfig.

How do you @AndersAbel see these options or do you have other ideas?

[JukkaM]

If the third one let's people use any other crypto algorithm as well then I would suggest it. We need .NET Framework support so targeting only to .NET Standard 2.1 should be avoided.

[AndersAbel]

This turned out to be non-trivial. Dependencies is a complicated issue when writing a library that is integrated in multiple applications. Every single external dependency is a risk for version conflicts for the consumer of the library.

On the other hand, providing a hooking point is quite complex, it puts additional burden on the user. So far in the library I have tried very hard to keep the number of dependencies down while also providing out-of-the-box functionality.

One option here is to actually do some conditional references. We could add a .NET Standard 2.1 target that uses the included AES-GCM implementation. And on all other targets depend on BouncyCastle.Portable. Does that make sense? Is it a problem to add a dependency on BouncyCastle.Portable for the .NET Framework target?

[laurihelkkula]

Thinking about the possible dependency conflicts, even providing the SymmetricAlgorithm wrapper for AES-GCM may be a problem for someone, since it is registered in the static CryptoConfig class. This may be a theoretical problem for now, but .Net should have better support for this algorithm some day in the near future and then this will become real problem.

Option 3 with the modified but non-extendable RSAEncryptedXml would seem to be most feasible way. There would be no additional hooking points, since CryptoConfig already exists. This would provide a way to use AES-GCM in the current platform targets and would be as future proof as it now can be. As a reminder, I haven't looked at the development branch so this is all for v2.x.

For providing this out-of-the-box, using .Net's or BouncyCastle's AesGcm conditionally certainly is doable, but as already mentioned, it would bring a risky dependency to BC and the SymmetricAlgorithm wrapper doesn't feel like it belongs to this project.

[laurihelkkula]

I created a PR for option 3 with minimal changes to the current projects and included a sample of how to add AES-GCM support. This should be good enough at least for the v2 branch. The sample is only for adding AES-GCM support and not a complete sample of setting up SustainSys.Saml2 in .Net Core 3.1, since there are no differences in configuration SustainSys.Saml2 and a complete sample would also need an Identity Provider that uses AES-GCM.

[AndersAbel]

@laurihelkkula The PR is now merged. It is a very good non-intrusive implementation for v2.

[AndersAbel]

@laurihelkkula We are having some issues when testing this with the Finnish federation (I want to test it before release). Did you have a chance to try the merged version? If you can, please mail me at anders@sustainsys.com to get included in the troubleshooting e-mail thread.

[AndersAbel]

This is merged and done for v2, but I'm keeping the issue open because it needs to be added to develop too, after refactoring is done.

[scleikas]

This is merged and done for v2, but I'm keeping the issue open because it needs to be added to develop too, after refactoring is done.

Hi Anders - As this ticket is still open, i.e. possibly not implemented in the development version yet, I was wondering if you're planning on implementing this in the next major version (v3 perhaps).

Thanks,
Jarno

[AndersAbel]

@scleikas Yes, it's kept open as it should be implemented in the new code as well. Can't promise if it'll make to v3 though, more likely to a 3.1 or 3.2 something - I'd like to get a 3.0 out as soon as it supports the basic flows.