Skip to content

Commit

Permalink
Cherry-pick 7516fcd. rdar://122350269
Browse files Browse the repository at this point in the history
    Reland [WebAuthn] Use AS APIs passing clientData for WebAuthn (274023@main)
    https://bugs.webkit.org/show_bug.cgi?id=268748
    rdar://problem/122313244

    Reviewed by Brent Fulgham.

    This got rolled out because we started generating the clientDataJSON for requests
    within AuthenticationServices, which isn't currently mocked for test infrastructure.

    To fix this we generate it within WACP manually whenever we are in a test. I've
    created https://bugs.webkit.org/show_bug.cgi?id=268774 to address the test development.

    Original change description:
    We need to be able to pass clientData to AS API in order to make assertions
    and registerations in cases where callerOrigin is not the same as the passed
    rp.id. This is required because of the way ClientDataJSON is validated.

    This change stops generating ClientDataJSON in AuthenticatorCoordinator as
    it's no longer possible to pass the raw bytes / hash along. Instead we construct
    a ASPublicKeyCredentialClientData when creating the requests for ASController and
    include the callerOrigin there.

    * Source/WebCore/Modules/webauthn/AuthenticatorCoordinator.cpp:
    (WebCore::AuthenticatorCoordinator::create):
    (WebCore::AuthenticatorCoordinator::discoverFromExternalSource):
    * Source/WebCore/Modules/webauthn/AuthenticatorCoordinatorClient.h:
    * Source/WebCore/Modules/webauthn/AuthenticatorResponse.cpp:
    (WebCore::AuthenticatorResponse::tryCreate):
    * Source/WebCore/Modules/webauthn/AuthenticatorResponseData.h:
    (WebCore::AuthenticatorResponseData::AuthenticatorResponseData):
    (WebCore::AuthenticatorResponseData::getSerializableForm const):
    * Source/WebKit/Shared/WebCoreArgumentCoders.serialization.in:
    * Source/WebKit/UIProcess/WebAuthentication/Cocoa/AuthenticationServicesForwardDeclarations.h:
    * Source/WebKit/UIProcess/WebAuthentication/Cocoa/AuthenticationServicesSoftLink.h:
    * Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:
    (WebKit::WebAuthenticatorCoordinatorProxy::constructASController):
    (WebKit::WebAuthenticatorCoordinatorProxy::requestsForRegisteration):
    (WebKit::WebAuthenticatorCoordinatorProxy::requestsForAssertion):
    (WebKit::WebAuthenticatorCoordinatorProxy::performRequest):
    (WebKit::configureRegistrationRequestContext):
    (WebKit::configureAssertionOptions):
    (WebKit::configurationAssertionRequestContext):
    (WebKit::WebAuthenticatorCoordinatorProxy::contextForRequest):
    * Source/WebKit/UIProcess/WebAuthentication/WebAuthenticatorCoordinatorProxy.cpp:
    (WebKit::WebAuthenticatorCoordinatorProxy::makeCredential):
    (WebKit::WebAuthenticatorCoordinatorProxy::getAssertion):
    * Source/WebKit/UIProcess/WebAuthentication/WebAuthenticatorCoordinatorProxy.h:
    * Source/WebKit/UIProcess/WebAuthentication/WebAuthenticatorCoordinatorProxy.messages.in:
    * Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:
    (WebKit::WebAuthenticatorCoordinator::makeCredential):
    (WebKit::WebAuthenticatorCoordinator::getAssertion):
    * Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.h:

    Canonical link: https://commits.webkit.org/274120@main

Identifier: 273664.407@safari-7619.1.3-branch
  • Loading branch information
Pascoe authored and MyahCobbs committed Feb 7, 2024
1 parent 87d57ec commit 9cb457c
Show file tree
Hide file tree
Showing 16 changed files with 129 additions and 76 deletions.
8 changes: 8 additions & 0 deletions LayoutTests/TestExpectations
Original file line number Diff line number Diff line change
Expand Up @@ -1859,6 +1859,14 @@ imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/a
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/networkState_during_progress.html [ Failure Pass ]
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-object-element/object-fallback-failed-cross-origin-navigation.sub.html [ Failure Pass ]

# Requires additional test development to enable
webkit.org/b/268774 http/wpt/webauthn/idl.https.html [ Failure Pass ]
webkit.org/b/268774 http/wpt/webauthn/public-key-credential-create-success-ccid.https.html [ Failure Pass ]
webkit.org/b/268774 http/wpt/webauthn/public-key-credential-create-success-local.https.html [ Failure Pass ]
webkit.org/b/268774 http/wpt/webauthn/public-key-credential-create-success-u2f.https.html [ Failure Pass ]
webkit.org/b/268774 http/wpt/webauthn/public-key-credential-get-success-ccid.https.html [ Failure Pass ]
webkit.org/b/268774 http/wpt/webauthn/public-key-credential-get-success-hid.https.html [ Failure Pass ]

