Skip to content

MANTA-5521 Add optional scope parameter to CreateAccessKey, UpdateAccessKey, and DeleteAccessKey CloudAPI endpoints.#154

Open
cneira wants to merge 20 commits intomasterfrom
accesskey-per-bucket
Open

MANTA-5521 Add optional scope parameter to CreateAccessKey, UpdateAccessKey, and DeleteAccessKey CloudAPI endpoints.#154
cneira wants to merge 20 commits intomasterfrom
accesskey-per-bucket

Conversation

@cneira
Copy link
Copy Markdown

@cneira cneira commented Apr 28, 2026

Add optional JSON payload scope parameter to CloudAPI access key create/update/delete endpoints, that has the immediate effect of cache propagation to mahi/authcache redis.
The JSON payload for bucket scope is the following :

{
  "version": 1,
  "permissions": [
    { "bucket": "app-data", "level": "readwrite" },
    { "bucket": "logs-*", "level": "read" }
  ]
}

Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com

cneira and others added 15 commits April 13, 2026 15:29
Accept an optional scope parameter when creating or updating
access keys. The scope restricts the key to specific buckets
with configurable permission levels (read, readwrite, full).

Create: POST /:account/accesskeys with scope parameter
  Accepts full envelope {version,permissions} or shorthand array
  [{bucket,level},...]. Validates structure, bucket names (1-63
  chars), permission levels, and max 1000 entries.

Update: POST /:account/accesskeys/:id with scope parameter
  Replaces the scope. An empty string removes the scope entirely,
  making the key unrestricted again.

List/Get: translateAccessKey now returns scope as a parsed JSON
  object. When absent, scope is null (unrestricted key).

node-ufds addAccessKey and updateAccessKey already pass through
arbitrary attributes to UFDS, so no changes needed there.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add 50KB max scope JSON size validation in validateScope()
- Fix update endpoint to return HTTP 200 (not 201) for modifications

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Standardize validateScope to accept only the canonical envelope
object {version:1, permissions:[...]}. Remove shorthand array
auto-wrapping and JSON string parsing. Require version explicitly.
Add wildcard position check rejecting non-trailing wildcards.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace inline validateScope() with delegation to
scopeSchema.validateScope() from node-mahi. Remove local
MAX_SCOPE_ENTRIES and VALID_SCOPE_LEVELS constants.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Scoped access keys had a 2-10s replication delay (UFDS → replicator →
Redis) before becoming usable for S3 requests.  Add fire-and-forget
cachePush() calls after UFDS writes so scoped keys are immediately
available in Redis.  The replicator remains the authoritative fallback.

- create(): push scoped keys only (unscoped use UFDS read-through)
- update(): push when scope or status changes on scoped keys
- del(): scopeRevoke all keys (no scope info at delete time)
- Bump node-mahi 2.3.3 → 2.5.0 for cachePush/scopeRevoke methods

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sync lockfile with package.json mahi 2.5.0 dependency to fix
Jenkins npm ci failure. Preserves lockfileVersion 1.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The published mahi@2.5.0 on npm does not include scopeSchema,
cachePush, or scopeRevoke — those exist only in the node-mahi
repo. Pin to the git commit like manta-buckets-api does.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Previously only scoped keys were pushed to mahi Redis on creation.
Unscoped keys relied solely on the UFDS replicator, leaving a 2-15s
window where the key was not in Redis and auth would fail (since the
UFDS read-through in sigv4 was dead due to the ufdsPool/ufds property
name mismatch, now fixed in mahi).

Push all keys on creation for immediate availability.  The cost is one
fire-and-forget HTTP POST per key creation; the benefit is eliminating
the auth failure window for all keys, not just scoped ones.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The stale lockfile pinned ufds to the npm registry version (1.6.1),
overriding the git URL in package.json.  npm install resolved the
registry version instead of fetching commit f8ea6ad (v1.9.2) from
GitHub.  Regenerated with npm v6.4.1 on Node v6.17.1 to capture
the git resolution.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1. Create cachePush used hardcoded status 'Active' instead of
   the caller-provided status, causing Redis/UFDS disagreement
   when creating keys with status 'Inactive'.

