Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Multiple Credentials in Kernel Mode Schannel #4096

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

nibanks
Copy link
Member

@nibanks nibanks commented Jan 29, 2024

Description

Adds a new API to support passing multiple credetials to Schannel in kernel mode. Fixes #3141.

Testing

TODO

Documentation

TODO

Copy link

codecov bot commented Jan 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@dd316f7). Learn more about missing BASE report.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4096   +/-   ##
=======================================
  Coverage        ?   85.97%           
=======================================
  Files           ?       56           
  Lines           ?    15521           
  Branches        ?        0           
=======================================
  Hits            ?    13344           
  Misses          ?     2177           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@anrossi
Copy link
Contributor

anrossi commented Jan 29, 2024

Looks fine to me.
Regarding testing, the setup script could create two certificates for the server, instead of just one (which chain up to the same root certificate), and those two certificates could have different parameters (key algorithm, and hash algorithm).
Then the client could ban one algorithm in its configuration that matches one server cert, but not the other, and ensure the client connects successfully. and then ban the other algorithm and verify again. Also, ban both, and verify the client fails.

src/inc/msquic.h Outdated Show resolved Hide resolved
src/inc/msquic.h Outdated Show resolved Hide resolved
@ProjectsByJackHe
Copy link
Contributor

Looks like this PR is causing some certificate error on our lab machines / VMs:
image
https://github.com/microsoft/netperf/actions/runs/9274043389/job/25515646313

@anrossi
Copy link
Contributor

anrossi commented Jun 14, 2024

This PR is currently blocked on bugs discovered in external components.

@anrossi anrossi marked this pull request as draft June 18, 2024 06:57
@nibanks nibanks marked this pull request as ready for review August 14, 2024 12:42
@nibanks
Copy link
Member Author

nibanks commented Aug 14, 2024

@anrossi:

MsQuicTestRoot not found! Creating new MsQuicTestRoot certificate...

    Directory: C:\Users\runneradmin\AppData\Local\Temp

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
-a---           8/14/20[24](https://github.com/microsoft/msquic/actions/runs/10387767120/job/28761939740?pr=4096#step:4:25) 12:43 PM            512 MsQuicTestRoot.cer
New MsQuicTestRoot certificate installed!
MsQuicTestRootRSA not found! Creating new MsQuicTestRootRSA certificate...
prepare-machine.ps1: D:\a\_temp\2b4b17e7-4051-4462-ac94-73e040ca9959.ps1:2
Line |
   2 |  scripts/prepare-machine.ps1
     |  ~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | A positional parameter cannot be found that accepts argument '8/14/20[29](https://github.com/microsoft/msquic/actions/runs/10387767120/job/28761939740?pr=4096#step:4:30) 12:43:04 PM'. This failure might be
     | caused by applying the default parameter binding. You can disable the default parameter binding in
     | $PSDefaultParameterValues by setting $PSDefaultParameterValues["Disabled"] to be $true, and retry. The following
     | default parameter was successfully bound for this cmdlet when the error occurred: -ErrorAction
Error: Process completed with exit code 1.

@nibanks
Copy link
Member Author

nibanks commented Aug 14, 2024

@anrossi, kernel build failure

D:\a\msquic\msquic\src\test\lib\HandshakeTest.cpp(3054,21): error C2065: 'QUIC_STATUS_UNSUPPORTED_CERTIFICATE': undeclared identifier [D:\a\msquic\msquic\src\test\lib\testlib.kernel.vcxproj]

@@ -353,6 +353,8 @@ if(QUIC_TLS STREQUAL "schannel")
list(APPEND QUIC_COMMON_DEFINES QUIC_DISABLE_CHACHA20_TESTS)
message(STATUS "Enabling anonymous client auth tests")
list(APPEND QUIC_COMMON_DEFINES QUIC_ENABLE_ANON_CLIENT_AUTH_TESTS)
message(STATUS "Enabling certificate algorithm tests")
list(APPEND QUIC_COMMON_DEFINES QUIC_ENABLE_CERT_ALG_TESTS)
Copy link
Member Author

Choose a reason for hiding this comment

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

Set these in the kernel mode projects?

@nibanks
Copy link
Member Author

nibanks commented Aug 15, 2024

Looks like all the new tests failed:

[ RUN      ] Handshake/WithMultiCertArgs.ConnectServerAllowedCertificateAlgorithms/0
D:\a\msquic\msquic\src\test\lib\HandshakeTest.cpp(3079): error: Client.GetIsConnected() not equal to ExpectedConnectionSuccess

[  FAILED  ] Handshake/WithMultiCertArgs.ConnectServerAllowedCertificateAlgorithms/0, where GetParam() = Hash/RSA first/None allowed (55 ms)
[----------] 1 test from Handshake/WithMultiCertArgs (56 ms total)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for multiple server certificates
3 participants