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

Problem with authn selection extension #152

Closed
vijaybh opened this issue Jul 27, 2016 · 1 comment
Closed

Problem with authn selection extension #152

vijaybh opened this issue Jul 27, 2016 · 1 comment

Comments

@vijaybh
Copy link
Contributor

vijaybh commented Jul 27, 2016

The current text says:

If an authenticator was selected from AuthenticatorSelectionList, its AAGUID MUST be added by the client to the ClientData as the client data value for this extension.

However this creates a chicken-and-egg problem. The client may send out a request to a number of authenticators. It needs to include the AAGUID of the authenticator that finally responds in its clientData. But by the time the client knows the AAGUID, the clientData has already been sent to the authenticator and signed over.

One way (the simplest way?) to address this is to simply remove this requirement for adding anything to the clientData, since the AAGUID of the selected authenticator will be in the attestation anyways.

@equalsJeffH
Copy link
Contributor

It seems to me that removing the above-quoted requirement to add AAGUID to to ClientData is ok, given that claimedAAGUID is returned in the attestation Though I suggest we have Rolf sign off on it also -- he may have had a specific reason for stipulating AAGUID's inclusion in ClientData.

@equalsJeffH equalsJeffH added this to the CR milestone Aug 29, 2016
vijaybh pushed a commit that referenced this issue Sep 1, 2016
#169)

* Adding the User Verification Mode extension as per discussions in the WG weekly metings. After a lot of thought I retained the sub-array instead of converting the sub-items in the extension to a map. This was primarily to avoid having to define string based keys that increases the size of the extension. Given that the array is well defined I think it keeps it easier to process in software and extendable for future additions as well.

* Cleaned up exposition of processing rules (#154)

* Replace facet with origin

Facet was a holdover from the old FIDO specs and origin is the term used
everywhere in this spec (as well as in recent FIDO specs)

* Clean up explanation of computing clientDataHash and passing to authenticator

Fixes #153

* Remove text from authnsel extension to avoid chicken-and-egg problem

Fixes #152

* Address comments from @equalsJeffH

* Update index.bs with proposed location extension (#157)

* Update index.bs

* Update index.bs

* Update index.bs

* Update index.bs

* Adding the User Verification Mode extension as per discussions in the WG weekly metings. After a lot of thought I retained the sub-array instead of converting the sub-items in the extension to a map. This was primarily to avoid having to define string based keys that increases the size of the extension. Given that the array is well defined I think it keeps it easier to process in software and extendable for future additions as well.

* fixing another merge issue with uvm extension being repeated following upstream merge
vijaybh added a commit that referenced this issue Sep 20, 2016
* Replace facet with origin

Facet was a holdover from the old FIDO specs and origin is the term used
everywhere in this spec (as well as in recent FIDO specs)

* Clean up explanation of computing clientDataHash and passing to authenticator

Fixes #153

* Remove text from authnsel extension to avoid chicken-and-egg problem

Fixes #152

* Clean up attestation

- Standardize the authenticator data so all formats have equal support
for AAGUID, extensions, etc. This also removes a lot of duplication
across structures.
- Add structure to the definition of attestation formats. Fixes #126.
Fixes #127.
- Simplify the naming of the attestation types to make it easier to
understand
- Clean up mentions of GUID. Fixes #148, fixes #149, fixes #150.
- Clarifies how to use self attestation. Fixes #115.
- More detailed pointers on how to generate a TPM attestation.
- Simplify Android attestation to remove fields that were not really
attested by authenticator and were therefore creating a false sense of
assurance.

* Fix typo (thanks Travis!)

* Incorporated feedback from @rlin1

Also cleaned up wording and naming for consistency.

Added Android N attestation format. Fixes #103.

Changed name for SafetyNet attestation format. Fixes #128.

* Clarify that attestation is not optional

Fixes #86

Also clarify that at least self attestation must be used. Fixes #115
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants