Sensible limits for RP and User Entity fields. #667

akshayku · 2017-11-01T11:57:30Z

Preview | Diff

akshayku · 2017-11-01T12:09:25Z

PR for #660

emlun · 2017-11-01T17:32:09Z

Is this meant for resident credentials only?

equalsJeffH · 2017-11-01T23:34:27Z

@emlun:

resident credentials

I presume you are referring to Client-side-resident Credential Private Keys ?

emlun · 2017-11-01T23:56:34Z

@equalsJeffH Yes, that's what I meant. The presumption that the values will be stored make sense only in that context, right?

Actually there's no mention at all in #op-make-cred about storing any of the userEntity members except id. Is that a problem?

equalsJeffH

This seems nominally ok to me. Suggested rephrasing below, hence changes requested.

Overall, authenticator-vendor type folk ought to weigh-in on whether these stipulations are OK.

equalsJeffH · 2017-11-01T23:48:06Z

index.bs

@@ -1274,11 +1274,14 @@ associated.
 <div dfn-type="dict-member" dfn-for="PublicKeyCredentialEntity">
    :   <dfn>name</dfn>
    ::  A human-friendly identifier for the entity. For example, this could be a company name for a [=[RP]=], or a
-        user's name. This identifier is intended for display.
+        user's name. This identifier is intended for display. Authenticators while creating a credential MUST support
+        minimum of 64 bytes for this field and optionally can truncate how much it wants to store if this field is more than 64 bytes.


suggested re-write:
[=Authenticators=] MUST support a 64 byte minimum length for a [=PublicKeyCredentialEntity/name=] members's value. Authenticators MAY truncate a [=PublicKeyCredentialEntity/name=] member's value to a length equal to or greater than 64 bytes.

equalsJeffH · 2017-11-01T23:53:45Z

index.bs

-        a user's avatar or a [=[RP]=]'s logo. This URL MUST be an [=a priori authenticated URL=].
+        a user's avatar or a [=[RP]=]'s logo. This URL MUST be an [=a priori authenticated URL=]. Authenticators while
+        creating a credential MUST support minimum of 128 bytes for this field and optionally can drop this field if
+        this field is more than 128 bytes.


suggested re-write:
[=Authenticators=] MUST support a 128 byte minimum length for a [=PublicKeyCredentialEntity/icon=] members's value. Authenticators MAY ignore a [=PublicKeyCredentialEntity/icon=] members's value if its length is greater than 128 byes.

equalsJeffH · 2017-11-01T23:55:09Z

index.bs

@@ -1315,7 +1318,8 @@ credential.
    ::  The [=user handle=] of the user account entity.

    :   <dfn>displayName</dfn>
-    ::  A friendly name for the user account (e.g., "John P. Smith").
+    ::  A friendly name for the user account (e.g., "John P. Smith"). Authenticators while creating a credential MUST support
+        minimum of 64 bytes for this field and optionally can truncate how much it wants to store if this field is more than 64 bytes.


suggested re-write:
[=Authenticators=] MUST support a 64 byte minimum length for a [=PublicKeyCredentialEntity/displayName=] members's value. Authenticators MAY truncate a [=PublicKeyCredentialEntity/displayName=] member's value to a length equal to or greater than 64 bytes.

jyasskin · 2017-11-01T23:58:42Z

@emlun #623 adds the discussion of storing things.

I think a length limit makes sense for both authenticator-stored and RP-stored credential sources.

equalsJeffH · 2017-11-02T00:00:34Z

review submitted, changes requested: #667 (review)

emlun · 2017-11-02T00:21:32Z

Sorry, I didn't mean to imply that a length limit is nonsensical for RP-stored credentials. My thinking was that the MUST could seem to clash with if the authenticator won't store the values at all - like U2F devices, do they "support" 64 bytes by not storing the value?

Anyway, #623 looks like it addresses most of my above thoughts.

akshayku · 2017-11-02T02:55:33Z

@equalsJeffH : Can you review again?
@emlun: U2F is different and they don't get these fields to be stored in U2F_REGISTER. IMO, We are fine with this wording

jovasco

"support" is open to interpretation.

jovasco · 2017-11-02T16:24:28Z

index.bs

-        user's name. This identifier is intended for display.
+        user's name. This identifier is intended for display. [=Authenticators=] MUST support a 64 byte minimum length
+        for a [=PublicKeyCredentialEntity/name=] members's value. Authenticators MAY truncate a
+        [=PublicKeyCredentialEntity/name=] member's value to a length equal to or greater than 64 bytes.



"support" is open to interpretation. Are you requiring for example that the authenticator must be able to display 64 bytes worth of characters?

Displaying is not applicable for UI less authenticators on authenticator side but needed for multiple accounts scenario where Platform shows the UI. So Authenticators MUST "Store". I think we don't need to be explain every type of authenticators now or come in future? I believe "support" is better word. What do you suggest?

