-
Notifications
You must be signed in to change notification settings - Fork 1
Implement credential selection #43
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
Implement credential selection #43
Conversation
} | ||
AuthenticatorResponse::CredentialsAsserted(get_assertion_response) => { | ||
CredentialResponse::from_get_assertion( | ||
// TODO: Implement credential selection for hybrid |
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'm thinking about this now: I don't think we need credential selection for hybrid, since the authenticator is capable of displaying it's own UI. For hybrid, I think we can assume it's just returning one for now and just leave a comment in case some hybrid authenticator without a display ever appears in the wild.
.assertions | ||
.iter() | ||
.map(|x| Credential { | ||
id: x |
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.
This will need to be used as a handle for the UI to return which credential the user selected. So this should return an error (unwrap is fine for now) rather than mapping to a non-unique string.
Separately, I think we should use an opaque ID here so that the credential ID is not leaked to the (untrusted) UI code.
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.
Good catch regarding the <unknown>
-fallback!
I also hashed the ID now, before sending it.
Yeah, I put on a bunch of variables hoping to be able to derive the UI components declaratively from the current state, but raw GTK doesn't quite work that way, and I haven't put much effort into getting there. We could probably get rid of quite a few state variables since it's ultimately imperative. Also, up until recently, the UI code "owned" the data that would be sent back via D-Bus, but it doesn't anymore, and it properly belongs to the credential service now. So I think the selected credential field in the UI is doubly-irrelevant.
One thing to note is that the UI code is considered untrusted, so we should only send minimal data to the UI. I haven't considered the privacy implications of the credential IDs: whether the IDs would be any more useful to an attacker that has compromised the UI code than the username and other metadata already included. If none, sending the credential ID would probably be fine, though an opaque identifier could prevent mistakes like the UI returning a completely separate credential ID and somehow tricking the credential service into returning it. (E.g., if we used the same mechanism for selecting a credential out of the platform authenticator.) Sorry for all the confusion. All on me. Edit: Remove nonsense about misnamed stack page. |
Ah, scratch some of that. Now that I'm reading this on a laptop and not on mobile, I see more clearly what's going on. choose_credential is the correct view to use here. We don't need to store |
I hashed the credential ID now, before sending it to the UI. |
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.
This looks good, thank you!
Closes #30. |
Couple of notes:
selected_credential
, which I'm not sure is needed at all (see various todo-comments in this PR). I haven't removed all of that yet, as I wanted to have confirmation first, but I don't think it's used anywhere.