Align registries draft and WebAuthn draft and address extension issues #386
Conversation
|
PR should be generated with respect to https://github.com/w3c/webauthn/blob/master/draft-hodges-webauthn-registries.xml. |
|
I don't think this does enough to close #270.
There are probably other problems. I don't think this should block merging this PR; just please don't close #270 as a result. :) |
AFAICT this PR does so, there's 4 files affected, |
|
My sincere apologies. The edits were done on the .xml file. Thanks for taking this forward. |
|
The 3 examples you cited can (and probably should) be filed as 3 new issues with respect to the relevant extensions so that the editors for these subsections can address them. Do you agree? |
|
@gmandyam I've copied my comment to #270 (comment) so that discussing it doesn't slow down this PR. |
There was a problem hiding this comment.
Overall, this is great, thanks (!), tho it needs some editorial polishing, and also the splitting into two separate PRs as we discussed on the webauthn call earlier today.
-
needs a merge-from-master since master changed today
-
"algorithm issue":
split this PR into two PRs:
- extension-oriented markups in index.bs & draft-hodges-webauthn-registries.xml
- the algorithm changes (note that this perhaps should include the new paragraph at line # 2251. see my comment on it below)
-
"attestation format" is old term that was in the very stale draft-hodges-webauthn-registries, correct term is "Attestation Statement Format" -- noted in review below
-
should the citation of [[!WebAuthn-Registries]] be normative? if so, then: s/[[WebAuthn-Registries]]/[[!WebAuthn-Registries]]/g
thanks again :)
draft-hodges-webauthn-registries.xml
Outdated
| @@ -107,7 +114,15 @@ | |||
| <t> | |||
| Registration requests consist of at least the following information: | |||
| <list style="symbols" > | |||
| <t>WebAuthn Attestation Format Identifier: </t> | |||
| <t> | |||
| WebAuthn Attestation Format Identifier: | |||
There was a problem hiding this comment.
s/WebAuthn Attestation Format/WebAuthn Attestation Statement Format/
| Names may not match other registered names in a case-insensitive manner | ||
| unless the Designated Experts state that there is a compelling reason | ||
| to allow an exception. | ||
| </t> |
There was a problem hiding this comment.
@selfissued (Mike Jones) relays that the rationale for having the attstn stmt format ident be case-sensitive is to simplify code -- there is not a need to be sure to tolower() it, eases developer burden. IMHO we should capture this rationale in a "design rationale/decision appendix" See for example: https://tools.ietf.org/html/rfc6797#appendix-A
draft-hodges-webauthn-registries.xml
Outdated
| @@ -223,19 +238,17 @@ | |||
|
|
|||
| <section title="IANA Considerations" anchor="sctn-iana-cons"> | |||
|
|
|||
| <section title="WebAuthn Attestation Formats and Extension Identifiers Registries" | |||
| <section title="WebAuthn Attestation Format Identifier and Extension Identifiers Registries" | |||
There was a problem hiding this comment.
s/WebAuthn Attestation Format/WebAuthn Attestation Statement Format/
draft-hodges-webauthn-registries.xml
Outdated
| anchor="sctn-attstn-extn-regs"> | ||
|
|
||
| <t> | ||
| This specification establishes two registries: | ||
| <list style="symbols"> | ||
| <t> | ||
| the WebAuthn Attestation Formats registry; see <xref target="sctn-reg-new-attstn-format"/>. Initial registry contents are given in | ||
| <xref target="sctn-attstn-format-reg-cnts"/>. | ||
| the "WebAuthn Attestation Format Identifier" registry; see <xref target="sctn-reg-new-attstn-format"/>. |
There was a problem hiding this comment.
s/WebAuthn Attestation Format/WebAuthn Attestation Statement Format/
draft-hodges-webauthn-registries.xml
Outdated
| </section> | ||
|
|
||
| <section title="Acknowledgements"> | ||
| <section anchor="History" title="Document History"> |
There was a problem hiding this comment.
hrm, i wudn't change the section title but whatever. the anchor should be "sctn-history" per this spec's conventions :) thx.
index.bs
Outdated
| - Description: Used with FIDO U2F authenticators | ||
| - Specification Document: Section [[#fido-u2f-attestation]] of this specification | ||
|
|
||
| ## WebAuthn Extension Identifier Registrations ## (#ExtensionReg) |
There was a problem hiding this comment.
overall comment wrt extension definitions in down in the "defined extensions" section (which is not (yet) modified by this PR): each extn dfn should clearly note whether it is a "registration extension" or "authentication extension" (or both) -- I've denoted them as such in comments below.
Also, it would be good if in the "weauthn extensions" section (above) that the terms "registration extension" or "authentication extension" could be given the appropriate <dfn>...</dfn> markup, and then usage instances of the terms ought to get the [=...=] markup.
index.bs
Outdated
| - Specification Document: Section [[#supported-extensions-extension]] of this specification | ||
|
|
||
| - WebAuthn Extension Identifier: uvi | ||
| - Description: The user verification index (UVI) is a value uniquely identifying a user verification data record. The UVI data can be used by servers to understand whether an authentication was authorized by the exact same biometric data as the initial key generation. This allows the detection and prevention of "friendly fraud". |
There was a problem hiding this comment.
I believe this is both a registration extension and authentication extension, yes?
index.bs
Outdated
| - Specification Document: Section [[#extension-txauth-generic]] of this specification | ||
|
|
||
| - WebAuthn Extension Identifier: authnSel | ||
| - Description: This registration extension allows a WebAuthn Relying Party to guide the selection of the authenticator that will be leveraged when creating the credential. It is intended primarily for WebAuthn Relying Parties that wish to tightly control the experience around credential creation. |
There was a problem hiding this comment.
this is a registration extn
index.bs
Outdated
| - Specification Document: Section [[#uvi-extension]] of this specification | ||
|
|
||
| - WebAuthn Extension Identifier: loc | ||
| - Description: The location extension provides the client device's current location to the WebAuthn relying party, if supported by the client device and subject to user consent. |
There was a problem hiding this comment.
I believe this is both a registration extension and authentication extension, yes?
index.bs
Outdated
| - Specification Document: Section [[#uvi-location]] of this specification | ||
|
|
||
| - WebAuthn Extension Identifier: uvm | ||
| - Description: The user verification mode (UVM) extension returns to the Webauthn relying party which user verification methods (factors) were used for the WebAuthn operation. |
There was a problem hiding this comment.
I believe this is both a registration extension and authentication extension, yes?
… use TypeError, per @jyasskin
|
This is ready for @vijaybh's review now and please merge-to-master if it passes muster. |
|
Merging this in to get ready for IETF. |
|
Thanks, Vijay! |
… and address extension issues (#386)
…e TypeError, per @jyasskin (#389) Major polishing of definition and exposition of extensions by selfissued - yay, thx! includes: * Separated proposed changes to extension semantics from PR #386 and use TypeError, per @jyasskin * Added client data descriptions to all extensions. Accepted suggestions by @jyasskin and @vijaybh. * Addressed comments by @jyasskin in issue #270 * Gave distinct names to extension inputs and outputs to make descriptions more precise. * Corrected indexing errors * Addressed additional comments by Jeff Hodges and Jeffrey Yasskin
This prepares draft-hodges-webauthn-registries for submission as an Internet Draft at the beginning of the IETF meeting next week, aligning the contents of the registries and WebAuthN drafts. It also clears up some ambiguities in the general-purpose extension semantics, which should enable us to close issues #270 and #295 , among others.