Adding CDDL to txAuthSimple #724

gmandyam · 2017-12-12T00:38:25Z

equalsJeffH

well, yeah, this is something to clean up, but we have an issue for cleaning up all extension inputs/outputs: #626, and so perhaps this should all be done together?

equalsJeffH · 2017-12-15T23:52:17Z

index.bs

+txAuthSimple: tstr,
+)
+</pre>
+


tho headed in the correct direction IMO (see #725 (comment)).

i am thinking this ought to be spec'd more similarly to how #sctn-generic-txauth-extension does it:

txAuthSimpleArg = ( tstr )

equalsJeffH · 2017-12-15T23:53:53Z

index.bs

+txAuthSimple: tstr,
+)
+</pre>
+


I'm not sure we need this CDDL representation of the extension output. If we were to use something like it, we'd want to go thru and apply the same treatment to all extension definitions IMO.

also I think the CDDL is not quite what we'd want, it'd be more like..

txAuthSimpleRetVal = ( tstr )

...I think (?)

equalsJeffH · 2017-12-15T23:57:09Z

reviewed: #724 (review)

gmandyam · 2017-12-20T16:35:09Z

@equalsJeffH

Thanks for the review. I have tried to address your comments, and I still am not sure we are there yet.

a) extidArg and extidRetVal conventions are fine, in my opinion. They should be specified in https://www.w3.org/TR/webauthn/#sctn-extension-specification, but as far as I can tell they are not.

b) I didn't leverage your suggested CDDL notation exactly, because I thought it would be clearer to represent the values as CBOR maps.

c) Although it was not the original intent of this PR, should we be more explicit about what is expected in the client extension input? "A single JSON string prompt" seems like it can be interpreted in multiple ways. If you have a suggestion, I can incorporate it into the PR and take care of that as well.

nadalin · 2018-01-17T00:32:20Z

@equalsJeffH Have you reviewed the changes ?

selfissued

The Biometric Authenticator language has nothing to do with the topic of the PR.

gmandyam · 2018-01-17T21:08:14Z

@selfissued Requested change has been made.

selfissued

With these updates, this now looks good to me

selfissued

Looking at this again, I agree with @equalsJeffH 's comments that adding CDDL to one extension just adds entropy to the spec. Why do it for this extension and not the others?

I plan to close this with no action early next week if rationale isn't expressed for keeping it here when it's not anywhere else.

nadalin · 2018-01-30T23:45:49Z

@equalsJeffH @selfissued is this one that we just close

gmandyam · 2018-01-30T23:50:03Z

@selfissued, @equalsJeffH, @nadalin

As per #679, I was assigned to provide CDDL for each extension. My approach has been to do this one at a time. It seems that there is an issue with my approach to this rather than the end goal.

selfissued · 2018-01-31T05:45:29Z

It may be a goal to add CDDL to each extension, but if so, let's create a PR that does that. Adding to some but not others, which is what this PR does, only makes the spec less parallel and more confusing. As such, I think this one should never be merged. It should therefore be closed.

gmandyam · 2018-01-31T14:55:19Z

@selfissued

Actually this is exactly the way I did it with loc: see #695. The spec in this regard is already 'imbalanced'.

jcjones · 2018-01-31T18:35:42Z

This can merge now that #765 is merged. We agree this is imbalanced, but the consensus of the WG is that we can just be sure to write CDDL for all the other extensions before publishing the next document.

jcjones · 2018-01-31T21:54:06Z

