Allow cross-platform CAPI-compatible remote key exchange #11185
Conversation
|
|
|
This will likely fix the local debugging experience for Azure Functions on macOS and Linux which catastrophically fails when you store a SecureString to a variable which triggers the key exchange. Im excited at the impact this change could have! |
|
SecureString is not implemented with encryption on non-Windows platforms. So encryption with the symmetric key over the wire seems unneeded. It may be better to remove it entirely. Adding committee review tag. |
|
Yeah, session encryption during serialization is entirely redundant if the transport is encrypted. Here I was trying to make the smallest change possible while maintaining compatibility with older version remote shells. Another thing for consideration: I think the core AES/RSA classes fall back on the windows native libraries in their implementation? If so, you may consider just dropping Win32PSRSACryptoServiceProvider altogether. |
|
Yeah, I am beginning to see your point. This may be the best way moving forward and keeping backward compatibility. There is still the deadlock issue when server initiates key exchange, but I think that can be fixed separately. I would like to get rid of |
|
Personally, I think getting rid of I'd be happy to refactor it out. It'll allow a lot of code and compiler conditions to be removed from |
…atform compatible implementation using AES and RSA abstractions.
a311102 to
7d3b364
Compare
|
I updated this PR to completely remove the native windows dll references and change the entire key exchange over to using dotnet core compatible aes and rsa classes. I also consolidated the CAPI conversion code into CryptoUtils.cs and used the existing pattern of byte constant naming to be more explicit about the structure of the keyblob format conversions. Let me know what you think. |
|
Thanks @silijon, I'll take a look this week. |
There was a problem hiding this comment.
Overall this looks good to me. I have tested this on Windows platform and SecureString objects are passed correctly between PowerShell versions. However, key exchange is not currently working on non-Windows platforms, but I don't think it is due to these changes and is just not supported since it was not needed before. I'll continue to look into it.
|
Removing |
|
Key exchange was broken because of remaining #ifdefs in code for non-Windows platform. Just need to change |
Add minor code review changes
|
@PoshChan please retry linux |
|
@SteveL-MSFT, successfully started retry of |
|
@PoshChan please retry windows |
|
@silijon, you are not authorized to request a rebuild |
|
@PoshChan please retry windows |
|
@vexx32, successfully started retry of |
e6d2fc2 to
68646a8
Compare
|
@TravisEz13 Where are we on this PR? Is there anything preventing the merge? |
|
@silijon Thanks for your contribution! |
|
🎉 Handy links: |
PR Summary
Creates a dotnet core native implementation of the CryptoUtils key exchange enabling cross-platform remote sessions to successfully serialize SecureString.
PR Context
Currently, after importing a remote session between a Windows and a Linux host (in either direction), any imported cmdlets parameterized with SecureString are broken-by-design. There is a lazily triggered operation that happens during a SecureString serialization that uses the windows native CAPI crypto functions (e.g. CryptGenKey) to do the following:
The implementation of this protocol is conditioned on the UNIX compile flag and, for non-Windows builds, it simply throws a PSCryptoException. The result is that any and all cmdlets that use a SecureString parameter are broken for cross-platform importation/invocation.
This PR fixes this problem by doing the following:
Extracts an interface from PSRSACryptoServiceProvider.
Re-names the current PSRSACryptoServiceProvider to Win32PSRSACryptoServiceProvider.
Implements CorePSRSACryptoServiceProvider with the same external semantics as Win32PSRSACryptoServiceProvider but uses the core-compatible AES and RSA classes to do key generation, importation, exportation, encrypt, and decrypt.
Uses a small set of conversions to enable CorePSRSACryptoServiceProvider to return/digest the key blob formats to/from the documented CAPI formats so as to retain compatibility with native windows systems.
Conditions the use of CorePSRSACryptoServiceProvider on the UNIX compile flag.
Notes
I looked for an issue specifically for this problem and, while there are a number of closed issues relating to more general SecureString problems (having to do with the DPAPI and other things) I didn't see one directly related to this. Would be happy to tag an issue in here if I just missed it.
This PR fixes the same issue raised with the PS team in discussion with the SkyKick team. Specifically @PaulHigin (via email) and @TylerLeonhardt (on his livestream).
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.