macos authenticator #337
macos authenticator #337
Conversation
| @@ -0,0 +1,3 @@ | |||
| # MacAuthn | |||
|
|
|||
| A description of this package. | |||
There was a problem hiding this comment.
I know it requires a fully bundled app with entitlements, can you please note the specifics since you've done it previously? It'd save some research for future-folks.
| DerivedData/ | ||
| .swiftpm/config/registries.json | ||
| .swiftpm/xcode/package.xcworkspace/contents.xcworkspacedata | ||
| .netrc |
There was a problem hiding this comment.
A nested .gitignore is a dark pattern here, can you move the things up to the root one please.
| if options.authenticatorSelection.requireResidentKey == true { | ||
| securityKeyRequest.residentKeyPreference = .required | ||
| } else { | ||
| securityKeyRequest.residentKeyPreference = .preferred |
There was a problem hiding this comment.
This should use the value of AuthenticatorSelectionCriteria.resident_key; and default to discouraged if require_resident_key = false: https://www.w3.org/TR/webauthn-2/#dom-authenticatorselectioncriteria-residentkey
At the moment, this will always try to use resident keys if a key supports them, potentially filling the authenticator's limited storage with useless RKs.
There was a problem hiding this comment.
Agreed, this needs to default to discouraged. Because the current code here effectively is forcing "true" on all code paths.
| /// Perform a registration action using the ASAuthorization API. | ||
| fn perform_register( | ||
| &mut self, | ||
| _origin: Url, |
There was a problem hiding this comment.
Because macOS is using the RP ID for both the origin field of the Client Data JSON and MakeCredentialRequest.(RelyingParty)rp.id, and doesn't have a way to pass other RelyingParty fields, I feel there should be an explicit check here.
Windows passes them as two separate fields, and I think our native CTAP2 implementation doesn't actually check it.
| // Setting allowedCredentials can hang for some reason: https://developer.apple.com/forums/thread/727267 | ||
| securityKeyRequest.allowedCredentials = [] | ||
|
|
||
| let authController = ASAuthorizationController(authorizationRequests: [platformKeyRequest, securityKeyRequest]) |
There was a problem hiding this comment.
Given that ASAuthorizationSecurityKeyPublicKeyCredentialProvider is extremely limited (compared to Windows), I feel like this should be optional (ie: default to both, but use authenticator_attachment to select which backends it tries to use).
| window.center() | ||
| window.makeKeyAndOrderFront(window) | ||
|
|
||
| let applicationDelegate = ApplicationDelegate(window: window, authController: authController) |
There was a problem hiding this comment.
Could this section cause conflicts if there's already a running NSApplication for the process?
| if options.authenticatorSelection.requireResidentKey == true { | ||
| securityKeyRequest.residentKeyPreference = .required | ||
| } else { | ||
| securityKeyRequest.residentKeyPreference = .preferred |
There was a problem hiding this comment.
Agreed, this needs to default to discouraged. Because the current code here effectively is forcing "true" on all code paths.
experimental hacking