Adding TLS verify support & key.pem support. - #160
Conversation
| if (!string.IsNullOrEmpty(CertificateLocation)) | ||
| { | ||
| #if !NET46 | ||
| throw new InvalidOperationException("TLS authetication is not supported in .NET Core."); |
| caCert = new X509Certificate2(System.IO.Path.Combine(CertificateLocation, CAFileName)); | ||
| } | ||
|
|
||
| cred.ServerCertificateValidationCallback = (o, c, ch, er) => |
There was a problem hiding this comment.
Probably since this isn't Go you should spell out these argument names.
| (chain.ChainStatus.Length == 1 && chain.ChainStatus[0].Status == X509ChainStatusFlags.UntrustedRoot); | ||
| } | ||
| } | ||
| catch{ return true;} |
| var client = DockerFactory.CreateClient(null, null); | ||
| var listParams = new ImagesListParameters() { All = true }; | ||
| await client.Images.ListImagesAsync(listParams); | ||
| //result.Wait(); |
|
I hadn't intended to leave that test code in there... it won't really work on anyone else's machine, so it's not a valid test to keep in the product. |
| chain2.ChainPolicy.VerificationFlags = X509VerificationFlags.AllFlags; | ||
| chain2.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck; | ||
|
|
||
| chain2.ChainPolicy.ExtraStore.Add(caCert); |
There was a problem hiding this comment.
What happens here if caCert == null
There was a problem hiding this comment.
If tlsVerify is true, we know we've gone through the code that tries to construct the caCert using the X509Certificate2 constructor. That constructor will throw if the file is missing or not of the right format. So I don't expect caCert to every be null. If it does end up being null, ExtraStore.Add will fail, and we will fail TLS verification, refusing to connect to the server.
There was a problem hiding this comment.
Oh I see. Its actually the if above ... || !tlsVerify) that verifies this case.
| EndProject | ||
| Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "test", "test", "{42C1BB74-7512-4B4B-AF57-F1448CD7A337}" | ||
| EndProject | ||
| Project("{8BB2217D-0F2D-49D1-97BC-3654ED321F3B}") = "src", "test\src\src.xproj", "{B7044204-872F-41F0-A98A-232A43572089}" |
There was a problem hiding this comment.
In my last review I asked about the name of this. Do we really want to call it src?
There was a problem hiding this comment.
Ah... I removed these files, but didn't update the solution. I'll do that.
| new X509Certificate2( | ||
| System.IO.Path.Combine(CertificateLocation, KeyFileName), | ||
| certPass)); | ||
| RSAUtil.GetCertFromPEMFiles( |
There was a problem hiding this comment.
Is there any reason at all to check if the extension is pfx or pem in CertificateLocation? IE: Should we support either option?
There was a problem hiding this comment.
Well, to handle pfx, we'd need to have some way for the user to provide the password, which we are trying to avoid. For now, I think it's ok to say we only support the dual-pem files, since that's what docker supports.
| "src", | ||
| "src/Docker.DotNet" | ||
| "src/Docker.DotNet", | ||
| "test/src" |
|
LGTM. Just remove the |
With this change, the cmdlets will be able to use the ca.pem/key.pem combination in the same way as the docker CLI, removing the need for generating a separate, Windows-specific PFX. Unforunately, the mechanisms required to make this work are not yet available in .NET Core, so certificate support is Windows only at the moment. Once .NET Core with the latest networking fixes is released, we can unblock that scenario for cross-platform.
|
John Starks (@jstarks) Is this ready to merge? |
Review feedback addressed
John Starks (@jstarks) Justin (@jterry75)
With this change, the cmdlets will be able to use the ca.pem/key.pem combination in the same way as the docker CLI, removing the need for generating a separate, Windows-specific PFX. Unforunately, the mechanisms required to make this work are not yet available in .NET Core, so certificate support is Windows only at the moment. Once .NET Core with the latest networking fixes is released, we can unblock that scenario for cross-platform. Pulls in the latest Docker.DotNet to get PEM support.