Round #2 of LibSsl shim work #4026

eerhardt · 2015-10-21T05:52:40Z

Add shims for the next set of SSL interop methods.
I also moved the SSL SafeHandle classes out of the Interop class to be
better aligned with the rest of the SafeHandles.

Instead of having managed code duplicate the SSL_CIPHER struct, moving all the code that uses the cipher struct into a native shim method.

That also pulled a lot of algorithm constants to be defined in native instead of managed.

@bartonjs @stephentoub @vijaykota @rajansingh10 @shrutigarg @nguerrera

vijaykota · 2015-10-21T09:21:25Z

src/Native/System.Security.Cryptography.Native/pal_ssl.cpp

Doubt: Doesn't this macro already exist in ssl header files?

I'm equally confused. I see this for 0.9.8zg in ssl_locl.h. If that's not automatically in the include chain, should we just include it? At least for the 0.9.8 series?

Yes, these values are in the source file ssl_locl.h. But neither that file nor those defines appear anywhere in the header files on my Mac.
Any suggestions?

neither that file nor those defines appear anywhere in the header files on my Mac.

Ah, well, that's a good enough reason for me :)

Agree.. sounds reasonable to me. We had same problem with some other macros that are only available in the source code but not in public headers

vijaykota · 2015-10-21T09:57:47Z

LGTM

stephentoub · 2015-10-21T14:18:35Z

src/Native/System.Security.Cryptography.Native/pal_err.cpp

Do we expose these two ErrGetError and ErrGetErrorAlloc APIs because the only error code we need to know about is ERR_R_MALLOC_FAILURE, and this is a simpler approach than PAL'ing all error codes?

Yes, the MALLOC_FAILURE is the only error code we needed to know about. When we need to know about more, we can make a PAL enum for it.

stephentoub · 2015-10-21T14:30:35Z

LGTM

bartonjs · 2015-10-21T14:32:05Z

src/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs

Casing consistency: Everything else was "Ssl", this is "SSL"

bartonjs · 2015-10-21T14:58:43Z

A few minor points, but otherwise LGTM.

nguerrera · 2015-10-21T17:39:06Z

src/Native/System.Security.Cryptography.Native/pal_ssl.h

The declarations in the .h should be things that managed code can depend on unconditionally. There shouldn't be ifdefs changing enum values. If these are only used in the implementation and not expected to be known values to managed code, move them to the .cpp.

That makes sense. I'll move them to the .cpp as these values are only used by the native code, and not managed.

eerhardt · 2015-10-21T17:57:33Z

I don't have problem switching to def/undef, but we should be consisten throughout.
This was easier to audit when we had everything in configure.cmake and pal_config.h.in with the same #cmakedefine01 mechanic.

I am actively working on this in a separate change (now that I am understanding CMake a little more...). I plan on using #cmakedefin01, configure.cmake, config.h.in, like the existing files. These will be in separate, Crypto specific files.

nguerrera · 2015-10-21T17:58:24Z

I am actively working on this in a separate change.

Thanks. :)

stephentoub · 2015-10-21T20:02:51Z

@eerhardt, just FYI, this needs to be rebased to fix conflicts.

I also moved the SSL SafeHandle classes out of the Interop class to be better aligned with the rest of the SafeHandles.

Instead of having managed code duplicate the SSL_CIPHER struct, moving all the code that uses the cipher struct into a native shim method. That also pulled a lot of algorithm constants to be defined in native instead of managed.

eerhardt · 2015-10-21T21:45:38Z

Rebased and responded to PR feedback.

nguerrera · 2015-10-21T21:49:59Z

src/Native/System.Security.Cryptography.Native/pal_ssl.cpp

General question: why do we have to hard-code these constants? Can't we get them from openssl headers? What guarantees/checks do we have to make sure they're right?

See #4026 (comment)

Honestly AFAIK, there isn't a way to check/guarantee these values since they aren't defined in headers on either my Linux or OSX machine. I sort of question whether what we are using is a "public" API. The struct is visible, but it just exposes unsigned long members with no exposed defines for their values.

