-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add protobuf definition of DelegationProfile resource
#60253
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
| // Resources that will be accessible in sessions created using this profile. | ||
| // | ||
| // In the Resource ID format: /<cluster>/<kind>/<name>/<sub resource name> | ||
| repeated string required_resources = 1; |
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.
I went for the string representation here instead of types.ResourceID to make the YAML representation more easily editable.
Not sure if there's a better way to do that? Maybe at the marshal/unmarshal function level?
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.
Overriding the marshal/unmarshal for proto generated structs feels like a bit of an anti-pattern - we've definitely had some issues with that before.
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.
FYI, we are likely moving away from the old resource id to this new json format:
https://github.com/gravitational/teleport/pull/59288/files
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.
Thanks for the heads-up, @greedy52! The JSON-based format discussed in that RFD isn't very human-writable either, but they're apparently changing tack and the old slash-delimited format will keep working (see: #59288 (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.
thanks. mcp-tools can be set as constraints tho, similar to logins, db_users
(sorry for carrying that discussion here. i didn't know about constraints stuff when reviewing the delegation RFD).
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.
just to throw in my 2 cents, using the serialized /<cluster>/<kind>/<name> string path format here feels like a potential foot-gun w/r/t forwards-compatibility...
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.
Hmmm, okay maybe it's worth having our own user-facing representation that "compiles down" to AllowedResourceIDs or ConstrainedResourceID?
Something like:
required_resources:
- kind: mcp
name: github
tools: [merge_pull_request]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.
i like the idea. it is more user-friendly than reusing ConstrainedResourceID but downside you have to maintain this separately. one small nit maybe mcp_tools instead of tools? like
required_resources:
- kind: mcp
name: github
mcp_tools: [merge_pull_request]
- kind: db
name: postgres
db_users: [teleport-admin]
db_names: [sales]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.
Approved pending final RFD approvals.
See: RFD 0029e