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

using openssl when running on unix/coreclr #328

Merged
merged 1 commit into from
Dec 9, 2015
Merged

Conversation

tushargupta51
Copy link
Contributor

Fixing #324. I am not changing ECDsa to use OpenSSL yet since that requires updating ECDsaSecurityKey to work on unix (#325). Will come to that later.

@tushargupta51
Copy link
Contributor Author

@brentschmaltz @pinpointtownes

@kevinchalet
Copy link
Contributor

It looks fine as a temporary fix, but you should definitely consider updating RsaSecurityKey's constructor to take a RSA instance instead of creating one in the signature provider.

@kevinchalet
Copy link
Contributor

:shipit:

@ericstj
Copy link

ericstj commented Feb 2, 2016

@bartonjs don't we have a cross-plat type for RSACng/RSAOpenSsl?

The current implementation will cause the JIT to hit the OpenSSL assembly, even on Windows. That won't work in the future.

The only reason this may work today is because we've delayed the packaging changes to move these platform specific libraries to platform specific folders in the packages. We're delaying that work because folks cannot yet represent platform-specific dependencies in nuget/project.json. https://github.com/dotnet/cli/issues/465 NuGet/Home#1660

@bartonjs
Copy link

@ericstj We didn't 10 days ago, no. But https://github.com/dotnet/corefx/issues/2953 was fixed 2 days ago (which @PinpointTownes celebrated).

So this particular change can be switched to using RSA.Create() now, and there'll be a couple of builds overlap between RSA.Create() existing and the OpenSsl package going back to registered as unix-only (https://github.com/dotnet/corefx/issues/6046).

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.

5 participants