S3UTILS-222 Tool to update CRR policies#375
S3UTILS-222 Tool to update CRR policies#375nicolas2bert wants to merge 7 commits intodevelopment/1.17from
Conversation
Hello nicolas2bert,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| httpAgent: new http.Agent({ keepAlive: true }), | ||
| httpsAgent: new https.Agent({ | ||
| keepAlive: true, | ||
| rejectUnauthorized: false, |
Check failure
Code scanning / CodeQL
Disabling certificate validation High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, the fix is to stop disabling TLS certificate validation and either (a) rely on the default system trust store, or (b) explicitly configure a trusted CA/certificate instead of setting rejectUnauthorized: false. This keeps HTTPS secure while still allowing connections to custom or self‑signed endpoints when configured.
In this file, the single best change is to remove rejectUnauthorized: false from the https.Agent used by NodeHttpHandler inside createIAMClient, and (without altering existing behavior for standard environments) add optional support for a custom CA certificate path passed in through the existing config object. If config.caCertPath is provided, we read that file and pass its contents as the ca option to https.Agent. If it’s not provided, we construct a default agent without rejectUnauthorized: false, so certificate validation uses Node’s default trust store. This keeps existing functionality (HTTPS vs HTTP controlled by config.useHttps, endpoint/port logic unchanged), while making TLS properly validated and still supporting custom CAs when explicitly configured.
Concretely, in replicationAudit/fix-missing-replication-permissions.js:
- Add a small helper to load an optional CA from a path (using the already‑imported
fs) or just inline thefs.readFileSynccall when building the agent. - Modify the
httpsAgentconstruction increateIAMClient:- Remove the
rejectUnauthorized: falseline. - Optionally add
ca: fs.readFileSync(config.caCertPath)only ifconfig.caCertPathis set.
No new imports are needed, sincefs,http, andhttpsare already required at the top of the file.
- Remove the
| @@ -121,18 +121,22 @@ | ||
| /** Create an IAM client for a given account */ | ||
| function createIAMClient(config, accessKeyId, secretKey) { | ||
| const protocol = config.useHttps ? 'https' : 'http'; | ||
| const httpsAgentOptions = { | ||
| keepAlive: true, | ||
| }; | ||
| // If a custom CA certificate path is provided in config, use it to | ||
| // validate the HTTPS connection instead of disabling verification. | ||
| if (config.caCertPath) { | ||
| httpsAgentOptions.ca = fs.readFileSync(config.caCertPath); | ||
| } | ||
|
|
||
| return new IAMClient({ | ||
| region: 'us-east-1', | ||
| endpoint: `${protocol}://${config.vaultHost}:${config.iamPort}`, | ||
| credentials: { accessKeyId, secretAccessKey: secretKey }, | ||
| requestHandler: new NodeHttpHandler({ | ||
| httpAgent: new http.Agent({ keepAlive: true }), | ||
| // TBD: rejectUnauthorized: false disables certificate validation. | ||
| // Consider accepting a CA cert path via CLI option instead. | ||
| httpsAgent: new https.Agent({ | ||
| keepAlive: true, | ||
| rejectUnauthorized: false, | ||
| }), | ||
| httpsAgent: new https.Agent(httpsAgentOptions), | ||
| }), | ||
| }); | ||
| } |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return { | ||
| inputFile, | ||
| vaultHost, | ||
| vaultAdminPort: 8600, |
There was a problem hiding this comment.
vaultAdminPort is hardcoded to 8600, but iamPort is configurable via --iam-port. If someone passes --iam-port 9600, IAM operations go to 9600 while generateAccountAccessKey still targets 8600. Consider reusing iamPort here or adding a --vault-port flag. — Claude Code
|
Two issues found:
Review by Claude Code |
nicolas2bert
force-pushed
the
improvement/S3UTILS-222/check-replication-permissions
branch
from
dd53dc3 to
341df13
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development/1.17 #375 +/- ##
====================================================
- Coverage 43.62% 42.67% -0.96%
====================================================
Files 84 85 +1
Lines 5973 6106 +133
Branches 1255 1269 +14
====================================================
Hits 2606 2606
- Misses 3321 3453 +132
- Partials 46 47 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nicolas2bert
force-pushed
the
improvement/S3UTILS-222/check-replication-permissions
branch
from
341df13 to
d329715
Compare
…t bodies The allowed-tools glob pattern Bash(gh pr comment *) fails to match when the command contains literal newlines from $'...' quoting. Switch to <br> for line breaks in comment bodies (except inside code blocks and suggestion blocks) to keep commands on a single line.
Review by Claude Code |
|
LGTM |
|
@copilot /review-pr |
|
@nicolas2bert I've opened a new pull request, #376, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
LGTM |
Switching from one policy per role (covering multiple buckets) to one policy per bucket eliminates stale-policy issues on re-runs: since the policy document is always identical for a given bucket, EntityAlreadyExists becomes a true no-op with no version management needed.
nicolas2bert
force-pushed
the
improvement/S3UTILS-222/check-replication-permissions
branch
from
c612530 to
4d361a5
Compare
|
LGTM |
- Avoid mutating checkBucketPermissions result object; use spread instead - Add comment about missing pagination in test cleanup
|
LGTM |
|
|
||
| for (let i = 0; i < entries.length; i++) { | ||
| const entry = entries[i]; | ||
| const { accountId, roleName } = parseRoleArn(entry.sourceRole); |
There was a problem hiding this comment.
parseRoleArn is called outside the try block (which starts at line 250). A malformed sourceRole in any entry will crash the entire script mid-run instead of logging the error and continuing to the next entry. Move it inside the try block so the existing catch handler records the failure in outcome.errors and processes remaining entries.
— Claude Code
|
- Align test input with real check script output: remove destinationRole/missingActions, add policies array - Idempotent test: verify no extra audit-fix policies on the role - Key cleanup test: snapshot access keys before/after to verify temp keys are actually deleted, not just counted - Clarify TBD comment about idempotency via policy name check
|
LGTM |
No description provided.