Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Create native interop library System.Security.Cryptography.Native #745

Merged
merged 1 commit into from
Apr 21, 2015

Conversation

bartonjs
Copy link
Member

Create an native library to make a cross-system p/invoke layer into OpenSSL (or any ABI-compatible libcrypto).

This library is similar in purpose to the CLR PAL, but scoped only to the needs for the System.Security.Cryptography assemblies.

@bartonjs
Copy link
Member Author

cc: @jkotas @janvorli


find_library(CRYPTO NAMES crypto)
if(CRYPTO STREQUAL CRYPTO-NOTFOUND)
message(WARNING "Cannot find libcrypto, skipping build for System.Security.Cryptography.Native. .NET cryptography is not expected to function. Try installing openssl-dev (or the appropriate package for your platform)")
Copy link
Member

Choose a reason for hiding this comment

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

Is this the right name of the package? I am not able to get it on my Ubuntu. Maybe it should be libssl-dev?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are correct. I have updated the PR (already squashed down to still being only one commit)

@bartonjs bartonjs force-pushed the crypto-interop-library branch from b76cb3c to fdc9a04 Compare April 20, 2015 23:04
*/
int
GetX509NameRawBytes(
X509_NAME* x509Name,
Copy link
Member

Choose a reason for hiding this comment

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

Is it important to differentiate empty string from the failure case?

Copy link
Member

Choose a reason for hiding this comment

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

(Some of the other methods have similar corner cases.)

Copy link
Member Author

Choose a reason for hiding this comment

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

GetX509NameRawBytes and GetX509PublicKeyParameterBytes both return DER-encoded data, so there's no zero-length valid value (an empty item still has a tag byte and a length(0) byte).

GetAsn1StringBytes is the only thing that looks vulnerable, since it would return -0 if pBuf is NULL, but then will return 1 if pBuf wasn't NULL. That one is merely a little silly.

I don't see any cases where we would want to see the error and throw, so I don't think anything has slipped through on the corners .

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks for double checking

@jkotas
Copy link
Member

jkotas commented Apr 20, 2015

LGTM modulo comments

jkotas added a commit that referenced this pull request Apr 21, 2015
Create native interop library System.Security.Cryptography.Native
@jkotas jkotas merged commit d424ed8 into dotnet:master Apr 21, 2015
@kangaroo
Copy link

I like the direction, but hate the native library naming:

System.Security.Cryptography.Native.dylib

Is this a standard we want to adopt?

@stephentoub
Copy link
Member

@kangaroo, ultimately we'd like these little native libraries to build and ship along side their managed library that consumes them, as part of the same NuGet package, etc. Logically these native libs are a part of the managed consumer, not meant to be consumed by anyone else, and are only separated out into a separate library out of necessity, so we were thinking we'd have a naming scheme that followed from that, e.g. System.IO.FileSystem.dll using and shipping with System.IO.FileSystem.Native.dll/so/dylib.

But since you "hate" this, what would you recommend instead? We're not locked on anything yet.

@kangaroo
Copy link

It just doesn't follow the operating system conventions. I have a few questions from your statement tho:

1> What are the security guarantees going to be over distributing native code in nuget packages?
2> How are multiple arch/os combinations going to be solved in nuget packages?
3> What is the naming convention for crossgen images going to be? Will there be confusion between these tho?

I'm not sure I have a significantly better proposal at this time, but I do have questions :)
libsystem-security-cryptography-native.dylib might be better? Not sure yet.

@stephentoub
Copy link
Member

1> What are the security guarantees going to be over distributing native code in nuget packages?

Should be approximately the same as for managed code in NuGet packages. Are there particular differences you have concerns about (especially once we start thinking about distribution of AOT compiled managed libraries)?

2> How are multiple arch/os combinations going to be solved in nuget packages?

The current plan is that the NuGet package will carry all necessary flavors, and the system will choose the right library from the package based on the build OS/arch/etc. The new version of NuGet is meant to support such things.

3> What is the naming convention for crossgen images going to be? Will there be confusion between these tho?

Not sure. We haven't done it yet; I've no doubt there will be issues to work through ;)

I'm not sure I have a significantly better proposal at this time, but I do have questions :)
libsystem-security-cryptography-native.dylib might be better?

Thanks for the suggestion; I'd be fine with that, too. I'm not particularly concerned with naming here as it's meant to be a private implementation detail of the consuming library, but if there's still value in following the target OS' conventions (which we don't do for the managed libs), then a similar scheme that prefixes with "lib", uses all lowercase, and uses dashes instead of dots is fine by me. The primary thing I care about here is making sure that the name of the native lib unambiguously ties it to the name of the managed lib; beyond that I'm very flexible.

@kangaroo
Copy link

1> I'm not sure what guarantees are made today. Can I as a malicious actor post a nuget package without any vetting? If the package is open source, will microsoft do the build for all supported platforms? Will some kind of native code signing be enforced so I can verify against code tampering?

I think we have a lot of considerations to make in this regard (some may already be addressed in nuget, I'm by no means an expert there).

2> Whats the flow for a new platform coming on board. Will the nuget servers automatically rebuild every version of every package? What if the package author doesn't provide source in a fashion which supports other-os. If the package authors have to provide the binaries, what happens if they don't have build servers for all the combinations?

3> See above :)

4> I'm not particularly married to it, I guess I'm more concerned about the stuff in #1-3, but it does feel 'cleaner' to me. With your current scheme, what happens if we need a native support lib on windows, System.Security.Cryptography.Native.dll which might conflict with the managed dll name?

@stephentoub
Copy link
Member

1> I'm not sure what guarantees are made today

@Eilon, would you or someone from your team want to address @kangaroo's questions related to NuGet and security?

