This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
Remove OpenSSL dependency on macOS (dev/apple_crypto -> master) #17011
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since the class is helpful to both OpenSsl and Apple SecurityTransforms
With this change the System.Security.Cryptography.Algorithms library has no direct dependency on OpenSSL (it has an indirect one from OID lookup via the S.S.C.Encoding library). Observable changes: * The test library runs in ~1.7 seconds before this change, ~30 seconds (up to 48) after. * The tests complete in ~1.2 seconds when all key generation is disabled. * ~21 seconds with ECDSA enabled (26 tests), RSA disabled * ~10 seconds with RSA enabled (13 tests), ECDSA disabled * Ephemeral key generation populates the keychain, this is not desired, so will be disabled in the next commit pending a workaround. * DSA key generation is not possible via Apple's libraries. * DSA is limited to FIPS 186-2 behaviors, a loss of functionality. * ECDSA only supports the 3 main NIST curves (256, 384, 521). * ECDSA explicit import/export is no longer possible.
By disabling ephemeral key generation the keychain doesn't grow during every test execution, making things much nicer on the CI machines (and people using development builds). Some of the tests which used key generation were changed to use fixed parameters instead. While that could be made permanent, the expectation is this commit is just going to get reverted once the keychain pollution problem is solved.
Use Apple SecurityTransforms for RSA, DSA, and ECDSA on macOS.
[dev/apple_crypto] Use setMachineAffinity instead of legacy labels
The old automation for arm64 isn't available. Remove until we get a replacement
Remove arm64_corefx testing
Apple's Security framework doesn't seem to expose any OID resolution or ASN.1/DER pretty-printing, so the "make a native call here" functions just always report that they have failed. The pretty-printing of SubjectAlternativeName was manually written into this library to retain support for libraries which are parsing the ToString output.
Make the Crypto.Encoding library work on macOS without OpenSSL
Conflicts: src/Common/tests/System/Security/Cryptography/AlgorithmImplementations/DSA/DSASignVerify.cs src/System.Security.Cryptography.Algorithms/src/Resources/Strings.resx src/System.Security.Cryptography.Encoding/src/System.Security.Cryptography.Encoding.builds src/System.Security.Cryptography.Encoding/src/project.json
* AsymmetricAlgorithm-derived types need `new` on Create methods. * Marshal.PtrToStringUTF8 is not available, temporarily disable that optimization * Re-enable an OSX build for System.Security.Cryptography.Encoding
netci.groovy as of a84543b
The built-in derived types of X509Extensions use an internal constructor which makes use of the OidGroup lookup on Oid.FromOidValue. But Oid.FromOidValue throws when it has no match, and so the X509Extensions derived types fail. Now the four special extensions we support will correctly resolve OID names, using the static value of what is currently defined for OpenSSL for those OIDs.
…ion_2Oids The test doesn't care that the OIDs be resolvable, just that their value field is present. Since these OIDs are not resolvable on macOS, change to the constructor so resolution is allowed to fail.
Merge dotnet/master to dotnet/dev/apple_crypto
…e_crypto Conflicts: src/System.Security.Cryptography.Encoding/src/Configurations.props src/System.Security.Cryptography.Encoding/src/System.Security.Cryptography.Encoding.csproj
Once Keychain enumeration was working locally it was observed that a NIST-P521 certificate calling GetECDsaPublicKey() was asserting. This should already have been caught by the tests, but key generation was disabled due to perf and keychain contamination.
Fix a 512 vs 521 typo in ECDSA keysize assert.
… for macOS (#16445) Broken by this change: * A lot of TLS CipherSuites have no metadata defined. * macOS does not support version skipping in TLS. So `Tls | Tls12` is an invalid choice. In this change: General: * All OSStatus related exceptions now look up the error message. X509Certificates: * X509Certificate moves to using SecCertificateRef from OpenSSL's X509. * X509 metadata comes from a managed reader after being loaded by Security.framework, due to the significant amount of data that has no public export in Apple's libraries. * Significant code was factored out to be shared by OpenSSL and Apple implementations for X500DistinguishedName and X509Certficate2Collection.Find. * Loading a PFX (or, rather, the private keys from a PFX) via Apple's platform requires importing into a Keychain, and a Keychain requires a file on disk. A temporary keychain is created during cert loading and erased when safe. Like the perphemeral key load on Windows this can leak files due to abnormal program termination. * The X.509 My store for CurrentUser and LocalMachine are the default (user) and System keychains. * The X.509 Root store is an interpretation of the Apple SecTrustSettings data. * The X.509 Disallowed store hasn't been implemented yet, but should be a very small change. * Other X.509 stores cannot be created due to keychain complexity. HttpClient: * Initialization no longer wakes up OpenSSL SslStream: * New implementation based on Apple SecureTransport. * Currently has support for SNI (for AuthenticateAsClient)
Generate RSA and ECDSA keys via a temporary keychain. When no keychain is given, SecKeyGeneratePair will generate the keys into the default/login keychain. Since the keys were not expected to have been in the keychain, they permanently leak out and the keychain grows over time. Instead, have the managed code provide a temporary keychain into which the keys will be created. Then, export them and re-import them to the NULL keychain. The temporary keychain can then be deleted, and the hard problem of delayed cleanup is avoided. Files on disk will still leak if the process terminates during the key generation phase, but that is a much smaller window than other 'temporary file on disk' solutions.
There weren't very many that weren't part of method signatures, so all of the usages of int were replaced with int32_t. `char` (the other common not-always-right type) was checked and all instances are for strings, none were being used when uint8_t was meant instead.
* Enable add/remove from a keychain-backed store. * Improve X509KeyStorageFlags usage * PersistKeySet will save a PFX key (and certificate) into the default keychain * EphemeralKeySet will throw PNSE * Exportable is respected for PFX * Add support for reading the Disallowed trust. * This will only read "full deny" behaviors * Enable or permanent-assign all remaining ActiveIssue(-1) tests. * Add DSA support to cert.PrivateKey from the Apple PAL * And add a test for it
The platform-specific interop type providers for RSA and ECDSA still had the SupportsKeyGeneration properties defined. Nothing calls them anymore, so delete them before they make it to master.
Clean up unused SupportsKeyGeneration properties
The non-ciphersuite ciphersuite values were removed, as were the SSL2-only values, and the values that have unknown values (Cipher:IDEA, KEA:Fortezza). The dictionary is asserted as fully populated with a fixed-length cctor test, and the additional test of ensuring all defined values have defined metadata has been performed. TlsMapping calls were built by regex substitution expressions, and have been checked manually for consistency.
* Make the DEBUG_VERBOSE code always run (as DEBUG) * Use expression-bodied members for the calls to Build to reduce vertical noise * Consistently declare the helper methods as internal
stephentoub
reviewed
Mar 12, 2017
| Debug.Assert(!cfString.IsInvalid); | ||
| Debug.Assert(!cfString.IsClosed); | ||
|
|
||
| #if HAVE_PTRTOSTRINGUTF8 |
There was a problem hiding this comment.
Why do we need this? Can't we just use PtrToStringAnsi?
stephentoub
reviewed
Mar 12, 2017
|
|
||
| protected override bool ReleaseHandle() | ||
| { | ||
| lock (s_lookup) |
There was a problem hiding this comment.
How much contention are we going to have on this? I'm wondering if we're creating a scalability problem and if we should consider ConcurrentDictionary, or if this won't be a bottleneck and is fine like this.
stephentoub
reviewed
Mar 12, 2017
|
|
||
| protected override void Dispose(bool disposing) | ||
| { | ||
| if (disposing && SafeHandleCache<SafeTemporaryKeychainHandle>.IsCachedInvalidHandle(this)) |
There was a problem hiding this comment.
Just curious, is the disposing check needed here? If we're not disposing, we're finalizing, and since this would be a cached handle, wouldn't that necessarily mean it's getting finalized due to the process shutting down such that the static root was dropped?
stephentoub
reviewed
Mar 12, 2017
| base.KeySize = value; | ||
|
|
||
| _keys?.Dispose(); | ||
| _keys = null; |
There was a problem hiding this comment.
Nit: as long as we're doing the null check, we can guard the assignment as well:
if (_keys != null)
{
_keys.Dispose();
_keys = null;
}
stephentoub
reviewed
Mar 12, 2017
| if (disposing) | ||
| { | ||
| _keys?.Dispose(); | ||
| _keys = null; |
stephentoub
approved these changes
Mar 12, 2017
|
@bartonjs The netci.groovy needs to be pre-merged, though we could just generate the 10.12 jobs here and run them manually for verification so we have a clean merge. @dotnet-bot test ci please |
|
After much mis-typing I got it. |
Conflicts: src/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs
|
Weird, somehow 5694965 ended up with the Remove function but not the Add function. |
Merge dotnet:master to dotnet:dev/apple_crypto (and add lost function)
|
@dotnet-bot Test OuterLoop OSX10.12 Debug x64 please |
2 tasks
This was referenced Dec 4, 2017
6 tasks
picenka21
pushed a commit
to picenka21/runtime
that referenced
this pull request
Feb 18, 2022
Remove OpenSSL dependency on macOS (dev/apple_crypto -> master) Commit migrated from dotnet/corefx@50218ea
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With this (set of) change(s) only the types whose name includes "OpenSsl" (e.g. RSAOpenSsl) will use OpenSSL on macOS. Instead, everything is provided by Security.framework.
Fixes #9394.