Skip to content

Commit

Permalink
Revert "Reland "Reland "Android CCV/CRS: Turn on ChromeRootStoreUsed …
Browse files Browse the repository at this point in the history 
…in fieldtial_testing_config.json"""

This reverts commit 7a9033f.

Reason for revert: Break chrome_test_apk on the following builders:
https://ci.chromium.org/ui/p/chrome/builders/ci/android-arm64-tests/13078/overview
https://ci.chromium.org/ui/p/chrome/builders/ci/android-arm-tests/17805/overview


Original change's description:
> Reland "Reland "Android CCV/CRS: Turn on ChromeRootStoreUsed in fieldtial_testing_config.json""
>
> This reverts commit 38ddf3f.
>
> Reason for revert: https://bugs.chromium.org/p/chromium/issues/detail?id=1446426 was found and fixed, hoping that's the cause. Can't repro anymore locally, and CQ run with android-internal-rel and android-internal-dbg run passed.
>
> Original change's description:
> > Revert "Reland "Android CCV/CRS: Turn on ChromeRootStoreUsed in fieldtial_testing_config.json""
> >
> > This reverts commit 212497b.
> >
> > Reason for revert: Causing android-arm-tests to fail chrome_test_apk with "EmbeddedTestServer$EmbeddedTestServerFailure: Failed to install root certificate"
> >
> > Original change's description:
> > > Reland "Android CCV/CRS: Turn on ChromeRootStoreUsed in fieldtial_testing_config.json"
> > >
> > > This reverts commit 3cab5ea.
> > >
> > > Reason for revert: Original change broke internal bots when code was run statically on class load; moving back to a ClassHook implementation.
> > >
> > > Original change's description:
> > > > Revert "Android CCV/CRS: Turn on ChromeRootStoreUsed in fieldtial_testing_config.json"
> > > >
> > > > This reverts commit c55272f.
> > > >
> > > > Reason for revert: Broke internal bots
> > > > https://bugs.chromium.org/p/chromium/issues/detail?id=1442472
> > > >
> > > > Original change's description:
> > > > > Android CCV/CRS: Turn on ChromeRootStoreUsed in fieldtial_testing_config.json
> > > > >
> > > > > This required modifying tests to ensure that the test root is loaded in X509Utils.java via a ClassHook in ContentJUnit4ClassRunner before the user-added roots are pulled in net/cert/internal/trust_store_android.cc, so that the chrome cert verifier would correctly verify trust for connections to the EmbeddedTestServer. Previous attempts to do this did this at other points in the test loading process, but were unsuccessful for a variety of different reasons, usually in loading the root too late.
> > > > >
> > > > > We also rely on the fact that the root cert for EmbeddedTestServer is a statically checked in file (at net/data/ssl/certificates/root_ca_cert.pem) as the current solution (starting up the EmbeddedTestServer in a separate APK + calling to get the root cert) requires calling into native code which can't be done easily before the chrome cert verifier loads the user-added roots.
> > > > >
> > > > > This would be easier if the EmbeddedTestServer was run in process instead of in its own APK; I don't know if it would be feasible or how much work it would be to change this.
> > > > >
> > > > > Change-Id: Ie035190666dfe32ec7b13d66792ff6cbd7b3c1e7
> > > > > Cq-Do-Not-Cancel-Tryjobs: true
> > > > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4490464
> > > > > Commit-Queue: Hubert Chao <hchao@chromium.org>
> > > > > Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
> > > > > Reviewed-by: Andrew Grieve <agrieve@chromium.org>
> > > > > Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
> > > > > Reviewed-by: David Benjamin <davidben@chromium.org>
> > > > > Cr-Commit-Position: refs/heads/main@{#1138721}
> > > >
> > > > Bug: 1442472
> > > > Change-Id: I4dafbcbc606ec6e05161721aaa96c0654bc42a13
> > > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4508199
> > > > Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> > > > Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
> > > > Commit-Queue: Andrew Grieve <agrieve@chromium.org>
> > > > Owners-Override: Andrew Grieve <agrieve@chromium.org>
> > > > Cr-Commit-Position: refs/heads/main@{#1140285}
> > >
> > > Bug: 1365571
> > > Change-Id: If028ab634564d34881c4af917fbcabc6281132dc
> > > Cq-Do-Not-Cancel-Tryjobs: true
> > > Include-Ci-Only-Tests: true
> > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4512699
> > > Reviewed-by: David Benjamin <davidben@chromium.org>
> > > Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
> > > Reviewed-by: Andrew Grieve <agrieve@chromium.org>
> > > Commit-Queue: Hubert Chao <hchao@chromium.org>
> > > Cr-Commit-Position: refs/heads/main@{#1143409}
> >
> > Bug: 1365571, 1442472
> > Change-Id: I7e6c92d4aea327eb8718d570360085575e6d5699
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4540342
> > Auto-Submit: Sky Malice <skym@chromium.org>
> > Commit-Queue: Sky Malice <skym@chromium.org>
> > Owners-Override: Sky Malice <skym@chromium.org>
> > Reviewed-by: Andrew Grieve <agrieve@chromium.org>
> > Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
> > Cr-Commit-Position: refs/heads/main@{#1144960}
>
> Bug: 1365571, 1442472
> Change-Id: I2bf4e7f615c4de063b6cb19f2db34bc5f5d12bd6
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4544545
> Reviewed-by: David Benjamin <davidben@chromium.org>
> Reviewed-by: Andrew Grieve <agrieve@chromium.org>
> Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
> Commit-Queue: Hubert Chao <hchao@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1149712}

Bug: 1365571, 1442472
Change-Id: I5035ed7d2b368e503961a5ca258c642256cae0eb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4571879
Reviewed-by: Struan Shrimpton <sshrimp@google.com>
Owners-Override: Struan Shrimpton <sshrimp@google.com>
Commit-Queue: Haiyang Pan <hypan@google.com>
Reviewed-by: Haiyang Pan <hypan@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1149974}
  • Loading branch information
Haiyang Pan authored and Chromium LUCI CQ committed May 26, 2023
1 parent 5f3e53d commit 68e174e
Show file tree
Hide file tree
Showing 7 changed files with 4 additions and 91 deletions.
1 change: 0 additions & 1 deletion content/public/test/android/BUILD.gn
View file
Expand Up @@ -28,7 +28,6 @@ android_library("content_java_test_support") {
"//content/public/android:content_java",
"//mojo/public/java:bindings_java",
"//net/android:net_java",
"//net/android:net_java_test_support",
"//services/service_manager/public/java:service_manager_java",
"//third_party/androidx:androidx_annotation_annotation_java",
"//third_party/androidx:androidx_test_monitor_java",
Expand Down
6 changes: 0 additions & 6 deletions ...roid/javatests/src/org/chromium/content_public/browser/test/ContentJUnit4ClassRunner.java
View file
Expand Up @@ -10,7 +10,6 @@

import org.chromium.base.test.BaseJUnit4ClassRunner;
import org.chromium.base.test.util.SkipCheck;
import org.chromium.net.test.EmbeddedTestServer;
import org.chromium.ui.test.util.DeviceRestrictionSkipCheck;
import org.chromium.ui.test.util.UiDisableIfSkipCheck;
import org.chromium.ui.test.util.UiRestrictionSkipCheck;
Expand Down Expand Up @@ -45,9 +44,4 @@ protected List<SkipCheck> getSkipChecks() {
protected List<TestHook> getPreTestHooks() {
return addToList(super.getPreTestHooks(), new ChildProcessAllocatorSettingsHook());
}

@Override
protected List<ClassHook> getPreClassHooks() {
return addToList(super.getPreClassHooks(), EmbeddedTestServer.getPreClassHook());
}
}
5 changes: 3 additions & 2 deletions net/android/BUILD.gn
View file
Expand Up @@ -79,7 +79,6 @@ android_library("net_java_test_support") {
":embedded_test_server_aidl_java",
":net_java",
"//base:base_java",
"//base:base_java_test_support",
"//base:base_java_url_utils_for_test",
"//base:jni_java",
"//third_party/androidx:androidx_annotation_annotation_java",
Expand Down Expand Up @@ -145,7 +144,9 @@ android_apk("net_test_support_apk") {
# Used as an additional_apk in test scripts.
never_incremental = true

enable_multidex = true
# Multidex requires a custom Application class to initialize it. Simpler to
# just disable it.
enable_multidex = false

# Required on Android Q+ to read from /sdcard when installing certs.
target_sdk_version = 28
Expand Down
36 changes: 0 additions & 36 deletions net/android/java/src/org/chromium/net/X509Util.java
View file
Expand Up @@ -162,11 +162,6 @@ private static List<X509Certificate> checkServerTrustedIgnoringRuntimeException(
*/
private static boolean sLoadedSystemKeyStore;

/**
* A root that will be installed as a user-trusted root for testing purposes.
*/
private static X509Certificate sTestRoot;

/**
* Lock object used to synchronize all calls that modify or depend on the trust managers.
*/
Expand Down Expand Up @@ -324,24 +319,17 @@ public static X509Certificate createCertificateFromBytes(byte[] derBytes) throws
new ByteArrayInputStream(derBytes));
}

/**
* Add a test root certificate for use by the Android Platform verifier.
*/
public static void addTestRootCertificate(byte[] rootCertBytes)
throws CertificateException, KeyStoreException, NoSuchAlgorithmException {
X509Certificate rootCert = createCertificateFromBytes(rootCertBytes);
synchronized (sLock) {
ensureTestInitializedLocked();
// Add the cert to be used by the Android Platform Verifier.
sTestKeyStore.setCertificateEntry(
"root_cert_" + Integer.toString(sTestKeyStore.size()), rootCert);
reloadTestTrustManager();
}
}

/**
* Clear test root certificates in use by the Android Platform verifier.
*/
public static void clearTestRootCertificates()
throws NoSuchAlgorithmException, CertificateException, KeyStoreException {
synchronized (sLock) {
Expand All @@ -355,22 +343,6 @@ public static void clearTestRootCertificates()
}
}

/**
* Set a test root certificate for use by CertVerifierBuiltin.
*/
public static void setTestRootCertificateForBuiltin(byte[] rootCertBytes)
throws NoSuchAlgorithmException, CertificateException, KeyStoreException {
X509Certificate rootCert = createCertificateFromBytes(rootCertBytes);
synchronized (sLock) {
// Add the cert to be used by CertVerifierBuiltin.
//
// This saves the root so it is returned from getUserAddedRoots, for TrustStoreAndroid.
// This is done for the Java EmbeddedTestServer implementation and must run before
// native code is loaded, when getUserAddedRoots is first run.
sTestRoot = rootCert;
}
}

private static final char[] HEX_DIGITS = {
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
'a', 'b', 'c', 'd', 'e', 'f',
Expand Down Expand Up @@ -531,14 +503,6 @@ public static byte[][] getUserAddedRoots() {
Log.e(TAG, "Error reading cert aliases: %s", e);
return new byte[0][];
}

if (sTestRoot != null) {
try {
userRootBytes.add(sTestRoot.getEncoded());
} catch (CertificateEncodingException e) {
Log.e(TAG, "Error encoding test root cert, error %s", e);
}
}
}

return userRootBytes.toArray(new byte[0][]);
Expand Down
3 changes: 1 addition & 2 deletions net/test/android/javatests/AndroidManifest.xml
View file
Expand Up @@ -11,8 +11,7 @@
<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE"/>
<uses-permission android:name="android.permission.INTERNET"/>

<application android:label="ChromiumNetTestSupport"
android:name="androidx.multidex.MultiDexApplication">
<application android:label="ChromiumNetTestSupport">

<uses-library android:name="android.test.runner" />

Expand Down
29 changes: 0 additions & 29 deletions net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java
View file
Expand Up @@ -18,8 +18,6 @@

import org.chromium.base.Log;
import org.chromium.base.ThreadUtils;
import org.chromium.base.test.BaseJUnit4ClassRunner.ClassHook;
import org.chromium.base.test.util.UrlUtils;
import org.chromium.net.X509Util;
import org.chromium.net.test.util.CertTestUtil;

Expand Down Expand Up @@ -527,31 +525,4 @@ public String getRootCertPemPath() {
throw new EmbeddedTestServerFailure("Failed to get root cert's path", e);
}
}

public static ClassHook getPreClassHook() {
return (targetContext, testClass) -> EmbeddedTestServer.setUpClass(testClass);
}

private static boolean sTestRootInitDone;

public static void setUpClass(Class<?> clazz) {
if (sTestRootInitDone) {
return;
}

// Always try to add the testing HTTPS root to the cert verifier. We do this here because we
// need this to happen before the native code loads the user-added roots, and this is the
// safest place to put it.
try {
// Use the same PEM file as net/test/embedded_test_server/embedded_test_server.cc.
String rootCertPemPath =
UrlUtils.getIsolatedTestFilePath("net/data/ssl/certificates/root_ca_cert.pem");
byte[] rootCertBytesDer = CertTestUtil.pemToDer(rootCertPemPath);
X509Util.setTestRootCertificateForBuiltin(rootCertBytesDer);
sTestRootInitDone = true;
} catch (Exception e) {
throw new EmbeddedTestServer.EmbeddedTestServerFailure(
"Failed to install root certificate.", e);
}
}
}
15 changes: 0 additions & 15 deletions testing/variations/fieldtrial_testing_config.json
View file
Expand Up @@ -2659,21 +2659,6 @@
]
}
],
"CertVerifierBuiltin": [
{
"platforms": [
"android"
],
"experiments": [
{
"name": "Enabled",
"enable_features": [
"ChromeRootStoreUsed"
]
}
]
}
],
"ChangeExtensionEventPageSuspendDelay": [
{
"platforms": [
Expand Down

0 comments on commit 68e174e

Please sign in to comment.