If you mean accept and store, then write accept and store? It is what you need while "support" covers every possible use of the string, including displaying.

Alright.. I changed it to "accept and store".

equalsJeffH · 2017-11-02T21:54:51Z

I approved these revised changes from my non-authnr-vendor-perspective, as they appear to be a step in the right direction.

However, I offer the following observations/caveats:

@jovasco has a good point over in Define sensible limits for User and RP Entity to be stored on Authenticator as part of create credential #660 (comment) that arbitrary truncation of these byte strings may result in mal-formed unicode encodings. We may want to Note: this.
nominally, DOMStrings contain UTF-16 code units, though that it not required. We should probably declare which strings we are expecting to be UTF-8 encoded to explicitly be UTF-8 encoded.
These constraints the PR adds are authenticator-specific and begs the question of having a specific authenticator implementation considerations section rather than scattering such things throughout the spec (e.g., in the portion of the spec aimed at the browser developer audience.
An authenticator implementation considerations section would be the appropriate place to provide a more full definition of "support" (which @jovasco is asking about above)

These items perhaps should be turned into yet more issues :)

equalsJeffH · 2017-11-02T22:40:20Z

btw, I observed several bikeshed errors when compiling index.bs -- we oughta clean those up before any merging.

akshayku · 2017-11-03T08:27:32Z

Incorporated feedback and merging.

…elds. (#667)

Sensible limits for RP and User Entity fields.

272a663

akshayku added this to the WD-07 milestone Nov 1, 2017

akshayku requested review from equalsJeffH and selfissued November 1, 2017 12:03

Trying to make travis checks happy

a44746d

akshayku added priority:implementation type:technical labels Nov 1, 2017

akshayku mentioned this pull request Nov 1, 2017

Define sensible limits for User and RP Entity to be stored on Authenticator as part of create credential #660

Closed

equalsJeffH suggested changes Nov 1, 2017

View reviewed changes

Incorporating Feedback

b2c1417

nadalin assigned akshayku Nov 2, 2017

jovasco reviewed Nov 2, 2017

View reviewed changes

equalsJeffH approved these changes Nov 2, 2017

View reviewed changes

Incorporating feedback and trying to get travis/bikeshed happy

7f1068f

akshayku merged commit 0418f3e into master Nov 3, 2017

akshayku deleted the akshayku-user-rp-entity-limits branch November 3, 2017 08:29

WebAuthnBot pushed a commit that referenced this pull request Nov 3, 2017

Built by Travis-CI: 0418f3e Sensible limits for RP and User Entity fi…

b973de4

…elds. (#667)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sensible limits for RP and User Entity fields. #667

Sensible limits for RP and User Entity fields. #667

akshayku commented Nov 1, 2017 •

edited by pr-preview bot

Loading

akshayku commented Nov 1, 2017

emlun commented Nov 1, 2017

equalsJeffH commented Nov 1, 2017

emlun commented Nov 1, 2017

equalsJeffH left a comment

equalsJeffH Nov 1, 2017

equalsJeffH Nov 1, 2017

equalsJeffH Nov 1, 2017

jyasskin commented Nov 1, 2017

equalsJeffH commented Nov 2, 2017

emlun commented Nov 2, 2017

akshayku commented Nov 2, 2017

jovasco left a comment

jovasco Nov 2, 2017

akshayku Nov 2, 2017

jovasco Nov 2, 2017

akshayku Nov 3, 2017

equalsJeffH commented Nov 2, 2017

equalsJeffH commented Nov 2, 2017

akshayku commented Nov 3, 2017

Sensible limits for RP and User Entity fields. #667

Sensible limits for RP and User Entity fields. #667

Conversation

akshayku commented Nov 1, 2017 • edited by pr-preview bot Loading

akshayku commented Nov 1, 2017

emlun commented Nov 1, 2017

equalsJeffH commented Nov 1, 2017

emlun commented Nov 1, 2017

equalsJeffH left a comment

Choose a reason for hiding this comment

equalsJeffH Nov 1, 2017

Choose a reason for hiding this comment

equalsJeffH Nov 1, 2017

Choose a reason for hiding this comment

equalsJeffH Nov 1, 2017

Choose a reason for hiding this comment

jyasskin commented Nov 1, 2017

equalsJeffH commented Nov 2, 2017

emlun commented Nov 2, 2017

akshayku commented Nov 2, 2017

jovasco left a comment

Choose a reason for hiding this comment

jovasco Nov 2, 2017

Choose a reason for hiding this comment

akshayku Nov 2, 2017

Choose a reason for hiding this comment

jovasco Nov 2, 2017

Choose a reason for hiding this comment

akshayku Nov 3, 2017

Choose a reason for hiding this comment

equalsJeffH commented Nov 2, 2017

equalsJeffH commented Nov 2, 2017

akshayku commented Nov 3, 2017

akshayku commented Nov 1, 2017 •

edited by pr-preview bot

Loading