2. Delete sent 204 before scopeRevoke completed, creating a
   security window where a deleted key remained usable in Redis.
   Now waits for scopeRevoke callback before responding.

3. Update cachePush did not guard against rawSecret being
   undefined (UFDS may strip sensitive attrs on update).
   Now skips cache-push and logs a warning if secret is missing.

4. Create scope check used truthiness (swallows 0/false/'')
   instead of !== undefined/null, inconsistent with update.

AI-generated code.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@cneira cneira changed the title Allow bucket-scoped access keys MANTA-5521 Add optional scope parameter to CreateAccessKey, UpdateAccessKey, and DeleteAccessKey CloudAPI endpoints. Apr 28, 2026
@cneira cneira requested a review from a team April 28, 2026 19:31
Copy link
Copy Markdown
Member

@travispaul travispaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly nits, but I think we want to update the docs/index.md file with the changes here.


log.debug('POST %s => %j', req.path(), accesskey);
res.send(201, accesskey);
res.send(200, accesskey);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof, thanks for fixing that.

Could you add this change as an entry to the version changelog here: https://github.com/TritonDataCenter/sdc-cloudapi/blob/master/docs/index.md#versions

Along with an entry for the new scope prop?

Also separately, could you add details about the new scope prop to the relevant endpoint docs here: https://github.com/TritonDataCenter/sdc-cloudapi/blob/master/docs/index.md#accesskeys

Comment thread package.json
"name": "cloudapi",
"description": "Triton CloudAPI",
"version": "9.21.0",
"version": "9.21.1",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non blocking nit (that I can be easily talked out of) but maybe we want a minor bump here since we're adding functionality?

Comment thread lib/endpoints/accesskeys.js Outdated
}

/*
* Update bucket scope. An empty string removes the
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment should read:

An empty string or null value removes the scope (makes the key unrestricted again)

Comment thread lib/endpoints/accesskeys.js
Copy link
Copy Markdown
Contributor

@danmcd danmcd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not approving this yet because:

  • Travis has open comments.
  • The too-narrow problem manifests here as well.

Comment thread lib/endpoints/accesskeys.js
Copy link
Copy Markdown
Contributor

@danmcd danmcd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, it's hard to tell which code lines are indented SO FAR IN that you can't really clean them up vs. ones that have tabs vs. spaces. I blame github for this. :)

Mostly set on this, however.

@cneira cneira requested review from danmcd and travispaul May 4, 2026 21:04
Copy link
Copy Markdown
Contributor

@danmcd danmcd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More + strings that don't need to be. Also, I see cachePush gets consumed here but it doesn't seem to be exported from node-mahi.

accesskeyid: params.accesskeyid
}, 'cache-push best-effort' +
' failed');
}, 'cache-push best-effort' + ' failed');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can just be:

}, 'cache-push best-effort failed');

Comment thread lib/endpoints/accesskeys.js Outdated
log.warn({err: pushErr,
accesskeyid:
accesskey.accesskeyid
}, 'cache-push best-effort' +
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compress and remove the +.

Comment thread lib/endpoints/accesskeys.js Outdated
log.warn({err: revokeErr,
accesskeyid:
req.params.accesskeyid
}, 'cache-revoke best-effort' +
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compress and remove the +.

@danmcd
Copy link
Copy Markdown
Contributor

danmcd commented May 7, 2026

As in MANTA-5522, I need to know why cachePush isn't being exported there, but consumed here? Knowing the situation that, plus knowing @travispaul is satisfied, will push me to +1.

@cneira
Copy link
Copy Markdown
Author

cneira commented May 7, 2026

, I see cachePush gets consumed here but it doesn't seem to be exported from node-mahi.

cachePush is not exported directly. It's defined as a method on MahiClient.prototype, that allows it to become available on the object returned by mahi.createClient because that object is a MahiClient instance.
https://github.com/TritonDataCenter/node-mahi/blob/5e85be39e333eded99db508a2009a4b17d2c4bdc/lib/client.js#L894

@cneira cneira requested a review from danmcd May 7, 2026 15:17
Copy link
Copy Markdown
Contributor

@danmcd danmcd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure @travispaul gets his things addressed, please, before pushing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants