Refactor and enhance LDAP plugin configuration and validation
Some checks failed
Docs CI / lint-and-preview (push) Has been cancelled
Some checks failed
Docs CI / lint-and-preview (push) Has been cancelled
- Updated `LdapPluginOptions` to enforce TLS and client certificate requirements. - Added validation checks for TLS configuration in `LdapPluginOptionsTests`. - Improved error handling in `DirectoryServicesLdapConnectionFactory` for StartTLS negotiation. - Enhanced logging in `LdapCredentialStore` to include detailed audit properties for credential verification. - Introduced `StubStructuredRetriever` and `StubVectorRetriever` for testing in `ToolsetServiceCollectionExtensionsTests`. - Refactored `AdvisoryGuardrailPipelineTests` to improve test clarity and structure. - Added `FileSystemAdvisoryTaskQueueTests` for testing queue functionality. - Updated JSON test data for consistency with new requirements. - Modified `AdvisoryPipelineOrchestratorTests` to reflect changes in metadata keys.
This commit is contained in:
@@ -167,6 +167,59 @@ public class LdapPluginOptionsTests : IDisposable
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Validate_Throws_WhenRequireTlsWithoutTlsConfiguration()
|
||||
{
|
||||
var options = ValidOptions();
|
||||
options.Connection.Host = "ldap://ldap.example.internal";
|
||||
options.Connection.Port = 389;
|
||||
options.Connection.UseStartTls = false;
|
||||
options.Security.RequireTls = true;
|
||||
|
||||
options.Normalize(Path.Combine(tempRoot, "ldap.yaml"));
|
||||
|
||||
var ex = Assert.Throws<InvalidOperationException>(() => options.Validate("corp-ldap"));
|
||||
Assert.Contains("requires TLS", ex.Message, StringComparison.OrdinalIgnoreCase);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Validate_AllowsRequireTlsWithStartTls()
|
||||
{
|
||||
var options = ValidOptions();
|
||||
options.Connection.Host = "ldap.example.internal";
|
||||
options.Connection.Port = 389;
|
||||
options.Connection.UseStartTls = true;
|
||||
options.Security.RequireTls = true;
|
||||
|
||||
options.Normalize(Path.Combine(tempRoot, "ldap.yaml"));
|
||||
options.Validate("corp-ldap");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Validate_Throws_WhenRequireClientCertificateWithoutConfiguration()
|
||||
{
|
||||
var options = ValidOptions();
|
||||
options.Security.RequireClientCertificate = true;
|
||||
|
||||
options.Normalize(Path.Combine(tempRoot, "ldap.yaml"));
|
||||
|
||||
var ex = Assert.Throws<InvalidOperationException>(() => options.Validate("corp-ldap"));
|
||||
Assert.Contains("requireClientCertificate", ex.Message, StringComparison.OrdinalIgnoreCase);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Normalize_ParsesLdapsSchemeAndSetsPort()
|
||||
{
|
||||
var options = ValidOptions();
|
||||
options.Connection.Host = "ldaps://ldap.example.internal:1636";
|
||||
options.Connection.Port = 636;
|
||||
|
||||
options.Normalize(Path.Combine(tempRoot, "ldap.yaml"));
|
||||
|
||||
Assert.Equal("ldap.example.internal", options.Connection.Host);
|
||||
Assert.Equal(1636, options.Connection.Port);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Normalize_DeduplicatesCipherSuites()
|
||||
{
|
||||
|
||||
@@ -39,20 +39,41 @@ internal sealed class DirectoryServicesLdapConnectionFactory : ILdapConnectionFa
|
||||
cancellationToken.ThrowIfCancellationRequested();
|
||||
|
||||
var options = optionsMonitor.Get(pluginName);
|
||||
var identifier = new LdapDirectoryIdentifier(options.Connection.Host!, options.Connection.Port, fullyQualifiedDnsHostName: false, connectionless: false);
|
||||
var connectionOptions = options.Connection;
|
||||
var securityOptions = options.Security;
|
||||
|
||||
var identifier = new LdapDirectoryIdentifier(connectionOptions.Host!, connectionOptions.Port, fullyQualifiedDnsHostName: false, connectionless: false);
|
||||
var connection = new LdapConnection(identifier)
|
||||
{
|
||||
Timeout = TimeSpan.FromSeconds(10)
|
||||
};
|
||||
|
||||
connection.SessionOptions.ProtocolVersion = 3;
|
||||
connection.SessionOptions.ReferralChasing = securityOptions.ReferralChasing
|
||||
? ReferralChasingOptions.All
|
||||
: ReferralChasingOptions.None;
|
||||
|
||||
ConfigureCertificateValidation(connection, options);
|
||||
ConfigureClientCertificate(connection, options);
|
||||
|
||||
if (options.Connection.UseStartTls)
|
||||
if (securityOptions.AllowedCipherSuites.Length > 0)
|
||||
{
|
||||
connection.SessionOptions.StartTransportLayerSecurity(null);
|
||||
logger.LogWarning("LDAP plugin {Plugin} configured security.allowedCipherSuites, but custom cipher selection is not supported on this platform. Falling back to OS defaults.", pluginName);
|
||||
}
|
||||
else if (options.Connection.Port == 636)
|
||||
|
||||
if (connectionOptions.UseStartTls)
|
||||
{
|
||||
try
|
||||
{
|
||||
connection.SessionOptions.StartTransportLayerSecurity(null);
|
||||
}
|
||||
catch (LdapException ex)
|
||||
{
|
||||
logger.LogError(ex, "LDAP plugin {Plugin} failed to negotiate StartTLS.", pluginName);
|
||||
throw new LdapOperationException("Failed to negotiate StartTLS handshake.", ex);
|
||||
}
|
||||
}
|
||||
else if (connectionOptions.UsesLdaps || securityOptions.RequireTls || connectionOptions.Port == 636)
|
||||
{
|
||||
connection.SessionOptions.SecureSocketLayer = true;
|
||||
}
|
||||
|
||||
@@ -48,6 +48,11 @@ internal sealed class LdapCredentialStore : IUserCredentialStore
|
||||
CancellationToken cancellationToken)
|
||||
{
|
||||
var auditProperties = new List<AuthEventProperty>();
|
||||
auditProperties.Add(new AuthEventProperty
|
||||
{
|
||||
Name = "plugin.name",
|
||||
Value = ClassifiedString.Public(pluginName)
|
||||
});
|
||||
|
||||
if (string.IsNullOrWhiteSpace(username) || string.IsNullOrEmpty(password))
|
||||
{
|
||||
@@ -58,7 +63,21 @@ internal sealed class LdapCredentialStore : IUserCredentialStore
|
||||
}
|
||||
|
||||
var normalizedUsername = NormalizeUsername(username);
|
||||
auditProperties.Add(new AuthEventProperty
|
||||
{
|
||||
Name = "ldap.username",
|
||||
Value = ClassifiedString.Public(normalizedUsername)
|
||||
});
|
||||
|
||||
var options = optionsMonitor.Get(pluginName);
|
||||
if (!string.IsNullOrWhiteSpace(options.Connection.Host))
|
||||
{
|
||||
auditProperties.Add(new AuthEventProperty
|
||||
{
|
||||
Name = "ldap.server",
|
||||
Value = ClassifiedString.Public(options.Connection.Host!)
|
||||
});
|
||||
}
|
||||
|
||||
try
|
||||
{
|
||||
@@ -70,17 +89,29 @@ internal sealed class LdapCredentialStore : IUserCredentialStore
|
||||
connection,
|
||||
options,
|
||||
normalizedUsername,
|
||||
cancellationToken).ConfigureAwait(false);
|
||||
cancellationToken,
|
||||
auditProperties).ConfigureAwait(false);
|
||||
|
||||
if (userEntry is null)
|
||||
{
|
||||
logger.LogWarning("LDAP plugin {Plugin} could not find user {Username}.", pluginName, normalizedUsername);
|
||||
auditProperties.Add(new AuthEventProperty
|
||||
{
|
||||
Name = "ldap.failure",
|
||||
Value = ClassifiedString.Public("user_not_found")
|
||||
});
|
||||
return AuthorityCredentialVerificationResult.Failure(
|
||||
AuthorityCredentialFailureCode.InvalidCredentials,
|
||||
"Invalid credentials.",
|
||||
auditProperties: auditProperties);
|
||||
}
|
||||
|
||||
auditProperties.Add(new AuthEventProperty
|
||||
{
|
||||
Name = "ldap.entry_dn",
|
||||
Value = ClassifiedString.Public(userEntry.DistinguishedName)
|
||||
});
|
||||
|
||||
try
|
||||
{
|
||||
await ExecuteWithRetryAsync<bool>(
|
||||
@@ -90,11 +121,17 @@ internal sealed class LdapCredentialStore : IUserCredentialStore
|
||||
await connection.BindAsync(userEntry.DistinguishedName, password, ct).ConfigureAwait(false);
|
||||
return true;
|
||||
},
|
||||
cancellationToken).ConfigureAwait(false);
|
||||
cancellationToken,
|
||||
auditProperties).ConfigureAwait(false);
|
||||
}
|
||||
catch (LdapAuthenticationException)
|
||||
{
|
||||
logger.LogWarning("LDAP plugin {Plugin} received invalid credentials for {Username}.", pluginName, normalizedUsername);
|
||||
auditProperties.Add(new AuthEventProperty
|
||||
{
|
||||
Name = "ldap.failure",
|
||||
Value = ClassifiedString.Public("invalid_credentials")
|
||||
});
|
||||
return AuthorityCredentialVerificationResult.Failure(
|
||||
AuthorityCredentialFailureCode.InvalidCredentials,
|
||||
"Invalid credentials.",
|
||||
@@ -102,11 +139,26 @@ internal sealed class LdapCredentialStore : IUserCredentialStore
|
||||
}
|
||||
|
||||
var descriptor = BuildDescriptor(userEntry, normalizedUsername, passwordRequiresReset: false);
|
||||
auditProperties.Add(new AuthEventProperty
|
||||
{
|
||||
Name = "ldap.result",
|
||||
Value = ClassifiedString.Public("success")
|
||||
});
|
||||
return AuthorityCredentialVerificationResult.Success(descriptor, auditProperties: auditProperties);
|
||||
}
|
||||
catch (LdapTransientException ex)
|
||||
{
|
||||
logger.LogWarning(ex, "LDAP plugin {Plugin} experienced transient failure when verifying user {Username}.", pluginName, normalizedUsername);
|
||||
auditProperties.Add(new AuthEventProperty
|
||||
{
|
||||
Name = "ldap.failure",
|
||||
Value = ClassifiedString.Public("transient_error")
|
||||
});
|
||||
auditProperties.Add(new AuthEventProperty
|
||||
{
|
||||
Name = "ldap.error",
|
||||
Value = ClassifiedString.Public(ex.Message)
|
||||
});
|
||||
return AuthorityCredentialVerificationResult.Failure(
|
||||
AuthorityCredentialFailureCode.UnknownError,
|
||||
"Authentication service temporarily unavailable.",
|
||||
@@ -116,6 +168,16 @@ internal sealed class LdapCredentialStore : IUserCredentialStore
|
||||
catch (LdapOperationException ex)
|
||||
{
|
||||
logger.LogError(ex, "LDAP plugin {Plugin} failed to verify user {Username} due to an LDAP error.", pluginName, normalizedUsername);
|
||||
auditProperties.Add(new AuthEventProperty
|
||||
{
|
||||
Name = "ldap.failure",
|
||||
Value = ClassifiedString.Public("operation_error")
|
||||
});
|
||||
auditProperties.Add(new AuthEventProperty
|
||||
{
|
||||
Name = "ldap.error",
|
||||
Value = ClassifiedString.Public(ex.Message)
|
||||
});
|
||||
return AuthorityCredentialVerificationResult.Failure(
|
||||
AuthorityCredentialFailureCode.UnknownError,
|
||||
"Authentication service error.",
|
||||
@@ -161,7 +223,8 @@ internal sealed class LdapCredentialStore : IUserCredentialStore
|
||||
ILdapConnectionHandle connection,
|
||||
LdapPluginOptions options,
|
||||
string normalizedUsername,
|
||||
CancellationToken cancellationToken)
|
||||
CancellationToken cancellationToken,
|
||||
List<AuthEventProperty> auditProperties)
|
||||
{
|
||||
if (!string.IsNullOrWhiteSpace(options.Connection.UserDnFormat))
|
||||
{
|
||||
@@ -186,16 +249,24 @@ internal sealed class LdapCredentialStore : IUserCredentialStore
|
||||
? options.Queries.Attributes
|
||||
: new[] { "displayName", "cn", "mail" };
|
||||
|
||||
auditProperties.Add(new AuthEventProperty
|
||||
{
|
||||
Name = "ldap.lookup.filter",
|
||||
Value = ClassifiedString.Public(filter)
|
||||
});
|
||||
|
||||
return await ExecuteWithRetryAsync(
|
||||
"lookup",
|
||||
ct => connection.FindEntryAsync(searchBase, filter, attributes, ct),
|
||||
cancellationToken).ConfigureAwait(false);
|
||||
cancellationToken,
|
||||
auditProperties).ConfigureAwait(false);
|
||||
}
|
||||
|
||||
private async Task<T> ExecuteWithRetryAsync<T>(
|
||||
string operation,
|
||||
Func<CancellationToken, ValueTask<T>> action,
|
||||
CancellationToken cancellationToken)
|
||||
CancellationToken cancellationToken,
|
||||
List<AuthEventProperty>? auditProperties = null)
|
||||
{
|
||||
var attempt = 0;
|
||||
Exception? lastException = null;
|
||||
@@ -206,7 +277,17 @@ internal sealed class LdapCredentialStore : IUserCredentialStore
|
||||
|
||||
try
|
||||
{
|
||||
return await action(cancellationToken).ConfigureAwait(false);
|
||||
var result = await action(cancellationToken).ConfigureAwait(false);
|
||||
if (attempt > 0 && auditProperties is not null)
|
||||
{
|
||||
auditProperties.Add(new AuthEventProperty
|
||||
{
|
||||
Name = "ldap.retries",
|
||||
Value = ClassifiedString.Public(attempt.ToString(CultureInfo.InvariantCulture))
|
||||
});
|
||||
}
|
||||
|
||||
return result;
|
||||
}
|
||||
catch (LdapTransientException ex)
|
||||
{
|
||||
@@ -220,7 +301,7 @@ internal sealed class LdapCredentialStore : IUserCredentialStore
|
||||
}
|
||||
|
||||
var delay = TimeSpan.FromMilliseconds(BaseDelay.TotalMilliseconds * Math.Pow(2, attempt - 1));
|
||||
logger.LogWarning(ex, "LDAP operation {Operation} transient failure (attempt {Attempt}/{MaxAttempts}).", operation, attempt, MaxAttempts);
|
||||
logger.LogWarning(ex, "LDAP plugin {Plugin} operation {Operation} transient failure (attempt {Attempt}/{MaxAttempts}).", pluginName, operation, attempt, MaxAttempts);
|
||||
await delayAsync(delay, cancellationToken).ConfigureAwait(false);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -29,6 +29,28 @@ internal sealed class LdapPluginOptions
|
||||
Connection.Validate(pluginName);
|
||||
Security.Validate(pluginName);
|
||||
Queries.Validate(pluginName);
|
||||
|
||||
EnsureSecurityRequirements(pluginName);
|
||||
}
|
||||
|
||||
private void EnsureSecurityRequirements(string pluginName)
|
||||
{
|
||||
if (Security.RequireClientCertificate)
|
||||
{
|
||||
if (Connection.ClientCertificate is null || !Connection.ClientCertificate.IsConfigured)
|
||||
{
|
||||
throw new InvalidOperationException($"LDAP plugin '{pluginName}' requires connection.clientCertificate to be configured when security.requireClientCertificate is true.");
|
||||
}
|
||||
}
|
||||
|
||||
if (Security.RequireTls)
|
||||
{
|
||||
var tlsConfigured = Connection.UseStartTls || Connection.UsesLdaps || Connection.Port == 636;
|
||||
if (!tlsConfigured)
|
||||
{
|
||||
throw new InvalidOperationException($"LDAP plugin '{pluginName}' requires TLS. Configure connection.useStartTls=true or supply an LDAPS endpoint/port.");
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -56,6 +78,8 @@ internal sealed class LdapConnectionOptions
|
||||
|
||||
public string? BindPasswordSecret { get; set; }
|
||||
|
||||
internal bool UsesLdaps { get; private set; }
|
||||
|
||||
internal void Normalize(string configPath)
|
||||
{
|
||||
Host = NormalizeString(Host);
|
||||
@@ -65,6 +89,30 @@ internal sealed class LdapConnectionOptions
|
||||
BindDn = NormalizeString(BindDn);
|
||||
BindPasswordSecret = NormalizeString(BindPasswordSecret);
|
||||
|
||||
UsesLdaps = false;
|
||||
if (!string.IsNullOrWhiteSpace(Host)
|
||||
&& Uri.TryCreate(Host, UriKind.Absolute, out var uri)
|
||||
&& (string.Equals(uri.Scheme, "ldap", StringComparison.OrdinalIgnoreCase)
|
||||
|| string.Equals(uri.Scheme, "ldaps", StringComparison.OrdinalIgnoreCase)))
|
||||
{
|
||||
Host = uri.Host;
|
||||
|
||||
if (uri.Port > 0)
|
||||
{
|
||||
Port = uri.Port;
|
||||
}
|
||||
else if (string.Equals(uri.Scheme, "ldaps", StringComparison.OrdinalIgnoreCase))
|
||||
{
|
||||
Port = 636;
|
||||
}
|
||||
else if (Port == 636)
|
||||
{
|
||||
Port = 389;
|
||||
}
|
||||
|
||||
UsesLdaps = string.Equals(uri.Scheme, "ldaps", StringComparison.OrdinalIgnoreCase);
|
||||
}
|
||||
|
||||
if (ClientCertificate is { })
|
||||
{
|
||||
ClientCertificate.Normalize(configPath);
|
||||
@@ -196,6 +244,8 @@ internal sealed class LdapSecurityOptions
|
||||
|
||||
public bool RequireTls { get; set; } = true;
|
||||
|
||||
public bool RequireClientCertificate { get; set; }
|
||||
|
||||
public bool AllowInsecureWithEnvToggle { get; set; }
|
||||
|
||||
public bool ReferralChasing { get; set; }
|
||||
|
||||
@@ -18,6 +18,7 @@
|
||||
> 2025-11-03: LDAP plugin RFC accepted; review notes in `docs/notes/2025-11-03-authority-plugin-ldap-review.md`. Follow-up implementation items PLG7.IMPL-001..005 added per review outcomes.
|
||||
> 2025-11-03: PLG7.IMPL-001 completed – created `StellaOps.Authority.Plugin.Ldap` + tests projects, implemented configuration normalization/validation (client certificate, trust store, insecure toggle) with coverage (`dotnet test src/Authority/StellaOps.Authority/StellaOps.Authority.Plugin.Ldap.Tests/StellaOps.Authority.Plugin.Ldap.Tests.csproj`), and refreshed `etc/authority.plugins/ldap.yaml`.
|
||||
> 2025-11-04: PLG7.IMPL-002 progress – StartTLS initialization now uses `StartTransportLayerSecurity(null)` and LDAP result-code handling normalized for `System.DirectoryServices.Protocols` 8.0 (invalid credentials + transient cases). Plugin tests rerun via `dotnet test src/Authority/StellaOps.Authority/StellaOps.Authority.Plugin.Ldap.Tests/StellaOps.Authority.Plugin.Ldap.Tests.csproj` (green).
|
||||
> 2025-11-04: PLG7.IMPL-002 progress – enforced TLS/client certificate validation, added structured audit properties and retry logging for credential lookups, warned on unsupported cipher lists, updated sample config, and reran `dotnet test src/Authority/StellaOps.Authority/StellaOps.Authority.Plugin.Ldap.Tests/StellaOps.Authority.Plugin.Ldap.Tests.csproj --no-restore`.
|
||||
|
||||
> Update statuses to DOING/DONE/BLOCKED as you make progress. Always run `dotnet test` for touched projects before marking DONE.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user