Skip to content

Commit c612530

Browse files
committed
S3UTILS-222 use one IAM policy per bucket instead of per role
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.
1 parent 04f34aa commit c612530

File tree

3 files changed

+104
-110
lines changed

3 files changed

+104
-110
lines changed

replicationAudit/README.md

Lines changed: 40 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -484,9 +484,10 @@ Output saved to: /tmp/missing.json
484484
Reads the output of `check-replication-permissions.js` and creates IAM policies
485485
with `s3:ReplicateObject` for roles that are missing it.
486486

487-
The script applies **minimal changes**: one policy per role (covering all affected
488-
buckets), with an explicit Statement ID (`AllowReplicateObjectAuditFix`) so the
489-
policies are easily identifiable later.
487+
The script applies **minimal changes**: one policy per bucket, with an explicit
488+
Statement ID (`AllowReplicateObjectAuditFix`) so the policies are easily
489+
identifiable later. Using one policy per bucket makes re-runs truly idempotent:
490+
if the policy already exists, its document is guaranteed to be identical.
490491

491492
### Prerequisites
492493

@@ -517,19 +518,19 @@ node fix-missing-replication-permissions.js <input-file> <vault-host> <admin-con
517518

518519
### How It Works
519520

520-
1. **Reads** the missing permissions file and groups buckets by account and role
521+
1. **Reads** the missing permissions file and enriches each entry with parsed role fields
521522
2. **Maps** account IDs to names using `ownerDisplayName` from the input (no API call)
522-
3. For each account:
523+
3. For each bucket entry (grouped by account for credential reuse):
523524
- **Generates** a temporary access key via vault admin API (15-minute auto-expiry)
524-
- **Creates** one IAM policy per role with `s3:ReplicateObject` for all affected buckets
525+
- **Creates** one IAM policy per bucket with `s3:ReplicateObject`
525526
- **Attaches** the policy to the role
526527
- **Deletes** the temporary access key (falls back to auto-expiry on failure)
527528
4. **Writes** results to the output file
528529

529530
### Policy Created
530531

531-
For each role, the script creates a single policy named
532-
`s3-replication-audit-fix-<roleName>`:
532+
For each bucket, the script creates a policy named
533+
`s3-replication-audit-fix-<bucketName>`:
533534

534535
```json
535536
{
@@ -538,10 +539,7 @@ For each role, the script creates a single policy named
538539
"Sid": "AllowReplicateObjectAuditFix",
539540
"Effect": "Allow",
540541
"Action": "s3:ReplicateObject",
541-
"Resource": [
542-
"arn:aws:s3:::bucket-old-1/*",
543-
"arn:aws:s3:::bucket-old-2/*"
544-
]
542+
"Resource": "arn:aws:s3:::bucket-old-1/*"
545543
}]
546544
}
547545
```
@@ -556,10 +554,10 @@ For each role, the script creates a single policy named
556554
"inputFile": "missing.json",
557555
"dryRun": false,
558556
"counts": {
559-
"totalRolesProcessed": 1,
557+
"totalBucketsProcessed": 3,
560558
"totalBucketsFixed": 3,
561-
"policiesCreated": 1,
562-
"policiesAttached": 1,
559+
"policiesCreated": 3,
560+
"policiesAttached": 3,
563561
"keysCreated": 1,
564562
"keysDeleted": 1,
565563
"errors": 0
@@ -571,9 +569,19 @@ For each role, the script creates a single policy named
571569
"accountName": "testaccount",
572570
"roleName": "crr-role-outdated",
573571
"roleArn": "arn:aws:iam::267390090509:role/crr-role-outdated",
574-
"policyName": "s3-replication-audit-fix-crr-role-outdated",
575-
"policyArn": "arn:aws:iam::267390090509:policy/s3-replication-audit-fix-crr-role-outdated",
576-
"buckets": ["bucket-old-1", "bucket-old-2", "bucket-old-3"],
572+
"policyName": "s3-replication-audit-fix-bucket-old-1",
573+
"policyArn": "arn:aws:iam::267390090509:policy/s3-replication-audit-fix-bucket-old-1",
574+
"bucket": "bucket-old-1",
575+
"status": "success"
576+
},
577+
{
578+
"accountId": "267390090509",
579+
"accountName": "testaccount",
580+
"roleName": "crr-role-outdated",
581+
"roleArn": "arn:aws:iam::267390090509:role/crr-role-outdated",
582+
"policyName": "s3-replication-audit-fix-bucket-old-2",
583+
"policyArn": "arn:aws:iam::267390090509:policy/s3-replication-audit-fix-bucket-old-2",
584+
"bucket": "bucket-old-2",
577585
"status": "success"
578586
}
579587
],
@@ -589,18 +597,24 @@ Input: missing.json
589597
Output: replication-fix-results.json
590598
Vault/IAM: 13.50.166.21:8600
591599
592-
Processing 1 role(s)
600+
Processing 3 bucket(s)
593601
594-
[1/1] Role "crr-role-outdated" — account "testaccount" (3 bucket(s))
595-
Created policy "s3-replication-audit-fix-crr-role-outdated"
602+
[1/3] Bucket "bucket-old-1" — role "crr-role-outdated" — account "testaccount"
603+
Created policy "s3-replication-audit-fix-bucket-old-1"
604+
Attached policy to role "crr-role-outdated"
605+
[2/3] Bucket "bucket-old-2" — role "crr-role-outdated" — account "testaccount"
606+
Created policy "s3-replication-audit-fix-bucket-old-2"
607+
Attached policy to role "crr-role-outdated"
608+
[3/3] Bucket "bucket-old-3" — role "crr-role-outdated" — account "testaccount"
609+
Created policy "s3-replication-audit-fix-bucket-old-3"
596610
Attached policy to role "crr-role-outdated"
597611
Deleted temp key for account "testaccount" (267390090509)
598612
599613
=== Summary ===
600-
Roles processed: 1
614+
Buckets processed: 3
601615
Buckets fixed: 3
602-
Policies created: 1
603-
Policies attached: 1
616+
Policies created: 3
617+
Policies attached: 3
604618
Keys created: 1
605619
Keys deleted: 1
606620
Errors: 0
@@ -614,7 +628,8 @@ Done.
614628

615629
The script is safe to run multiple times:
616630

617-
- If the policy already exists, it is reused (not duplicated)
631+
- Each bucket has its own policy, so if it already exists the document is
632+
guaranteed to be identical — `EntityAlreadyExists` is a true no-op
618633
- Attaching an already-attached policy is a no-op in IAM
619634
- Temporary access keys auto-expire after 15 minutes even if deletion fails
620635

replicationAudit/fix-missing-replication-permissions.js

Lines changed: 26 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -83,45 +83,15 @@ function parseRoleArn(arn) {
8383
return { accountId: match[1], roleName: match[2] };
8484
}
8585

86-
/**
87-
* Group missing entries by role, collecting all affected buckets per role.
88-
*
89-
* Input (results from check-replication-permissions.js):
90-
* [
91-
* { bucket: "bucket-old-1", ownerDisplayName: "testaccount", sourceRole: "arn:aws:iam::123:role/crr-role" },
92-
* { bucket: "bucket-old-2", ownerDisplayName: "testaccount", sourceRole: "arn:aws:iam::123:role/crr-role" },
93-
* ]
94-
*
95-
* Output:
96-
* [
97-
* { accountId: "123", accountName: "testaccount", roleName: "crr-role",
98-
* roleArn: "arn:aws:iam::123:role/crr-role", buckets: ["bucket-old-1", "bucket-old-2"] }
99-
* ]
100-
*/
101-
function groupByRole(results) {
102-
const roles = Object.groupBy(results, entry => entry.sourceRole);
103-
104-
return Object.entries(roles).map(([roleArn, entries]) => {
105-
const { accountId, roleName } = parseRoleArn(roleArn);
106-
return {
107-
accountId,
108-
accountName: entries[0].ownerDisplayName,
109-
roleName,
110-
roleArn,
111-
buckets: entries.map(e => e.bucket),
112-
};
113-
});
114-
}
115-
116-
/** Build the IAM policy document */
117-
function buildPolicyDocument(buckets) {
86+
/** Build the IAM policy document for a single bucket */
87+
function buildPolicyDocument(bucket) {
11888
return {
11989
Version: '2012-10-17',
12090
Statement: [{
12191
Sid: STATEMENT_ID,
12292
Effect: 'Allow',
12393
Action: 's3:ReplicateObject',
124-
Resource: buckets.map(b => `arn:aws:s3:::${b}/*`),
94+
Resource: `arn:aws:s3:::${bucket}/*`,
12595
}],
12696
};
12797
}
@@ -190,13 +160,11 @@ async function main() {
190160
process.exit(1);
191161
}
192162

193-
// Group by role, sorted by account so roles in the same account are
194-
// processed consecutively — reduces the chance of cached credentials
195-
// expiring while other accounts are being processed.
196-
const roles = groupByRole(entries)
197-
.sort((a, b) => (a.accountId < b.accountId ? -1 : 1));
163+
// Sort by sourceRole so entries in the same account are processed
164+
// consecutively, maximising credential cache hits.
165+
entries.sort((a, b) => (a.sourceRole < b.sourceRole ? -1 : 1));
198166

199-
log(`Processing ${roles.length} role(s)`);
167+
log(`Processing ${entries.length} bucket(s)`);
200168
log('');
201169

202170
// Read admin credentials
@@ -223,7 +191,7 @@ async function main() {
223191
inputFile: config.inputFile,
224192
dryRun: config.dryRun,
225193
counts: {
226-
totalRolesProcessed: roles.length,
194+
totalBucketsProcessed: entries.length,
227195
totalBucketsFixed: 0,
228196
policiesCreated: 0,
229197
policiesAttached: 0,
@@ -236,33 +204,34 @@ async function main() {
236204
errors: [],
237205
};
238206

239-
// Cache IAM client per account to reuse connections across roles
207+
// Cache IAM client per account to reuse connections across buckets
240208
// and for cleanup (key deletion).
241209
// Map<accountId, { accountName, accessKeyId, iamClient }>
242210
const accountCache = new Map();
243211

244-
for (let i = 0; i < roles.length; i++) {
245-
const { accountId, accountName, roleName, roleArn, buckets } = roles[i];
246-
const policyName = `${POLICY_PREFIX}-${roleName}`;
247-
const policyDocument = buildPolicyDocument(buckets);
212+
for (let i = 0; i < entries.length; i++) {
213+
const entry = entries[i];
214+
const { accountId, roleName } = parseRoleArn(entry.sourceRole);
215+
const { bucket, ownerDisplayName: accountName } = entry;
216+
const policyName = `${POLICY_PREFIX}-${bucket}`;
217+
const policyDocument = buildPolicyDocument(bucket);
248218

249-
log(`[${i + 1}/${roles.length}] Role "${roleName}" — account "${accountName}" (${buckets.length} bucket(s))`);
219+
log(`[${i + 1}/${entries.length}] Bucket "${bucket}" — role "${roleName}" — account "${accountName}"`);
250220

251221
const fix = {
252222
accountId,
253223
accountName,
254224
roleName,
255-
roleArn,
225+
roleArn: entry.sourceRole,
256226
policyName,
257-
buckets,
227+
bucket,
258228
status: 'pending',
259229
};
260230

261231
if (config.dryRun) {
262232
fix.status = 'dry-run';
263233
log(` [DRY-RUN] Would create policy "${policyName}"`);
264234
log(` [DRY-RUN] Would attach to role "${roleName}"`);
265-
log(` Buckets: ${buckets.join(', ')}`);
266235
outcome.fixes.push(fix);
267236
continue;
268237
}
@@ -284,9 +253,10 @@ async function main() {
284253

285254
const { iamClient } = accountCache.get(accountId);
286255

287-
// Idempotent/safe to re-run: CreatePolicy reuses an existing policy
288-
// with the same name, and AttachRolePolicy is a no-op if
289-
// the policy is already attached to the role.
256+
// Idempotent/safe to re-run: each bucket has its own policy,
257+
// so EntityAlreadyExists means the exact same policy document
258+
// already exists — a true no-op. AttachRolePolicy is also
259+
// a no-op if the policy is already attached to the role.
290260
let policyArn;
291261
try {
292262
const resp = await iamClient.send(new CreatePolicyCommand({
@@ -300,7 +270,7 @@ async function main() {
300270
if (err.name === 'EntityAlreadyExistsException'
301271
|| err.Code === 'EntityAlreadyExists') {
302272
policyArn = `arn:aws:iam::${accountId}:policy/${policyName}`;
303-
log(` Policy "${policyName}" already exists, reusing`);
273+
log(` Policy "${policyName}" already exists, skipping`);
304274
} else {
305275
throw err;
306276
}
@@ -316,7 +286,7 @@ async function main() {
316286
log(` Attached policy to role "${roleName}"`);
317287

318288
fix.status = 'success';
319-
outcome.metadata.counts.totalBucketsFixed += buckets.length;
289+
outcome.metadata.counts.totalBucketsFixed++;
320290
} catch (err) {
321291
fix.status = 'error';
322292
fix.error = err.message;
@@ -325,6 +295,7 @@ async function main() {
325295
accountId,
326296
accountName,
327297
roleName,
298+
bucket,
328299
policyName,
329300
message: err.message,
330301
});
@@ -356,7 +327,7 @@ async function main() {
356327

357328
// Print summary
358329
log('\n=== Summary ===');
359-
log(`Roles processed: ${outcome.metadata.counts.totalRolesProcessed}`);
330+
log(`Buckets processed: ${outcome.metadata.counts.totalBucketsProcessed}`);
360331
log(`Buckets fixed: ${outcome.metadata.counts.totalBucketsFixed}`);
361332
log(`Policies created: ${outcome.metadata.counts.policiesCreated}`);
362333
log(`Policies attached: ${outcome.metadata.counts.policiesAttached}`);

0 commit comments

Comments
 (0)