-
Notifications
You must be signed in to change notification settings - Fork 14
MANTA-5485 Add support for STS and IAM #27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: Claude <[email protected]>
- Add complete STS implementation with AssumeRole/GetSessionToken - Implement v1.1 JWT session tokens with secure key rotation - Add temporary credential generation and session token validation - Enhance SigV4 auth to handle STS-issued temporary credentials - Add UFDS integration for credential storage and lookup - Implement payload-based key identification for rotation support - Add multi-secret verification with configurable grace periods Generated with Claude Code Co-Authored-By: Claude <[email protected]>
danmcd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checkpoint, continue on lib/server/server.js
| return; | ||
| } | ||
|
|
||
| var accesskeyid = changes.accesskeyid[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned over in #25, do these inputs that aren't changes.status need any further sanitizing or checking apart from "exists"? Or do they get passed along to entities that sanitized or check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my current understanding, when access keys(or any other object) are replicated from the redis cache, UFDS will enforce the schema constraints for the object and fail to add the record if they are not valid. So we should not have inconsistent records. But, when mahi store policies, the policies are only validated when an assumerole call tries to use them.
| if (!modEntry._owner) { | ||
| cb(new Error('_owner is required')); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same questions about sanitizing or checking here.
| name: name, | ||
| account: account, | ||
| policies: memberpolicy | ||
| policies: memberpolicy, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this percolate through to non-S3 functions of mahi and more? If so are they ignored? Also if so, a non-S3-blessed user can't scribble things in here for nefarious purposes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The property added for this STS work is assumerolepolicydocument. This is a new property that current UFDS clients are not using except mahi. So I think today is not a problem.
|
Also does this impact node-mahi#10 ? |
@danmcd I'm not sure about node-mahi but this will likely impact TritonDataCenter/sdc-cloudapi/pull/153, TritonDataCenter/node-triton/pull/350 and we might also need to make some changes to node-ufds since the temporary keys are using the same Update after testing today: Will open a PR for node-ufds and update the PRs mentioned above to account for the temporary keys. Nothing is broken and nothing here will depend upon those changes but I'd like the user-facing side to be a little nicer. |
danmcd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incremental checkpoint. Thanks for using xxd(1) of /dev/random.
danmcd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops, premature approval, sorry.
danmcd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checkpoint, resuming with lib/server/session-token.js
| @@ -0,0 +1,323 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
License & copyright?
| svccfg import $SVC_ROOT/smf/manifests/mahi-redis.xml | ||
| svcadm enable redis | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copyright update above needed?
lib/server/server.js
Outdated
| * | ||
| * @error 401 Authentication required if caller headers missing | ||
| * | ||
| * @since 1.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silly question: These @note, @param.... tags: these are generated, or input for something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are for doxygen, with those tags the doxygen parser can generate documentation. I don't know if we are using another method to document the source code. But, if there is one, I'll change the comments to accomodate the one that's in use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like a lot of Triton repos use JSDoc, but I don't think we really do anything with it, and jsdoc is only listed as a dev dependency in a few places.
What is here looks to be the same format though, or is at least close enough to compatible:
$ npm install [email protected] # Need an older version to work with node v6.17.1
$ ./node_modules/.bin/jsdoc --verbose lib/server/server.js
Parsing /home/travis/mahi/lib/server/server.js ...
Generating output files...
Finished running in 0.59 seconds.
My only nit is that I think @since 1.0.0 should be @since 2.1.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add distinct access key prefixes for STS temporary credentials: - MSTS: GetSessionToken credentials (cannot call IAM APIs) - MSAR: AssumeRole credentials (can call IAM APIs)
| cb(null, batch); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should modifications to temporary access keys be permitted? If so, we may need to account for the following scenario:
User had a temporary access key:
$ triton accesskey get MSTSA88BD414A9F51D44
ACCESSKEYID STATUS CREDENTIALTYPE UPDATED
MSTSA88BD414A9F51D44 Active temporary 2025-12-08T17:42:55.256Z
It is in Mahi, as expected:
[root@6d8f09c5 (authcache) ~]$ curl -is "http://localhost/accounts/930896af-bf8c-48d4-885c-6573a94b1853" | json -Ha
{
...
"MSTSA88BD414A9F51D44": {
"secret": "tdc_GxOjeStlKuPPakJBwgAkOQSl4qIpuROyISKPmFVhvDvr4w8Q",
"type": "temporary",
"expiration": "2025-12-08T18:40:40.539Z",
"sessionToken": "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1dWlkIjoiOTMwODk2YWYtYmY4Yy00OGQ0LTg4NWMtNjU3M2E5NGIxODUzIiwicm9sZUFybiI6ImFybjphd3M6c3RzOjo5MzA4OTZhZi1iZjhjLTQ4ZDQtODg1Yy02NTczYTk0YjE4NTM6c2Vzc2lvbiIsInNlc3Npb25OYW1lIjoic2Vzc2lvbi0xNzY1MjE1NjQwNTM5IiwidG9rZW5UeXBlIjoic3RzLXNlc3Npb24iLCJ0b2tlblZlcnNpb24iOiIxLjEiLCJrZXlJZCI6ImluaXRpYWwtMjAyNTEyMDUtMTY1MDM4IiwiaXNzIjoibWFudGEtbWFoaSIsImF1ZCI6Im1hbnRhLXMzIiwiaWF0IjoxNzY1MjE1NjQwLCJleHAiOjE3NjUyMTkyNDAsIm5iZiI6MTc2NTIxNTY0MH0.HgXi8mD1BC45ykGL0bw-36gq-Xr7LaRTpHLhooDjW8k",
"principalUuid": "930896af-bf8c-48d4-885c-6573a94b1853",
"assumedRole": null
},
...
}
User changed the status to Inactive:
$ triton accesskey update MSTSA88BD414A9F51D44 status=Inactive
accesskey update MSTSA88BD414A9F51D44 status=Inactive
Updated access key MSTSA88BD414A9F51D44 (fields: status)
The key gets removed from Mahi as the status is non-Active.
This seems correct and fine to me, so far.
However, if the user changes the status back to Active:
$ triton accesskey update MSTSA88BD414A9F51D44 status=Active
Updated access key MSTSA88BD414A9F51D44 (fields: status)
Then it gets added back to Mahi the same way as a permanent key without the additional details that a temporary key has:
[root@6d8f09c5 (authcache) ~]$ curl -is "http://localhost/accounts/930896af-bf8c-48d4-885c-6573a94b1853" | json -Ha
{
...
"MSTSA88BD414A9F51D44": "tdc_GxOjeStlKuPPakJBwgAkOQSl4qIpuROyISKPmFVhvDvr4w8Q"
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! We definitely don't need to allow modifications to temporary access keys.
Added Secure Token Service (STS) and Identity and Access Management ( Roles and Policies).
This change adds a couple of new endpoints to generate temporary keys and associate Roles and Policies, temporary access keys expire and are required to use a session token to be authenticated by mahi using Sigv4.
This is what this PR implements:
the arn:aws|manta|triton:iam::UUID:role/name was kept using the same AWS format so is easier to migrate policies between Manta S3 and AWS.
I'm doing this PR as draft as it includes components that I need to be sure that's the right approach before submitting.
This change is tied to sdc-ufds as the schema for access keys to reflect the temporary ones is done in that repo.