Skip to content

Commit

Permalink
Add flag for backward compatibility with TLS checks (#421)
Browse files Browse the repository at this point in the history 
* change option name

* update flag name

* remove braces

* nit; fix breaking TestUtils initialization for TLS options

* cleanup of PR

* minor rephrasing

---------

Co-authored-by: Vasileios Zois <vazois@microsoft.com>
Co-authored-by: Badrish Chandramouli <badrishc@microsoft.com>
  • Loading branch information
@msft-paddy14 @vazois @badrishc
3 people authored May 30, 2024
1 parent 4991f92 commit 12e9656
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 11 deletions.
7 changes: 6 additions & 1 deletion libs/host/Configuration/Options.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,10 @@ internal sealed class Options
[Option("cluster-tls-client-target-host", Required = false, HelpText = "Name for the client target host when using TLS connections in cluster mode.")]
public string ClusterTlsClientTargetHost { get; set; }

[OptionValidation]
[Option("server-certificate-required", Required = false, HelpText = "Whether server TLS certificate is required by clients established on the server side, e.g., for cluster gossip and replication.")]
public bool? ServerCertificateRequired { get; set; }

[OptionValidation]
[Option("tls", Required = false, HelpText = "Enable TLS.")]
public bool? EnableTLS { get; set; }
Expand All @@ -243,7 +247,7 @@ internal sealed class Options
public int CertificateRefreshFrequency { get; set; }

[OptionValidation]
[Option("client-certificate-required", Required = false, HelpText = "Whether TLS client certificate required.")]
[Option("client-certificate-required", Required = false, HelpText = "Whether client TLS certificate is required by the server.")]
public bool? ClientCertificateRequired { get; set; }

[Option("certificate-revocation-check-mode", Required = false, HelpText = "Certificate revocation check mode for certificate validation (NoCheck, Online, Offline).")]
Expand Down Expand Up @@ -580,6 +584,7 @@ public GarnetServerOptions GetServerOptions(ILogger logger = null)
CertificateRefreshFrequency,
EnableCluster.GetValueOrDefault(),
ClusterTlsClientTargetHost,
ServerCertificateRequired.GetValueOrDefault(),
logger: logger) : null,
LatencyMonitor = LatencyMonitor.GetValueOrDefault(),
MetricsSamplingFrequency = MetricsSamplingFrequency,
Expand Down
5 changes: 4 additions & 1 deletion libs/host/defaults.conf
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,12 @@
/* TLS certificate refresh frequency in seconds (0 to disable). */
"CertificateRefreshFrequency" : 0,

/* Whether TLS client certificate required. */
/* Whether client TLS certificate is required by the server. */
"ClientCertificateRequired" : true,

/* Whether server TLS certificate is required by clients established on the server side, e.g., for cluster gossip and replication. */
"ServerCertificateRequired" : true,

/* Certificate revocation check mode for certificate validation (NoCheck, Online, Offline). */
"CertificateRevocationCheckMode" : "NoCheck",

Expand Down
46 changes: 39 additions & 7 deletions libs/server/TLS/GarnetTlsOptions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,29 @@ public class GarnetTlsOptions : IGarnetTlsOptions
readonly string CertSubjectName;
readonly int CertificateRefreshFrequency;

/// <summary>
/// Whether server requires a valid client certificate
/// </summary>
readonly bool ClientCertificateRequired;

/// <summary>
/// Whether client requires a valid server certificate
/// </summary>
readonly bool ServerCertificateRequired;

/// <summary>
/// Certificate revocation mode (shared by client and server)
/// </summary>
readonly X509RevocationMode CertificateRevocationCheckMode;

string ClusterTlsClientTargetHost;
/// <summary>
/// Target (server) host name used by the client
/// </summary>
string ClientTargetHost;

/// <summary>
/// Issuer certificate path
/// </summary>
string IssuerCertificatePath = string.Empty;

