Skip to content

Parse signCount and expose at AuthenticatorData#sign_count#70

Merged
brauliomartinezlm merged 3 commits intocedarcode:masterfrom
sorah:sign_count
Oct 3, 2018
Merged

Parse signCount and expose at AuthenticatorData#sign_count#70
brauliomartinezlm merged 3 commits intocedarcode:masterfrom
sorah:sign_count

Conversation

@sorah
Copy link
Contributor

@sorah sorah commented Oct 1, 2018

This patch allows RP to check signature count mismatch.
https://www.w3.org/TR/2018/CR-webauthn-20180807/#sign-counter

Note: I assume AuthenticatorData is exposed to library users in this PR; see #69

@brauliomartinezlm
Copy link
Member

@sorah Thank you for the PR!
Did you planned this PR as a first step to add the sign_count validation in a later one?

We initially didn't add support to it because it got deprioritized at first as it was state in the recommendation that how to react to the detection of a mismatch depended individually on the RP

https://www.w3.org/TR/2018/CR-webauthn-20180807/#sign-counter

Detecting a signature counter mismatch does not indicate whether the current operation was performed by a cloned authenticator or the original authenticator. Relying Parties should address this situation appropriately relative to their individual situations, i.e., their risk tolerance.

But I'm 100% all in to add this validation as long as we provide a way for the gem user to fairly decide on how to react to a mismatch in the sign count. Would love to see opinions in such regard given that the Candidate Recommendation leaves the decision open.

@sorah
Copy link
Contributor Author

sorah commented Oct 2, 2018

Did you planned this PR as a first step to add the sign_count validation in a later one?

Yes.

But I'm 100% all in to add this validation as long as we provide a way for the gem user to fairly decide on how to react to a mismatch in the sign count.

Agreed, I don't think we don't need to implement sign counter validation in valid? method right now. But we lack a choice for RP implementation (at least I want to check and log,) so I believe exposing the value is a good starting point.

@sorah
Copy link
Contributor Author

sorah commented Oct 3, 2018

WDYT? I understand there's no reason to hide the data since #69 is merged.

@brauliomartinezlm
Copy link
Member

Yeah, this is good to go. Sorry for the delay and thank you for doing this PR 👍

@brauliomartinezlm brauliomartinezlm merged commit abd0d15 into cedarcode:master Oct 3, 2018
@sorah
Copy link
Contributor Author

sorah commented Oct 3, 2018

No worries, thank you! 😺

@sorah sorah deleted the sign_count branch October 3, 2018 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants