-
Notifications
You must be signed in to change notification settings - Fork 3
Tech Debt: Decompose ParameterConfiguration.cs (547 lines) into small... #113
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
Open
github-actions
wants to merge
3
commits into
master
Choose a base branch
from
agent/issue-108
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
149 changes: 149 additions & 0 deletions
149
....Health.Fhir.Anonymizer.Shared.Core/AnonymizerConfigurations/CryptographicKeyValidator.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,149 @@ | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Security; | ||
| using System.Text; | ||
| using Microsoft.Extensions.Logging; | ||
| using Microsoft.Health.Fhir.Anonymizer.Core.Exceptions; | ||
|
|
||
| namespace Microsoft.Health.Fhir.Anonymizer.Core.AnonymizerConfigurations | ||
| { | ||
| /// <summary> | ||
| /// Static utility class that encapsulates all cryptographic key security validation logic | ||
| /// for anonymization configuration parameters. | ||
| /// | ||
| /// SECURITY: This class enforces that keys are not placeholder values, not whitespace-only, | ||
| /// not obviously weak values, and meet minimum length/size requirements. | ||
| /// </summary> | ||
| public static class CryptographicKeyValidator | ||
| { | ||
| private static readonly ILogger s_logger = AnonymizerLogging.LoggerFactory.CreateLogger(typeof(CryptographicKeyValidator).FullName); | ||
|
|
||
| /// <summary> | ||
| /// Valid AES key sizes in bits. Used to validate EncryptKey without allocating an Aes instance. | ||
| /// AES supports 128-bit (16 bytes), 192-bit (24 bytes), and 256-bit (32 bytes) keys. | ||
| /// </summary> | ||
| private static readonly HashSet<int> s_validAesKeySizeBits = new HashSet<int> { 128, 192, 256 }; | ||
|
|
||
| /// <summary> | ||
| /// Dangerous placeholder patterns that must be rejected. | ||
| /// </summary> | ||
| private static readonly string[] s_dangerousPlaceholderPatterns = new[] | ||
| { | ||
| "$HMAC_KEY", | ||
| "YOUR_KEY_HERE", | ||
| "YOUR_SECURE_KEY", | ||
| "YOUR_ENCRYPTION_KEY", | ||
| "PLACEHOLDER", | ||
| "CHANGE_ME", | ||
| "CHANGEME", | ||
| "REPLACE_ME", | ||
| "EXAMPLE_KEY", | ||
| "TEST_KEY", | ||
| "SAMPLE_KEY", | ||
| "INSERT_KEY_HERE", | ||
| "<YOUR_KEY>", | ||
| "[YOUR_KEY]", | ||
| "{{YOUR_KEY}}", | ||
| "TODO", | ||
| "FIXME" | ||
| }; | ||
|
|
||
| /// <summary> | ||
| /// Validate a key parameter doesn't contain placeholder values or consist solely of whitespace. | ||
| /// SECURITY CRITICAL: Prevents use of example/template keys and whitespace-only values in production. | ||
| /// </summary> | ||
| /// <param name="keyValue">The key value to validate.</param> | ||
| /// <param name="parameterName">The configuration parameter name (for error messages).</param> | ||
| /// <param name="keyType">Human-readable key type description (for error messages).</param> | ||
| public static void ValidateKeyParameter(string keyValue, string parameterName, string keyType) | ||
| { | ||
| if (string.IsNullOrEmpty(keyValue)) | ||
| { | ||
| return; // Empty/null keys are allowed if the feature is not used | ||
| } | ||
|
|
||
| // SECURITY: Reject whitespace-only keys — they provide no entropy | ||
| if (string.IsNullOrWhiteSpace(keyValue)) | ||
| { | ||
| throw new SecurityException( | ||
| $"SECURITY ERROR: Whitespace-only {keyType} key detected in '{parameterName}'. " + | ||
| "A key consisting entirely of whitespace characters provides no entropy and must not be used. " + | ||
| "Generate a cryptographically secure random key using: openssl rand -base64 32"); | ||
| } | ||
|
|
||
| // Trim and convert to uppercase for case-insensitive comparison | ||
| var normalizedKey = keyValue.Trim().ToUpperInvariant(); | ||
|
|
||
| // Check against all dangerous placeholder patterns | ||
| foreach (var pattern in s_dangerousPlaceholderPatterns) | ||
| { | ||
| if (normalizedKey.Contains(pattern)) | ||
| { | ||
| throw new SecurityException( | ||
| $"SECURITY ERROR: Placeholder {keyType} key detected in '{parameterName}'.\n\n" + | ||
| $"The configuration contains a placeholder value ('{pattern}') that must be replaced " + | ||
| "with a cryptographically secure key before use.\n\n" + | ||
| "TO GENERATE A SECURE KEY:\n" + | ||
| " Linux/macOS: openssl rand -base64 32\n" + | ||
| " Windows: pwsh -Command \"[Convert]::ToBase64String((1..32 | ForEach-Object { Get-Random -Minimum 0 -Maximum 256 }))\"\n" + | ||
| " .NET: var key = Convert.ToBase64String(RandomNumberGenerator.GetBytes(32));\n\n" + | ||
| "SECURITY WARNING: Using placeholder keys in production:\n" + | ||
| " - Compromises cryptographic operations\n" + | ||
| " - May lead to predictable hash values\n" + | ||
| " - Enables re-identification attacks\n" + | ||
| " - Violates privacy guarantees\n\n" + | ||
| "BEST PRACTICES:\n" + | ||
| " - Never commit actual keys to version control\n" + | ||
| " - Use environment variables: Environment.GetEnvironmentVariable(\"CRYPTO_KEY\")\n" + | ||
| " - Use Azure Key Vault, AWS Secrets Manager, or similar for production\n" + | ||
| " - Rotate keys periodically according to your security policy\n" + | ||
| " - Use different keys for different environments (dev/staging/production)\n"); | ||
| } | ||
| } | ||
|
|
||
| // Additional checks for weak or test keys | ||
| if (keyValue.Length < 16) | ||
| { | ||
| s_logger.LogWarning( | ||
| $"The {keyType} key in '{parameterName}' is very short ({keyValue.Length} characters). " + | ||
| "Recommended minimum is 32 bytes (44 characters in Base64). " + | ||
| "Short keys provide inadequate security and may be vulnerable to brute force attacks."); | ||
| } | ||
|
|
||
| // Check for obviously weak patterns | ||
| if (keyValue.Equals("12345678", StringComparison.Ordinal) || | ||
| keyValue.Equals("password", StringComparison.OrdinalIgnoreCase) || | ||
| keyValue.Equals("secret", StringComparison.OrdinalIgnoreCase) || | ||
| keyValue.Equals("key", StringComparison.OrdinalIgnoreCase) || | ||
| keyValue.All(c => c == keyValue[0])) // All same character | ||
| { | ||
| throw new SecurityException( | ||
| $"SECURITY ERROR: Weak {keyType} key detected in '{parameterName}'. " + | ||
| "The key appears to be a common weak value (e.g., 'password', '12345678', repeated characters). " + | ||
| "Generate a cryptographically secure random key using: openssl rand -base64 32"); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Validate that the encrypt key size is a valid AES key size (128, 192, or 256 bits). | ||
| /// Uses a static HashSet of valid sizes to avoid allocating an Aes instance on every call. | ||
| /// Only validates when encryptKey is non-null and non-empty. | ||
| /// </summary> | ||
| /// <param name="encryptKey">The encryption key string to validate.</param> | ||
| public static void ValidateEncryptKeySize(string encryptKey) | ||
| { | ||
| if (string.IsNullOrEmpty(encryptKey)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var encryptKeySize = Encoding.UTF8.GetByteCount(encryptKey) * 8; | ||
| if (!s_validAesKeySizeBits.Contains(encryptKeySize)) | ||
| { | ||
| throw new AnonymizerConfigurationException( | ||
| $"Invalid encrypt key size : {encryptKeySize} bits! Please provide key sizes of 128, 192 or 256 bits."); | ||
| } | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
🔒 Security (medium): The static logger in CryptographicKeyValidator is created with the wrong generic type argument: AnonymizerLogging.CreateLoggerParameterConfiguration(). This means all log entries from CryptographicKeyValidator will be categorized under the 'ParameterConfiguration' logger category. While not a direct data-exposure vulnerability, this creates misleading audit/log trails — security-related key validation warnings will appear attributed to ParameterConfiguration rather than CryptographicKeyValida...
💡 Change to AnonymizerLogging.CreateLoggerCryptographicKeyValidator() so log categories correctly reflect the source component.