Bug 237567 – [WebAuthn] Support authenticatorSelection.residentKey ResidentKeyRequirement

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[[e-mail]]

As part of the Web Authentication level 2 spec, authenticatorSelection.residentKey to allow Relying Parties to signify their preference for client-side discoverable credentials with greater granularity, for example, creating one if possible and falling back if not via 'Preferred.'

[[e-mail]]

rdar://89788378

[[e-mail]]

Created attachment 454051 [details]
Patch

[[e-mail]]

Created attachment 454138 [details]
Patch

[Brent Fulgham]

Comment on attachment 454138 [details]
Patch

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

The logic looks good, but I think some of the naming could be clearer.

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:69
> +        std::optional<ResidentKeyRequirement> residentKey;

Suggeste calling this "residentKeyRequirement", or just "keyRequirement"

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.idl:75
> +    ResidentKeyRequirement       residentKey;

Ditto "residentKeyRequirement" or "keyRequirement"

> Source/WebKit/UIProcess/API/Cocoa/_WKAuthenticatorSelectionCriteria.h:45
> +@property (nonatomic) _WKResidentKeyRequirement residentKey;

Should this be named "residentKeyRequirement"? You aren't returning the key -- you are returning the key requirement.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:738
> +static std::optional<WebCore::ResidentKeyRequirement> residentKey(_WKResidentKeyRequirement uv)

I think we often name things like this "toWebCore()".

[Chris Dumez]

Comment on attachment 454138 [details]
Patch

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

>> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.idl:75
>> +    ResidentKeyRequirement       residentKey;
> 
> Ditto "residentKeyRequirement" or "keyRequirement"

Where is the specification for this? I see no such dictionary member in the spec: https://w3c.github.io/webauthn/#dictionary-makecredentialoptions

Is this an Apple-only extension?

[Chris Dumez]

Comment on attachment 454138 [details]
Patch

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

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:141
> +        return std::nullopt;

This never sets residentKey on result. It is decoding and ignoring the value.

I haven't looked at the rest of the patch yet but r- due to this bug.

[Chris Dumez]

Comment on attachment 454138 [details]
Patch

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

There are changes to IDLs files yet no layout tests, this is a bit suspicious. Why can't this be tested?

>>> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.idl:75
>>> +    ResidentKeyRequirement       residentKey;
>> 
>> Ditto "residentKeyRequirement" or "keyRequirement"
> 
> Where is the specification for this? I see no such dictionary member in the spec: https://w3c.github.io/webauthn/#dictionary-makecredentialoptions
> 
> Is this an Apple-only extension?

Part of the reason I'm asking is that Brent seems to be picking the name and given that this is Web-exposed, I'd expect this to be standardized?

> Source/WebCore/Modules/webauthn/ResidentKeyRequirement.h:34
> +enum class ResidentKeyRequirement {

: uint8_t

> Source/WebCore/Modules/webauthn/fido/AuthenticatorSupportedOptions.cpp:71
> +    return m_residentKeyAvailability;

One liner, should be inlined in the header.

> Source/WebCore/Modules/webauthn/fido/AuthenticatorSupportedOptions.h:54
> +    enum class ResidentKeyAvailability {

: bool

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 454138 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454138&action=review
> 
> > Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:141
> > +        return std::nullopt;
> 
> This never sets residentKey on result. It is decoding and ignoring the value.
> 
> I haven't looked at the rest of the patch yet but r- due to this bug.

This likely indicates test coverage is insufficient. Ideally the bots would have found this.

[[e-mail]]

(In reply to Chris Dumez from comment #5)
> Comment on attachment 454138 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454138&action=review
> 
> >> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.idl:75
> >> +    ResidentKeyRequirement       residentKey;
> > 
> > Ditto "residentKeyRequirement" or "keyRequirement"
> 
> Where is the specification for this? I see no such dictionary member in the
> spec: https://w3c.github.io/webauthn/#dictionary-makecredentialoptions
> 
> Is this an Apple-only extension?

(In reply to Brent Fulgham from comment #4)
> > Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:69
> > +        std::optional<ResidentKeyRequirement> residentKey;
> 
> Suggeste calling this "residentKeyRequirement", or just "keyRequirement"
> 
> > Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.idl:75
> > +    ResidentKeyRequirement       residentKey;
> 
> Ditto "residentKeyRequirement" or "keyRequirement"
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKAuthenticatorSelectionCriteria.h:45
> > +@property (nonatomic) _WKResidentKeyRequirement residentKey;
> 
> Should this be named "residentKeyRequirement"? You aren't returning the key
> -- you are returning the key requirement.

This name is in the level 2 of the Web Authentication spec here: https://www.w3.org/TR/webauthn-2/#dictdef-authenticatorselectioncriteria

Thank you both for the comments, I will address them.

[Chris Dumez]

(In reply to [e-mail] from comment #9)
> (In reply to Chris Dumez from comment #5)
> > Comment on attachment 454138 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=454138&action=review
> > 
> > >> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.idl:75
> > >> +    ResidentKeyRequirement       residentKey;
> > > 
> > > Ditto "residentKeyRequirement" or "keyRequirement"
> > 
> > Where is the specification for this? I see no such dictionary member in the
> > spec: https://w3c.github.io/webauthn/#dictionary-makecredentialoptions
> > 
> > Is this an Apple-only extension?
> 
> (In reply to Brent Fulgham from comment #4)
> > > Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:69
> > > +        std::optional<ResidentKeyRequirement> residentKey;
> > 
> > Suggeste calling this "residentKeyRequirement", or just "keyRequirement"
> > 
> > > Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.idl:75
> > > +    ResidentKeyRequirement       residentKey;
> > 
> > Ditto "residentKeyRequirement" or "keyRequirement"
> > 
> > > Source/WebKit/UIProcess/API/Cocoa/_WKAuthenticatorSelectionCriteria.h:45
> > > +@property (nonatomic) _WKResidentKeyRequirement residentKey;
> > 
> > Should this be named "residentKeyRequirement"? You aren't returning the key
> > -- you are returning the key requirement.
> 
> This name is in the level 2 of the Web Authentication spec here:
> https://www.w3.org/TR/webauthn-2/#dictdef-authenticatorselectioncriteria
> 
> Thank you both for the comments, I will address them.

This link is NOT for the same dictionary I commented on though. I commented on PublicKeyCredentialCreationOptions. The link for level 2 would be:
- https://www.w3.org/TR/webauthn-2/#dictionary-makecredentialoptions

However, I don't see that member in there.

[[e-mail]]

(In reply to Chris Dumez from comment #10)
> (In reply to [e-mail] from comment #9)
> > (In reply to Chris Dumez from comment #5)
> > > Comment on attachment 454138 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=454138&action=review
> > > 
> > > >> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.idl:75
> > > >> +    ResidentKeyRequirement       residentKey;
> > > > 
> > > > Ditto "residentKeyRequirement" or "keyRequirement"
> > > 
> > > Where is the specification for this? I see no such dictionary member in the
> > > spec: https://w3c.github.io/webauthn/#dictionary-makecredentialoptions
> > > 
> > > Is this an Apple-only extension?
> > 
> > (In reply to Brent Fulgham from comment #4)
> > > > Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:69
> > > > +        std::optional<ResidentKeyRequirement> residentKey;
> > > 
> > > Suggeste calling this "residentKeyRequirement", or just "keyRequirement"
> > > 
> > > > Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.idl:75
> > > > +    ResidentKeyRequirement       residentKey;
> > > 
> > > Ditto "residentKeyRequirement" or "keyRequirement"
> > > 
> > > > Source/WebKit/UIProcess/API/Cocoa/_WKAuthenticatorSelectionCriteria.h:45
> > > > +@property (nonatomic) _WKResidentKeyRequirement residentKey;
> > > 
> > > Should this be named "residentKeyRequirement"? You aren't returning the key
> > > -- you are returning the key requirement.
> > 
> > This name is in the level 2 of the Web Authentication spec here:
> > https://www.w3.org/TR/webauthn-2/#dictdef-authenticatorselectioncriteria
> > 
> > Thank you both for the comments, I will address them.
> 
> This link is NOT for the same dictionary I commented on though. I commented
> on PublicKeyCredentialCreationOptions. The link for level 2 would be:
> - https://www.w3.org/TR/webauthn-2/#dictionary-makecredentialoptions
> 
> However, I don't see that member in there.

The IDL for AuthenticatorSelectionCriteria (from this link https://www.w3.org/TR/webauthn-2/#dictdef-authenticatorselectioncriteria) is in PublicKeyCredentialCreationOptions.idl. AuthenticatorSelectionCriteria authenticatorSelection is a field on PublicKeyCredentialCreationOptions (The 7th member in https://www.w3.org/TR/webauthn-2/#dictionary-makecredentialoptions). So the member is in creationOptions.authenticatorSelection.residentKey.

[Chris Dumez]

(In reply to [e-mail] from comment #11)
> (In reply to Chris Dumez from comment #10)
> > (In reply to [e-mail] from comment #9)
> > > (In reply to Chris Dumez from comment #5)
> > > > Comment on attachment 454138 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=454138&action=review
> > > > 
> > > > >> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.idl:75
> > > > >> +    ResidentKeyRequirement       residentKey;
> > > > > 
> > > > > Ditto "residentKeyRequirement" or "keyRequirement"
> > > > 
> > > > Where is the specification for this? I see no such dictionary member in the
> > > > spec: https://w3c.github.io/webauthn/#dictionary-makecredentialoptions
> > > > 
> > > > Is this an Apple-only extension?
> > > 
> > > (In reply to Brent Fulgham from comment #4)
> > > > > Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:69
> > > > > +        std::optional<ResidentKeyRequirement> residentKey;
> > > > 
> > > > Suggeste calling this "residentKeyRequirement", or just "keyRequirement"
> > > > 
> > > > > Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.idl:75
> > > > > +    ResidentKeyRequirement       residentKey;
> > > > 
> > > > Ditto "residentKeyRequirement" or "keyRequirement"
> > > > 
> > > > > Source/WebKit/UIProcess/API/Cocoa/_WKAuthenticatorSelectionCriteria.h:45
> > > > > +@property (nonatomic) _WKResidentKeyRequirement residentKey;
> > > > 
> > > > Should this be named "residentKeyRequirement"? You aren't returning the key
> > > > -- you are returning the key requirement.
> > > 
> > > This name is in the level 2 of the Web Authentication spec here:
> > > https://www.w3.org/TR/webauthn-2/#dictdef-authenticatorselectioncriteria
> > > 
> > > Thank you both for the comments, I will address them.
> > 
> > This link is NOT for the same dictionary I commented on though. I commented
> > on PublicKeyCredentialCreationOptions. The link for level 2 would be:
> > - https://www.w3.org/TR/webauthn-2/#dictionary-makecredentialoptions
> > 
> > However, I don't see that member in there.
> 
> The IDL for AuthenticatorSelectionCriteria (from this link
> https://www.w3.org/TR/webauthn-2/#dictdef-authenticatorselectioncriteria) is
> in PublicKeyCredentialCreationOptions.idl. AuthenticatorSelectionCriteria
> authenticatorSelection is a field on PublicKeyCredentialCreationOptions (The
> 7th member in
> https://www.w3.org/TR/webauthn-2/#dictionary-makecredentialoptions). So the
> member is in creationOptions.authenticatorSelection.residentKey.

But you added:
ResidentKeyRequirement residentKey;

directly to the PublicKeyCredentialCreationOptions dictionary. This does not match the spec and seems wrong.

[Chris Dumez]

Comment on attachment 454138 [details]
Patch

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

>>>>>>>> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.idl:75
>>>>>>>> +    ResidentKeyRequirement       residentKey;
>>>>>>> 
>>>>>>> Ditto "residentKeyRequirement" or "keyRequirement"
>>>>>> 
>>>>>> Where is the specification for this? I see no such dictionary member in the spec: https://w3c.github.io/webauthn/#dictionary-makecredentialoptions
>>>>>> 
>>>>>> Is this an Apple-only extension?
>>>>> 
>>>>> Part of the reason I'm asking is that Brent seems to be picking the name and given that this is Web-exposed, I'd expect this to be standardized?
>>>> 
>>>> (In reply to Brent Fulgham from comment #4)
>>> 
>>> This link is NOT for the same dictionary I commented on though. I commented on PublicKeyCredentialCreationOptions. The link for level 2 would be:
>>> - https://www.w3.org/TR/webauthn-2/#dictionary-makecredentialoptions
>>> 
>>> However, I don't see that member in there.
>> 
>> The IDL for AuthenticatorSelectionCriteria (from this link https://www.w3.org/TR/webauthn-2/#dictdef-authenticatorselectioncriteria) is in PublicKeyCredentialCreationOptions.idl. AuthenticatorSelectionCriteria authenticatorSelection is a field on PublicKeyCredentialCreationOptions (The 7th member in https://www.w3.org/TR/webauthn-2/#dictionary-makecredentialoptions). So the member is in creationOptions.authenticatorSelection.residentKey.
> 
> But you added:
> ResidentKeyRequirement residentKey;
> 
> directly to the PublicKeyCredentialCreationOptions dictionary. This does not match the spec and seems wrong.

Oh, my bad. I was confused by the file name. You did had it to the right dictionary. The IDL file just contains more than 1 dictionary.

Note that you cannot change the name on IDL side like Brent suggested (since we need to match the spec). We may be able to use Brent's suggested name on native side though (by using [ImplementedAs=xxx] in the IDL. However, we'd have to decide if the lack of consistency between native and IDL is worth it.

[[e-mail]]

(In reply to Chris Dumez from comment #7)
> > Source/WebCore/Modules/webauthn/fido/AuthenticatorSupportedOptions.cpp:71
> > +    return m_residentKeyAvailability;
> 
> One liner, should be inlined in the header.


I'm doing this work around check-webkit-style. Whenever it's defined in the header I get the "Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]" error. 

There appears to be a few different common solutions to this problem that I found.

One is doing the suggested removing WEBCORE_EXPORT from the class and adding it to appropriate methods (which doesn't work because it complains about the same error when trying to export userVerificationAvailability(), for example.)

Another way is to ignore the style error (appears to be commonly done and would allow me to inline it in the header.)

Finally the way I chose here for now, moving the definition out-of-line. This keeps the style checker happy.

I'm open to changing to ignoring the style error, or another way if there is one.

[Chris Dumez]

(In reply to [e-mail] from comment #14)
> (In reply to Chris Dumez from comment #7)
> > > Source/WebCore/Modules/webauthn/fido/AuthenticatorSupportedOptions.cpp:71
> > > +    return m_residentKeyAvailability;
> > 
> > One liner, should be inlined in the header.
> 
> 
> I'm doing this work around check-webkit-style. Whenever it's defined in the
> header I get the "Inline functions should not be in classes annotated with
> WEBCORE_EXPORT. Remove the macro from the class and apply it to each
> appropriate method, or move the inline function definition out-of-line. 
> [build/webcore_export] [4]" error. 
> 
> There appears to be a few different common solutions to this problem that I
> found.
> 
> One is doing the suggested removing WEBCORE_EXPORT from the class and adding
> it to appropriate methods (which doesn't work because it complains about the
> same error when trying to export userVerificationAvailability(), for
> example.)
> 
> Another way is to ignore the style error (appears to be commonly done and
> would allow me to inline it in the header.)
> 
> Finally the way I chose here for now, moving the definition out-of-line.
> This keeps the style checker happy.
> 
> I'm open to changing to ignoring the style error, or another way if there is
> one.

I would just ignore the style error for now.

In a follow-up, we could consider marking individual non-inline functions as WEBCORE_EXPORT instead of the whole class.

[[e-mail]]

Created attachment 454382 [details]
Patch

[Chris Dumez]

Comment on attachment 454382 [details]
Patch

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

My comments seem to have been addressed and I couldn't find any obvious issue. Carrying over Brent's r+.

> Source/WebKit/UIProcess/API/Cocoa/_WKAuthenticatorSelectionCriteria.h:45
> +@property (nonatomic) _WKResidentKeyRequirement residentKey;

Seems this is missing the availability macro.

[[e-mail]]

Created attachment 454388 [details]
Patch for landing

[EWS]

Committed r291176 (248336@main): <https://commits.webkit.org/248336@main>

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