Skip to content
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

PublicKeyCredentialUserEntity difference between name, displayName and id not clear #622

Closed
jovasco opened this issue Oct 10, 2017 · 15 comments · Fixed by #666
Closed

PublicKeyCredentialUserEntity difference between name, displayName and id not clear #622

jovasco opened this issue Oct 10, 2017 · 15 comments · Fixed by #666
Assignees
Labels
privacy-tracker Group bringing to attention of Privacy, or tracked by the Privacy Group but not needing response. stat:Discuss stat:pr-open type:editorial

Comments

@jovasco
Copy link
Contributor

jovasco commented Oct 10, 2017

For name:

name, of type DOMString

A human-friendly identifier for the entity. For example, this could be a company name for a Relying Party, or a user’s name. This identifier is intended for display.

for displayName:

displayName, of type DOMString

A friendly name for the user account (e.g., "John P. Smith").

Both say they are friendly identifiers and are intended for display (one explicitly, the other by its name).

When displaying account selection, a unique displayable name is required so the user can always differentiate between his different accounts.

So name should be the login name, unique for the relying party and suitable for display.
displayName should just be a pretty printing name, uniqueness is not required.

for id:

id, of type BufferSource

A unique identifier for the user account entity. This is a reference to an opaque byte array value specified by the Relying Party. The maximum size of this array is 64 bytes.

id is not intended for display, yet the only field marked as a unique identifier.

@emlun
Copy link
Member

emlun commented Oct 10, 2017

Are two separate "names" even needed when we have id for both the User and RP variants?

@jyasskin
Copy link
Member

@emlun I believe the user.id field won't in general be human-readable, while user.name is likely to be something like "[email protected]".

rp.id is a domain name, so it is probably human-readable similar to user.name, and I believe rp.name is intended to be a pretty name, like user.displayName.

@emlun
Copy link
Member

emlun commented Oct 11, 2017

Yes, but then what's the point of user.name if we also have user.displayName? Couldn't "[email protected]" just as well go in user.id?

@jovasco
Copy link
Contributor Author

jovasco commented Oct 12, 2017

@emlun No. User.id was added by Google and contains a binary lookup key for their user database. It is not human readable and not intended to be.

As I understand it, Name is supposed to be the login name as used traditionally. Display Name is just a pretty printing name.

In an authenticator with a display or in the case of multiple accounts in a first factor situation (see CTAP), we need a unique, printable identifier to show to the user that uniquely identifies this credential (ie, the traditional user name which is instantly recognizable to the user). The displayName could be the pretty printing name of this account.

For example, if a user has multiple accounts on gmail:

{
    id : "00000001",
    name:  "[email protected]",
    displayName:  "John Doe",
},
{
    id : "00000002",
    name:  "[email protected]",
    displayName:  "John Doe",
}

User.id is a relying party specific field that is basically a database lookup field.

None of this is obvious from the current language.

@emlun
Copy link
Member

emlun commented Oct 12, 2017

Ok. But then if user.name uniquely identifies the user, why is user.idnecessary?

@emlun
Copy link
Member

emlun commented Oct 12, 2017

Oh right, I think I can answer that myself. It's to support first factor login while making it harder for an attacker with a stolen authenticator to identify the authenticator's user, right? And that's also why you shouldn't put something identifying like an email address in user.id.

But wait, aren't you saying that the UI for selecting a first factor credential will display the name and/or displayName to the user? Then what would prevent the attacker from identifying the user by simply trying a bunch of first factor authentication challenges?

@emlun
Copy link
Member

emlun commented Oct 12, 2017

I'll answer myself again: What prevents the attacker is the assumption that the authenticator will verify the user's identity before giving up the stored account info. Context: PR #558

@jovasco
Copy link
Contributor Author

jovasco commented Oct 12, 2017

As far as I can tell, it is only there to make Google's life easier.

It does not make it harder in a first factor situation because knowing the user name is not necessary for an attacker with the authenticator to use it as a first factor credential. He can just log in with each offered credential and figure out the user name that way.

