-
Notifications
You must be signed in to change notification settings - Fork 172
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 privacy considerations about credential IDs #1250
Conversation
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.
overall LGTM, tho there's subtleties wrt "sign-in" as compared to "re-authn" we probably ought to distinguish if we're going to go into this much use case detail....
Co-Authored-By: =JeffH <jdhodges@google.com>
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.
Alternative suggested text for the whole "Privacy leak via credential IDs" section:
Identifying the specific authenticators that are acceptable at the start of an authentication ceremony (in {{PublicKeyCredentialRequestOptions/allowCredentials}}) is necessary if any authenticator uses a non-resident credential, and is useful even in other cases to specify a preference order or transport hints. In these situations the credential ids are revealed before the user is authenticated (or at least before the user is fully authenticated). A credential id is effectively a random value specific to one authenticator and one account at one RP so revealing an id does not allow correlations across accounts or RPs. A given type of authenticator, though, is likely to create credential ids of the same length at each RP. And a given individual is likely to register the same authenticators at each RP. Consequently, for an individual with an unusually large number of authenticators that use unusual credential id lengths, that set of lengths is a slight privacy leak that could correlate different accounts.
One way to mitigate even that slight privacy leak is to include additional imaginary, but plausible, credential ids (see discussion in [[#sctn-username-enumeration]]) so no set of credential id lengths stands out.
@manger Thanks for the suggestion, I'll see if I can incorporate some of that. |
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 fine to me, although I'd probably also note that disclosing credential IDs allows someone with only momentary access to an authenticator to confirm guesses about the owner's identity. Or, if a database can be built, query many possibilities to establish their identity.
(E.g. try ssh whoami.filippo.io
for something somewhat similar.)
@agl Thanks, I added a mention of this. @manger Going through your proposal again it seems like the main points are already addressed by the text as currently written, so I ended up not making any changes to it. With that, unless anyone wants to re-review the addition of @agl's comment, I think we're good to merge as soon as @equalsJeffH finishes his review. |
"by only providing a username" may sound like it excludes the case where |
It does exclude that case - unless the "ambient credential" is just an unauthenticated username with no session key, which wouldn't really make much sense. |
There are many other ways to use "ambien credentials". For example, you could store a hash of credentialIDs instead of username. All these cases should be part of this privacy concern since no matter what you do, you end up sending credentialIDs for a get(). |
@maxhata I don't think I quite understand what you mean, but I changed the quoted sentence to
This is both more general by not emphasising a particular interaction flow, and more precise in what the problem is, and I think it should also address your concern, right? |
This is how it started the discussion. |
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 LGTM, thx @emlun !
Hm, looks I forgot that I branched PR #1270 off from this, and since that is now merged the diff here looks a bit strange. Sorry about the mess... |
are we waiting on merging this for @manger to approve? |
There's also a review requested from @akshayku. |
I need more time to see this. I started looking what we have specified regarding Credential ID privacy and there are more points to review related to username enumeration. I will finish this by TPAC. |
Fixes #1246. Fixes #1311.
Things to consider:
Preview | Diff