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

PRF extension. #1424

Merged
merged 12 commits into from
Jul 1, 2020
Merged

PRF extension. #1424

merged 12 commits into from
Jul 1, 2020

Conversation

agl
Copy link
Contributor

@agl agl commented May 25, 2020

Some applications such as password managers have requested the ability
to associate a symmetric key with a credential. The CTAP2 hmac-secret
extension allows something very like this, and is already widely
deployed. The limitation is that it's not possible to get an HMAC output
during registration because the extension only provides outputs for
assertions and it requires user presence. That gave me pause and we
could, instead, use the new credBlob extension. But I think the utility
of being able to rotate keys, and having existing hardware support, is
compelling enough and we'll have to see whether RPs can tolerate needing
two touches to setup.


Preview | Diff

Some applications such as password managers have requested the ability
to associate a symmetric key with a credential. The CTAP2 `hmac-secret`
extension allows something very like this, and is already widely
deployed. The limitation is that it's not possible to get an HMAC output
during registration because the extension only provides outputs for
assertions and it requires user presence. That gave me pause and we
could, instead, use the new credBlob extension. But I think the utility
of being able to rotate keys, and having existing hardware support, is
compelling enough and we'll have to see whether RPs can tolerate needing
two touches to setup.
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@akshayku
Copy link
Contributor

akshayku commented May 28, 2020

User verification choice is important in hmac-secret extension. I think it needs more clarification regarding implications of it.

If an RP uses this extension with UVDiscouraged for now and then in future tries to go with UVRequired, that transition is tricky. First they have to call webauthn get() using UVDiscouraged, decrypt their state, then call another webauthn get() call with UV required so that they can encrypt it properly.

RPs should not do UVPreferred as that is dependent on authenticator.

Probably, It's better that RPs make up their mind regarding UV before using it. If they want to remain one way all the time, no issues.

If use cases are with UVRequired only, may be it is easiest to have this extension applicable only to UVRequired case?

Copy link
Contributor

@equalsJeffH equalsJeffH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM tho IIUC there's some details wrt how hmac-secret works that are different than originally considered here.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated

This [=client extension|client=] [=registration extension=] and [=authentication extension=] allows a [=[RP]=] to evaluate outputs from one of two secret, pseudo-random functions (PRFs) associated with a [=credential=]. A pseudo-random function, F, is such that, given x<sub>1</sub>, x<sub>2</sub>, &hellip; x<sub>n</sub>, and F(x<sub>1</sub>), F(x<sub>2</sub>), &hellip; F(x<sub>n</sub>), no adversary can predict any bit of F(x<sub>m</sub>) for any m &gt; n. The PRFs provided by this extension map from {{DOMString}}s of any length to 32-byte {{ArrayBuffer}}s.

If a [=credential=] is created on an [=authenticator=] that supports this extension then two fresh PRFs are associated with it. One is evaluated when [=user verification=] is performed and the other is evaluated when [=user verification=] is not performed. It is the responsibility of the [=[RP]=] to set the {{PublicKeyCredentialRequestOptions/userVerification}} parameter accordingly.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If a [=credential=] is created on an [=authenticator=] that supports this extension then two fresh PRFs are associated with it. One is evaluated when [=user verification=] is performed and the other is evaluated when [=user verification=] is not performed. It is the responsibility of the [=[RP]=] to set the {{PublicKeyCredentialRequestOptions/userVerification}} parameter accordingly.
If a [=credential=] is created on an [=authenticator=] that supports this extension then two freshly-seeded PRFs are associated with it. Upon subsequent invocations of {{CredentialsContainer/get()|navigator.credentials.get()}}, one PRF is evaluated if [=user verification=] is performed and the other is evaluated if only a [=test of user presence|user presence test=] is performed. It is the responsibility of the [=[RP]=] to set the {{PublicKeyCredentialRequestOptions/userVerification}} parameter accordingly: [=[RPS]=] SHOULD _consistently_ use either of the "Required" or "Discouraged" values of {{PublicKeyCredentialRequestOptions/userVerification}} when evaluating the PRF, and SHOULD NOT use "Preferred". Regarding the prior "consistently" remark: if an RP uses "Required" to obtain a PRF output that is subsequently used as a key to encrypt some data, and then later obtains a PRF output using "Discouraged" (using the same salt value as before), the PRF output will be different and they will be unable to decrypt the data.

(am rushing a bit here, so the last sentence of the above is not as polished as I'd otherwise like...)

see also https://github.com/fido-alliance/fido-2-specs/issues/873

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Integrated, but have changed some wording. PTAL.

And thanks for https://github.com/fido-alliance/fido-2-specs/issues/873. I didn't notice that CTAP2.0 was different and I've updated the wording here accordingly.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
agl added 2 commits June 2, 2020 14:51
  · Now possible to pass in a set of PRF inputs, per-credential ID, when
    getting an assertion.
  · Inputs are now a structure rather than a list that had text
    specifying the valid lengths.
  · Wording updated to note that some authenticators may have only a
    single PRF.
@equalsJeffH equalsJeffH self-requested a review June 3, 2020 18:00
Copy link
Contributor

@equalsJeffH equalsJeffH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found some issues (and some very minor nits) in my second pass...

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@emlun emlun mentioned this pull request Jun 5, 2020
agl and others added 2 commits June 8, 2020 09:30
Co-authored-by: Emil Lundberg <emil@emlun.se>
Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good to me, just a couple more comments...

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
   · Drop the `enable` member and use presence of `prf` to enable.
   · Make the inputs ArrayBuffers and merge the two dictionaries.
index.bs Show resolved Hide resolved
@equalsJeffH equalsJeffH marked this pull request as ready for review June 10, 2020 19:26
Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me, please indulge me for a few more polish suggestions...

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@akshayku akshayku self-requested a review June 12, 2020 18:42
agl and others added 2 commits June 13, 2020 11:47
Co-authored-by: Emil Lundberg <emil@emlun.se>
Copy link
Contributor

@equalsJeffH equalsJeffH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks even better to me ;-)

index.bs Outdated Show resolved Hide resolved
The handling of userVerification by RPs needed to be updated. For
example, even if they consistently specified “discouraged” for both
create() and get(), if they also set requireResidentKey then Chroem, for
one, will force UV during create. Thus RPs that are using a future CTAP
extension to evaluate the PRFs during create will have to inspect the
authenticator data to learn which PRF the output is from.

Otherwise, this tweaks some corner cases, like whether an empty
extension is echoed in an assertion if no keys were recognised in the
input.
Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple more nits - sorry, I don't mean to be difficult... 🙂

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
agl and others added 2 commits June 22, 2020 08:31
Co-authored-by: Emil Lundberg <emil@emlun.se>
Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting very close now... 😄

index.bs Outdated Show resolved Hide resolved
Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @agl, and sorry for the barrage of reviews!


When requested, if a [=credential=] is created on an [=authenticator=] that supports this extension then either one or two freshly-seeded PRFs are associated with it. If two PRFs are associated then, when outputs are requested, one PRF is evaluated if [=user verification=] is performed and the other is evaluated if only a [=test of user presence|user presence test=] is performed. It is the responsibility of the [=[RP]=] to set the {{PublicKeyCredentialRequestOptions/userVerification}} parameter accordingly: [=[RPS]=] SHOULD *consistently* use either the {{UserVerificationRequirement/required}} or {{UserVerificationRequirement/discouraged}} values of {{UserVerificationRequirement}} when using this extension, otherwise the outputs may vary for a given input. To avoid mistakes, the default value of {{UserVerificationRequirement/preferred}} is prohibited when using this extension during [=assertion=] because that could cause the PRF used to vary between operations if [=user verification=] is later enabled on an [=authenticator=].

As a motivating example, PRF outputs could be used as symmetric keys to encrypt user data. Such encrypted data would be inaccessible without the ability to get assertions from the associated [=credential=]. By using the provision below to evaluate the PRF at two inputs in a single [=assertion=] operation, the encryption key could be periodically rotated during [=assertions=] by choosing a fresh, random input and reencrypting under the new output. If the evaluation inputs are unpredictable then even an attacker who could satisfy [=user verification=], and who had time-limited access to the authenticator, could not learn the encryption key without also knowing the correct PRF input.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that there should be a clarification about the exact process to perform a key rotation as this section is hard to parse.


This [=client extension|client=] [=registration extension=] and [=authentication extension=] allows a [=[RP]=] to evaluate outputs from a pseudo-random function (PRF) associated with a [=credential=]. The PRFs provided by this extension map from {{USVString}}s of any length to 32-byte {{ArrayBuffer}}s.

When requested, if a [=credential=] is created on an [=authenticator=] that supports this extension then either one or two freshly-seeded PRFs are associated with it. If two PRFs are associated then, when outputs are requested, one PRF is evaluated if [=user verification=] is performed and the other is evaluated if only a [=test of user presence|user presence test=] is performed. It is the responsibility of the [=[RP]=] to set the {{PublicKeyCredentialRequestOptions/userVerification}} parameter accordingly: [=[RPS]=] SHOULD *consistently* use either the {{UserVerificationRequirement/required}} or {{UserVerificationRequirement/discouraged}} values of {{UserVerificationRequirement}} when using this extension, otherwise the outputs may vary for a given input. To avoid mistakes, the default value of {{UserVerificationRequirement/preferred}} is prohibited when using this extension during [=assertion=] because that could cause the PRF used to vary between operations if [=user verification=] is later enabled on an [=authenticator=].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear if the "first" or "second" prf is the one returned during this exension. This must be clarified to state which of "first" or "second" are returned, and in which userverification scenarios related to the registration process.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(better yet, rename these from "first" and "second" to "default" and "verifiedonly"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like you might be confusing the "two PRFs" with the two first and second extension output fields. Is that so? The first and second outputs are both from the same PRF, and which PRF is determined by whether UV was used:

f = if UV=1 { PRF1 } else { PRF2 }
output.first = f(input.first)
if input.second {
  output.second = f(input.second)
}

I thought this was unambiguously defined by the introduction:

[...] If two PRFs are associated then, when outputs are requested, one PRF is evaluated if user verification is performed and the other is evaluated if only a user presence test is performed. [...]

and the definition of the results output:

The results of evaluating the PRF for the inputs given in eval or evalByCredential. Outputs may not be available during registration; see comments in eval.

How would you suggest clarifying it? Would a clarification also resolve your previous comment?

@@ -5638,6 +5639,97 @@ At this time, one [=credential property=] is defined: the [=resident key credent
: Authenticator extension output
:: None.

## Pseudo-random function extension (<dfn>prf</dfn>) ## {#prf-extension}

This [=client extension|client=] [=registration extension=] and [=authentication extension=] allows a [=[RP]=] to evaluate outputs from a pseudo-random function (PRF) associated with a [=credential=]. The PRFs provided by this extension map from {{USVString}}s of any length to 32-byte {{ArrayBuffer}}s.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This introduction needs clarification. It does not clearly or accurately describe what this extension does. It appears to read as a "generate a random number on this device". It's also unclear if the operation is yielding the prf content, or is actually signing with hmac in the device, and I think this introduction clearly needs to introduce what the data operations are and what is happening here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the RP perspective, does it matter what the precise implementation is? The extension defines an abstraction - indeed "generate a random number on this device", except a repeatable one determined by the extension input - and the RP doesn't need to know the implementation details. But of course the details are needed for interoperability between clients and authenticators, and the extension does define the details by reference to the CTAP hmac-secret extension.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but at the moment it doesn't say that clearly. It doesn't make it clear it's deterministic.

As well, knowing what it's doing, is going to be important for people to assess if this is correct for their needs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize now that you may not have noticed: there's a bot that adds to the top post a Preview link to the rendered spec. That preview is much easier to read, especially since it includes hyperlinks for [FIDO-CTAP] and other external references.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://pr-preview.s3.amazonaws.com/agl/webauthn/pull/1424.html#prf-extension

Even with that, this whole extension is really unclear. Variables like "first" and "second". Which one is used from the statement: " If two PRFs are associated then, when outputs are requested, one PRF is evaluated if user verification is performed and the other is evaluated if only a user presence test is performed."? It even mentions "key rollover" but without an example of the extension inputs you need to send to perform it or the steps required.

And as mentioned without knowing what it's doing (hmac, random number, ecdh etc) people aren't going to be able to assess if this is relevant.

I really really think this extension needs a great deal of work to clarify it's intent and what it's is meant to do.

I honestly have read it 6 times and I still am confused.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't make it clear it's deterministic.

The term "pseudo-random function" does include that... but I suppose this means we need to define or reference that term.

And as mentioned without knowing what it's doing (hmac, random number, ecdh etc) people aren't going to be able to assess if this is relevant.

To assess for relevance, it really is enough to know the abstraction: "a pseudo-random function (PRF) associated with a credential". That already tells you all you need to know to determine if it'll work for a given use case. On the other hand to assess for security, you'll need all the fine details anyway, not just a description, and all the details are there by reference to CTAP.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because today it looks like this is a random number generator function. But if someone is going to assess this, and you are linking to CTAP, why not just declare from the start that this is a Credential Related HMAC function, instead of this round-about language that just confuses people? What purpose does it serve to be so indirect about the function and behaviour of this extension?

It's worth pointing out that you as a specification author are very deep in this terminology and process, but many implementors of this standard as RP's are not as familiar. They don't have access to the meetings, the background and the history. As well, many of them may also be english-second language and that adds yet another barrier for communication. This is yet another reason why clear and accessible communication and language is so critical in a specification that will be assessed for and used in security related areas.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Believe it or not, I agree with you in principle. If readers don't understand the text, then the text needs to be improved. I agree. English is my second language, and I came to this working group as an RP implementer with no prior experience with any of these standards or of standards work. I agree we cannot expect readers to know everything about the field, but I think we should expect readers to read carefully and thoroughly.

Your comments do point out some improvements we need to make, and I appreciate that (although this is all probably moot in this particular case, as the PRF extension has since been dropped from the spec). But I don't agree that the primary focus on the PRF abstraction is a bad thing. It is arguably more direct, as it precisely and concisely states the offered capability without requiring prior knowledge of what HMAC is or what can be constructed with it. ​Another purpose is stated in the extension:

Note: this extension may be implemented for authenticators that do not use [FIDO-CTAP] so long as the behavior observed by a Relying Party is identical.

Still if one does want to know the (CTAP-specific) implementation, one can read on further - and in this case the implementation is even summarized in the first line of a paragraph, making it skim-friendly too. So I honestly cannot see what the improvement to that would be, but I would genuinely appreciate if you can offer a concrete suggestion. As for your other comments (#1424 (comment), #1424 (comment)), I am trying to help determine what exactly needs to be clarified and how best to do so. I apologize if I'm coming across as passive-aggressive, but I also request that others assume best intent as I do for them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we cannot expect readers to know everything about the field, but I think we should expect readers to read carefully and thoroughly.

Like I said - I have tried to read and understand this extension multiple times and I'm really still not getting it. :(

While I agree in principle people should be expected to read this carefully and thoroughly, it's still important that within this communication we are compassionate and empathetic to our readers and how we approach the language we choose.

I am trying to help determine what exactly needs to be clarified and how best to do so. I apologize if I'm coming across as passive-aggressive, but I also request that others assume best intent as I do for them.

No you aren't being passive aggressive - text is hard and it's always best to assume positive intent. Thank you for taking the time to engage in this discussion :)

Given that this extension has been dropped it is a bit moot and you are correct there, but it was brought up on #1595 . There is certainly some confusion there still about it, as can be seen by the comment:

It is key derivation. Based on a nonce as input you get back a key that you can use to directly encrypt/decrypt with or derive other keys from.

Because that doesn't seem to match the description or function as you/others have described it.

I think that for this to be clear about it's intent, rather than "Pseudo Random Function" which reads like a CSPRNG, it should be directly named based on it's function and intent:

  • It's a Key Derivation / Key Retrieval function.

Meaning, that the use of this extension is a way to retrieve associated bytes from a server known/client known input that returns a set of bytes that can be used for other operations.

  • OR it's an authenticator associated data verification function.

Meaning, that this authenticator/client, when it is interacted with is asserting that some input bytes are hashed and then signed by this extension. (given certain limitations of course)

Certainly, the second option of data verification would match the expectations people seem to want from #1595 here, allowing some input data to be verified during an assertion process.

Does that help?

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.

6 participants