Certificate validation callback #1134
Conversation
carlosmn
force-pushed
the
cmn/certs
branch
4 times, most recently
from
282ff21 to
03e6875
Compare
carlosmn
changed the title
|
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.
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.
There was a problem hiding this comment.
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.
We still have the answer to your question here.
There was a problem hiding this comment.
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.
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.
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.
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.
And only expose a Hash property?
There was a problem hiding this comment.
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.