-
-
Notifications
You must be signed in to change notification settings - Fork 288
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
feat(#370): Added support docker TLS endpoint #597
feat(#370): Added support docker TLS endpoint #597
Conversation
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.
Did I share my idea regarding inheritance the last time? I am thinking about something like:
internal class TlsEndpointAuthenticationProvider : DockerEndpointAuthenticationProvider
{
protected const string CaCertificateFileName = "ca.pem";
protected const string ClientCertificateFileName = "cert.pem";
protected const string ClientCertificateKeyFileName = "key.pem";
/// <summary>
/// Initializes a new instance of the <see cref="TlsEndpointAuthenticationProvider" /> class.
/// </summary>
/// <param name="customConfigurations">A list of custom configurations.</param>
public TlsEndpointAuthenticationProvider(params ICustomConfiguration[] customConfigurations)
: this(customConfigurations
.OrderByDescending(item => item.GetDockerTls() && item.GetDockerHost() != null)
.ThenByDescending(item => item.GetDockerTls())
.First())
{
}
private TlsEndpointAuthenticationProvider(ICustomConfiguration customConfiguration)
{
this.TlsEnabled = customConfiguration.GetDockerTls();
this.TlsVerifyEnabled = customConfiguration.GetDockerTlsVerify();
this.CertificatesDirectoryPath = customConfiguration.GetDockerCertPath() ?? Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), ".docker"); // TODO: Move default values to TestcontainersSettings (internal)?
this.DockerEngine = customConfiguration.GetDockerHost() ?? new Uri("tcp://localhost:2376");
}
protected bool TlsEnabled { get; }
protected bool TlsVerifyEnabled { get; }
protected string CertificatesDirectoryPath { get; }
protected Uri DockerEngine { get; }
/// <inheritdoc />
public override bool IsApplicable()
{
var certificatesFiles = new[] { CaCertificateFileName, ClientCertificateFileName };
return this.TlsEnabled && certificatesFiles.Select(fileName => Path.Combine(this.CertificatesDirectoryPath, fileName)).All(File.Exists);
}
/// <inheritdoc />
public override IDockerEndpointAuthenticationConfiguration GetAuthConfig()
{
var caCertificate = this.GetCaCertificate(); // TODO: Can we put the certificates in a using block?
var clientCertificate = this.GetClientCertificate();
if (clientCertificate != null)
{
var credentials = new CertificateCredentials(clientCertificate);
credentials.ServerCertificateValidationCallback = ServerCertificateValidationCallback;
return new DockerEndpointAuthenticationConfiguration(this.DockerEngine, credentials);
}
else
{
// TODO:
return null;
}
}
protected virtual X509Certificate2 GetCaCertificate()
{
return null;
}
protected virtual X509Certificate2 GetClientCertificate()
{
return null;
}
private static bool ServerCertificateValidationCallback(object sender, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors)
{
return false;
}
}
internal class MTlsEndpointAuthenticationProvider : TlsEndpointAuthenticationProvider
{
public override bool IsApplicable()
{
var certificatesFiles = new[] { CaCertificateFileName, ClientCertificateFileName, ClientCertificateKeyFileName };
return this.TlsEnabled && this.TlsVerifyEnabled && certificatesFiles.Select(fileName => Path.Combine(this.CertificatesDirectoryPath, fileName)).All(File.Exists);
}
}
I can set up a unit test incl. the approach from above. Both implementations share a lot of code, I think inheritance makes sense here.
using System.Linq; | ||
using DotNet.Testcontainers.Configurations; | ||
|
||
internal static class CustomConfigurationExtensions |
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.
Is that extension worth it? Without we can add the default value in advance or add additional checks:
customConfigurations
.Select(customConfiguration => customConfiguration.GetDockerCertPath())
.Append(DefaultCertPath)
.First(path => path != null)
customConfigurations
.Select(customConfiguration => customConfiguration.GetDockerCertPath())
.FirstOrDefault(Directory.Exists) ?? DefaultCertPath
Otherwise I would change it to something like
internal static class CustomConfigurationExtension
{
public static T GetProperty<T>(this IEnumerable<ICustomConfiguration> customConfigurations, Func<ICustomConfiguration, T> propertySelector)
{
return customConfigurations.Select(propertySelector).FirstOrDefault(item => item != null);
}
public static bool GetProperty<T>(this IEnumerable<ICustomConfiguration> customConfigurations, Func<ICustomConfiguration, bool> propertySelector)
{
return customConfigurations.Select(propertySelector).Any(item => true.Equals(item));
}
}
If we keep it, can we move it to DotNet.Testcontainers.Configurations
and rename it?
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 like your pragmatic thinking. Yes these extensions are not needed, It just simplify code for readability. Nothing more. I will remove it.
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.
Fixed in 9dd775f
Regarding your idea:
I know about it and yes you mentioned. I did not update the code here yet because it is not needed. There is no mTLS implementation. I wan to make it simple as much as possible. I will add it in next PR with mTLS. And regarding test. It's really a problem of the dockerd that needs CA cert even it should not want therfore it forces mTLS all the time. |
30b19b7
to
9dd775f
Compare
@HofmeisterAn I am thinking regarding inheritance which is really good suggestion little bit smaller change. Like this: diff --git a/src/Testcontainers/Builders/TlsEndpointAuthenticationProvider.cs b/src/Testcontainers/Builders/TlsEndpointAuthenticationProvider.cs
index e8f5c9a..467c7f2 100644
--- a/src/Testcontainers/Builders/TlsEndpointAuthenticationProvider.cs
+++ b/src/Testcontainers/Builders/TlsEndpointAuthenticationProvider.cs
@@ -9,15 +9,16 @@ namespace DotNet.Testcontainers.Builders
using DotNet.Testcontainers.Configurations.Credentials;
/// <inheritdoc cref="IDockerRegistryAuthenticationProvider" />
- internal sealed class TlsEndpointAuthenticationProvider : DockerEndpointAuthenticationProvider
+ internal class TlsEndpointAuthenticationProvider : DockerEndpointAuthenticationProvider
{
private const string DefaultUserDockerFolderName = ".docker";
private const string DefaultCaCertFileName = "ca.pem";
private static readonly Uri DefaultTlsDockerEndpoint = new Uri("tcp://localhost:2376");
private static readonly string DefaultCertPath = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), DefaultUserDockerFolderName);
- private readonly Lazy<X509Certificate2> caCertificate;
- private readonly Uri dockerEngine;
+ protected readonly string dockerCertPath;
+ protected readonly Lazy<X509Certificate2> caCertificate;
+ protected readonly Uri dockerEngine;
private readonly bool dockerTlsEnabled;
/// <summary>
@@ -34,18 +35,16 @@ namespace DotNet.Testcontainers.Builders
/// <param name="customConfigurations">A list of custom configurations.</param>
public TlsEndpointAuthenticationProvider(params ICustomConfiguration[] customConfigurations)
{
- var dockerCertPath = customConfigurations
+ this.dockerCertPath = customConfigurations
.Select(customConfiguration => customConfiguration.GetDockerCertPath())
.FirstOrDefault(value => value != null) ?? DefaultCertPath;
- var dockerCaCertFile = Path.Combine(dockerCertPath, DefaultCaCertFileName);
-
this.dockerEngine = customConfigurations
.Select(customConfiguration => customConfiguration.GetDockerHost())
.FirstOrDefault(value => value != null) ?? DefaultTlsDockerEndpoint;
this.dockerTlsEnabled = customConfigurations
.Select(customConfiguration => customConfiguration.GetDockerTls())
.Aggregate(false, (x, y) => x || y);
- this.caCertificate = new Lazy<X509Certificate2>(() => new X509Certificate2(dockerCaCertFile));
+ this.caCertificate = new Lazy<X509Certificate2>(() => new X509Certificate2(Path.Combine(this.dockerCertPath, DefaultCaCertFileName)));
}
/// <inheritdoc />
@@ -60,7 +59,7 @@ namespace DotNet.Testcontainers.Builders
return new DockerEndpointAuthenticationConfiguration(this.dockerEngine, new TlsCredentials(this.ServerCertificateValidationCallback));
}
- private bool ServerCertificateValidationCallback(object sender, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors)
+ protected bool ServerCertificateValidationCallback(object sender, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors)
{
switch (sslPolicyErrors)
{ and then /// <inheritdoc cref="IDockerRegistryAuthenticationProvider" />
internal sealed class MTlsEndpointAuthenticationProvider : TlsEndpointAuthenticationProvider
{
private const string DefaultClientCertFileName = "cert.pem";
private const string DefaultClientKeyFileName = "key.pem";
private static readonly Regex PemData = new Regex("-----BEGIN (.*)-----(.*)-----END (.*)-----", RegexOptions.Multiline);
private readonly bool dockerTlsVerifyEnabled;
private readonly string dockerClientCertFile;
private readonly string dockerClientKeyFile;
private readonly Lazy<MSX509.X509Certificate2> clientCertificate;
/// <summary>
/// Initializes a new instance of the <see cref="MTlsEndpointAuthenticationProvider" /> class.
/// </summary>
public MTlsEndpointAuthenticationProvider()
: this(PropertiesFileConfiguration.Instance, EnvironmentConfiguration.Instance)
{
}
/// <summary>
/// Initializes a new instance of the <see cref="MTlsEndpointAuthenticationProvider" /> class.
/// </summary>
/// <param name="customConfigurations">A list of custom configurations.</param>
public MTlsEndpointAuthenticationProvider(params ICustomConfiguration[] customConfigurations)
: base(customConfigurations)
{
this.dockerTlsVerifyEnabled = customConfigurations
.Select(customConfiguration => customConfiguration.GetDockerTlsVerify())
.Aggregate(false, (x, y) => x || y);
this.dockerClientCertFile = Path.Combine(this.dockerCertPath, DefaultClientCertFileName);
this.dockerClientKeyFile = Path.Combine(this.dockerCertPath, DefaultClientKeyFileName);
this.clientCertificate = new Lazy<MSX509.X509Certificate2>(this.GetClientCertificate);
}
/// <inheritdoc />
public override bool IsApplicable()
{
return this.dockerTlsVerifyEnabled && File.Exists(this.dockerClientCertFile) && File.Exists(this.dockerClientKeyFile);
}
/// <inheritdoc />
public override IDockerEndpointAuthenticationConfiguration GetAuthConfig()
{
return new DockerEndpointAuthenticationConfiguration(this.dockerEngine, new CertificateCredentials(this.clientCertificate.Value)
{
ServerCertificateValidationCallback = this.ServerCertificateValidationCallback,
});
}
... Result is that MTls version does not have any Tls specific code and also there is not any special empty methods. Each class handles own stuff which they need to ake it work. Full implementation is already available in updated PR #548. What do you think? |
We will merge everything with #548. |
This is second small increment of huge PR #548.
This PR brings support for secure communication between client and docker daemon server API.
This PR does not contain client authentication based on certificates (mTLS) which can be switch on by option DOCKER_TLS_VERIFY.