-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add device trust specific verbs #18397
Conversation
Friendly ping @fspmarshall ? |
b691523
to
984ac2e
Compare
984ac2e
to
6d52ed3
Compare
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.
Couple questions. Sorry if they are answered elsewhere. Glanced at the RFD wasn't super obvious to me:
-
Why is
create_enroll_token
being used instead of using the existing create verb with adevice_enroll_token
resource? We already have an established pattern for<token-type>: create
being the standard way creating tokens works. -
What is the difference between being allowed to create an enroll token, and actually enrolling a device? Typically if I want to add a node, I don't need
node: create
, I just need the permission to create the node joining token. I'm having difficulty imagining the scenario where we need both of these permissions as separate entities, rather than a single permission (eitherdevice: enroll
ordevice_enroll_token: create
). Shouldn't holding the token prove that I'm allowed to enroll the device?
6d52ed3
to
b161022
Compare
That's a good point. Happy to backtrack and change that, although I'll likely land this PR first, if that's alright.
Good questions, let me try to unpack them: Create enroll token vs enroll device: creating an enrollment token is a high privilege operation, whereas enrolling a device doesn't have to be (up to the Teleport customer). This allows a scenario where a device manager register new devices as they are acquired and creates enrollment tokens as they are handed out to users, but the users themselves perform the enrollment. In this case one could give the One could argue that Possession of enrollment token vs permission to enroll: while I did describe a scenario above where the end user does the enrollment, the security properties of the system are greatly enhanced if a trusted user performs device enrollment. In this case, the role that grants It may be that, in practice, the flexibility of the separate verbs is actually not needed, but the goal was to allow a variety of workflows with the same base system. |
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.
@codingllama Makes sense. Approving, but I do have a suggestion:
Using device: enroll
to limit the subset of users that are allowed to enroll devices feels like a pretty weak control to me. Only really protects you from some subset of users that are never supposed to be given an enroll token being given that enroll token. I'd be very surprised if this had much effect in practice, as most organizations that use device trust will probably just end up giving device: enroll
to all or nearly all users.
I'd suggest that a simpler and far more powerful control would be to let admins specify either username or role when creating the token, so that a given token could have its use limited to the target audience. IMO this would be both simpler to use (no need to update the user's role if the user needs to start using device trust), and much more secure (leaked token would be useless to any other user).
@fspmarshall thanks for the suggestion. I won't take action immediately, but I'll take note to follow up. |
Add verbs specific to Device Trust ("create_enroll_token" and "enroll") and change the editor preset to have full device administration powers.
I'll be conservative here and not retrofit permissions into editor, a fact to be called out in our to-be-written user-facing documentation.
https://github.com/gravitational/teleport.e/issues/514