It could conceivably be an additional privacy protection against eavesdropping on the authenticator traffic because the username is not passed along although I have been repeatedly told eavesdroppers are considered out of scope. It sounds thin to me.

Using the user.id as only valid unique identifier makes it too easy to spoof the user.name field and mislead the user about which credential he is logging in with (although to what end?).

I would turn user.id in a field that the client and authenticator MUST return in first factor situation but the RP MAY include and require the use of user.name as a unique identifier in all cases.

Is user verification required for first factor situations? or just user presence?

@emlun emlun added privacy-tracker Group bringing to attention of Privacy, or tracked by the Privacy Group but not needing response. type:editorial labels Oct 12, 2017
@emlun emlun self-assigned this Oct 12, 2017
@jovasco
Copy link
Contributor Author

jovasco commented Oct 12, 2017

Reading up on #558,

User.id MAY be included by the RP. If it is, client MUST pass it on. Authenticator MUST return it when only a RP ID is passed in and MUST NOT return it when a Credential ID is passed in.

Make User.name mandatory to include for RPs and authenticator MUST return it in first factor cases if user.id is not present.

Agreed?

@emlun
Copy link
Member

emlun commented Oct 12, 2017

It troubles me that PublicKeyCredentialEntity.name means different things when inherited by PublicKeyCredentialRpEntity vs PublicKeyCredentialUserEntity - in the former case it's a human-friendly display name, in the latter case it's a unique identifier for a user account. I would prefer to get rid of that ambiguity and rename rp.name to rp.displayName. Preferably also rename user.id to user.handle and user.name to user.id. The hierarchy would then look something like this:

dictionary PublicKeyCredentialEntity {
    required DOMString      displayName;
    USVString               icon;
};
dictionary PublicKeyCredentialRpEntity : PublicKeyCredentialEntity {
    required DOMString      id;
};
dictionary PublicKeyCredentialUserEntity : PublicKeyCredentialEntity {
    required DOMString      id;
    BufferSource   handle;
};

I haven't lifted id up to the parent here because it will have different descriptions in the child types. The authenticator shouldn't give out a PublicKeyCredentialUserEntity without first verifying the user, so it shouldn't hurt to make all three fields required, right?

I think this would improve clarity a lot, but it would be a breaking change. What do people think?

@jovasco
Copy link
Contributor Author

jovasco commented Oct 12, 2017

Why are we trying to inherit from a common dictionary when they have little or no meaning in common? There are only some identically named optional fields in common.

@equalsJeffH
Copy link
Contributor

@jovasco asked:

Why are we trying to inherit from a common dictionary when they have little or no meaning in common? There are only some identically named optional fields in common.

It is an artifact of the credman merge. seemed OK at the time, tho is evolving as the nuances (eg for supporting first factor aka passwordless) become more well-understood by webauthn participants.

WRT @emlun's proposal #622 (comment) -- I'm inclined to simply clarify the present spec rather than rename things, but could be convinced otherwise if there is support for doing #622 (comment) by our browser-implementer colleagues.

@jovasco
Copy link
Contributor Author

jovasco commented Oct 16, 2017

@emlun Would you be willing to move the renaming proposal to a separate issue and keep this one for clarifying the language?

@emlun
Copy link
Member

emlun commented Oct 16, 2017

Certainly: #646

@equalsJeffH equalsJeffH added this to the CR milestone Oct 19, 2017
@equalsJeffH
Copy link
Contributor

Ok, we decided to close issue #646, but are keeping this issue #622 open in that it is calling for clarifications in spec prose. I added it to the CR milestone.

emlun added a commit to emlun/webauthn that referenced this issue Oct 31, 2017
This resolves w3c#622. This also changes some display name examples to
include non-ASCII characters.
@emlun emlun closed this as completed in #666 Jan 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
privacy-tracker Group bringing to attention of Privacy, or tracked by the Privacy Group but not needing response. stat:Discuss stat:pr-open type:editorial
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants