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

RP operations: some extension processing may assume that the encompassing signature is valid #1711

Closed
equalsJeffH opened this issue Mar 23, 2022 · 2 comments · Fixed by #2167
Assignees
Milestone

Comments

@equalsJeffH
Copy link
Contributor

equalsJeffH commented Mar 23, 2022

In both of the RP Operations subsections (Registering a new cred, and verifying an authn assertion), the step for verifying/processing of extension outputs is placed before the step for verifying the signature value over "authenticator data".

This is fine for idempotent extensions that simply marshall data for eventual return to the RP as a part of the operation's response.

However, extensions such as devicePubKey / "DPK" (PR #1663) call for the RP to associate and store extension-generated data with the user's account. The RP probably should only do so if the overall credential creation or authentication results themselves validate correctly, which notably includes verifying the "encompassing" signature over "authenticator data".

Perhaps the step for verifying extension outputs should be moved to after the step(s) for verifying the "encompassing" signature over "authenticator data" in both of the registering a new cred, and verifying an authn assertion sections.

@emlun
Copy link
Member

emlun commented Mar 24, 2022

Related: #1064

@emlun
Copy link
Member

emlun commented Mar 20, 2023

I think the concern about devicePubKey is mostly resolved at the moment by the combination of #1807 and #1812, as the steps to store new DPK records are now deferred to after signature verification. But I wonder if in a broader perspective it's still worth moving extension processing to after signature verification anyway, so this doesn't come up again in future extensions - or extensions defined in other specs, for that matter. Thoughts on that? I'm happy to do it in that case.

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