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

UAs can't pass unknown extensions to authenticators #738

Closed
jyasskin opened this issue Jan 9, 2018 · 8 comments · Fixed by #765
Closed

UAs can't pass unknown extensions to authenticators #738

jyasskin opened this issue Jan 9, 2018 · 8 comments · Fixed by #765

Comments

@jyasskin
Copy link
Member

jyasskin commented Jan 9, 2018

https://w3c.github.io/webauthn/#extensions claims that

Clients wishing to support the widest possible range of extensions MAY choose to pass through any extensions that they do not recognize to authenticators, generating the authenticator extension input by simply encoding the client extension input in CBOR.

However,

  1. "simply encoding the client extension input in CBOR" is underspecified: it could refer to https://tools.ietf.org/html/rfc7049#section-4.2, but like with canonical CBOR, the RFC's suggestion is probably not quite right for our use. For example, should the value 1.9 be sent as-is, rejected, or converted to an integer first? If it's sent as-is, as suggested by the RFC, we'll need to extend CTAP's definition of canonical CBOR, which doesn't support floating point values.
  2. As long as extensions define their own conversions as requested by Extensions need to define how their parameters convert to/from CBOR #626, these conversions are likely to be different from the generic conversion, forcing authenticators to deal with two different input formats, which they're likely to do wrong.

To keep things simple, I suggest requiring that UAs drop unknown extensions instead of forwarding them. If someone disagrees, I'd like them to write a PR defining the generic conversion from JS objects to CBOR that UAs are supposed to use.

@selfissued
Copy link
Contributor

Yes, https://tools.ietf.org/html/rfc7049#section-4.2 plus applying our canonicalization rules is what is expected. You're right that if floating point is present, our canonicalization can't be performed, in which case, that extension would be dropped. We can say those things explicitly if would make things clearer.

@jyasskin
Copy link
Member Author

jyasskin commented Jan 9, 2018

We should say that explicitly, with tweaks since we're starting from JS objects instead of serialized JSON. Consider starting from a copy of https://tc39.github.io/ecma262/#sec-json.stringify to make sure we handle all cases.

We should probably also ban individual extensions from defining their own conversion-to-CBOR rules to avoid the need for authenticators to parse two formats.

@agl
Copy link
Contributor

agl commented Jan 10, 2018

From the discussion today:

Mike envisions that browsers will do generic transcoding of unknown extensions, but that extensions may also define a specific transcoding function for browsers that understand it. Thus tokens have to be able to handle either and the specific function must be careful not to be ambiguous with the generic output.

I think the generic transcoding needs to be more precisely defined. (For example, I assume that floats are banned and that ArrayBuffer turns into bytes, but I'm not sure.)

@equalsJeffH
Copy link
Contributor

this issue is essentially un-triaged (no assignee, no milestone) -- at least which milestone do we feel it ought to be assigned to?

@selfissued
Copy link
Contributor

I'll take a stab at this at the same time as the other extensions work.

@selfissued
Copy link
Contributor

I will create a PR for this once #765 is merged. #765 already cleaned up a lot of the sloppy definitions that would have caused problems doing this.

@selfissued
Copy link
Contributor

I will write a PR addressing the rest of this issue shortly.

@selfissued selfissued reopened this Jan 31, 2018
@selfissued
Copy link
Contributor

Mike will write a PR and ask @agl and @equalsJeffH to review it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants