Skip to content

Commit

Permalink
Revert "webauthn: implement getPublicKey() and friends."
Browse files Browse the repository at this point in the history 
This reverts commit c5d2367.

Reason for revert: I suspect that this CL causes Fido2CredentialRequestTest failures on android-pie-x86-rel:
https://ci.chromium.org/p/chromium/builders/ci/android-pie-x86-rel

Here is a snippet of callstack:
java.lang.AssertionError: expected:<11> but was:<0>
	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.failNotEquals(Assert.java:834)
	at org.junit.Assert.assertEquals(Assert.java:118)
	at org.junit.Assert.assertEquals(Assert.java:144)
	at org.chromium.chrome.browser.webauth.Fido2CredentialRequestTest.testMakeCredential_attestationIndirect(Fido2CredentialRequestTest.java:921)

Here is sample failure:
https://00e9e64bac4dbda9c3b8a07931c85e48f4420a03929ffbcaf5-apidata.googleusercontent.com/download/storage/v1/b/chromium-result-details/o/html%2Fchrome_public_test_apk_android-pie-x86-rel_990_2020_06_09_T16_33_45-UTC?qk=AD5uMEvQvMK8K2ZyV-tHIDwdTtLEnQK-X1Mw0ohWLQeOpgogj0sVgm_7y9e5CdF-FMV6ObBmwjpVxD6Jh8_7VTv4z2FpOutuS8Gd6qQH5OxikfytS8cKFb_SRCCQXIgWq45BwD7DediOcKTF-UZea_JAoFuHO0ljO0PRqnkFYSoIHcPONuqaszXChp63b5o4VAKYsofAHDhpHru_hiAf6xUyfK91I8cr-2sdHjQU6Ks4XQIIWVnq2PSAiysd6hNeq51qWPEaX7HS8hCwWIpe55Xq9hIWLOtab3MY1WLLxM3UuWMY8IDCKfcvnrTiQMcRyeD7Iii33jHhC8-rDGttfDEl39kpFnWynDC-C0TmPFoCJVkl7mHxz0uCNlQJ3I0Rt_F2DTqA96qIKrpwEltNs96-0j1VgSOJCjU3s4od6h7juoIxpH5HThyg7G-fB1-MWzv7mQfAE5ZBilodU0CKMsFrnN_5Kf4thAbDj-RJUiUNpy0A9swLOhMLjEsfxQeDRldVvy9OVUNvcakhJziml5iA7LWoDB58E3eGTyUi-dA5aOLN3KWLR7T452j4MwnCHC5vTRNxxGxCkemgQQDkl1fHJas1u6GUcY4_ncvhIuGvLsfg8uqWpzFuau4CFamGC2USpyVpNIeNhpcf2w-lYzG1rkHjHWYgUKSvhoaItY3oeRFjnkYMgZrDsy8QmneuVVGTPd4nzoWWaCZbaG9ZaF80xRu9YhYnTUXI7GqQF1rlst09Mqu8IvDXo9dgYzodInIA6p3H7azZgnvCfzYUxL4QVrvskyjTQCITmz-o2W6CPUzBqWJhWLRTpZYcVSV7z8zjj1cFwDQPBKpJzQDRrtC1sZCM0WrmQ7oSNeU-AH3uBaNWeGBF2rkY8-9gx8dlr38DGQvXoKZVWHWi61jVnQhac27ctKBLKQ&isca=1

Original change's description:
> webauthn: implement getPublicKey() and friends.
> 
> This change implements the getPublicKey(), getPublicKeyAlgorithm(), and
> getAuthenticatorData() functions described in the editor's draft of
> WebAuthn level two[1].
> 
> [1] https://w3c.github.io/webauthn/#sctn-public-key-easy
> 
> BUG=1083301
> 
> Change-Id: I3a03279d5239a9f8df50a78f2166f30d01d38d3e
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2204087
> Reviewed-by: Andrew Grieve <agrieve@chromium.org>
> Reviewed-by: Ken Buchanan <kenrb@chromium.org>
> Reviewed-by: Alex Russell <slightlyoff@chromium.org>
> Reviewed-by: Jared Saul <jsaul@google.com>
> Reviewed-by: Adam Langley <agl@chromium.org>
> Reviewed-by: Nina Satragno <nsatragno@chromium.org>
> Reviewed-by: Martin Kreichgauer <martinkr@google.com>
> Commit-Queue: Adam Langley <agl@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#776517}

TBR=kenrb@chromium.org,agl@chromium.org,slightlyoff@chromium.org,nsatragno@chromium.org,agrieve@chromium.org,jsaul@google.com,martinkr@google.com

Change-Id: I7429b5b7ecd7fa38c4e1ec2905b80a15e7c64216
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1083301
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2238749
Reviewed-by: Pavel Yatsuk <pavely@chromium.org>
Commit-Queue: Pavel Yatsuk <pavely@chromium.org>
Cr-Commit-Position: refs/heads/master@{#776637}
  • Loading branch information
Pavel Yatsuk authored and Commit Bot committed Jun 9, 2020
1 parent 2cb8141 commit e2c1b35
Show file tree
Hide file tree
Showing 20 changed files with 139 additions and 333 deletions.
1 change: 0 additions & 1 deletion chrome/android/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3063,7 +3063,6 @@ generate_jni("chrome_jni_headers") {
"java/src/org/chromium/chrome/browser/webapps/addtohomescreen/AddToHomescreenInstaller.java",
"java/src/org/chromium/chrome/browser/webapps/addtohomescreen/AddToHomescreenMediator.java",
"java/src/org/chromium/chrome/browser/webauth/AuthenticatorImpl.java",
"java/src/org/chromium/chrome/browser/webauth/Fido2Helper.java",
]

# Used for testing only, should not be shipped to end users.
Expand Down
23 changes: 7 additions & 16 deletions chrome/android/java/src/org/chromium/chrome/browser/webauth/Fido2CredentialRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -308,13 +308,9 @@ private void processPublicKeyCredential(Intent data) {
if (response instanceof AuthenticatorErrorResponse) {
processErrorResponse((AuthenticatorErrorResponse) response);
} else if (response instanceof AuthenticatorAttestationResponse) {
try {
mCallback.onRegisterResponse(AuthenticatorStatus.SUCCESS,
Fido2Helper.toMakeCredentialResponse(publicKeyCredential));
mCallback = null;
} catch (NoSuchAlgorithmException e) {
returnErrorAndResetCallback(AuthenticatorStatus.ALGORITHM_UNSUPPORTED);
}
mCallback.onRegisterResponse(AuthenticatorStatus.SUCCESS,
Fido2Helper.toMakeCredentialResponse(publicKeyCredential));
mCallback = null;
} else if (response instanceof AuthenticatorAssertionResponse) {
mCallback.onSignResponse(AuthenticatorStatus.SUCCESS,
Fido2Helper.toGetAssertionResponse(publicKeyCredential, mAppIdExtensionUsed));
Expand All @@ -335,15 +331,10 @@ private void processKeyResponse(Intent data) {
switch (mRequestStatus) {
case REGISTER_REQUEST:
Log.e(TAG, "Received a register response from Google Play Services FIDO2 API");
try {
mCallback.onRegisterResponse(AuthenticatorStatus.SUCCESS,
Fido2Helper.toMakeCredentialResponse(
AuthenticatorAttestationResponse.deserializeFromBytes(
data.getByteArrayExtra(
Fido.FIDO2_KEY_RESPONSE_EXTRA))));
} catch (NoSuchAlgorithmException e) {
returnErrorAndResetCallback(AuthenticatorStatus.ALGORITHM_UNSUPPORTED);
}
mCallback.onRegisterResponse(AuthenticatorStatus.SUCCESS,
Fido2Helper.toMakeCredentialResponse(
AuthenticatorAttestationResponse.deserializeFromBytes(
data.getByteArrayExtra(Fido.FIDO2_KEY_RESPONSE_EXTRA))));
break;
case SIGN_REQUEST:
Log.e(TAG, "Received a sign response from Google Play Services FIDO2 API");
Expand Down
44 changes: 3 additions & 41 deletions chrome/android/java/src/org/chromium/chrome/browser/webauth/Fido2Helper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@
import com.google.android.gms.fido.fido2.api.common.UvmEntries;

import org.chromium.base.Log;
import org.chromium.base.annotations.CalledByNative;
import org.chromium.base.annotations.NativeMethods;
import org.chromium.blink.mojom.AuthenticatorAttachment;
import org.chromium.blink.mojom.AuthenticatorStatus;
import org.chromium.blink.mojom.AuthenticatorTransport;
Expand Down Expand Up @@ -126,25 +124,14 @@ public static PublicKeyCredentialCreationOptions toMakeCredentialOptions(
* @return Response to be passed to the renderer.
*/
public static MakeCredentialAuthenticatorResponse toMakeCredentialResponse(
AuthenticatorAttestationResponse data) throws NoSuchAlgorithmException {
AuthenticatorAttestationResponse data) {
MakeCredentialAuthenticatorResponse response = new MakeCredentialAuthenticatorResponse();
CommonCredentialInfo info = new CommonCredentialInfo();

response.attestationObject = data.getAttestationObject();
AttestationObjectParts parts = new AttestationObjectParts();
if (!Fido2HelperJni.get().parseAttestationObject(response.attestationObject, parts)) {
// A failure to parse the attestation object is fatal to the request
// on desktop and so the same behavior is used here.
throw new NoSuchAlgorithmException();
}
response.publicKeyAlgo = parts.coseAlgorithm;
info.authenticatorData = parts.authenticatorData;
response.publicKeyDer = parts.spki;

// An empty transports array indicates that we don't have any
// information about the available transports.
response.transports = new int[] {};

info.id = encodeId(data.getKeyHandle());
info.rawId = data.getKeyHandle();
info.clientDataJson = data.getClientDataJSON();
Expand All @@ -153,7 +140,7 @@ public static MakeCredentialAuthenticatorResponse toMakeCredentialResponse(
}

public static MakeCredentialAuthenticatorResponse toMakeCredentialResponse(
PublicKeyCredential data) throws NoSuchAlgorithmException {
PublicKeyCredential data) {
MakeCredentialAuthenticatorResponse response =
toMakeCredentialResponse((AuthenticatorAttestationResponse) data.getResponse());
return response;
Expand Down Expand Up @@ -202,9 +189,9 @@ public static GetAssertionAuthenticatorResponse toGetAssertionResponse(
AuthenticatorAssertionResponse data, boolean appIdExtensionUsed) {
GetAssertionAuthenticatorResponse response = new GetAssertionAuthenticatorResponse();
CommonCredentialInfo info = new CommonCredentialInfo();
response.authenticatorData = data.getAuthenticatorData();
response.signature = data.getSignature();
response.echoAppidExtension = appIdExtensionUsed;
info.authenticatorData = data.getAuthenticatorData();
info.id = encodeId(data.getKeyHandle());
info.rawId = data.getKeyHandle();
info.clientDataJson = data.getClientDataJSON();
Expand Down Expand Up @@ -401,29 +388,4 @@ private static double adjustTimeout(TimeDelta timeout) {
Math.min(MAX_TIMEOUT_SECONDS,
TimeUnit.MICROSECONDS.toSeconds(timeout.microseconds)));
}

// AttestationObjectParts is used to group together the return values of
// |parseAttestationObject|, below.
public static final class AttestationObjectParts {
@CalledByNative("AttestationObjectParts")
void setAll(byte[] authenticatorData, byte[] spki, int coseAlgorithm) {
this.authenticatorData = authenticatorData;
this.spki = spki;
this.coseAlgorithm = coseAlgorithm;
}

public byte[] authenticatorData;
public byte[] spki;
public int coseAlgorithm;
}

@NativeMethods
interface Natives {
// parseAttestationObject parses a CTAP2 attestation[1] and extracts the
// parts that the browser provides via Javascript API [2].
//
// [1] https://www.w3.org/TR/webauthn/#attestation-object
// [2] https://w3c.github.io/webauthn/#sctn-public-key-easy
boolean parseAttestationObject(byte[] attestationObject, AttestationObjectParts result);
}
}
2 changes: 1 addition & 1 deletion chrome/android/javatests/src/org/chromium/chrome/browser/webauth/Fido2ApiTestHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ public static void validateUserVerificationMethods(
* @param response The response from the Fido2 API.
*/
public static void validateGetAssertionResponse(GetAssertionAuthenticatorResponse response) {
Assert.assertArrayEquals(response.info.authenticatorData, TEST_AUTHENTICATOR_DATA);
Assert.assertArrayEquals(response.authenticatorData, TEST_AUTHENTICATOR_DATA);
Assert.assertArrayEquals(response.signature, TEST_SIGNATURE);
Assert.assertArrayEquals(response.info.rawId, TEST_KEY_HANDLE);
Assert.assertEquals(response.info.id, TEST_ENCODED_KEY_HANDLE);
Expand Down
1 change: 0 additions & 1 deletion chrome/browser/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2794,7 +2794,6 @@ static_library("browser") {
"android/webapps/add_to_homescreen_params.h",
"android/webapps/webapp_registry.cc",
"android/webapps/webapp_registry.h",
"android/webauth/fido2helper_native_android.cc",
"autofill/accessory_controller.h",
"autofill/address_accessory_controller.h",
"autofill/address_accessory_controller_impl.cc",
Expand Down
77 changes: 0 additions & 77 deletions chrome/browser/android/webauth/fido2helper_native_android.cc
View file

This file was deleted.

2 changes: 1 addition & 1 deletion components/autofill/core/browser/payments/credit_card_fido_authenticator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,7 @@ base::Value CreditCardFIDOAuthenticator::ParseAssertionResponse(
response.SetKey("credential_id",
BytesToBase64(assertion_response->info->raw_id));
response.SetKey("authenticator_data",
BytesToBase64(assertion_response->info->authenticator_data));
BytesToBase64(assertion_response->authenticator_data));
response.SetKey("client_data",
BytesToBase64(assertion_response->info->client_data_json));
response.SetKey("signature", BytesToBase64(assertion_response->signature));
Expand Down
17 changes: 1 addition & 16 deletions content/browser/webauth/authenticator_common.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
#include "device/fido/fido_transport_protocol.h"
#include "device/fido/get_assertion_request_handler.h"
#include "device/fido/make_credential_request_handler.h"
#include "device/fido/public_key.h"
#include "device/fido/public_key_credential_descriptor.h"
#include "device/fido/public_key_credential_params.h"
#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
Expand Down Expand Up @@ -319,9 +318,6 @@ CreateMakeCredentialResponse(
auto common_info = blink::mojom::CommonCredentialInfo::New();
common_info->client_data_json.assign(client_data_json.begin(),
client_data_json.end());
common_info->authenticator_data = response_data.attestation_object()
.authenticator_data()
.SerializeToByteArray();
if (response_data.android_client_data_ext()) {
DCHECK(base::FeatureList::IsEnabled(device::kWebAuthPhoneSupport));
common_info->client_data_json = *response_data.android_client_data_ext();
Expand Down Expand Up @@ -377,17 +373,6 @@ CreateMakeCredentialResponse(
response->attestation_object =
response_data.GetCBOREncodedAttestationObject();

const device::PublicKey* public_key = response_data.attestation_object()
.authenticator_data()
.attested_data()
->public_key();
response->public_key_algo = public_key->algorithm;
const base::Optional<std::vector<uint8_t>>& public_key_der =
public_key->der_bytes;
if (public_key_der) {
response->public_key_der.emplace(public_key_der.value());
}

return response;
}

Expand All @@ -406,7 +391,7 @@ blink::mojom::GetAssertionAuthenticatorResponsePtr CreateGetAssertionResponse(
common_info->raw_id = response_data.raw_credential_id();
common_info->id = response_data.GetId();
response->info = std::move(common_info);
response->info->authenticator_data =
response->authenticator_data =
response_data.auth_data().SerializeToByteArray();
response->signature = response_data.signature();
if (echo_appid_extension) {
Expand Down
Loading

0 comments on commit e2c1b35

Please sign in to comment.