# Newly importing W3C tests needed support for reftest-wait.
webkit.org/b/186045 imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/track/track-element/track-cue-rendering-after-controls-added.html [ Skip ]
webkit.org/b/186045 imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/track/track-element/track-cue-rendering-after-controls-removed.html [ Skip ]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ ScopeAndCrossOriginParent CredentialsContainer::scopeAndCrossOriginParent() cons
if (!crossOriginParent)
return std::pair { WebAuthn::Scope::SameOrigin, std::nullopt };
if (isSameSite)
return std::pair { WebAuthn::Scope::SameSite, std::nullopt };
return std::pair { WebAuthn::Scope::SameSite, crossOriginParent };
return std::pair { WebAuthn::Scope::CrossOrigin, crossOriginParent };
}

Expand Down
26 changes: 8 additions & 18 deletions Source/WebCore/Modules/webauthn/AuthenticatorCoordinator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -167,10 +167,6 @@ void AuthenticatorCoordinator::create(const Document& document, CredentialCreati

options.extensions = extensionInputs;

// Step 14-16.
auto clientDataJson = buildClientDataJson(ClientDataType::Create, options.challenge, callerOrigin, scope);
auto clientDataJsonHash = buildClientDataJsonHash(clientDataJson);

// Step 4, 18-22.
if (!m_client) {
promise.reject(Exception { ExceptionCode::UnknownError, "Unknown internal error."_s });
Expand All @@ -193,14 +189,13 @@ void AuthenticatorCoordinator::create(const Document& document, CredentialCreati
});
}