(Note: When merging this, let's file issues for the remaining CDDL.)

equalsJeffH · 2018-02-02T18:41:56Z

@gmandyam

I thought it would be clearer to represent the values as CBOR maps.

if using maps, is not the key "txAuthSimpleArg" also part of the CBOR-encoded object? If so, do we really need to transport those bytes down the stack (do they add any value)? The input arg for txAuthSimple is presently stated as:

The client extension input encoded as a CBOR text string (major type 3).

..which is just a tstr with no accompanying "key" such as "txAuthSimpleArg", which would seem to be needlessly consuming bytes on-the-wire (?).

gmandyam · 2018-02-02T21:32:12Z

@equalsJeffH

if using maps, is not the key "txAuthSimpleArg" also part of the CBOR-encoded object? If so, do we really need to transport those bytes down the stack (do they add any value)?

As per https://w3c.github.io/webauthn/#sctn-authenticator-extension-processing, "The CBOR authenticator extension input value of each processed authenticator extension is included in the extensions data part of the authenticator request. This part is a CBOR map, with CBOR extension identifier values as keys, and the CBOR authenticator extension input value of each extension as the value."

I interpreted the above to mean that the authenticator extension input is always presented to the authenticator as a CBOR map. I agree that this may be inefficient in the case of txAuthSimple, but I don't know how else to interpret the above requirement.

equalsJeffH · 2018-02-02T22:17:01Z

@gmandyam
doh! carry on then...

equalsJeffH

LGTM

emlun · 2018-02-05T14:46:29Z

@gmandyam @equalsJeffH

Sorry for being late to the party, but I had read §9.5 to mean that the whole of the extensions authenticator input - i.e., the inputs for all extensions - is a CBOR map, but the individual extension inputs are not necessarily CBOR maps. The structure of this CBOR map is then analogous to AuthenticationExtensionsClientInputs, which seems more correct to me and also saves some bandwidth as @equalsJeffH points out. CTAP also refers to a previous WebAuthn draft with an example with such a structure. I think @equalsJeffH's previously suggested CDDL txAuthSimpleRetVal = ( tstr ) is better in that case.

emlun

See #724 (comment)

gmandyam · 2018-02-05T15:09:38Z

@emlun

Please see the text I cited again, this time with a different emphasis:

As per https://w3c.github.io/webauthn/#sctn-authenticator-extension-processing, "The CBOR authenticator extension input value of each processed authenticator extension is included in the extensions data part of the authenticator request. This part is a CBOR map, with CBOR extension identifier values as keys, and the CBOR authenticator extension input value of each extension as the value."

I don't agree with your interpretation of the above text, because the CBOR map corresponds to "each processed authenticator extension", not to all collective extensions.

If this was not the intent of the cited text (which is something that the editors need to address), then they should strongly consider a revision of the above.

emlun · 2018-02-05T15:28:42Z

This is how I read the grammar in that section - read the emphasized parts as one sentence:

The CBOR authenticator extension input value of each processed authenticator extension is included in the extensions data part of the authenticator request. This part is a CBOR map, with CBOR extension identifier values as keys, and the CBOR authenticator extension input value of each extension as the value.

I agree this section needs to be revised regardless of which interpretation is deemed to be correct. I can take a stab at that once we reach consensus here.

equalsJeffH · 2018-02-05T21:36:15Z

upon review I think @emlun is correct here & here (good catch, thx) -- getting @selfissued's input would be good since he did much work on the extensions mechanism.

@emlun

agree with @emlun's requested changes.

selfissued

Please rename the parameters as suggested to better align with the nomenclature used elsewhere when describing extensions. After that, I'm good with this being merged.

selfissued · 2018-02-05T23:59:47Z

index.bs

+<pre>
+CDDL:
+{
+txAuthSimpleArg: tstr,


Please rename txAuthSimpleArg to txAuthSimpleInput

selfissued · 2018-02-06T00:00:09Z

index.bs

+<pre>
+CDDL:
+{
+txAuthSimpleRetVal: tstr,


Please rename txAuthSimpleRetVal to txAuthSimpleOutput.

nadalin · 2018-02-06T00:23:08Z

@gmandyam Where do we stand on this ?

gmandyam · 2018-02-06T00:42:42Z

@nadalin

I am fine with the suggestion by @emlun and @equalsJeffH and @selfissued. The latest changes take these into account.

I hope the text in https://w3c.github.io/webauthn/#sctn-authenticator-extension-processing will also be revised to better reflect the conclusions reached on this PR.

selfissued · 2018-02-06T16:56:25Z

@emlun I'm not sure which section you're referring to with your comment "I agree this section needs to be revised regardless of which interpretation is deemed to be correct. I can take a stab at that once we reach consensus here." But if you want to create a PR to make something clearer, I'll gladly review it.

nadalin · 2018-02-06T18:12:50Z

@gmandyam So looks like a go to merge

…ter4

gmandyam added 5 commits December 6, 2017 08:37

Update index.bs

c456302

Update index.bs

b6f9e7f

Merge branch 'master' into master

77c3c89

Adding CDDL to txAuthSimple

f319f47

Update index.bs

cc501ba

gmandyam mentioned this pull request Dec 12, 2017

txAuthSimple does not conform to extension requirements #725

Closed

nadalin added this to the CR milestone Dec 13, 2017

equalsJeffH self-requested a review December 15, 2017 23:44

equalsJeffH suggested changes Dec 15, 2017

View reviewed changes

Update index.bs

0cd24a0

nadalin assigned gmandyam Dec 20, 2017

nadalin added type:editorial type:technical labels Dec 20, 2017

selfissued requested changes Jan 17, 2018

View reviewed changes

Update index.bs

60637e1

selfissued approved these changes Jan 24, 2018

View reviewed changes

selfissued requested changes Jan 26, 2018

View reviewed changes

jcjones approved these changes Jan 31, 2018

View reviewed changes

selfissued approved these changes Jan 31, 2018

View reviewed changes

equalsJeffH reviewed Feb 2, 2018

View reviewed changes

equalsJeffH previously approved these changes Feb 2, 2018

View reviewed changes

emlun requested changes Feb 5, 2018

View reviewed changes

selfissued requested changes Feb 6, 2018

View reviewed changes

Update index.bs

d6bba25

selfissued approved these changes Feb 6, 2018

View reviewed changes

emlun approved these changes Feb 6, 2018

View reviewed changes

emlun mentioned this pull request Feb 6, 2018

§9.5. "Authenticator extension processing" is ambiguous #775

Closed

gmandyam merged commit 5124c61 into w3c:master Feb 6, 2018

WebAuthnBot pushed a commit that referenced this pull request Feb 6, 2018

Built by Travis-CI: 5124c61 Merge pull request #724 from gmandyam/mas…

d6b9886

…ter4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding CDDL to txAuthSimple #724

Adding CDDL to txAuthSimple #724

gmandyam commented Dec 12, 2017 •

edited by pr-preview bot

Loading

equalsJeffH left a comment

equalsJeffH Dec 15, 2017

equalsJeffH Dec 15, 2017

equalsJeffH commented Dec 15, 2017

gmandyam commented Dec 20, 2017

nadalin commented Jan 17, 2018

selfissued left a comment

gmandyam commented Jan 17, 2018

selfissued left a comment

selfissued left a comment

nadalin commented Jan 30, 2018

gmandyam commented Jan 30, 2018

selfissued commented Jan 31, 2018

gmandyam commented Jan 31, 2018

jcjones commented Jan 31, 2018

jcjones commented Jan 31, 2018

equalsJeffH commented Feb 2, 2018

gmandyam commented Feb 2, 2018

equalsJeffH commented Feb 2, 2018

equalsJeffH left a comment

emlun commented Feb 5, 2018

emlun left a comment

gmandyam commented Feb 5, 2018

emlun commented Feb 5, 2018

equalsJeffH commented Feb 5, 2018

selfissued left a comment

selfissued Feb 5, 2018

selfissued Feb 6, 2018

nadalin commented Feb 6, 2018

gmandyam commented Feb 6, 2018

selfissued commented Feb 6, 2018

nadalin commented Feb 6, 2018

Adding CDDL to txAuthSimple #724

Adding CDDL to txAuthSimple #724

Conversation

gmandyam commented Dec 12, 2017 • edited by pr-preview bot Loading

equalsJeffH left a comment

Choose a reason for hiding this comment

equalsJeffH Dec 15, 2017

Choose a reason for hiding this comment

equalsJeffH Dec 15, 2017

Choose a reason for hiding this comment

equalsJeffH commented Dec 15, 2017

gmandyam commented Dec 20, 2017

nadalin commented Jan 17, 2018

selfissued left a comment

Choose a reason for hiding this comment

gmandyam commented Jan 17, 2018

selfissued left a comment

Choose a reason for hiding this comment

selfissued left a comment

Choose a reason for hiding this comment

nadalin commented Jan 30, 2018

gmandyam commented Jan 30, 2018

selfissued commented Jan 31, 2018

gmandyam commented Jan 31, 2018

jcjones commented Jan 31, 2018

jcjones commented Jan 31, 2018

equalsJeffH commented Feb 2, 2018

gmandyam commented Feb 2, 2018

equalsJeffH commented Feb 2, 2018

equalsJeffH left a comment

Choose a reason for hiding this comment

emlun commented Feb 5, 2018

emlun left a comment

Choose a reason for hiding this comment

gmandyam commented Feb 5, 2018

emlun commented Feb 5, 2018

equalsJeffH commented Feb 5, 2018

selfissued left a comment

Choose a reason for hiding this comment

selfissued Feb 5, 2018

Choose a reason for hiding this comment

selfissued Feb 6, 2018

Choose a reason for hiding this comment

nadalin commented Feb 6, 2018

gmandyam commented Feb 6, 2018

selfissued commented Feb 6, 2018

nadalin commented Feb 6, 2018

gmandyam commented Dec 12, 2017 •

edited by pr-preview bot

Loading