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

Condition Windows SslCertificateTrust test on Registry value #65848

Merged
merged 4 commits into from
Mar 16, 2022

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Feb 24, 2022

Fixes #65515

Sending trusted issuers list on Windows is problematic (depends on registry settings), so there were no tests. This PR conditionally enables existing tests on Windows if the relevant registry setting is set.

@ghost
Copy link

ghost commented Feb 24, 2022

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #65515

Author: rzikm
Assignees: -
Labels:

area-System.Net.Security

Milestone: -


if (IsWindows)
{
// Sending TrustedIssuers is conditioned on the registry.
Copy link
Member

Choose a reason for hiding this comment

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

I think Windows7 sends the list by default. e.g. even without registry. There are some test that condition on Windows7 but they really should use this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I use different default values depending if we are running on Windows7 or not, so we should be okay here, unless sending cert issuer list can't be disabled on Windows7 using the registry key below.

@wfurt
Copy link
Member

wfurt commented Feb 24, 2022

Do you know if all tests pass now when SendTrustedIssuerList is set to 1?

@rzikm
Copy link
Member Author

rzikm commented Feb 24, 2022

Do you know if all tests pass now when SendTrustedIssuerList is set to 1?

I tried only tests in System.Net.Security, and one of the EKU certificate tests was failing, the cert was filtered out because it did not match any of the issuers. I think it was SslStream_SelfSignedClientEKUClientAuth_Ok.

@rzikm
Copy link
Member Author

rzikm commented Feb 24, 2022

I will have to approach it a bit differently, tests fail on Windows7 because it does not support specifying custom trust store

 System.Net.Security.Tests.SslStreamCertificateTrustTest.SslStream_SendCertificateTrust_CertificateStore [FAIL]
      System.Security.Authentication.AuthenticationException : Authentication failed, see inner exception.
      ---- System.ComponentModel.Win32Exception : The function requested is not supported

@wfurt
Copy link
Member

wfurt commented Feb 24, 2022

I will have to approach it a bit differently, tests fail on Windows7 because it does not support specifying custom trust store

 System.Net.Security.Tests.SslStreamCertificateTrustTest.SslStream_SendCertificateTrust_CertificateStore [FAIL]
      System.Security.Authentication.AuthenticationException : Authentication failed, see inner exception.
      ---- System.ComponentModel.Win32Exception : The function requested is not supported

There are two parts IMHO.
Sending the list impacts client certificate selection. That can be done by default or by changing the registry. It would be nice IMHO if our test don't fail in either case.
The second part is the SendCertificateTrust - It would be nice IMHO to throw PlatformNotSupportedException consitently if given OS/TLS stack cannot do it ... including Windows7.
And have test to verify that guarded by !SupportsSendingCANamesInTls.

@rzikm
Copy link
Member Author

rzikm commented Mar 2, 2022

It would be nice IMHO to throw PlatformNotSupportedException consitently if given OS/TLS stack cannot do it ... including Windows7.

Should we also throw on Windows if sending is not enabled using the registry? That way we would have to check registry at runtime, which we are not doing for any feature AFAIK

@rzikm rzikm force-pushed the 65515-SslCertificateTrustTest-Windows branch from e9ab8f0 to 8656a0a Compare March 2, 2022 10:57
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM

@rzikm
Copy link
Member Author

rzikm commented Mar 3, 2022

@wfurt can you answer my question from #65848 (comment), please?

@rzikm
Copy link
Member Author

rzikm commented Mar 3, 2022

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wfurt
Copy link
Member

wfurt commented Mar 7, 2022

It would be nice IMHO to throw PlatformNotSupportedException consitently if given OS/TLS stack cannot do it ... including Windows7.

Should we also throw on Windows if sending is not enabled using the registry? That way we would have to check registry at runtime, which we are not doing for any feature AFAIK

I would be in favor to throw in product as well if it is not enabled in registry to make it visible. Would that be any problem for AAD @avparuch? (e.g. do you have code that expect. ti send the list but the the registry is not set?)

@rzikm
Copy link
Member Author

rzikm commented Mar 16, 2022

CI failures are #66100

@rzikm
Copy link
Member Author

rzikm commented Mar 16, 2022

PR for updating docs: dotnet/dotnet-api-docs#7832

@rzikm rzikm merged commit 3427a24 into dotnet:main Mar 16, 2022
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
…65848)

Sending trusted issuers list on Windows is problematic (depends on registry settings), so there were no tests. This PR conditionally enables existing tests on Windows if the relevant registry setting is set.
@karelz karelz added this to the 7.0.0 milestone Apr 8, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SslCertificateTrust is not sent on Windows
3 participants