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

ignore expiration in NoCallback_RevokedCertificate_NoRevocationChecking_Succeeds test #83157

Closed
wants to merge 1 commit into from

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Mar 8, 2023

It seems like chromium/badssl.com#515 is broken for a while not getting any traction.
I updated the test to ignore expiration and do all other checks. It should work as well if certificate will get renewed. (and revoked)

fixes #77726

@wfurt wfurt added area-System.Net.Http test-enhancement Improvements of test source code labels Mar 8, 2023
@wfurt wfurt requested review from rzikm and a team March 8, 2023 20:10
@wfurt wfurt self-assigned this Mar 8, 2023
@ghost
Copy link

ghost commented Mar 8, 2023

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

Issue Details

It seems like chromium/badssl.com#515 is broken for a while not getting any traction.
I updated the test to ignore expiration and do all other checks. It should work as well if certificate will get renewed. (and revoked)

fixes #77726

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Http, test-enhancement

Milestone: -

@@ -228,14 +228,38 @@ public async Task NoCallback_BadCertificate_ThrowsException(string url)
}

[OuterLoop("Uses external servers")]
[ActiveIssue("https://github.com/dotnet/runtime/issues/77726")]
[ConditionalFact(nameof(ClientSupportsDHECipherSuites))]
public async Task NoCallback_RevokedCertificate_NoRevocationChecking_Succeeds()
Copy link
Member

Choose a reason for hiding this comment

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

Name of the test now seems not fully correct, as we do have a callback now 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

good point. I can make it work without callback - but only for SocketsHttpHandler as that is the only one exposing SSL options and therefore certificate validation policy.
Or we can just rename the test.
Cast your vote.

Copy link
Member

Choose a reason for hiding this comment

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

I have a feeling this group of tests starting with "NoCallback_" was supposed to validate HttpClient's default behavior... so can we have both tests? The one without callback for SocketsHttpHandler (which should technically cover HttpClientHandler without callback on platforms where it has SocketsHttpHandler inside), and the one with the callback you've added for general HttpClientHandler?

Copy link
Member Author

Choose a reason for hiding this comment

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

One way how to solve this is removing dependency on badssl.com. It may need some more infrastructure changes but perhaps it is better fix.

@wfurt
Copy link
Member Author

wfurt commented Mar 29, 2023

I'm turning this to draft to explore possibility without badssl.com. If that fails I'll split the tests the way @CarnaViire suggested.

@wfurt wfurt marked this pull request as draft March 29, 2023 03:16
@ghost ghost closed this Apr 28, 2023
@ghost
Copy link

ghost commented Apr 28, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@wfurt wfurt reopened this Apr 28, 2023
@ghost ghost closed this May 28, 2023
@ghost
Copy link

ghost commented May 28, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 27, 2023
@karelz karelz added this to the 8.0.0 milestone Jul 3, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http test-enhancement Improvements of test source code
Projects
None yet
3 participants