Skip to content

Commit 61e4d4b

Browse files
bert-ebert-e
committed
Merge branch 'improvement/S3UTILS-222/fix-crr-policy' into tmp/octopus/w/1/improvement/S3UTILS-222/fix-crr-policy
2 parents 1294cab + 92d0e98 commit 61e4d4b

File tree

9 files changed

+1310
-45
lines changed

9 files changed

+1310
-45
lines changed

‎.claude/skills/review-pr/SKILL.md‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -59,10 +59,10 @@ gh pr diff <number> --repo <owner/repo>
5959
For each specific issue, post a comment on the exact file and line:
6060

6161
```bash
62-
gh api -X POST -H "Accept: application/vnd.github+json" "repos/<owner/repo>/pulls/<number>/comments" -f body=$'Your comment\n\n— Claude Code' -f path="path/to/file" -F line=<line_number> -f side="RIGHT" -f commit_id="<headRefOid>"
62+
gh api -X POST -H "Accept: application/vnd.github+json" "repos/<owner/repo>/pulls/<number>/comments" -f body="Your comment<br><br>— Claude Code" -f path="path/to/file" -F line=<line_number> -f side="RIGHT" -f commit_id="<headRefOid>"
6363
```
6464

65-
**The command must stay on a single bash line.** Use `$'...'` quoting for the `-f body=` value, with `\n` for line breaks. Never use `<br>` — it renders as literal text inside code blocks and suggestion blocks.
65+
**The command must stay on a single bash line.** Never use newlines in bash commands — use `<br>` for line breaks in comment bodies. Never put `<br>` inside code blocks or suggestion blocks — use `\n` within `$'...'` quoting only for content inside fenced code blocks.
6666

