Enhance msquic to support openssl as a TLS backend #4959
Conversation
|
@microsoft-github-policy-service agree [company="OpenSSL"] |
|
@microsoft-github-policy-service agree company="OpenSSL" |
Need to split them for the new openssl implementation
Getting ready to replace it with callbacks
quictls is a fork of openssl, not openssl. It should use the quictls name in its configurations. Move openssl and openssl3 to quictls and quictls3, and use openssl for the canonical upstream sources.
OpenSSL yields secrets independently, and so in this function it is possible you will have the write key prior to installing the read key, which will occur on the next call to SSL_do_handshake
nhorman
force-pushed
the
openssl-quic-api
branch
from
1e7e38e to
69d5261
Compare
|
I do think that renaming openssl -> quictls is a good change. I have some concern with the fact that it would cause a use to go from building with quitls to building with openssl without any action on their end or warning.
|
|
@guhetier I think such a change would be fine, though I'd be curious to see what others thought about the version being added to the name - i.e. is doing this committing to adding an openssl4/5/6 in the future? Given that msquic builds against whatever openssl variant is defined in the associated submodule, I'm not sure thats needed, though if you want to support multiple openssl versions, it might be beneficial. |
|
@nhorman All these are great questions, we need to clarify how we see openssl upgrades happen in the future. A different name pattern might also be an option - my concern is mainly about avoid the re-use of the same parameter with a different meaning. Lets gather some opinions about how to handle this best. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@guhetier I understand what you're getting at. I don't have a whole lot of skin in that game, but I'll gladly implement what you and the rest of the dev team think is best. Just to float another idea for you to consider (and I grant this is additional departure from your current build approach, and so may not be viable), but you might consider:
i.e. check in cmake to see if SSL_provide_quic_data exists, if it does, then build the quictls platform adaptation layer, if SSL_set_quic_tls_cbs exists, then build the openssl platform adaptation layer. That would disconnect you from needing to select a specific implementation manually at configure time. In the interim, I'll start looking at these CI failures. Looks like I did something wrong in the openssl/quictls migration for the workflow files |
| @@ -1,6 +1,6 @@ | |||
| parameters: | |||
| config: '' | |||
| tls: 'openssl' | |||
| tls: 'quictls' | |||
There was a problem hiding this comment.
We will need to update the CI to build and run tests with openssl on all the supported platforms (could be as a follow up)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4959 +/- ##
==========================================
+ Coverage 84.98% 87.03% +2.05%
==========================================
Files 57 59 +2
Lines 17952 17989 +37
==========================================
+ Hits 15256 15657 +401
+ Misses 2696 2332 -364 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nhorman
force-pushed
the
openssl-quic-api
branch
from
16b10e1 to
eafd672
Compare
|
ok, comments and CamelCase/snake_case issues fixed. Will start looking at CI failures next |
Also fix the quictls url, as its pointing to the wrong repo at the moment
nhorman
force-pushed
the
openssl-quic-api
branch
from
da65cc5 to
af45354
Compare
nhorman
force-pushed
the
openssl-quic-api
branch
from
af45354 to
3cbded3
Compare
|
FYI, I have most of the cargo CI jobs building currently, but the windows static builds are failing, seemingly due to last nights windows-latest github runner update, which migrated from cmake 3 to cmake 4 which is having various build problems, that may be causing you trouble in other pull requests. We hit some similar problems in openssl this morning. |
Recent updates to github runners have moved us to cmake 4.0, which has introduced some frutrating problems, most notably that windows paths are now getting treated as though "\" is an escape character. Update the build.rs script to migrate input paths to use the unix separator
Recent update to rust 1.86 is causing cargo clippy to trip over a linting error in src/rs/error.rs in which we reimplement the ok function. Fix that.
|
Thanks for fixing that on top of everything else! Sorry for the delay in reviewing the PR, I'll get to it as soon as I can. |
|
don't rush, I know you all are busy. Feel free to cherry-pick any of the cmake/rust fixes to another PR if you want to prioritize those. |
| [submodule "openssl"] | ||
| path = submodules/openssl | ||
| url = https://github.com/openssl/openssl | ||
| branch = master |
There was a problem hiding this comment.
To officially support, we must point to a release branch, not mainline.
There was a problem hiding this comment.
I'll update this as soon as 3.5 releases, should be this coming tuesday
| if ($Tls -eq "openssl" -Or $Tls -eq "openssl3") { | ||
| Write-Error "Arm64EC does not support openssl" | ||
| if ($Tls -eq "quictls" -Or $Tls -eq "quictls3") { | ||
| Write-Error "Arm64EC does not support quictls" |
There was a problem hiding this comment.
We support arm64, unsure about arm64EC, so I excluded it for the time being.
| CxPlatWatchdog Watchdog(2000); | ||
| CxPlatWatchdog Watchdog(20000); |
There was a problem hiding this comment.
This shouldn't be necessary IMO.
There was a problem hiding this comment.
It shouldn't, I agree, but for some reason I've not gotten to the root cause of the additional delay, so I left this in here for the time being.
| // Note: Nominally, if we have the write key, we should have the read key | ||
| // but there might be a delay with openssl as it yields read and write | ||
| // secrets independently. |
There was a problem hiding this comment.
Can this be abstracted from the core QUIC layer?
There was a problem hiding this comment.
I had initially attempted to do so by delaying the installation of new keys until both read and write were available, but doing so resulted in other failures when the quic stack expected keys be available on return from ProcessData. I can play with it some more, but if you have additional input here as to how best to do this, I would appreciate it.
|
I'm thinking that we should move away from submodules for TLS dependencies. I never likely having two for quictls/openssl v1.1 and v3, but having 3 submodules only makes this worse. I'm open to suggestions, but I'm thinking about having CMake dynamically get the submodule and put it in a well-known path (perhaps still submodules/openssl, or perhaps the build folder). Any better ideas? P.S. Depending on how we do this (likely in a separate PR first), we might be able to isolate most of the changes in this PR (all the name changes)... |
crypt_quictls and selfsign_quictls.c aren't any different between openssl forks, keep the original name
|
In regards to the submodule issue, I agree, avoiding them would be beneficial. I suggested an alternative, which is in keeping with other implementations here |
Need to add in the quictls sources here
After discussion with the team, it is clear that we want to avoid re-using "openssl" with a different meaning to avoid an unexpected change for user building MsQuic themselves. What option is best (new name vs larger changes to how we select a crypto backend) is not clear yet and could likely be split in a follow up PR, but we should try to not reuse the "openssl" name here without other change. |
|
So, what is the conclusion here? Would you like me to write a separate PR to move msquic to use non-submodule based configuration method, then rebase this PR on that one, so we don't have to use the QUIC_TLS selector? |
As @ghutier mentioned, we want to be careful about not changing assumptions out from under a client without their knowledge. We need to make this a clear breaking change. So, I'm thinking of the following in separate PRs (in this order):
Note, 1 and 2 may be done as a combined PR, but I'd prefer to separate 3 out. Also, we will worry about migrating away from submodules as a separate, follow up PR. |
Yes, we'd like to eventually get rid of the submodules, but we talked and decided to consider that a separate issue for later. So, for now, the end result would be a submodule for |
|
PR to do step 1 is here: |
Description
This commit enhances msquic to allow it to build using the canonical openssl sources as a TLS backend.
Testing
All existing tests should cover this code as far as I can see.
I've run this code base against the quic-interop-runner and it passes all supported test cases there (save for zerortt I believe) which fails with all TLS backends at the moment I think, but we have apis to support early data, so we should be able to get that up and running shortly.
All unit tests pass under windows currently, save for WithHandshakeArgs4.RandomLossResumeRejection/4 and WithHandshakeArgs4.RandomLossResumeRejection/5 which were failing scholastically. I believe I have those fixed by adjusting some of the asserts when built in debug mode which (If I read the code base right) assume that keys are updated in lock step, which the openssl quic callback api doesn't always do, instead updating keys in independent callbacks.
Documentation
Possibly. currently this PR renames openssl support (which initially pointed to quictls), to actually be named quictls[3] and takes over the QUIC_TLS=openssl configuration to use the canonical openssl sources. That may require build doc updates to reflect that, but I've not clearly found any (yet).