Bug 232977 – _WKWebAuthenticationPanel should expose a way to encode CTAP commands

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Garrett Davidson]

_WKWebAuthenticationPanel should expose a way to encode CTAP commands

[Garrett Davidson]

Created attachment 443892 [details]
Patch

[Garrett Davidson]

<rdar://problem/85279329>

[Garrett Davidson]

Created attachment 444012 [details]
Patch

[[e-mail]]

Comment on attachment 444012 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444012&action=review

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:644
> ++ (NSData *)encodeMakeCredentialCommandWithClientDataJSON:(NSData *)clientDataJSON options:(_WKPublicKeyCredentialCreationOptions *)options userVerificationAvailability:(_WKWebAuthenticationUserVerificationAvailability)userVerificationAvailability

Could we just take in the hash on these? Trying to cut down on the amount of times we generate clientDataJSON.

In web flows, it's originally calculated from CredentialsContainer.cpp:92 -> AuthenticatorCoordinator.cpp:246 (client data json generated here and hash passed along) -> WebAuthenticatorCoordinator.cpp:100 and then the hash gets ignored in the new WebAuthenticatorCoordinatorProxy::getAssertion instead of being passed along to ASC agent only to be regenerated later in a call to getAssertionWithChallenge.

[David Kilzer (:ddkilzer)]

Comment on attachment 444012 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444012&action=review

r- to add ASSERT_NOT_REACHED() and since it looks like tv/watch builds need to be fixed.

Otherwise looks good, but I'm not a domain expert.  :)

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:206
> +

Should have an `ASSERT_NOT_REACHED();` statement here before the `return` statement for Debug builds.

[Garrett Davidson]

Created attachment 444027 [details]
Patch

[Garrett Davidson]

Created attachment 444029 [details]
Patch

[[e-mail]]

(In reply to [e-mail] from comment #4)
> Comment on attachment 444012 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444012&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:644
> > ++ (NSData *)encodeMakeCredentialCommandWithClientDataJSON:(NSData *)clientDataJSON options:(_WKPublicKeyCredentialCreationOptions *)options userVerificationAvailability:(_WKWebAuthenticationUserVerificationAvailability)userVerificationAvailability
> 
> Could we just take in the hash on these? Trying to cut down on the amount of
> times we generate clientDataJSON.
> 
> In web flows, it's originally calculated from CredentialsContainer.cpp:92 ->
> AuthenticatorCoordinator.cpp:246 (client data json generated here and hash
> passed along) -> WebAuthenticatorCoordinator.cpp:100 and then the hash gets
> ignored in the new WebAuthenticatorCoordinatorProxy::getAssertion instead of
> being passed along to ASC agent only to be regenerated later in a call to
> getAssertionWithChallenge.

Any reason we can't do this?

[Garrett Davidson]

> Any reason we can't do this?

Sorry I thought I replied inline on the old patch but I'm not sure what happened to it.

Part of the WebAuthn API contract is that the caller needs the original clientDataJSON back, which is used during signature verification, so the full value needs to be exposed anywhere a command is encoded. That said, _WKWebAuthenticationPanel could definitely start using AuthenticatorCoordinator::produceClientDataJson internally instead of having its own copy, and/or the other methods on _WKWebAuthenticationPanel could accept a full clientDataJSON as well. I think those would both be independent of this change though.

[David Kilzer (:ddkilzer)]

Comment on attachment 444029 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444029&action=review

r=me
Let me know if you need cq+ as well.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:648
> +    RetainPtr<NSData> clientDataJSON;
> +
> +#if ENABLE(WEB_AUTHN)
> +    clientDataJSON = produceClientDataJson(type, challenge, origin);
> +#endif
> +
> +    return clientDataJSON.autorelease();

Another way to write this would be:

#if ENABLE(WEB_AUTHN)
    return produceClientDataJson(type, challenge, origin).autorelease();
#else
    return nullptr;
#endif

This does NOT need to be changed to land the patch, but it makes it clearer what the code path does when ENABLE(WEB_AUTHN) evaluates to false.

(Similar change could be done for the other methods, too, but no need to change this to land it.)

[EWS]

Committed r285763 (244210@main): <https://commits.webkit.org/244210@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444029 [details].

[[e-mail]]

(In reply to Garrett Davidson from comment #9)
> > Any reason we can't do this?
> 
> Sorry I thought I replied inline on the old patch but I'm not sure what
> happened to it.
> 
> Part of the WebAuthn API contract is that the caller needs the original
> clientDataJSON back, which is used during signature verification, so the
> full value needs to be exposed anywhere a command is encoded. That said,
> _WKWebAuthenticationPanel could definitely start using
> AuthenticatorCoordinator::produceClientDataJson internally instead of having
> its own copy, and/or the other methods on _WKWebAuthenticationPanel could
> accept a full clientDataJSON as well. I think those would both be
> independent of this change though.

We already made _WKWebAuthenticationPanel start using AuthenticatorCoordinator::produceClientDataJson internally in https://trac.webkit.org/changeset/285617/webkit as using a different implementation broke WebAuthn due to the signature not matching the client data json returned.

The complete ClientDataJson is only needed for returning via the api, which is already done in AuthenticatorCoordinator.cpp:238. Everywhere else, ClientDataHash can be passed around. The authenticator does not need this value, noted here: https://www.w3.org/TR/webauthn-3/#iface-authenticatorresponse

To complete same-site i-frames, we are going to have to add a crossOrigin flag to ClientDataJson, which is complicated by _WKWebAuthenticationPanel also having an SPI to calculate ClientDataJson, so we're going to have to end up removing it and likely changing these new SPIs.

[Garrett Davidson]

(In reply to [e-mail] from comment #12)

> We already made _WKWebAuthenticationPanel start using
> AuthenticatorCoordinator::produceClientDataJson internally in
> https://trac.webkit.org/changeset/285617/webkit as using a different
> implementation broke WebAuthn due to the signature not matching the client
> data json returned.

I didn't realize that. Neat!


> The complete ClientDataJson is only needed for returning via the api, which
> is already done in AuthenticatorCoordinator.cpp:238. Everywhere else,
> ClientDataHash can be passed around. The authenticator does not need this
> value, noted here:
> https://www.w3.org/TR/webauthn-3/#iface-authenticatorresponse
> 
> To complete same-site i-frames, we are going to have to add a crossOrigin
> flag to ClientDataJson, which is complicated by _WKWebAuthenticationPanel
> also having an SPI to calculate ClientDataJson, so we're going to have to
> end up removing it and likely changing these new SPIs.

Pascoe and I discussed this out of band. We're still thinking about the best way to reduce duplicated work for all of the intended clients here.