-
Notifications
You must be signed in to change notification settings - Fork 886
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
Certificate validation callback #1134
Conversation
282ff21
to
03e6875
Compare
Should be ready for review. I don't know if it's worth having a different test for accepting vs rejecting. I'm mostly concerned about exposing the data to the user here. |
cert.HashMD5.CopyTo(HashMD5, 0); | ||
|
||
HashSHA1 = new byte[20]; | ||
cert.HashSHA1.CopyTo(HashSHA1, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we've brought these arrays into C# already, I wonder if it'd be fine to assing HashMD5 = cert.HashMD5
and the same for SHA1.
pushed up some minor fixups. |
{ | ||
var scd = BuildSelfCleaningDirectory(); | ||
|
||
Assert.Throws<UserCancelledException>(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of my main issue with this Skipping feature no longer being doable with xUnit 2.0 is that a passing test may now "mean" two completely different thing.
- Yeah, everything worked perfectly.
- Sorry, dude, I bailed out.
And that somehow bothers me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still have the answer to your question here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that bit either, but as I had to use the xunit2 port, this was the best I could come up with to test that the ssh "cert" is in order without failing the test when built without ssh support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated https://github.com/xunit/samples.xunit/issues/8 to express the kind of use we'd make of such a dynamic ignore.
@bradwilson's answer left me under the impression that this might be doable with xUnit 2.0. I may have misunderstood it, though.
The SkippableFact example is now available. https://github.com/xunit/samples.xunit/tree/master/DynamicSkipExample |
💚 |
@bradwilson AWE. SOME. |
@carlosmn Now that @bradwilson has sprinkled some of his amazing magic, could you please update this to rely on the |
@nulltoken updated to use |
Pushed up a counter proposal. This actually fails. Any idea? |
@carlosmn Can you please check the |
{ | ||
var hostkey = (CertificateSsh)cert; | ||
Assert.True(hostkey.HasMD5); | ||
Assert.Equal("1627aca576282d36631b564debdfa648", BitConverter.ToString(hostkey.HashMD5)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a comment explaining how to discover the new hash would the hostkey change?
A caller might want to inspect the certificate or simply ignore who the server claims to be.
/// <summary> | ||
/// True if we have the MD5 hostkey hash from the server | ||
/// </summary> | ||
public readonly bool HasMD5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How many kinds of Hash can we get? Rather then exposing named properties, can't we this into an enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And only expose a Hash
property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to get both. My ssh and libssh2 only seem to give back MD5 for the servers I connect to, but it claims it could be both.
Certificate validation callback
💎 |
Great! Thanks for merging. |
@michakfromparis I'm going to publish in the following hours a new pre-release version of the NuGet package containing this feature. |
Published as NuGet pre-release package |
The first commit is just so I can run it on my machines, the others will get squashed.
I'm not sure how deeply to specify types for the certs. ~~~Maybe it'd be fine to expose the cert like in C where the same class may contain MD5 or SHA1 hashes.~~~ It looks like I forgot that it's theoretically possible to have both hostkey hashes at once, so it should be just the one.
I don't quite expect this to actually work yet, but the types and the structs should at least be correct.