2> Whats the flow for a new platform coming on board

NuGet is just the distribution mechanism. The producer of the libraries would build the libraries, build the NuGet package, and then upload the new package... the NuGet servers would not be responsible for producing the builds themselves. If the producer of the binaries doesn't have the right machines locally to do their own builds, they can use whatever cloud-hosted servers or other services they like, separate from NuGet.

3> what happens if we need a native support lib on windows, System.Security.Cryptography.Native.dll which might conflict with the managed dll name?

We'd just choose a name that didn't conflict, i.e. we wouldn't name the managed dll System.Security.Cryptography.Native.dll ;) And if we're following OS convention, we wouldn't want to name the .dll on Windows with the scheme employed on Linux/OSX, e.g. libsystem-security-cryptography-native.dll would look strange on Windows, just as you're pointing out that System.Security.Cryptography.Native.dylib would look strange on OSX.

@kangaroo
Copy link

This is why we need to have the discussion around naming conventions! yay! :)

I'm wondering if there isn't some platform agnostic convention we could adopt. I don't particularly like 'native' since its ambiguous. Some usage of 'pinvoke' .. 'pinvoke-support'. Dunno, bouncing ideas at this point.

@stephentoub
Copy link
Member

I still have a small preference for the System.Something.dll/System.Something.Suffix.{so/dylib/dll} naming, but if we did want to venture away from that, I think it would be difficult to come up with a platform-agnostic convention, if for no other reason than anything we choose is going to be capitalized in a way that more closely resembles one platform over another. That said, I don't see any reason why we'd have to have the same naming across platforms. We could name it libsystem-security-cryptography-native.dylib on OSX, System.Security.Cryptography.Native.dll on Windows (if we needed such a thing), etc.; this would likely mean having multiple managed compilations to specify different constants to the relevant DllImports (unless we relied on an out-of-the-assembly mechanism like dllmap), but we're already there in many cases, so that's not a big deal. As for the suffix, again, I'm not tied to "Native"... Native, PInvokes, PInvokeTargets, Thunks, Shims, Interop, Exports, Imports, Wrappers... probably a bunch of other candidates we could consider as well.

@bartonjs
Copy link
Member Author

When I first started this library it was "libnetcrypto", following the Linux pattern of "lib-" and being short (not quite competing with "libm.so"). Within a few hours of that, one colleague had mentioned like "aren't you worried that people might start depending on your library?" and another had mentioned that the name should be more verbose/closely associated with its managed consumers.

These interop libraries aren't intended for public consumption: they won't have a guaranteed ABI, they will only be as compatible as we need them to be to match the assemblies they support, etc. As such, having a name that doesn't follow the system convention seems to make a lot of sense. It's the filename equivalent of the Jedi Mind Trick (these are not the libraries you are looking for...).

Linux libraries are named all in lowercase, start with "lib" and have very little punctuation. Windows system libraries are either the new api-ms-win-core-*.dll format, or adhere to the old 8.3 convention (and tend to be named in lowercase on case-preserving/sensitive filesystems). "System.Security.Cryptography.Native.{ext}" adheres to neither of these conventions, making it (to me) a pretty good contender for the Library Mind Trick.

@jkotas
Copy link
Member

jkotas commented Apr 21, 2015

We may want to consider the naming convention for all native images, not just the PInvoke helper libraries.

Our strategy has been to blend the boundaries between pure managed and pure native images. The native images generated by crossgen on Windows contain native code, but also copy of the IL and metadata. I expect that we are going to have these blended images cross-platform too.

On Windows, the naming convention used for them is to either give them same name as the name of the original IL file with .dll suffix, or .ni.dll suffix - if there is need to differentiate, e.g. when both the original IL image and the native image need to live in the same directory.

What the right naming convention for these blended images should be on non-Windows platforms? The pattern established here - changing the suffix to .dylib, but otherwise keep the Camel.Cased name - looks pretty reasonable.

BTW: The crossgen has a custom linker today that does not support linking in foreign object files. I expect that we will gain ability to use the system linker and link foreign object files as part of the LLILC work. Once it is done, System.Security.Cryptography.Native can be linked into System.Security.Cryptography native image and not be actually present in the distributed bits (as an option - we can still keep the separate file for runtimes without support for these native images).

@bartonjs bartonjs deleted the crypto-interop-library branch April 23, 2015 00:01
@Eilon
Copy link

Eilon commented Apr 24, 2015

Happy to join the party!

I think this managed+native scenario is squarely in the area that @ericstj and @lodejard have been working on for some time now.

My recollection of the current direction is that there are two ways to have native "buddy" libraries:

  1. "Fat" packages, where all the required native code for every platform lives in the same package as the primary-consumed managed code. This means that to add a new platform (e.g. HoloLens 😄) you have to have control of the package (because you have to re-publish it), as well as the desire and ability to support that new platform.
  2. Packages that depend on "runtime" definitions, whereby the platform-specific native libraries live in separate NuGet packages that have a mapping on a per-platform basis in some "runtime.json" file. This allows new platforms to light up without necessarily affecting the original package (assuming the original package was written generically enough).

As far as naming conventions go, for (1) there's no issue. And for (2) I believe there's some pattern that's being suggested by @ericstj and @lodejard for at least the Microsoft-authored/owned packages, but then someone building a Raspberry Pi native package could surely do whatever they want, e.g. Cool.Raspberry.Pi.Delicious.Awesome.nupkg v3.1.4.

SedarG pushed a commit to SedarG/coreclr that referenced this pull request Jan 27, 2017
…e.a according to the naming convention as discussed on dotnet#745 and update the osx build of this library to be linked against icucore
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants