Bug 225519 – [Cocoa] _WKAuthenticatorAssertionResponse should specify the attachment type used

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

Include context about whether the _WKAuthenticatorAssertionResponse came from the platform authenticator or a security key.

[Brent Fulgham]

<rdar://problem/76554090>

[Brent Fulgham]

Created attachment 428007 [details]
Patch

[Jiewen Tan]

Comment on attachment 428007 [details]
Patch

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

> Source/WebCore/Modules/webauthn/AuthenticatorAssertionResponse.h:40
> +    static Ref<AuthenticatorAssertionResponse> create(Ref<ArrayBuffer>&& rawId, Ref<ArrayBuffer>&& authenticatorData, Ref<ArrayBuffer>&& signature, RefPtr<ArrayBuffer>&& userHandle, Optional<AuthenticationExtensionsClientOutputs>&&, AuthenticatorResponse::Attachment);

This is only used in the Web API. No need to add attachment to it.

> Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.h:36
> +    static Ref<AuthenticatorAttestationResponse> create(Ref<ArrayBuffer>&& rawId, Ref<ArrayBuffer>&& attestationObject, AuthenticatorResponse::Attachment);

Ditto.

> Source/WebCore/Modules/webauthn/AuthenticatorCoordinator.cpp:194
> +        if (auto response = AuthenticatorResponse::tryCreate(WTFMove(data), AuthenticatorResponse::Attachment::CrossPlatform)) {

You cannot assume this.

> Source/WebCore/Modules/webauthn/AuthenticatorCoordinator.cpp:265
> +        if (auto response = AuthenticatorResponse::tryCreate(WTFMove(data), AuthenticatorResponse::Attachment::CrossPlatform)) {

Ditto.

> Source/WebCore/Modules/webauthn/AuthenticatorResponse.cpp:37
> +RefPtr<AuthenticatorResponse> AuthenticatorResponse::tryCreate(AuthenticatorResponseData&& data, Attachment attachment)

It's only used in the Web API. No need for this.

> Source/WebCore/Modules/webauthn/AuthenticatorResponse.cpp:88
> +AuthenticatorResponse::Attachment AuthenticatorResponse::attachment() const

You can do it in the header.

> Source/WebCore/Modules/webauthn/AuthenticatorResponse.h:46
> +    enum class Attachment {

Instead of making a new enum, use WebCore::AuthenticatorAttachment.

> Source/WebCore/Modules/webauthn/AuthenticatorResponse.h:71
> +    Attachment m_attachment;

This is only for the SPI not the Web API. Let's make it Optional such that it's not necessary to serialize it back to the web process.

> Source/WebCore/Modules/webauthn/fido/DeviceResponseConverter.cpp:139
> +    return AuthenticatorAttestationResponse::create(credentialId, *attestationObject, AuthenticatorResponse::Attachment::CrossPlatform);

Add a new parameter to this function and do it in the Authenticator layer. It's not guaranteed that only external authenticators will use this function.

> Source/WebCore/Modules/webauthn/fido/DeviceResponseConverter.cpp:176
> +        response = AuthenticatorAssertionResponse::create(credentialId, authData, signature, userHandle, AuthenticatorResponse::Attachment::CrossPlatform);

Ditto.

> Source/WebCore/Modules/webauthn/fido/DeviceResponseConverter.cpp:192
> +        response = AuthenticatorAssertionResponse::create(credentialId, authData, signature, { }, AuthenticatorResponse::Attachment::CrossPlatform);

Ditto.

> Source/WebCore/Modules/webauthn/fido/U2fResponseConverter.cpp:170
> +    return AuthenticatorAttestationResponse::create(credentialId, attestationObject, AuthenticatorResponse::Attachment::CrossPlatform);

Ditto.

> Source/WebCore/Modules/webauthn/fido/U2fResponseConverter.cpp:190
> +    return AuthenticatorAssertionResponse::create(keyHandle, authData, signature, { }, AuthenticatorResponse::Attachment::CrossPlatform);

Ditto.

[Garrett Davidson]

Created attachment 429816 [details]
Updated patch to replace the current one

[Brent Fulgham]

Created attachment 429952 [details]
Garrett's Patch (Rebaselined)

[Brent Fulgham]

Comment on attachment 429952 [details]
Garrett's Patch (Rebaselined)

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

> Source/WebCore/Modules/webauthn/AuthenticatorAssertionResponse.h:40
> +    static Ref<AuthenticatorAssertionResponse> create(Ref<ArrayBuffer>&& rawId, Ref<ArrayBuffer>&& authenticatorData, Ref<ArrayBuffer>&& signature, RefPtr<ArrayBuffer>&& userHandle, Optional<AuthenticationExtensionsClientOutputs>&&, AuthenticatorAttachment);

Jiewen pointed out this is only used by the WebAPI -- is there any reason to keep the unused AuthenticatorAttachment argument?

> Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.h:36
> +    static Ref<AuthenticatorAttestationResponse> create(Ref<ArrayBuffer>&& rawId, Ref<ArrayBuffer>&& attestationObject, AuthenticatorAttachment);

Ditto.

> Source/WebCore/Modules/webauthn/AuthenticatorResponse.cpp:90
> +    return m_attachment;

Jiewen suggested you move this to the header: Any reason not to?

> Source/WebCore/Modules/webauthn/AuthenticatorResponse.h:67
> +    AuthenticatorAttachment m_attachment;

You didn't address Jiewen's comment that this isn't needed in the Web layer, so serializing it back/forth to the WebContent process seems wasteful. Can we make it Optional<>?

[Garrett Davidson]

(In reply to Brent Fulgham from comment #6)

> You didn't address Jiewen's comment that this isn't needed in the Web layer,
> so serializing it back/forth to the WebContent process seems wasteful. Can
> we make it Optional<>?

I'm new in this codebase, so this is probably just my lack of understanding, but what's being "wasted" here? Isn't it the same amount of space to encode an empty Optional<> as it is to encode an int? If we go to Optional<> route, we effectively end up with a partially initialized object which has more complex usage because you have to add code to handle not having a value, as opposed to guaranteeing it's always there.



> Jiewen suggested you move this to the header: Any reason not to?

I tried moving it to the header and ended up getting errors about an exported symbol being defined in multiple compilation units. My assumption was that you can't mix `WEBCORE_EXPORT` with a method defined in a header, but if I missed something I'm happy to move it :)

[Brent Fulgham]

Created attachment 430030 [details]
Patch (Rebase Attempt #2)

[Brent Fulgham]

(In reply to Garrett Davidson from comment #7)
> (In reply to Brent Fulgham from comment #6)
> 
> > You didn't address Jiewen's comment that this isn't needed in the Web layer,
> > so serializing it back/forth to the WebContent process seems wasteful. Can
> > we make it Optional<>?
> 
> I'm new in this codebase, so this is probably just my lack of understanding,
> but what's being "wasted" here? Isn't it the same amount of space to encode
> an empty Optional<> as it is to encode an int? If we go to Optional<> route,
> we effectively end up with a partially initialized object which has more
> complex usage because you have to add code to handle not having a value, as
> opposed to guaranteeing it's always there.

Fair enough. We encode a nullptr for this case, which would be the same size in IPC as an integer value.

> > Jiewen suggested you move this to the header: Any reason not to?
> 
> I tried moving it to the header and ended up getting errors about an
> exported symbol being defined in multiple compilation units. My assumption
> was that you can't mix `WEBCORE_EXPORT` with a method defined in a header,
> but if I missed something I'm happy to move it :)

Ah, yes. That's true. Inline and WEBCORE_EXPORT don't mix.

[Brent Fulgham]

Comment on attachment 430030 [details]
Patch (Rebase Attempt #2)

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

> Source/WebCore/Modules/webauthn/AuthenticatorCoordinatorClient.h:30
> +#include "AuthenticatorAttachment.h"

It would be better if we could forward-declare this, and include in the implementation file.

[Garrett Davidson]

(In reply to Brent Fulgham from comment #10)
> Comment on attachment 430030 [details]
> Patch (Rebase Attempt #2)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430030&action=review
> 
> > Source/WebCore/Modules/webauthn/AuthenticatorCoordinatorClient.h:30
> > +#include "AuthenticatorAttachment.h"
> 
> It would be better if we could forward-declare this, and include in the
> implementation file.

The only thing in AuthenticatorAttachment.h is the AuthenticatorAttachment enum. Can that be forward declared?

[Garrett Davidson]

Created attachment 430050 [details]
Rebased patch with fixed tests

[Alex Christensen]

Comment on attachment 430050 [details]
Rebased patch with fixed tests

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

> Source/WebCore/Modules/webauthn/AuthenticatorResponse.h:67
> +    AuthenticatorAttachment m_attachment;

Since we're storing this, this would probably be a good time to reduce the size.  Replace "enum class AuthenticatorAttachment" with "enum class AuthenticatorAttachment : bool", and you can also remove the EnumTraits in AuthenticatorAttachment.h

> Source/WebKit/UIProcess/WebAuthentication/WebAuthenticatorCoordinatorProxy.cpp:75
> +            handler({ }, (AuthenticatorAttachment)0, exception);

This is actually AuthenticatorAttachment::Platform.  Is that what you want?  Maybe you want Optional<AuthenticatorAttachment>

> Source/WebKit/WebAuthnProcess/WebAuthnConnectionToWebProcess.cpp:87
> +            handler({ }, (AuthenticatorAttachment)0, exception);

ditto

> Tools/ChangeLog:1
> [phone]  Garrett Davidson  <[e-mail]>

Is this the email address you want to use?

> Tools/ChangeLog:11
> +        Update the CTAP tests to specify the new attachment parameter. All of these tests
> +        assume a cross platform authenticator.

Is it possible to add a test that uses AuthenticatorAttachment::Platform?

[Darin Adler]

Comment on attachment 430050 [details]
Rebased patch with fixed tests

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

>> Source/WebCore/Modules/webauthn/AuthenticatorResponse.h:67
>> +    AuthenticatorAttachment m_attachment;
> 
> Since we're storing this, this would probably be a good time to reduce the size.  Replace "enum class AuthenticatorAttachment" with "enum class AuthenticatorAttachment : bool", and you can also remove the EnumTraits in AuthenticatorAttachment.h

Please note that ": bool" does not depend on "enum" vs. "enum class". Could do that whether or not we change this to an enum class.

[Brent Fulgham]

Created attachment 430291 [details]
Garrett's Patch with forward declaration.

[Brent Fulgham]

Comment on attachment 430291 [details]
Garrett's Patch with forward declaration.

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

Thanks for fixing the API tests. r=me, but waiting for EWS to complete.

> Source/WebKit/WebAuthnProcess/WebAuthnConnectionToWebProcess.cpp:87
> +            handler({ }, (AuthenticatorAttachment)0, exception);

This should be a C++ style cast. Or should we just be hard-coding it as AuthenticatorAttachment::Platform?

[Brent Fulgham]

Created attachment 430295 [details]
Garrett's Patch with fwd declaration (and the cast)

[EWS]

Committed r278358 (238390@main): <https://commits.webkit.org/238390@main>

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