ServerCertificateSelector serverCertificateSelector;
Expand All @@ -51,7 +70,8 @@ public GarnetTlsOptions(
bool clientCertificateRequired, X509RevocationMode certificateRevocationCheckMode, string issuerCertificatePath,
string certSubjectName, int certificateRefreshFrequency,
bool enableCluster,
string clusterTlsClientTargetHost,
string clientTargetHost,
bool serverCertificateRequired = false,
SslServerAuthenticationOptions tlsServerOptionsOverride = null,
SslClientAuthenticationOptions clusterTlsClientOptionsOverride = null,
ILogger logger = null)
Expand All @@ -63,7 +83,8 @@ public GarnetTlsOptions(
this.CertSubjectName = certSubjectName;
this.CertificateRefreshFrequency = certificateRefreshFrequency;

this.ClusterTlsClientTargetHost = clusterTlsClientTargetHost;
this.ServerCertificateRequired = serverCertificateRequired;
this.ClientTargetHost = clientTargetHost;
this.IssuerCertificatePath = issuerCertificatePath;

this.logger = logger;
Expand Down Expand Up @@ -122,7 +143,7 @@ SslServerAuthenticationOptions GetSslServerAuthenticationOptions()
if (CertificateRefreshFrequency < 0)
{
logger?.LogError("CertificateRefreshFrequency should not be less than 0.");
throw new Exception("CertificateRefreshFrequency should not be less than 0.");
throw new GarnetException("CertificateRefreshFrequency should not be less than 0.");
}

// End timer associated with old certificate selector, if any
Expand All @@ -148,11 +169,17 @@ SslServerAuthenticationOptions GetSslServerAuthenticationOptions()

SslClientAuthenticationOptions GetSslClientAuthenticationOptions()
{
if (ServerCertificateRequired && string.IsNullOrEmpty(ClientTargetHost))
{
logger?.LogError("ClientTargetHost should be provided when ServerCertificateRequired is enabled");
throw new GarnetException("ClientTargetHost should be provided when ServerCertificateRequired is enabled");
}
return new SslClientAuthenticationOptions
{
TargetHost = ClusterTlsClientTargetHost,
TargetHost = ClientTargetHost,
AllowRenegotiation = false,
RemoteCertificateValidationCallback = ValidateServerCertificateCallback(ClusterTlsClientTargetHost, IssuerCertificatePath),
CertificateRevocationCheckMode = CertificateRevocationCheckMode,
RemoteCertificateValidationCallback = ValidateServerCertificateCallback(ClientTargetHost, IssuerCertificatePath),
// We use the same server certificate selector for the server's own client as well
LocalCertificateSelectionCallback = (object sender, string targetHost, X509CertificateCollection localCertificates, X509Certificate remoteCertificate, string[] acceptableIssuers) =>
{
Expand All @@ -161,14 +188,19 @@ SslClientAuthenticationOptions GetSslClientAuthenticationOptions()
};
}


/// <summary>
/// Callback to verify the TLS certificate
/// </summary>
/// <param name="issuerCertificatePath">The path to issuer certificate file. </param>
/// <returns></returns>
RemoteCertificateValidationCallback ValidateServerCertificateCallback(string targetHostName, string issuerCertificatePath)
{
if (!ServerCertificateRequired)
{
logger?.LogWarning("ServerCertificateRequired is false. Remote certificate validation will always succeed.");
return (object _, X509Certificate certificate, X509Chain __, SslPolicyErrors sslPolicyErrors)
=> true;
}
var issuer = GetCertificateIssuer(issuerCertificatePath);
return (object _, X509Certificate certificate, X509Chain __, SslPolicyErrors sslPolicyErrors)
=> (sslPolicyErrors == SslPolicyErrors.None) || (sslPolicyErrors == SslPolicyErrors.RemoteCertificateChainErrors
Expand Down
9 changes: 7 additions & 2 deletions test/Garnet.test/TestUtils.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -461,8 +461,13 @@ public static GarnetServerOptions GetGarnetServerOptions(
clientCertificateRequired: true,
certificateRevocationCheckMode: X509RevocationMode.NoCheck,
issuerCertificatePath: null,
null, 0, true, null, null,
new SslClientAuthenticationOptions
certSubjectName: null,
certificateRefreshFrequency: 0,
enableCluster: true,
clientTargetHost: null,
serverCertificateRequired: true,
tlsServerOptionsOverride: null,
clusterTlsClientOptionsOverride: new SslClientAuthenticationOptions
{
ClientCertificates = certificates ?? [new X509Certificate2(certFile, certPassword)],
TargetHost = "GarnetTest",
Expand Down

0 comments on commit 12e9656

Please sign in to comment.