6767
Each inline comment must:
6868
- Be short and direct — say what's wrong, why it's wrong, and how to fix it in 1-3 sentences
@@ -76,7 +76,7 @@ Each inline comment must:
7676
Only suggest when you can show the exact replacement. For architectural or design issues, just describe the problem.
7777
Example with a suggestion block:
7878
```bash
79-
gh api ... -f body=$'Missing the shared-guidelines update command.\n\n```suggestion\n/plugin update shared-guidelines@scality-agent-hub\n/plugin update scality-skills@scality-agent-hub\n```\n\n— Claude Code' ...
79+
gh api ... -f body=$'Missing the shared-guidelines update command.<br><br>```suggestion\n/plugin update shared-guidelines@scality-agent-hub\n/plugin update scality-skills@scality-agent-hub\n```<br><br>— Claude Code' ...
8080
```
8181
- Escape single quotes inside `$'...'` as `\'` (e.g., `don\'t`)
8282
- End with: `— Claude Code`
@@ -86,10 +86,10 @@ Use the line number from the **new version** of the file (the line number you'd
8686
#### Part B: Summary comment
8787

8888
```bash
89-
gh pr comment <number> --repo <owner/repo> --body $'LGTM\n\nReview by Claude Code'
89+
gh pr comment <number> --repo <owner/repo> --body "LGTM<br><br>Review by Claude Code"
9090
```
9191

92-
**The command must stay on a single bash line.** Use `$'...'` quoting with `\n` for line breaks.
92+
**The command must stay on a single bash line.** Never use newlines in bash commands — use `<br>` for line breaks in comment bodies. Never put `<br>` inside code blocks or suggestion blocks.
9393

9494
Do not describe or summarize the PR. For each issue, state the problem on one line, then list one or more suggestions below it:
9595

‎.github/workflows/dependency-review.yaml‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,3 +14,10 @@ jobs:
1414

1515
- name: 'Dependency Review'
1616
uses: actions/dependency-review-action@v4
17+
with:
18+
# aws-sdk v2 is intentionally used as a devDependency because the
19+
# fix-missing-replication-permissions script runs inside the vault
20+
# container of older S3C versions (pre-9.5.2) where only v2 is
21+
# available. This low-severity advisory is informational (no patch
22+
# exists, v2 is end-of-life).
23+
allow-ghsas: GHSA-j965-2qgj-vjmq

‎package.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "s3utils",
3-
"version": "1.17.4",
3+
"version": "1.17.5",
44
"engines": {
55
"node": ">= 22"
66
},
@@ -55,6 +55,7 @@
5555
},
5656
"devDependencies": {
5757
"@aws-sdk/client-iam": "^3.962.0",
58+
"aws-sdk": "^2.1692.0",
5859
"@scality/eslint-config-scality": "scality/Guidelines#8.3.0",
5960
"@sinonjs/fake-timers": "^14.0.0",
6061
"eslint": "^9.14.0",

‎replicationAudit/README.md‎

Lines changed: 217 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,15 @@
11
# TL;DR Complete Workflow Example
22

3-
Here's a complete example running the two scripts and audit the IAM policies used by CRR:
3+
Here's a complete example running the two scripts and audit the IAM policies used by CRR.
4+
5+
Use the fix-missing-replication-permissions.js script (step 7) to correct any missing permissions found by the check script. If the fix script fails with "missing ownerDisplayName", re-run check-replication-permissions.js — this field was added in s3utils 1.17.5.
46

57
From your local machine: copy scripts to the supervisor
68

79
```bash
810
scp replicationAudit/list-buckets-with-replication.sh root@<supervisor-ip>:/root/
911
scp replicationAudit/check-replication-permissions.js root@<supervisor-ip>:/root/
12+
scp replicationAudit/fix-missing-replication-permissions.js root@<supervisor-ip>:/root/
1013
```
1114

1215
Connect to the supervisor
@@ -26,6 +29,8 @@ ansible -i env/$ENV_DIR/inventory runners_s3[0] -m copy \
2629
-a 'src=/root/list-buckets-with-replication.sh dest=/root/'
2730
ansible -i env/$ENV_DIR/inventory runners_s3[0] -m copy \
2831
-a 'src=/root/check-replication-permissions.js dest={{ env_host_logs}}/scality-vault{{ container_name_suffix | default("")}}/logs'
32+
ansible -i env/$ENV_DIR/inventory runners_s3[0] -m copy \
33+
-a 'src=/root/fix-missing-replication-permissions.js dest={{ env_host_logs}}/scality-vault{{ container_name_suffix | default("")}}/logs'
2934

3035
# Step 2: Run list-buckets-with-replication.sh
3136
ansible -i env/$ENV_DIR/inventory runners_s3[0] -m shell \
@@ -41,7 +46,7 @@ ansible -i env/$ENV_DIR/inventory md1-cluster1 -m shell \
4146
LEADER_IP=<leader-ip-from-step-3>
4247

4348
ansible -i env/$ENV_DIR/inventory runners_s3[0] -m shell \
44-
-a "mv /root/buckets-with-replication.json {{ env_host_logs}}/scality-vault{{ container_name_suffix | default("")}}/logs && \
49+
-a "cp /root/buckets-with-replication.json {{ env_host_logs}}/scality-vault{{ container_name_suffix | default("")}}/logs && \
4550
ctrctl exec scality-vault{{ container_name_suffix | default("")}} node /logs/check-replication-permissions.js \
4651
/logs/buckets-with-replication.json $LEADER_IP /logs/missing.json"
4752

@@ -50,12 +55,35 @@ ansible -i env/$ENV_DIR/inventory runners_s3[0] -m shell \
5055
-a 'cat {{ env_host_logs}}/scality-vault{{ container_name_suffix | default("")}}/logs/missing.json' \
5156
| grep -v CHANGED | tee /root/replicationAudit_missing.json
5257

53-
# Step 6: Clean up
58+
# Step 6: Fix missing permissions
59+
# Copy admin credentials and missing.json to vault container and run inside it
60+
ansible -i env/$ENV_DIR/inventory runners_s3[0] -m copy \
61+
-a "src=/srv/scality/s3/s3-offline/federation/env/$ENV_DIR/vault/admin-clientprofile/admin1.json dest={{ env_host_logs}}/scality-vault{{ container_name_suffix | default("")}}/logs"
62+
63+
ansible -i env/$ENV_DIR/inventory runners_s3[0] -m copy \
64+
-a 'src=/root/replicationAudit_missing.json dest={{ env_host_logs}}/scality-vault{{ container_name_suffix | default("")}}/logs/missing.json'
65+
66+
ansible -i env/$ENV_DIR/inventory runners_s3[0] -m shell \
67+
-a 'ctrctl exec scality-vault{{ container_name_suffix | default("")}} env NODE_PATH=/home/scality/vault/node_modules node /logs/fix-missing-replication-permissions.js \
68+
/logs/missing.json localhost /logs/admin1.json /logs/replication-fix-results.json'
69+
70+
# Retrieve fix results
71+
ansible -i env/$ENV_DIR/inventory runners_s3[0] -m shell \
72+
-a 'cat {{ env_host_logs}}/scality-vault{{ container_name_suffix | default("")}}/logs/replication-fix-results.json' \
73+
| grep -v CHANGED | tee /root/replicationAudit_fix_results.json
74+
75+
# Step 7: Re-run check to verify fixes (repeat steps 3-5)
76+
77+
# Step 8: Clean up remote files
5478
ansible -i env/$ENV_DIR/inventory runners_s3[0] -m shell \
5579
-a 'rm -f {{ env_host_logs}}/scality-vault{{ container_name_suffix | default("")}}/logs/missing.json \
5680
{{ env_host_logs}}/scality-vault{{ container_name_suffix | default("")}}/logs/check-replication-permissions.js \
81+
{{ env_host_logs}}/scality-vault{{ container_name_suffix | default("")}}/logs/fix-missing-replication-permissions.js \
5782
{{ env_host_logs}}/scality-vault{{ container_name_suffix | default("")}}/logs/buckets-with-replication.json \
58-
/root/list-buckets-with-replication.sh'
83+
{{ env_host_logs}}/scality-vault{{ container_name_suffix | default("")}}/logs/replication-fix-results.json \
84+
{{ env_host_logs}}/scality-vault{{ container_name_suffix | default("")}}/logs/admin1.json \
85+
/root/list-buckets-with-replication.sh \
86+
/root/buckets-with-replication.json'
5987
```
6088

6189
# Scripts Documentation
@@ -285,6 +313,13 @@ node check-replication-permissions.js [input-file] [leader-ip] [output-file] [--
285313

286314
### Output Format
287315

316+
> **Breaking change (since 1.17.5):** The output now includes `ownerDisplayName`
317+
> in each result entry. This field is required by
318+
> `fix-missing-replication-permissions.js` to identify accounts without an
319+
> extra API call. If you ran `check-replication-permissions.js` on version
320+
> 1.17.4 or earlier, **re-run it** to produce an output that
321+
> `fix-missing-replication-permissions.js` can consume.
322+
288323
The script produces a JSON file with metadata and results. The `results` array
289324
contains **only buckets missing the `s3:ReplicateObject` permission**.
290325

@@ -310,6 +345,7 @@ contains **only buckets missing the `s3:ReplicateObject` permission**.
310345
"results": [
311346
{
312347
"bucket": "bucket-old-1",
348+
"ownerDisplayName": "testaccount",
313349
"sourceRole": "arn:aws:iam::267390090509:role/crr-role-outdated",
314350
"policies": [
315351
{
@@ -461,3 +497,180 @@ Output saved to: /tmp/missing.json
461497
**Script timeout**
462498

463499
- For many buckets, run directly on the S3 connector node via interactive SSH
500+
501+
---
502+
503+
## fix-missing-replication-permissions.js
504+
505+
Reads the output of `check-replication-permissions.js` and creates IAM policies
506+
with `s3:ReplicateObject` for roles that are missing it.
507+
508+
The script applies **minimal changes**: one policy per bucket, with an explicit
509+
Statement ID (`AllowReplicateObjectAuditFix`) so the policies are easily
510+
identifiable later. Using one policy per bucket makes re-runs truly idempotent:
511+
if the policy already exists, its document is guaranteed to be identical.
512+
513+
### Prerequisites
514+
515+
- Output from `check-replication-permissions.js` (missing.json)
516+
- Vault admin credentials (`admin1.json` with `accessKey` and `secretKeyValue`).
517+
Found on the supervisor at:
518+
```
519+
/srv/scality/s3/s3-offline/federation/env/<ENV_DIR>/vault/admin-clientprofile/admin1.json
520+
```
521+
- The script runs inside the vault container (`scality-vault`), which has `vaultclient` and `aws-sdk` pre-installed
522+
523+
### Usage
524+
525+
```bash
526+
node fix-missing-replication-permissions.js <input-file> <vault-host> <admin-config> [output-file] [options]
527+
```
528+
529+
| Argument | Default | Description |
530+
|----------|---------|-------------|
531+
| `input-file` | (required) | Path to missing.json from check script |
532+
| `vault-host` | (required) | Vault admin host (use `localhost` when running inside the vault container) |
533+
| `admin-config` | (required) | Path to admin credentials JSON |
534+
| `output-file` | replication-fix-results.json | Output file path |
535+
| `--iam-port <port>` | 8600 | Vault admin and IAM API port |
536+
| `--https` | (not set) | Use HTTPS to connect to Vault |
537+
| `--dry-run` | (not set) | Show what would be done without making changes |
538+
539+
### How It Works
540+
541+
1. **Reads** the missing permissions file and enriches each entry with parsed role fields
542+
2. **Maps** account IDs to names using `ownerDisplayName` from the input (no API call)
543+
3. For each bucket entry (grouped by account for credential reuse):
544+
- **Generates** a temporary access key via vault admin API (15-minute auto-expiry)
545+
- **Creates** one IAM policy per bucket with `s3:ReplicateObject`
546+
- **Attaches** the policy to the role
547+
- **Deletes** the temporary access key (falls back to auto-expiry on failure)
548+
4. **Writes** results to the output file
549+
550+
### Policy Created
551+
552+
For each bucket, the script creates a policy named
553+
`s3-replication-audit-fix-<bucketName>`:
554+
555+
```json
556+
{
557+
"Version": "2012-10-17",
558+
"Statement": [{
559+
"Sid": "AllowReplicateObjectAuditFix",
560+
"Effect": "Allow",
561+
"Action": "s3:ReplicateObject",
562+
"Resource": "arn:aws:s3:::bucket-old-1/*"
563+
}]
564+
}
565+
```
566+
567+
### Output Format
568+
569+
```json
570+
{
571+
"metadata": {
572+
"timestamp": "2026-02-23T20:35:00.000Z",
573+
"durationMs": 65,
574+
"inputFile": "missing.json",
575+
"dryRun": false,
576+
"counts": {
577+
"totalBucketsProcessed": 3,
578+
"totalBucketsFixed": 3,
579+
"policiesCreated": 3,
580+
"policiesAttached": 3,
581+
"keysCreated": 1,
582+
"keysDeleted": 1,
583+
"errors": 0
584+
}
585+
},
586+
"fixes": [
587+
{
588+
"accountId": "267390090509",
589+
"accountName": "testaccount",
590+
"roleName": "crr-role-outdated",
591+
"roleArn": "arn:aws:iam::267390090509:role/crr-role-outdated",
592+
"policyName": "s3-replication-audit-fix-bucket-old-1",
593+
"policyArn": "arn:aws:iam::267390090509:policy/s3-replication-audit-fix-bucket-old-1",
594+
"bucket": "bucket-old-1",
595+
"status": "success"
596+
},
597+
{
598+
"accountId": "267390090509",
599+
"accountName": "testaccount",
600+
"roleName": "crr-role-outdated",
601+
"roleArn": "arn:aws:iam::267390090509:role/crr-role-outdated",
602+
"policyName": "s3-replication-audit-fix-bucket-old-2",
603+
"policyArn": "arn:aws:iam::267390090509:policy/s3-replication-audit-fix-bucket-old-2",
604+
"bucket": "bucket-old-2",
605+
"status": "success"
606+
}
607+
],
608+
"errors": []
609+
}
610+
```
611+
612+
### Example Run
613+
614+
```
615+
=== Fix Missing Replication Permissions ===
616+
Input: missing.json
617+
Output: replication-fix-results.json
618+
Vault/IAM: 13.50.166.21:8600
619+
620+
Processing 3 bucket(s)
621+
622+
[1/3] Bucket "bucket-old-1" — role "crr-role-outdated" — account "testaccount"
623+
Created policy "s3-replication-audit-fix-bucket-old-1"
624+
Attached policy to role "crr-role-outdated"
625+
[2/3] Bucket "bucket-old-2" — role "crr-role-outdated" — account "testaccount"
626+
Created policy "s3-replication-audit-fix-bucket-old-2"
627+
Attached policy to role "crr-role-outdated"
628+
[3/3] Bucket "bucket-old-3" — role "crr-role-outdated" — account "testaccount"
629+
Created policy "s3-replication-audit-fix-bucket-old-3"
630+
Attached policy to role "crr-role-outdated"
631+
Deleted temp key for account "testaccount" (267390090509)
632+
633+
=== Summary ===
634+
Buckets processed: 3
635+
Buckets fixed: 3
636+
Policies created: 3
637+
Policies attached: 3
638+
Keys created: 1
639+
Keys deleted: 1
640+
Errors: 0
641+
Duration: 0.7s
642+
Output saved to: replication-fix-results.json
643+
644+
Done.
645+
```
646+
647+
### Idempotency
648+
649+
The script is safe to run multiple times:
650+
651+
- Each bucket has its own policy, so if it already exists the document is
652+
guaranteed to be identical — `EntityAlreadyExists` is a true no-op
653+
- Attaching an already-attached policy is a no-op in IAM
654+
- Temporary access keys auto-expire after 15 minutes even if deletion fails
655+
656+
### Troubleshooting
657+
658+
**"No ownerDisplayName found for account"**
659+
660+
- The input file is missing `ownerDisplayName`. Re-run `check-replication-permissions.js`
661+
to generate a fresh output that includes this field.
662+
663+
**"Failed to generate temp key"**
664+
665+
- Verify admin credentials in the config file
666+
- Ensure vault admin API is reachable on the specified host and port
667+
668+
**IAM operation errors**
669+
670+
- Check that the IAM port is correct (default 8600, may differ per deployment)
671+
- Verify the role still exists in vault
672+
673+
**"Failed to delete temp key"**
674+
675+
- Non-critical: the key auto-expires after 15 minutes
676+
- The error is logged but does not prevent other operations

‎replicationAudit/check-replication-permissions.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -421,7 +421,7 @@ async function main() {
421421

422422
// Process each bucket
423423
for (let i = 0; i < buckets.length; i++) {
424-
const { bucket, sourceRole } = buckets[i];
424+
const { bucket, sourceRole, ownerDisplayName } = buckets[i];
425425

426426
if (!sourceRole) {
427427
logProgress(i + 1, buckets.length, bucket, 'SKIP (no role)');
@@ -440,12 +440,12 @@ async function main() {
440440
} else {
441441
const reason = result.error || 's3:ReplicateObject';
442442
logProgress(i + 1, buckets.length, bucket, `MISSING: ${reason}`);
443-
results.push(result);
443+
results.push({ ...result, ownerDisplayName });
444444
stats.missing++;
445445
}
446446
} catch (e) {
447447
logProgress(i + 1, buckets.length, bucket, `ERROR: ${e.message}`);
448-
results.push({ bucket, sourceRole, error: e.message, policies: [] });
448+
results.push({ bucket, sourceRole, ownerDisplayName, error: e.message, policies: [] });
449449
stats.errors++;
450450
}
451451
}

0 commit comments

Comments
 (0)