auto callback = [weakThis = WeakPtr { *this }, clientDataJson = WTFMove(clientDataJson), promise = WTFMove(promise), abortSignal = WTFMove(abortSignal)] (AuthenticatorResponseData&& data, AuthenticatorAttachment attachment, ExceptionData&& exception) mutable {
auto callback = [weakThis = WeakPtr { *this }, promise = WTFMove(promise), abortSignal = WTFMove(abortSignal)] (AuthenticatorResponseData&& data, AuthenticatorAttachment attachment, ExceptionData&& exception) mutable {
if (abortSignal && abortSignal->aborted()) {
promise.reject(Exception { ExceptionCode::AbortError, "Aborted by AbortSignal."_s });
return;
}

if (auto response = AuthenticatorResponse::tryCreate(WTFMove(data), attachment)) {
response->setClientDataJSON(WTFMove(clientDataJson));
promise.resolve(PublicKeyCredential::create(response.releaseNonNull()).ptr());
return;
}
Expand All @@ -209,19 +204,19 @@ void AuthenticatorCoordinator::create(const Document& document, CredentialCreati
};

if (m_isCancelling) {
m_queuedRequest = [weakThis = WeakPtr { *this }, weakFrame = WeakPtr { *frame }, clientDataJsonHash = WTFMove(clientDataJsonHash), createOptions = WTFMove(createOptions), callback = WTFMove(callback)]() mutable {
m_queuedRequest = [weakThis = WeakPtr { *this }, weakFrame = WeakPtr { *frame }, createOptions = WTFMove(createOptions), callback = WTFMove(callback)]() mutable {
if (!weakThis || !weakFrame)
return;
const auto options = createOptions.publicKey.value();
RefPtr frame = weakFrame.get();
if (!frame)
return;
weakThis->m_client->makeCredential(*weakFrame, clientDataJsonHash, options, createOptions.mediation, WTFMove(callback));
weakThis->m_client->makeCredential(*weakFrame, options, createOptions.mediation, WTFMove(callback));
};
return;
}
// Async operations are dispatched and handled in the messenger.
m_client->makeCredential(*frame, clientDataJsonHash, options, createOptions.mediation, WTFMove(callback));
m_client->makeCredential(*frame, options, createOptions.mediation, WTFMove(callback));
}

void AuthenticatorCoordinator::discoverFromExternalSource(const Document& document, CredentialRequestOptions&& requestOptions, const ScopeAndCrossOriginParent& scopeAndCrossOriginParent, CredentialPromise&& promise)
Expand Down Expand Up @@ -269,10 +264,6 @@ void AuthenticatorCoordinator::discoverFromExternalSource(const Document& docume
options.extensions->appid = appid;
}

// Step 10-12.
auto clientDataJson = buildClientDataJson(ClientDataType::Get, options.challenge, callerOrigin, scopeAndCrossOriginParent.first);
auto clientDataJsonHash = buildClientDataJsonHash(clientDataJson);

// Step 4, 14-19.
if (!m_client) {
promise.reject(Exception { ExceptionCode::UnknownError, "Unknown internal error."_s });
Expand All @@ -295,14 +286,13 @@ void AuthenticatorCoordinator::discoverFromExternalSource(const Document& docume
});
}

auto callback = [weakThis = WeakPtr { *this }, clientDataJson = WTFMove(clientDataJson), promise = WTFMove(promise), abortSignal = WTFMove(requestOptions.signal)] (AuthenticatorResponseData&& data, AuthenticatorAttachment attachment, ExceptionData&& exception) mutable {
auto callback = [weakThis = WeakPtr { *this }, promise = WTFMove(promise), abortSignal = WTFMove(requestOptions.signal)] (AuthenticatorResponseData&& data, AuthenticatorAttachment attachment, ExceptionData&& exception) mutable {
if (abortSignal && abortSignal->aborted()) {
promise.reject(Exception { ExceptionCode::AbortError, "Aborted by AbortSignal."_s });
return;
}

if (auto response = AuthenticatorResponse::tryCreate(WTFMove(data), attachment)) {
response->setClientDataJSON(WTFMove(clientDataJson));
promise.resolve(PublicKeyCredential::create(response.releaseNonNull()).ptr());
return;
}
Expand All @@ -311,19 +301,19 @@ void AuthenticatorCoordinator::discoverFromExternalSource(const Document& docume
};

if (m_isCancelling) {
m_queuedRequest = [weakThis = WeakPtr { *this }, weakFrame = WeakPtr { *frame }, clientDataJsonHash = WTFMove(clientDataJsonHash), requestOptions = WTFMove(requestOptions), scopeAndCrossOriginParent, callback = WTFMove(callback)]() mutable {
m_queuedRequest = [weakThis = WeakPtr { *this }, weakFrame = WeakPtr { *frame }, requestOptions = WTFMove(requestOptions), scopeAndCrossOriginParent, callback = WTFMove(callback)]() mutable {
if (!weakThis || !weakFrame)
return;
const auto options = requestOptions.publicKey.value();
RefPtr frame = weakFrame.get();
if (!frame)
return;
weakThis->m_client->getAssertion(*weakFrame, clientDataJsonHash, options, requestOptions.mediation, scopeAndCrossOriginParent, WTFMove(callback));
weakThis->m_client->getAssertion(*weakFrame, options, requestOptions.mediation, scopeAndCrossOriginParent, WTFMove(callback));
};
return;
}
// Async operations are dispatched and handled in the messenger.
m_client->getAssertion(*frame, clientDataJsonHash, options, requestOptions.mediation, scopeAndCrossOriginParent, WTFMove(callback));
m_client->getAssertion(*frame, options, requestOptions.mediation, scopeAndCrossOriginParent, WTFMove(callback));
}

void AuthenticatorCoordinator::isUserVerifyingPlatformAuthenticatorAvailable(const Document& document, DOMPromiseDeferred<IDLBoolean>&& promise) const
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ class AuthenticatorCoordinatorClient : public CanMakeWeakPtr<AuthenticatorCoordi
AuthenticatorCoordinatorClient() = default;
virtual ~AuthenticatorCoordinatorClient() = default;

virtual void makeCredential(const LocalFrame&, const Vector<uint8_t>&, const PublicKeyCredentialCreationOptions&, MediationRequirement, RequestCompletionHandler&&) = 0;
virtual void getAssertion(const LocalFrame&, const Vector<uint8_t>&, const PublicKeyCredentialRequestOptions&, MediationRequirement, const ScopeAndCrossOriginParent&, RequestCompletionHandler&&) = 0;
virtual void makeCredential(const LocalFrame&, const PublicKeyCredentialCreationOptions&, MediationRequirement, RequestCompletionHandler&&) = 0;
virtual void getAssertion(const LocalFrame&, const PublicKeyCredentialRequestOptions&, MediationRequirement, const ScopeAndCrossOriginParent&, RequestCompletionHandler&&) = 0;
virtual void isConditionalMediationAvailable(const SecurityOrigin&, QueryCompletionHandler&&) = 0;
virtual void isUserVerifyingPlatformAuthenticatorAvailable(const SecurityOrigin&, QueryCompletionHandler&&) = 0;
virtual void getClientCapabilities(const SecurityOrigin&, CapabilitiesCompletionHandler&&) = 0;
Expand Down
5 changes: 4 additions & 1 deletion Source/WebCore/Modules/webauthn/AuthenticatorResponse.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,16 @@ RefPtr<AuthenticatorResponse> AuthenticatorResponse::tryCreate(AuthenticatorResp
auto response = AuthenticatorAttestationResponse::create(data.rawId.releaseNonNull(), data.attestationObject.releaseNonNull(), attachment, WTFMove(data.transports));
if (data.extensionOutputs)
response->setExtensions(WTFMove(*data.extensionOutputs));
response->setClientDataJSON(data.clientDataJSON.releaseNonNull());
return WTFMove(response);
}

if (!data.authenticatorData || !data.signature)
return nullptr;

return AuthenticatorAssertionResponse::create(data.rawId.releaseNonNull(), data.authenticatorData.releaseNonNull(), data.signature.releaseNonNull(), WTFMove(data.userHandle), WTFMove(data.extensionOutputs), attachment);
Ref response = AuthenticatorAssertionResponse::create(data.rawId.releaseNonNull(), data.authenticatorData.releaseNonNull(), data.signature.releaseNonNull(), WTFMove(data.userHandle), WTFMove(data.extensionOutputs), attachment);
response->setClientDataJSON(data.clientDataJSON.releaseNonNull());
return WTFMove(response);
}

AuthenticatorResponseData AuthenticatorResponse::data() const
Expand Down
2 changes: 1 addition & 1 deletion Source/WebCore/Modules/webauthn/AuthenticatorResponse.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class AuthenticatorResponse : public RefCounted<AuthenticatorResponse> {
WEBCORE_EXPORT ArrayBuffer* rawId() const;
WEBCORE_EXPORT void setExtensions(AuthenticationExtensionsClientOutputs&&);
WEBCORE_EXPORT AuthenticationExtensionsClientOutputs extensions() const;
void setClientDataJSON(Ref<ArrayBuffer>&&);
WEBCORE_EXPORT void setClientDataJSON(Ref<ArrayBuffer>&&);
ArrayBuffer* clientDataJSON() const;
WEBCORE_EXPORT AuthenticatorAttachment attachment() const;

Expand Down
10 changes: 8 additions & 2 deletions Source/WebCore/Modules/webauthn/AuthenticatorResponseData.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,15 @@ struct AuthenticatorResponseBaseData {
struct AuthenticatorAttestationResponseData {
RefPtr<ArrayBuffer> rawId;
std::optional<AuthenticationExtensionsClientOutputs> extensionOutputs;
RefPtr<ArrayBuffer> clientDataJSON;
RefPtr<ArrayBuffer> attestationObject;
Vector<WebCore::AuthenticatorTransport> transports;
};

struct AuthenticatorAssertionResponseData {
RefPtr<ArrayBuffer> rawId;
std::optional<AuthenticationExtensionsClientOutputs> extensionOutputs;
RefPtr<ArrayBuffer> clientDataJSON;
RefPtr<ArrayBuffer> authenticatorData;
RefPtr<ArrayBuffer> signature;
RefPtr<ArrayBuffer> userHandle;
Expand All @@ -70,11 +72,13 @@ struct AuthenticatorResponseData {
isAuthenticatorAttestationResponse = true;
rawId = v.rawId;
extensionOutputs = v.extensionOutputs;
clientDataJSON = v.clientDataJSON;
attestationObject = v.attestationObject;
transports = v.transports;
}, [&](const AuthenticatorAssertionResponseData& v) {
rawId = v.rawId;
extensionOutputs = v.extensionOutputs;
clientDataJSON = v.clientDataJSON;
authenticatorData = v.authenticatorData;
signature = v.signature;
userHandle = v.userHandle;
Expand All @@ -89,6 +93,8 @@ struct AuthenticatorResponseData {
// Extensions
std::optional<AuthenticationExtensionsClientOutputs> extensionOutputs;

RefPtr<ArrayBuffer> clientDataJSON;

// AuthenticatorAttestationResponse
RefPtr<ArrayBuffer> attestationObject;

Expand All @@ -105,12 +111,12 @@ struct AuthenticatorResponseData {
return nullptr;

if (isAuthenticatorAttestationResponse && attestationObject)
return AuthenticatorAttestationResponseData { rawId, extensionOutputs, attestationObject, transports };
return AuthenticatorAttestationResponseData { rawId, extensionOutputs, clientDataJSON, attestationObject, transports };

if (!authenticatorData || !signature)
return AuthenticatorResponseBaseData { rawId, extensionOutputs };

return AuthenticatorAssertionResponseData { rawId, extensionOutputs, authenticatorData, signature, userHandle };
return AuthenticatorAssertionResponseData { rawId, extensionOutputs, clientDataJSON, authenticatorData, signature, userHandle };
}
};

Expand Down
2 changes: 2 additions & 0 deletions Source/WebKit/Shared/WebCoreArgumentCoders.serialization.in
Original file line number Diff line number Diff line change
Expand Up @@ -1464,13 +1464,15 @@ struct WebCore::WebLockManagerSnapshot {
[CustomHeader] struct WebCore::AuthenticatorAttestationResponseData {
RefPtr<JSC::ArrayBuffer> rawId;
std::optional<WebCore::AuthenticationExtensionsClientOutputs> extensionOutputs;
RefPtr<JSC::ArrayBuffer> clientDataJSON;
RefPtr<JSC::ArrayBuffer> attestationObject;
Vector<WebCore::AuthenticatorTransport> transports;
};

[CustomHeader] struct WebCore::AuthenticatorAssertionResponseData {
RefPtr<JSC::ArrayBuffer> rawId;
std::optional<WebCore::AuthenticationExtensionsClientOutputs> extensionOutputs;
RefPtr<JSC::ArrayBuffer> clientDataJSON;
RefPtr<JSC::ArrayBuffer> authenticatorData;
RefPtr<JSC::ArrayBuffer> signature;
RefPtr<JSC::ArrayBuffer> userHandle;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,11 @@ typedef NS_ENUM(NSInteger, ASAuthorizationPublicKeyCredentialLargeBlobAssertionO

- (ASAuthorizationPlatformPublicKeyCredentialAssertionRequest *)createCredentialAssertionRequestWithChallenge:(NSData *)challenge NS_SWIFT_NAME(createCredentialAssertionRequest(challenge:));

- (ASAuthorizationPlatformPublicKeyCredentialRegistrationRequest *)createCredentialRegistrationRequestWithClientData:(ASPublicKeyCredentialClientData *)clientData name:(NSString *)name userID:(NSData *)userID;

- (ASAuthorizationPlatformPublicKeyCredentialAssertionRequest *)createCredentialAssertionRequestWithClientData:(ASPublicKeyCredentialClientData *)clientData;


@property (nonatomic, readonly, copy) NSString *relyingPartyIdentifier;
@end

Expand Down Expand Up @@ -323,6 +328,10 @@ typedef NSString *ASAuthorizationPublicKeyCredentialResidentKeyPreference;

- (ASAuthorizationSecurityKeyPublicKeyCredentialAssertionRequest *)createCredentialAssertionRequestWithChallenge:(NSData *)challenge NS_SWIFT_NAME(createCredentialAssertionRequest(challenge:));

- (ASAuthorizationSecurityKeyPublicKeyCredentialRegistrationRequest *)createCredentialRegistrationRequestWithClientData:(ASPublicKeyCredentialClientData *)clientData displayName:(NSString *)displayName name:(NSString *)name userID:(NSData *)userID;

- (ASAuthorizationSecurityKeyPublicKeyCredentialAssertionRequest *)createCredentialAssertionRequestWithClientData:(ASPublicKeyCredentialClientData *)clientData;

@property (nonatomic, readonly, copy) NSString *relyingPartyIdentifier;

@end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ SOFT_LINK_CLASS_FOR_HEADER(WebKit, ASAuthorization);
SOFT_LINK_CLASS_FOR_HEADER(WebKit, ASAuthorizationController);
SOFT_LINK_CLASS_FOR_HEADER(WebKit, ASAuthorizationPlatformPublicKeyCredentialProvider);
SOFT_LINK_CLASS_FOR_HEADER(WebKit, ASAuthorizationSecurityKeyPublicKeyCredentialProvider);
SOFT_LINK_CLASS_FOR_HEADER(WebKit, ASAuthorizationWebBrowserPlatformPublicKeyCredentialProvider);
SOFT_LINK_CLASS_FOR_HEADER(WebKit, ASPublicKeyCredentialClientData);
SOFT_LINK_CLASS_FOR_HEADER(WebKit, ASAuthorizationPlatformPublicKeyCredentialRegistration);
SOFT_LINK_CLASS_FOR_HEADER(WebKit, ASAuthorizationSecurityKeyPublicKeyCredentialRegistration);
Expand Down
Loading

0 comments on commit 9cb457c

Please sign in to comment.