Skip to content
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

API proposal: DSA.IsSupported property #53017

Closed
filipnavara opened this issue May 20, 2021 · 19 comments
Closed

API proposal: DSA.IsSupported property #53017

filipnavara opened this issue May 20, 2021 · 19 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Security untriaged New issue has not been triaged by the area owner

Comments

@filipnavara
Copy link
Member

filipnavara commented May 20, 2021

Background and Motivation

Newly added AEAD cryptographic algorithm classes in .NET have the IsSupported property that can be used to determine whether a platform supports particular algorithm or not. This was approved in the ChaCha20Poly1305 API proposal (#45130) and also added to AesGcm and AesCcm classes to maintain API symmetry.

With the addition of new platforms in .NET 6 some of the old cryptographic algorithms, specifically DSA, are no longer available on all the platforms. iOS, tvOS and Mac Catalyst don't support or implement the DSA algorithm. APIs like DSA.Create methods now throw PlatformNotSupportedException (Issue #52758, PR #52978) on these platforms.

It would be useful to offer a way to detect platforms where DSA algorithm is not supported without having to call DSA.Create and checking for an exception.

Proposed API

namespace System.Security.Cryptography.Algorithms
{
    public abstract partial class DSA : AsymmetricAlgorithm
    {
+        public static bool IsSupported { get; }
    }
}

with implementation similar to:

namespace System.Security.Cryptography.Algorithms
{
    public abstract partial class DSA : AsymmetricAlgorithm
    {
+        [UnsupportedOSPlatformGuard("ios")]
+        [UnsupportedOSPlatformGuard("tvos")]
+        [UnsupportedOSPlatformGuard("maccatalyst")]
+        public static bool IsSupported => !OperatingSystem.IsIOS() && !OperatingSystem.IsTvOS() && !OperatingSystem.IsMacCatalyst();
    }
}

Usage Examples

Guard DSA specific code paths:

var asymmetricAlgorithm = keyAlgorithm switch
{
    Oids.Rsa => RSA.Create(),
    Oids.Dsa when DSA.IsSupported => DSA.Create(),
    Oids.EcPublicKey when IsECDsa(certificate) => ECDsa.Create(),
    Oids.EcPublicKey when IsECDiffieHellman(certificate) => ECDiffieHellman.Create(),
    _ => throw new CryptographicException(SR.Format(SR.Cryptography_UnknownKeyAlgorithm, keyAlgorithm)),
};

asymmetricAlgorithm.ImportPkcs8PrivateKey(data, out _);
return asymmetricAlgorithm;

Use specific condition in unit tests:

[ConditionalClass(typeof(DSA), nameof(DSA.IsSupported))]

Alternative Designs

  1. Don't add the IsSupported property, expect consumers of the API to catch PlatformNotSupportedException and to rely on platform compatibility analyzer. This is feasible until we hit another platform where DSA is not supported. Since the algorithm is effectively deprecated by NIST in FIPS 186-5 it's possible that some existing platforms may actually drop the support for the algorithm in future.

  2. Don't add the IsSupported property, expect consumers to call CryptoConfig.CreateFromName("DSA") instead which is non-throwing. It returns null on non-supported platforms. CryptoConfig is not very linker friendly though. It also always creates an instance of the DSA object on supported platforms which may not be desired for particular use case.

Risks

@filipnavara filipnavara added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 20, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Security untriaged New issue has not been triaged by the area owner labels May 20, 2021
@ghost
Copy link

ghost commented May 20, 2021

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and Motivation

Newly added AEAD cryptographic algorithm classes in .NET have the IsSupported property that can be used to determine whether a platform supports particular algorithm or not. This was approved in the ChaCha20Poly1305 API proposal (#45130) and also added to AesGcm and AesCcm classes to maintain API symmetry.

With the addition of new platforms in .NET 6 some of the old cryptographic algorithms, specifically DSA, are no longer available on all the platforms. iOS, tvOS and Mac Catalyst don't support or implement the DSA algorithm. APIs like DSA.Create methods now throw PlatformNotSupportedException (Issue #52758, PR #52978) on these platforms.

It would be useful to offer a way to detect platforms where DSA algorithm is not supported without having to call DSA.Create and checking for an exception.

Proposed API

namespace System.Security.Cryptography.Algorithms
{
    public abstract partial class DSA : AsymmetricAlgorithm
    {
+        public static bool IsSupported { get; }
    }
}

with implementation similar to:

namespace System.Security.Cryptography.Algorithms
{
    public abstract partial class DSA : AsymmetricAlgorithm
    {
+        [UnsupportedOSPlatformGuard("ios")]
+        [UnsupportedOSPlatformGuard("tvos")]
+        [UnsupportedOSPlatformGuard("maccatalyst")]
+        public static bool IsSupported => !OperatingSystem.IsIOS() && !OperatingSystem.IsTvOS() && !OperatingSystem.IsMacCatalyst();
    }
}

Usage Examples

Guard DSA specific code paths:

                return keyAlgorithm switch
                {
                    Oids.Rsa => ...,
                    Oids.Dsa when DSA.IsSupported => ...,
                    Oids.EcPublicKey when IsECDsa(certificate) => ...,
                    Oids.EcPublicKey when IsECDiffieHellman(certificate) => ...,
                    _ => throw new CryptographicException(SR.Format(SR.Cryptography_UnknownKeyAlgorithm, keyAlgorithm)),
                };

Loading PKCS blobs (example inspired by #52758):

private AsymmetricAlgorithm LoadAnything(ReadOnlySpan<byte> data)
{
    DSA dsa = DSA.Create();
    ECDsa ecdsa = ECDsa.Create();

    try
    {
        RSA rsa = RSA.Create();
        rsa.ImportPkcs8PrivateKey(data, out _);
        return rsa;
    }
    catch (CryptographicException)
    {
    }
    
    // repeat for ECDsa
    
    if (DSA.IsSupported)
    {
        try
        {
            DSA rsa = DSA.Create();
            dsa.ImportPkcs8PrivateKey(data, out _);
            return dsa;
        }
        catch (CryptographicException)
        {
        }
    }

    // fail
}

Alternative Designs

Don't add the IsSupported property, expect consumers of the API to catch PlatformNotSupportedException and to rely on platform compatibility analyzer. This is feasible until we hit another platform where DSA is not supported. Since the algorithm is effectively deprecated by NIST in FIPS 186-5 it's possible that some existing platforms may actually drop the support for the algorithm in future.

Risks

Author: filipnavara
Assignees: -
Labels:

api-suggestion, area-System.Security, untriaged

Milestone: -

@akoeplinger
Copy link
Member

Sounds reasonable to me however we might want to actually add IsSupported to most of the crypto types in System.Security.Cryptography.Algorithms since e.g. for Browser almost none except SHA* are supported.

@bartonjs what do you think?

@vcsjones
Copy link
Member

One thing the AesGcm et al. classes have going for them is they are sealed. Most of the other crypto primitives are not. Using DSA as an example, IsSupported would be accessible on other platform-specific implementations. e.g. DSACng.IsSupported. It would be weird for this to return true on Linux and macOS.

This is fixable by shadowing the IsSupported on other platform types. So I would consider that in the API proposal (and other places we might add IsSupported properties to):

namespace System.Security.Cryptography
{
    public sealed partial class DSACng : DSA
    {
+        public static new bool IsSupported { get; }
    }

    public sealed partial class DSAOpenSsl : DSA
    {
+        public static new bool IsSupported { get; }
    }
}

@filipnavara
Copy link
Member Author

@vcsjones That's an excellent point. The problem is that there could be custom implementations outside of the .NET Runtime which would not be aware of this and would not get the shadowing treatment. I am not sure how to approach that. I actually intended to supply a managed DSA implementation as a compatibility shim that would be consumable as a NuGet.

@vcsjones
Copy link
Member

vcsjones commented May 20, 2021

The problem is that there could be custom implementations outside of the .NET Runtime which would not be aware of this and would not get the shadowing treatment

Yeah, that problem already exists with the Create factory method.. a derived implementation could exist and not shadow Create, so MyCustomDSA.Create() would return the Libraries' internal implementation.

My 2 cents: it's fine to add a new API that implementors will have to shadow if they want it to behave correctly for their implementation. In that respect it's similar to adding a new virtual where the base implementation might do the right thing for the base, but it might not for your derived type.

But we should add this for the platform specific types if we decide to add it to the bases.

@GrabYourPitchforks
Copy link
Member

Do we have a critical mass of applications using DSA such that this property would actually be useful? DSA is effectively a dead algorithm.

The reason we added an IsSupported property to algorithms like ChaCha20Poly1305 and AesGcm is that it's feasible a modern application may attempt to use those algorithms as its preferred selection, falling back to secondary choices if the current platform does not support them. (These types also sport a modern API surface unconstrained by legacy patterns.) I don't see this being the case for ancient algorithms like DSA. It's not reasonable that many applications would be using DSA as their preferred default and want to fall back to a non-DSA secondary algorithm if DSA support is unavailable. Rather, if they're calling into DSA, it's almost certainly because they're forced to for some reason, which also implies they need to fail the operation if DSA is unavailable.

@filipnavara
Copy link
Member Author

filipnavara commented May 20, 2021

@vcsjones You actually made me realize that the shadowed properties could be useful for determining if alternate implementation is available. I would be able to check for DSAOpenSsl.IsSupported on macOS and use it instead of the default implementation.

@GrabYourPitchforks I agree it's effectively a dead algorithm but it's also one that still has widespread legacy use because of protocols that used it for signing of stored content. PGP introduced it around 1998 as the preferred algorithm with the second revision of OpenPGP specification. S/MIME (PKCS #7) also supports it as one of the signing algorithms although it never gained such prominence there.

For OpenPGP I maintain my toy implementation at https://github.com/1hub/springburg. It's essentially heavily rewritten version of BouncyCastle OpenPGP code with some improvements to API which uses native .NET cryptography classes where possible. DSA is already broken on macOS where it fails nearly every test (with exception of 1024 bit keys). If I were to support iOS I would have to offer some fallback, possibly to a naïve managed implementation or optionally to OpenSSL bundled with the application. A static property could make my life easier if I could write var dsa = DSA.IsSupported ? DSA.Create() : DSAManaged.Create(). Alternatively I could say that DSA is not supported on platforms where .NET doesn't support it and it would be easier to guard the tests with DSA.IsSupported than with an exclusion list of operating systems.

S/MIME is implemented in the System.Security.Cryptography.Pkcs assembly. Based on my PR #52978 that annotates the code with UnsupportedOSPlatform attributes and disables the relevant tests there were two places in the code that needed a DSA.IsSupported check in the implementation and couple dozen in the tests.

It's likely that any high-level protocol based on similar patterns as OpenPGP and S/MIME will have similar incidence of the number of checks (single digit number of checks in implementation, few dozen in unit tests). I came up with the proposal because the experience with annotating the existing high-level code made me believe that it's something that will come up even in 3rd-party code.

If the assumption is that the set of platforms supporting or not supporting DSA would not change then it may not be worth it. However, it may happen that some platforms drop support for DSA in future versions (macOS already fails to support it in number of newer APIs) and then the property would start to depend on OS version in addition to set of supported platforms. Similar situation could happen on browser target where DSA is not supported today. It may eventually get implemented (PRs using SubtleCrypto and shared array buffers are already there, other experiments are underway) but be supported only on certain browsers (eg. everything but Safari).

@GrabYourPitchforks
Copy link
Member

But in your scenarios, you're not falling back to an alternative algorithm. You're either calling into your own custom implementation (which you could just do anyway), or you're skipping the operation entirely (in the case of a unit test), or you're failing the operation entirely (in the case of OpenPGP mandating using DSA for some operation). None of these scenarios requires an IsSupported property. And for the latter two scenarios, since you're already bringing in your own custom DSA implementation, you could call it directly and bypass the built-in implementation entirely. What value does IsSupported add here that I'm missing?

@GrabYourPitchforks
Copy link
Member

GrabYourPitchforks commented May 20, 2021

Alternatively, what if we allowed you to utilize CryptoConfig to substitute your own implementation for DSA.Create()? Then IsSupported isn't quite necessary, since you could always query CryptoConfig for this information or even implement it yourself.

@filipnavara
Copy link
Member Author

What value does IsSupported add here that I'm missing?

It offers a way to determine whether DSA.Create would fail on given runtime, OS and OS version. In case of unit tests it's significantly easier to consume. In case of implementation it could avoid executing some code to decode DSA keys from storage format if I knew that it would fail later anyway.

(I don't necessarily want to ship alternate DSA implementation under the same principle that I don't want to write my own crypto but something like fallback to DSAOpenSsl and then to hard failure could be useful.)

Alternatively, what if we allowed you to utilize CryptoConfig to substitute your own implementation for DSA.Create()? Then IsSupported isn't quite necessary, since you could always query CryptoConfig for this information or even implement it yourself.

I was toying with the idea myself. It MAY work. The tricky part is to ensure that it is linker friendly since CryptoConfig uses reflection. If I use the DSA type directly it's the easy case. If I would use something like AsymmetricAlgorithm.CreateFromName("DSA") then the DSA type is only used in reflection.

@GrabYourPitchforks
Copy link
Member

I don't necessarily want to ship alternate DSA implementation under the same principle that I don't want to write my own crypto but something like fallback to DSAOpenSsl and then to hard failure could be useful.

But I keep coming back to "why?" In the scenarios you've laid out, you're not calling into DSA for fun. You're calling into it because you need it. Which means one of two things: you're either going to fail if it doesn't work, at which point IsSupported isn't buying you much; or you're going to fall back to your own known-good-works-everywhere implementation, at which point since you're carrying along such an implementation you may as well skip IsSupported and just call your implementation directly. I'm not seeing that the existence of an IsSupported property enables you to do something that you can't already do today.

@filipnavara
Copy link
Member Author

filipnavara commented May 20, 2021

For example, code like this:

static partial void PrepareRegistrationDsa(Dictionary<string, CmsSignature> lookup)
{
if (Helpers.IsDSASupported)
{
lookup.Add(Oids.DsaWithSha1, new DSACmsSignature(Oids.DsaWithSha1, HashAlgorithmName.SHA1));
lookup.Add(Oids.DsaWithSha256, new DSACmsSignature(Oids.DsaWithSha256, HashAlgorithmName.SHA256));
lookup.Add(Oids.DsaWithSha384, new DSACmsSignature(Oids.DsaWithSha384, HashAlgorithmName.SHA384));
lookup.Add(Oids.DsaWithSha512, new DSACmsSignature(Oids.DsaWithSha512, HashAlgorithmName.SHA512));
lookup.Add(Oids.Dsa, new DSACmsSignature(null, default));
}
}

It never actually calls DSA.Create so the PNSE exception would happen much later in the process. Calling DSA.Create as part of initialisation to determine support would needlessly allocate in case DSA is supported. In case DSA is not supported it would introduce an exception overhead.

Hard-coding the check to if (!OperatingSystem.IsIOS() && !OperatingSystem.IsTvOS() && !OperatingSystem.IsMacCatalyst()) may work today. It's not really future proof though if platforms drop DSA support (macOS? Android?) or if DSA support is added (browser, possibly only some browsers).

@GrabYourPitchforks
Copy link
Member

Ok, I understand your scenario now. Thanks for the patience. :)

@filipnavara
Copy link
Member Author

filipnavara commented May 20, 2021

Thanks for bearing with me. I know that this may not meet the bar for consideration given the legacy nature of the algorithm. It's kinda the opposite concern than the AEAD IsSupported property where I expect the platforms to gain support for newer algorithms. In this case I expect the platforms to drop the support. The implementations of high-level algorithm agnostic containers (X509 certificates, PKCS#7 signed data, OpenPGP, etc.) that support DSA as one of the algorithms have to deal with it in one way or another. Having a cheap way to check for the DSA support may make it slightly easier. At the end of the day I can implement the property myself in 3rd-party code as

static bool IsDSASupported { get; } = DetectDSASupport();

static bool DetectDSASupport()
{
     try { using (DSA.Create()) { } }
     catch (PlatformNotSupportedException) { return false; }
     return true;
}

but it has some overhead.

@GrabYourPitchforks
Copy link
Member

The "inherited" nature of static properties does make DSACng.IsSupported and friends a bit strange, but we might be able to work around that at a language level. I've been looking at crypto polyfills for unrelated reasons, so it'd also be useful to figure out if I as a plugin author can somehow make DSA.IsSupported return true, even on platforms without built-in support.

@bartonjs
Copy link
Member

I see where the usefulness comes from, but don't really think it's a good idea.

  • There's the static bleed-through problem that @vcsjones brought up already.
  • macOS can't create DSA keys (but can work with existing ones). Aside from our unit tests, no one has ever seemed to notice or care.
  • We already have a disparity between FIPS 186-2-only and FIPS 186-3+ DSA, but we've never had anyone ask for a way to tell on that.
  • For RSA we have similar "flavors" problems.
    • For a while, only RSACng could do RSAES-OAEP with a SHA-2 digest algorithm.
    • RSACryptoServiceProvider can't do PSS. Or RSAES-OAEP-SHA-2
    • I think on Android we couldn't make SignHash work? Maybe I'm misremembering.
  • Assuming I remember right about Android's RSA.SignHash, then DSA.SignHash and ECDsa.SignHash are probably in the same boat.

IsSupported is mostly useful when you have options. "I'm encrypting new data, I want to use AES-GCM if it's available (and I promise I know what I'm doing. What's nonce reuse? What? How do I? Oh, maybe I shouldn't... OK), and if not then I'll fall back to AES-CBC-PKCS#7+HMAC-SHA256, and leave an algorithm flavor marker in the data. If you're creating new signatures, you're probably not using DSA. If you are, you probably didn't have another choice, so it's just do or do not, there is no IsSupported check.

Now, when you're reading the data back, if it says to use AES-GCM you can check IsSupported to get a clear statement of why you need to present a particular error message, or you can just let the exception bleed through, or you can catch it... but no matter what, you will not get to read your message today. It's just variations on "fail".


If not for the static bleed-through thing, I'd probably just shrug. But, I do think that the IsSupported is only useful for "this is too new, it's not available yet" (you can't have your first choice). When the algorithm has been forced on you there's almost no benefit in checking it.

@FilipToth
Copy link
Contributor

Algorithms like Sha256 already have the IsSupported property, It would be terrible design if we weren’t consistent and didn’t add it to other crypto types like DSA. It is up to the consumer of our APIs on what they do when an algorithm is not supported.

@filipnavara
Copy link
Member Author

Algorithms like Sha256 already have the IsSupported property

They actually don't. There's SHA256 algorithm class with no IsSupported property. The Sha256 class is in a different namespace and it's an instruction set of a processor (ie. hardware accelerated implementation). The hardware intrinsics had IsSupported property since day 1 because they inherently depend on the capabilities of the processor the application is running on.

@filipnavara
Copy link
Member Author

filipnavara commented May 21, 2021

There's the static bleed-through problem that @vcsjones brought up already.

That's something I originally didn't think about and that makes me want to abandon the proposal. On the other hand it would be useful to have DSAOpenSsl.IsSupported for fallback to better DSA implementation on macOS.

In general I would be interested in a mechanism for polyfill like @GrabYourPitchforks mentioned.

macOS can't create DSA keys (but can work with existing ones). Aside from our unit tests, no one has ever seemed to notice or care.
We already have a disparity between FIPS 186-2-only and FIPS 186-3+ DSA, but we've never had anyone ask for a way to tell on that.

I actually did notice and it's one of the reasons why my company still ships BouncyCastle despite my dislike of it (ref: 1hub/springburg#16). I don't particularly care about DSA key creation since that's something actionable (ie. don't let user do it) but I work with data already signed by large DSA keys and that's problematic.

I never bothered to rise an issue about it because there's really not much that can be done. As long as you rely on the OS provided cryptography implementation the macOS implementation of DSA is FUBAR. It's FIPS 186-2-only and barely passable at that.

Would it help me to know that it's FIPS 186-2-only? Not really since there's nothing to fallback to short of shipping my own DSA implementation which is what I wanted to avoid in the first place. The best I could achieve would be better error than Interop+AppleCrypto+AppleCFErrorCryptographicException : The operation couldn’t be completed. (Internal CSSM error error -2147415807 - Internal error #80010901 at SignTransform_block_invoke /AppleInternal/BuildRoot/Library/Caches/com.apple.xbs/Sources/Security/Security-59754.41.1/OSX/libsecurity_transform/lib/SecSignVerifyTransform.c:326). Apparently @vcsjones noticed this as well (#28634, #35712).

The DSA.IsSupported property would not be too useful either but it would allow for more graceful fallback and error handling.

At the end of the day I am afraid I would still need to provide an optional polyfill DSA implementation whether .NET implements a generic polyfill mechanism or whether I have to do it myself at the library level.

@dotnet dotnet locked as resolved and limited conversation to collaborators Jun 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Security untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

No branches or pull requests

6 participants