Nabsul/gateway api #131
Nabsul/gateway api #131
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request implements Gateway API support for KCert by adding testing infrastructure and refactoring the codebase for improved configurability and maintainability.
- Adds comprehensive testing setup with justfile automation for creating temporary clusters with DigitalOcean
- Refactors Helm chart configuration to use environment variables for better flexibility
- Simplifies service registration and removes attribute-based dependency injection
Reviewed Changes
Copilot reviewed 52 out of 53 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| testing/* | New testing infrastructure including justfile automation, Gateway API configuration, and test app |
| charts/kcert/* | Simplified Helm chart with environment variable-based configuration |
| Services/* | Refactored services to remove attribute-based DI and improve async handling |
| Challenge/* | Updated challenge providers with better error handling and simplified logic |
| Extensions.cs | New service registration extension replacing attribute-based approach |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| public async ListMet<JsonObject> ListAsync(CancellationToken tok) | ||
| { | ||
| await foreach (var obj in k8s.listcustom | ||
| { | ||
| yield return obj; | ||
| } | ||
| } |
There was a problem hiding this comment.
The method return type 'ListMet' is invalid and the method body is incomplete. The return type should be 'IAsyncEnumerable' and the method body needs to be completed with proper Kubernetes API calls.
| private static AcmeDirectoryResponse? _dir; | ||
| public static AcmeDirectoryResponse Dir => _dir ?? throw new Exception("ACME directory not initialized."); |
There was a problem hiding this comment.
The static field '_dir' is not thread-safe. Multiple threads could access and modify this field concurrently, leading to race conditions. Consider using proper synchronization or making this instance-based.
| public Task<AcmeAuthzResponse> GetAuthzAsync(string key, Uri authzUri, string kid, CancellationToken tok) => PostAsync<AcmeAuthzResponse>(key, authzUri, null, kid, tok); | ||
| public Task<AcmeChallengeResponse> TriggerChallengeAsync(string key, Uri challengeUri, string kid, CancellationToken tok) => PostAsync<AcmeChallengeResponse>(key, challengeUri, new { }, kid, tok); | ||
|
|
||
| public static async Task ReadDirectoryAsync(KCertConfig cfg, CancellationToken tok) |
There was a problem hiding this comment.
Making this method static while the class has instance state (like '_nonce') creates inconsistent API design. The method should be instance-based to maintain proper state management.
| public string AcmeEabKeyId => GetRequiredString("Acme:EabKeyId"); | ||
| public string AcmeHmacKey => GetRequiredString("Acme:EabHmacKey"); |
There was a problem hiding this comment.
The UseEabKey property calls AcmeEabKeyId, which calls GetRequiredString and will throw an exception if the value is null or empty. This creates a circular dependency where checking if EAB is available throws an exception. Consider using GetString instead of GetRequiredString for the properties used in the UseEabKey check.
| public string AcmeEabKeyId => GetRequiredString("Acme:EabKeyId"); | |
| public string AcmeHmacKey => GetRequiredString("Acme:EabHmacKey"); | |
| public string AcmeEabKeyId => GetString("Acme:EabKeyId"); | |
| public string AcmeHmacKey => GetString("Acme:EabHmacKey"); |
|
|
||
| public string? SmtpEmailFrom => GetString("Smtp:EmailFrom"); | ||
| public string? SmtpHost => GetString("Smtp:Host"); | ||
| public bool SmtpEnabled => !string.IsNullOrEmpty(SmtpEmailFrom); |
There was a problem hiding this comment.
Similar to the EAB key issue, SmtpEnabled property calls SmtpEmailFrom which uses GetRequiredString and will throw an exception if the value is missing. This prevents checking if SMTP is enabled without throwing an exception. Consider using GetString for the check.
| public bool SmtpEnabled => !string.IsNullOrEmpty(SmtpEmailFrom); | |
| public bool SmtpEnabled => !string.IsNullOrEmpty(GetString("Smtp:EmailFrom")); |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| public string HandleChallenge(string token) | ||
| { | ||
| log.LogInformation("Received ACME Challenge: {token}", token); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the issue, sanitize the token string before logging it. Because logs are typically viewed as text, the primary concern is controlling newline and control characters. The simplest approach is to replace all carriage returns (\r) and linefeeds (\n) in the token string with empty strings before logging. This should be done inline in the call to LogInformation in Challenge/HttpChallengeProvider.cs line 15. No additional dependencies are required; use string.Replace methods. Only the line logging the token needs changing. No logic changes are needed, and the return value remains unsanitized (as it is part of the ACME protocol).
| @@ -12,7 +12,7 @@ | ||
|
|
||
| public string HandleChallenge(string token) | ||
| { | ||
| log.LogInformation("Received ACME Challenge: {token}", token); | ||
| log.LogInformation("Received ACME Challenge: {token}", token.Replace("\r", "").Replace("\n", "")); | ||
| var thumbprint = cert.GetThumbprint(); | ||
| return $"{token}.{thumbprint}"; | ||
| } |
| await SendAsync(subject, body, tok); | ||
| } | ||
|
|
||
| private async Task SendAsync(string subject, string text, CancellationToken tok) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
The best fix is to ensure that sensitive exception stack trace information is not sent via email to potentially untrusted recipients. Instead, log the full error message—including stack traces—securely on the server's logging infrastructure, and send only a generic message or, at most, the summary (ex.Message) in notification emails. For the affected areas:
- In
NotifyFailureAsync, replace the body construction to excludeex.StackTrace, and include only a brief error message. - In
RenewalMessage, remove or limit the exposure of exception information; exclude the stack trace and, optionally, the exception message, replacing with a generic error message. - Optionally, add a logging call so the stack trace is still preserved for server-side diagnostics (
log.LogError(ex, "Unhandled exception in NotifyFailureAsync")). - Avoid modifying email formatting or other logic.
Change lines 41, 72 (and possibly the logging around exception handling) in Services/EmailClient.cs.
| @@ -38,7 +38,8 @@ | ||
| } | ||
|
|
||
| var subject = "KCert encountered an unexpected error"; | ||
| var body = $"{message}\n\n{ex.Message}\n\n{ex.StackTrace}"; | ||
| log.LogError(ex, "Unhandled exception in NotifyFailureAsync: {Message}", message); | ||
| var body = $"{message}\n\nError: {ex.Message}"; | ||
| await SendAsync(subject, body, tok); | ||
| } | ||
|
|
||
| @@ -69,7 +70,7 @@ | ||
| { | ||
| lines.Add("\nLogs:\n"); | ||
| lines.Add(string.Join('\n', ex.Logs)); | ||
| lines.Add($"Error:\n\n{ex.Message}\n\n{ex.StackTrace}"); | ||
| lines.Add($"Error:\n\n{ex.Message}"); | ||
| } | ||
|
|
||
| return string.Join('\n', lines); |
| { | ||
| await prev; | ||
| tok.ThrowIfCancellationRequested(); | ||
| log.LogInformation("Starting renewal process for secret {ns}/{secretName} with hosts {hosts}", ns, secretName, string.Join(", ", hosts)); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To mitigate log forging, sanitize all user input before it is logged. In this code base, the problematic field is the ns parameter, received from the URL and used in log fields. We should ensure that, prior to any log call, the ns value is normalized to remove newline or other control characters that could corrupt the log structure.
The most robust approach is to use a utility method that strips or replaces dangerous characters (\r, \n, etc.) from the ns string. To avoid changing existing logic and maintain clarity, the sanitization should happen as close as possible to the logging call or before passing the value to the logger.
We should sanitize both ns and, for completeness, any other future user-provided values incorporated into logs as structured fields—here, the key focus is on ns. The best option is to have a local private static method (e.g., SanitizeForLog(string value)) to perform this, using String.Replace to remove or replace line endings.
Edit needed:
- In
Services/KCertClient.cs, before logging inStartRenewalProcessAsync, sanitize (assign to local) thensvalue. - Add a helper method
SanitizeForLogwithinKCertClient. - Use the sanitized
nsin the logging call.
No added dependencies are required: just use basic C# string manipulation.
| @@ -33,7 +33,8 @@ | ||
| await _semaphore.WaitAsync(tok); | ||
| try | ||
| { | ||
| log.LogInformation("Starting renewal process for secret {ns}/{secretName} with hosts {hosts}", ns, secretName, string.Join(", ", hosts)); | ||
| var sanitizedNs = SanitizeForLog(ns); | ||
| log.LogInformation("Starting renewal process for secret {ns}/{secretName} with hosts {hosts}", sanitizedNs, secretName, string.Join(", ", hosts)); | ||
| await RenewCertAsync(ns, secretName, hosts, tok); | ||
| } | ||
| finally | ||
| @@ -65,4 +66,10 @@ | ||
| log.LogError(ex, "Unexpected renewal failure"); | ||
| } | ||
| } | ||
| private static string SanitizeForLog(string value) | ||
| { | ||
| if (value == null) return null; | ||
| // Remove CR and LF to prevent log forging | ||
| return value.Replace("\r", "").Replace("\n", ""); | ||
| } | ||
| } |
| { | ||
| await prev; | ||
| tok.ThrowIfCancellationRequested(); | ||
| log.LogInformation("Starting renewal process for secret {ns}/{secretName} with hosts {hosts}", ns, secretName, string.Join(", ", hosts)); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the problem, before logging the potentially untrusted user input (secretName), sanitize it to remove newlines and carriage returns (and ideally any other problematic formatting). This is typically done by replacing \n and \r characters with empty strings to prevent log forging via multiline entries.
In this case, sanitize the secretName value just before logging, in StartRenewalProcessAsync, by using secretName.Replace("\r", "").Replace("\n", ""). Store the sanitized value in a local variable and pass that to log.LogInformation. Only the logging line (and potentially the lines immediately around it) need changing—other uses of secretName are likely not in log sinks and don't require modification.
No new dependencies are needed: we're using standard string methods. No other files or lines require modification.
| @@ -33,7 +33,8 @@ | ||
| await _semaphore.WaitAsync(tok); | ||
| try | ||
| { | ||
| log.LogInformation("Starting renewal process for secret {ns}/{secretName} with hosts {hosts}", ns, secretName, string.Join(", ", hosts)); | ||
| var sanitizedSecretName = secretName.Replace("\r", "").Replace("\n", ""); | ||
| log.LogInformation("Starting renewal process for secret {ns}/{secretName} with hosts {hosts}", ns, sanitizedSecretName, string.Join(", ", hosts)); | ||
| await RenewCertAsync(ns, secretName, hosts, tok); | ||
| } | ||
| finally |
No description provided.