Skip to content

Conversation

@guardrex
Copy link
Contributor

@guardrex guardrex commented May 3, 2017

Fixes #824

Adds notes to System.Security.Cryptography,RSACng and System.Security.Cryptography.RSAOpenSsl instructing devs to use RSA.Create (platform agnostic) in xplat scenarios.

cc/ @joshfree @bartonjs

@guardrex guardrex self-assigned this May 3, 2017
## Remarks
> [!NOTE]
> The <xref:System.Security.Cryptography.RSAOpenSsl> class is a CNG implementation for Linux and macOS platforms. In a cross-platform scenario, you should use the platform agnostic <xref:System.Security.Cryptography.RSA.Create%2A?displayProperty=fullName> to create an instance of the default implementation for the platform where your code will run.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSAOpenSsl isn't "a CNG implementation". It's a specific implementation of RSA, using OpenSSL's libcrypto, and as such can do platform interop (for example, to make use of a smartcard/HSM).

- <xref:System.Security.Cryptography.RSACng> performs such operations as signing and verifying signatures by using properties of the <xref:System.Security.Cryptography.RSACng> object, just as <xref:System.Security.Cryptography.ECDsaCng> uses its object properties to control signing and verification operations.
> [!NOTE]
> The <xref:System.Security.Cryptography.RSACng> class is a CNG implementation for Windows platforms. In a cross-platform scenario, you should use the platform agnostic <xref:System.Security.Cryptography.RSA.Create%2A?displayProperty=fullName> to create an instance of the default implementation for the platform where your code will run.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSACng isn't "a CNG implementation". It's a specific implementation of CNG, using Windows CNG. It can can be used for platform specific interop, like opening named keys.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meta: Why not RSACryptoServiceProvider? Why not ECDsaCng/ECDsaOpenSsl?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSACng isn't "a CNG implementation"

Ref: https://docs.microsoft.com/en-us/dotnet/api/system.security.cryptography.rsacng?view=netframework-4.7

Provides a Cryptography Next Generation (CNG) implementation of the RSA algorithm.

reads ... "a ... CNG ... implementation ..."

... along with the note from @joshfree ...

developers should be using RSA.Create (platform agnostic) instead of directly using the concrete RSACng (windows) or RSAOpenSsl (linux, mac) classes

"RSACng (windows)" ... led to ... "... for Windows".

RSA.Create was also floated there, so that's what made the note as the suggestion for xplat development.

Of course, I can make it say whatever you want. Do you want to give me the note language, and I'll shoot it right in there?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like "The RSACng class is an implementation of the RSA algorithm using the Windows CNG libraries, and is not available on operating systems other than Windows. Applications which are not doing Windows interop are encouraged to use RSA.Create() instead of referencing this type directly."

"The RSAOpenSsl class is an implementation of the RSA algorithm using OpenSSL. It is not available on Windows, and is only available on other operating systems when OpenSSL is installed. Applications which are not doing OpenSSL-specific interop are encouraged to use RSA.Create() instead of referencing this type directly."

@guardrex
Copy link
Contributor Author

guardrex commented May 4, 2017

@bartonjs I'm concerned a bit about the term "interop" in these lines, which I know the fast 🏃 way to state it ...

For applications which aren't doing Windows interop ...

For applications which aren't doing OpenSSL-specific interop ...

Can't we explicitly flesh those out a bit to ...

For RSACng:

For applications that won't have access to Windows CNG libraries, such as cross-platform applications running on Linux or macOS, you're encouraged to use RSA.Create() instead of referencing this type directly.

For RSAOpenSsl:

For applications that won't have access to the RSA algorithm using OpenSSL, such as cross-platform applications running on Windows, you're encouraged to use RSA.Create() instead of referencing this type directly.

@bartonjs
Copy link
Member

bartonjs commented May 4, 2017

It's not just the fast way... even if you were to only ever run on Windows I'd still encourage you to use RSA.Create() instead of caring what particular provider was being used, unless you needed to use CNG-specific functionality (like "I have a SafeNCryptKeyHandle, let's use it" or "I have a CNG blob, I need to import it", or "I have a named key, I need to open it"). And similarly with the OpenSSL version.

Perhaps there's a more standard way of expressing that in documentation, which I don't know. I'm just saying that from a technical perspective the guidance is sort of "Don't use these types. Pretend they don't exist. If you find you can't achieve what you need from RSA.Create() and the RSA (base) class, then see if these provider-specific classes can solve your problem... but be careful, you might have just lost the ability to be cross-platform"

@guardrex
Copy link
Contributor Author

guardrex commented May 4, 2017

It's not just the fast way

I meant "fast" in the sense that "aren't doing Windows interop" describes the failure scenario that @Raviuppa highlighted in #824 in very few words, but since the prior sentence ends with "isn't available on operating systems other than Windows," I think the concept is clear enough. The word "interop" bothers me a bit ... that's all.

RE: "encouraged" ... It 💥 on @Raviuppa merely trying to instantiate the class. He has no choice. I wonder if it should say "... you should use ...".

It's been a good discussion, but I don't want to drag it out any further if you feel the last commit covers all the bases. ⚾ Are we ready to call for reviewer 👀?

@bartonjs
Copy link
Member

bartonjs commented May 4, 2017

I think the current state covers it, yeah.

@guardrex
Copy link
Contributor Author

@mairaw This one is ready for a final 👀+merge.

Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM. I'll :shipit:

@BillWagner BillWagner merged commit 5eaa26a into dotnet:master May 30, 2017
@guardrex guardrex deleted the guardrex/RSACng-RSAOpenSsl-updates branch May 30, 2017 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants