Move UIN hash pepper to SSM instead of secret#449
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds SSM Parameter Store as a configuration source: new Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application Startup
participant Refresh as refreshSecretConfig()
participant SM as AWS Secrets Manager
participant SSM as AWS SSM Parameter Store
participant Config as app.secretConfig
App->>Refresh: Initialize configuration
rect rgb(240,248,255)
Note over Refresh: Fetch secrets (existing flow)
Refresh->>SM: Get secrets (ConfigurationSecretIds)
SM-->>Refresh: allSecrets[]
end
rect rgb(250,245,230)
Note over Refresh: Fetch SSM parameters (new)
Refresh->>SSM: For each ConfigurationParameterIds -> getSsmParameter()
SSM-->>Refresh: allParameters[] (compact-key/value)
end
rect rgb(240,255,240)
Note over Refresh: Merge and set config
Refresh->>Refresh: Combine allSecrets + allParameters -> allConfig
Refresh->>Config: Reduce allConfig -> app.secretConfig
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
💰 Infracost reportMonthly estimate generatedThis comment will be updated when code changes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
utils/setSsmParameter.py (3)
59-59: Use explicit Optional type annotation.The function signature uses implicit
Optionalwhich is prohibited by PEP 484. Update to use explicit union syntax.🔎 Proposed fix
-def put_parameter_multi_region(name: str, value: str, regions: list[str] = None) -> list[dict]: +def put_parameter_multi_region(name: str, value: str, regions: list[str] | None = None) -> list[dict]:
64-68: Concurrent execution could exceed AWS rate limits.The
ThreadPoolExecutorwithmax_workers=len(target_regions)creates one thread per region. With the default 2 regions this is fine, but if users override with many regions, this could trigger AWS API rate limits forPutParameteroperations.Consider adding a configurable max_workers limit (e.g., 5) to prevent rate limit issues when working with many regions.
🔎 Proposed fix
- with ThreadPoolExecutor(max_workers=len(target_regions)) as executor: + # Limit concurrent workers to avoid AWS rate limits + max_workers = min(5, len(target_regions)) + with ThreadPoolExecutor(max_workers=max_workers) as executor:
89-90: Consider adding confirmation for sensitive operations.The script uses
getpassto securely read the parameter value (good!), but it immediately proceeds with multi-region updates without confirmation. For sensitive parameters like UIN hash pepper, consider adding a confirmation prompt showing which regions will be updated.🔎 Example implementation
value = getpass.getpass("Enter parameter value: ") if not value: print("Error: Parameter value cannot be empty") sys.exit(1) target_regions = args.regions or REGIONS + + # Confirmation prompt for safety + print(f"\nThis will update '{args.name}' in: {', '.join(target_regions)}") + confirm = input("Continue? (yes/no): ").lower() + if confirm != "yes": + print("Aborted") + sys.exit(0) + print(f"\nSetting parameter '{args.name}' in {len(target_regions)} region(s)...\n")utils/README.md (1)
1-7: Add usage examples and prerequisites.The documentation is minimal. Consider adding:
- Prerequisites (AWS credentials, Python dependencies)
- Usage example with actual command syntax
- Link to the Python script
🔎 Enhanced documentation example
# Utils Random useful scripts ## Create Multiregion Parameter -Create the same AWS SSM ParameterStore Secure string in all regions we need. +Create the same AWS SSM ParameterStore SecureString parameter across multiple regions. + +### Prerequisites +- Python 3.9+ with boto3 +- AWS credentials configured (AWS CLI or environment variables) +- Appropriate IAM permissions for SSM PutParameter and KMS Decrypt + +### Usage + +```bash +# Create parameter in default regions (us-east-2, us-west-2) +python setSsmParameter.py /infra-core-api/uin-pepper + +# Override regions +python setSsmParameter.py /infra-core-api/uin-pepper --regions us-east-1 us-west-1 +``` + +The script will prompt for the parameter value securely (not displayed or logged).src/api/utils.ts (2)
46-50: Remove redundant| undefinedfrom optional parameter.The
?optional marker already makes the typeSSMClient | undefined, so the explicit| undefinedis redundant.🔎 Proposed fix
type GetSsmParameterInputs = { parameterName: string; logger: ValidLoggers; - ssmClient?: SSMClient | undefined; + ssmClient?: SSMClient; };
57-62: Consider validating parameter name format.AWS SSM parameter names must follow specific rules (start with
/, valid characters, max length). Consider adding validation to fail fast with a clear error message rather than making an API call that will fail.🔎 Example validation
export const getSsmParameter = async ({ parameterName, logger, ssmClient, }: GetSsmParameterInputs) => { + // Validate parameter name format + if (!parameterName.startsWith('/')) { + logger.error(`Invalid parameter name: ${parameterName} (must start with /)`); + throw new InternalServerError({ message: "Invalid parameter name format" }); + } + const client = ssmClient || new SSMClient({});
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
src/api/index.tssrc/api/package.jsonsrc/api/utils.tssrc/common/config.tsterraform/modules/lambdas/main.tfutils/README.mdutils/setSsmParameter.py
🧰 Additional context used
🧬 Code graph analysis (2)
src/api/index.ts (1)
src/api/utils.ts (1)
getSsmParameter(52-77)
src/api/utils.ts (3)
src/api/types.d.ts (1)
ValidLoggers(14-14)src/api/sqs/logger.ts (1)
logger(2-2)src/common/errors/index.ts (1)
InternalServerError(74-86)
🪛 Ruff (0.14.10)
utils/setSsmParameter.py
51-51: Do not catch blind exception: Exception
(BLE001)
59-59: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Application
- GitHub Check: Run Unit Tests
🔇 Additional comments (7)
terraform/modules/lambdas/main.tf (2)
212-215: Secrets Manager reference remains for uin-pepper.The IAM policy still grants access to
infra-core-api-uin-pepperin Secrets Manager (line 214), but the PR title indicates moving the UIN hash pepper to SSM. This suggests either:
- A gradual migration is planned (SSM added, Secrets Manager to be removed later)
- Both locations will be supported for backward compatibility
Please clarify the migration strategy and consider adding a TODO comment or tracking issue if the Secrets Manager permission is intended to be removed in a future PR.
193-208: Remove unused SSM action permissions to follow least-privilege principle.The KMS resource ARN for the AWS-managed SSM key is correctly formatted. However, the SSM policy statement grants three actions (
ssm:GetParameter,ssm:GetParameters,ssm:GetParametersByPath) when onlyssm:GetParameter(singular) is actually used in the codebase. Remove the unusedssm:GetParametersandssm:GetParametersByPathactions:Action = [ "ssm:GetParameter" ],utils/setSsmParameter.py (1)
21-56: LGTM - Robust error handling for regional parameter updates.The function correctly handles AWS-specific exceptions and returns structured results. The broad exception catch on line 51 is acceptable here since it's in an error-recording context where all exceptions need to be captured and returned to the caller.
src/api/package.json (1)
18-19: LGTM - Consistent AWS SDK versions.The AWS SDK dependencies are updated to version 3.922.0 consistently, and the new
@aws-sdk/client-ssmdependency supports the SSM parameter retrieval functionality.Also applies to: 26-26
src/api/utils.ts (1)
7-7: No action needed: BASE_RETRY_DELAY is actively used in the exponential backoff calculation.The
BASE_RETRY_DELAYconstant at line 7 is used at line 32 in theretryDynamoTransactionWithBackofffunction:const exponentialDelay = BASE_RETRY_DELAY * 2 ** attempt;. Since this is a new file where both the constant and the function are introduced together, they form an intended, cohesive implementation for retry logic with exponential backoff.Likely an incorrect or invalid review comment.
src/api/index.ts (1)
65-66: LGTM: Imports are correct.The new imports for SSM functionality are properly added and will be used in the
refreshSecretConfigfunction.src/common/config.ts (1)
29-29: LGTM: New configuration field added correctly.The
ConfigurationParameterIdsfield is properly typed and integrated into theConfigTypeinterface.
1 participant
Summary by CodeRabbit
New Features
Tools
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.