@vijaykota and @shrutigarg - do you have any ideas on this issue?

I'm going to go ahead and merge this. If there's a better way to approach this, we can do it in a separate PR. Thanks!

I filed https://github.com/dotnet/corefx/issues/4074 to track that.

Hey guys, maybe not exactly relevant to this discussion, but I upgraded a new mac as is to El Capitan (v10.11.1) and it wasn't succeeding git clean -xdf; ./build.sh native with latest AppleClang / Xcode (yesterday's updates) applied. I had to run brew link --force openssl (yeah i had to install brew first). Now it compiles 100% there as well. (more and moar platforms coverage is the spirit!) :)

Thanks, @jasonwilliams200OK . See #3924 for more details on issues with El Capitan.

since they aren't defined in headers on either my Linux or OSX machine

@eerhardt, we haven't found a way around this either

nguerrera · 2015-10-21T21:50:31Z

LGTM. I had one more question.

stephentoub · 2015-10-22T00:52:24Z

LGTM

Round #2 of LibSsl shim work

…to getdomainname and looking for a warning. Fixing unused parameters build error on OSX. Update for PR feedback. - Collapse the 3 HAVE_INOTIFY defines into a single define.

Round dotnet/corefx#2 of LibSsl shim work Commit migrated from dotnet/corefx@00f9ebc

joshfree added the 2 - In Progress label Oct 21, 2015

dnfclas added the cla-already-signed label Oct 21, 2015

vijaykota reviewed Oct 21, 2015
View reviewed changes

stephentoub reviewed Oct 21, 2015
View reviewed changes

bartonjs reviewed Oct 21, 2015
View reviewed changes

eerhardt mentioned this pull request Oct 21, 2015

Populate DataHash key size in SslConnectionInfo #3916

Closed

nguerrera reviewed Oct 21, 2015
View reviewed changes

eerhardt added 3 commits October 21, 2015 15:56

Add shims for the next set of SSL interop methods.

d3939b5

I also moved the SSL SafeHandle classes out of the Interop class to be better aligned with the rest of the SafeHandles.

Move SSL_CIPHER struct usage into native shim.

30c98cc

Instead of having managed code duplicate the SSL_CIPHER struct, moving all the code that uses the cipher struct into a native shim method. That also pulled a lot of algorithm constants to be defined in native instead of managed.

Rebase and respond to PR feedback.

dc637b7

eerhardt force-pushed the MoreLibSsl branch from 1173109 to dc637b7 Compare October 21, 2015 21:41

nguerrera reviewed Oct 21, 2015
View reviewed changes

stephentoub added a commit that referenced this pull request Oct 22, 2015

Merge pull request #4026 from eerhardt/MoreLibSsl

00f9ebc

Round #2 of LibSsl shim work

stephentoub merged commit 00f9ebc into dotnet:master Oct 22, 2015

stephentoub removed the 2 - In Progress label Oct 22, 2015

eerhardt deleted the MoreLibSsl branch October 22, 2015 15:09

ghost mentioned this pull request Nov 13, 2015

Improve support for brew install openssl on OSX #4498

Merged

karelz modified the milestone: 1.0.0-rtm Dec 3, 2016

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#4026 from eerhardt/MoreLibSsl

8e44f18

Round dotnet/corefx#2 of LibSsl shim work Commit migrated from dotnet/corefx@00f9ebc

Round #2 of LibSsl shim work #4026

Round #2 of LibSsl shim work #4026

Uh oh!

Conversation

eerhardt commented Oct 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vijaykota commented Oct 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Oct 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartonjs commented Oct 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt commented Oct 21, 2015

Uh oh!

nguerrera commented Oct 21, 2015

Uh oh!

stephentoub commented Oct 21, 2015

Uh oh!

eerhardt commented Oct 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nguerrera commented Oct 21, 2015

Uh oh!

stephentoub commented Oct 22, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants