Modify CTLogger to take in the LogListUpdateStatus object instead of
individually passed parameters
This CL also moves the DataStore updates of the failure count into
CTLogger directly (CTDownloader will only modify the failure count on
complete success).
Note: due to limitations with Mockito (not being able to mock static
classes like the auto-generated CTStatsLog), there is no test class for
CTLoggerImpl.
Bug: 394278886
Bug: 378626065
Flag: com.android.net.ct.flags.certificate_transparency_service
Test: atest NetworkSecurityUnitTests
Change-Id: I4498bfe75e84c6ba1654cfa55912c46836f61e03
diff --git a/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyDownloader.java b/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyDownloader.java
index 34a2066..1fbb3f3 100644
--- a/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyDownloader.java
+++ b/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyDownloader.java
@@ -35,6 +35,7 @@
import java.security.GeneralSecurityException;
import java.util.ArrayList;
import java.util.List;
+import java.util.Optional;
/** Helper class to download certificate transparency log files. */
@RequiresApi(Build.VERSION_CODES.VANILLA_ICE_CREAM)
@@ -201,14 +202,9 @@
// TODO(b/391327942): parse file and log the timestamp of the log list
if (!updateStatus.isSignatureVerified()) {
- updateFailureCount();
Log.w(TAG, "Log list did not pass verification");
- mLogger.logCTLogListUpdateStateChangedEvent(
- updateStatus.state(),
- mDataStore.getPropertyInt(
- Config.LOG_LIST_UPDATE_FAILURE_COUNT, /* defaultValue= */ 0),
- updateStatus.signature());
+ mLogger.logCTLogListUpdateStateChangedEvent(updateStatus);
return;
}
@@ -227,47 +223,28 @@
mDataStore.setPropertyInt(Config.LOG_LIST_UPDATE_FAILURE_COUNT, /* value= */ 0);
mDataStore.store();
} else {
- updateFailureCount();
mLogger.logCTLogListUpdateStateChangedEvent(
- CTLogListUpdateState.VERSION_ALREADY_EXISTS,
- mDataStore.getPropertyInt(
- Config.LOG_LIST_UPDATE_FAILURE_COUNT, /* defaultValue= */ 0),
- updateStatus.signature());
+ updateStatus
+ .toBuilder()
+ .setState(CTLogListUpdateState.VERSION_ALREADY_EXISTS)
+ .build());
}
}
private void handleDownloadFailed(DownloadStatus status) {
Log.e(TAG, "Download failed with " + status);
- updateFailureCount();
- int failureCount =
- mDataStore.getPropertyInt(
- Config.LOG_LIST_UPDATE_FAILURE_COUNT, /* defaultValue= */ 0);
-
- // Unable to log the signature, as that is dependent on successful downloads
+ LogListUpdateStatus.Builder updateStatus = LogListUpdateStatus.builder();
if (status.isHttpError()) {
- mLogger.logCTLogListUpdateStateChangedEvent(
- CTLogListUpdateState.HTTP_ERROR,
- failureCount,
- status.reason());
+ updateStatus
+ .setState(CTLogListUpdateState.HTTP_ERROR)
+ .setHttpErrorStatusCode(status.reason());
} else {
// TODO(b/384935059): handle blocked domain logging
- mLogger.logCTLogListUpdateStateChangedEventWithDownloadStatus(
- status.reason(), failureCount);
+ updateStatus.setDownloadStatus(Optional.of(status.reason()));
}
- }
- /**
- * Updates the data store with the current number of consecutive log list update failures.
- */
- private void updateFailureCount() {
- int failure_count =
- mDataStore.getPropertyInt(
- Config.LOG_LIST_UPDATE_FAILURE_COUNT, /* defaultValue= */ 0);
- int new_failure_count = failure_count + 1;
-
- mDataStore.setPropertyInt(Config.LOG_LIST_UPDATE_FAILURE_COUNT, new_failure_count);
- mDataStore.store();
+ mLogger.logCTLogListUpdateStateChangedEvent(updateStatus.build());
}
private long download(String url) {
diff --git a/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyLogger.java b/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyLogger.java
index 0b415f0..967a04b 100644
--- a/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyLogger.java
+++ b/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyLogger.java
@@ -20,35 +20,12 @@
public interface CertificateTransparencyLogger {
/**
- * Logs a CTLogListUpdateStateChanged event to statsd, when failure is from DownloadManager.
+ * Logs a CTLogListUpdateStateChanged event to statsd.
*
- * @param downloadStatus DownloadManager failure status why the log list wasn't updated
- * @param failureCount number of consecutive log list update failures
+ * @param updateStatus status object containing details from this update event (e.g. log list
+ * signature, log list timestamp, failure reason if applicable)
*/
- void logCTLogListUpdateStateChangedEventWithDownloadStatus(
- int downloadStatus, int failureCount);
-
- /**
- * Logs a CTLogListUpdateStateChanged event to statsd without a HTTP error status code.
- *
- * @param failureReason reason why the log list wasn't updated
- * @param failureCount number of consecutive log list update failures
- * @param logListSignature signature used during log list verification
- */
- void logCTLogListUpdateStateChangedEvent(
- CTLogListUpdateState failureReason, int failureCount, String logListSignature);
-
- /**
- * Logs a CTLogListUpdateStateChanged event to statsd with an HTTP error status code.
- *
- * @param failureReason reason why the log list wasn't updated (e.g. DownloadManager failures)
- * @param failureCount number of consecutive log list update failures
- * @param httpErrorStatusCode if relevant, the HTTP error status code from DownloadManager
- */
- void logCTLogListUpdateStateChangedEvent(
- CTLogListUpdateState failureReason,
- int failureCount,
- int httpErrorStatusCode);
+ void logCTLogListUpdateStateChangedEvent(LogListUpdateStatus updateStatus);
/**
* Intermediate enum for use with CertificateTransparencyStatsLog.
diff --git a/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyLoggerImpl.java b/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyLoggerImpl.java
index 4a0689a..9c3210d 100644
--- a/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyLoggerImpl.java
+++ b/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyLoggerImpl.java
@@ -35,49 +35,56 @@
/** Implementation for logging to statsd for Certificate Transparency. */
class CertificateTransparencyLoggerImpl implements CertificateTransparencyLogger {
- @Override
- public void logCTLogListUpdateStateChangedEventWithDownloadStatus(
- int downloadStatus, int failureCount) {
- logCTLogListUpdateStateChangedEvent(
- downloadStatusToFailureReason(downloadStatus),
- failureCount,
- /* httpErrorStatusCode= */ 0,
- /* signature= */ "");
+ private final DataStore mDataStore;
+
+ CertificateTransparencyLoggerImpl(DataStore dataStore) {
+ mDataStore = dataStore;
}
@Override
- public void logCTLogListUpdateStateChangedEvent(
- CTLogListUpdateState failureReason, int failureCount, String signature) {
- logCTLogListUpdateStateChangedEvent(
- localEnumToStatsLogEnum(failureReason),
- failureCount,
- /* httpErrorStatusCode= */ 0,
- signature);
- }
+ public void logCTLogListUpdateStateChangedEvent(LogListUpdateStatus updateStatus) {
+ int updateState =
+ updateStatus
+ .downloadStatus()
+ .map(s -> downloadStatusToFailureReason(s))
+ .orElseGet(() -> localEnumToStatsLogEnum(updateStatus.state()));
+ int failureCount =
+ mDataStore.getPropertyInt(
+ Config.LOG_LIST_UPDATE_FAILURE_COUNT, /* defaultValue= */ 0);
- @Override
- public void logCTLogListUpdateStateChangedEvent(
- CTLogListUpdateState failureReason,
- int failureCount,
- int httpErrorStatusCode) {
logCTLogListUpdateStateChangedEvent(
- localEnumToStatsLogEnum(failureReason),
+ updateState,
failureCount,
- httpErrorStatusCode,
- /* signature= */ "");
+ updateStatus.httpErrorStatusCode(),
+ updateStatus.signature());
}
private void logCTLogListUpdateStateChangedEvent(
- int failureReason, int failureCount, int httpErrorStatusCode, String signature) {
+ int updateState, int failureCount, int httpErrorStatusCode, String signature) {
+ updateFailureCount();
+
CertificateTransparencyStatsLog.write(
CERTIFICATE_TRANSPARENCY_LOG_LIST_UPDATE_STATE_CHANGED,
- failureReason,
+ updateState,
failureCount,
httpErrorStatusCode,
signature,
/* logListTimestampMs= */ 0);
}
+ /**
+ * Updates the data store with the current number of consecutive log list update failures.
+ */
+ private void updateFailureCount() {
+ int failure_count =
+ mDataStore.getPropertyInt(
+ Config.LOG_LIST_UPDATE_FAILURE_COUNT, /* defaultValue= */ 0);
+ int new_failure_count = failure_count + 1;
+
+ mDataStore.setPropertyInt(Config.LOG_LIST_UPDATE_FAILURE_COUNT, new_failure_count);
+ mDataStore.store();
+ }
+
/** Converts DownloadStatus reason into failure reason to log. */
private int downloadStatusToFailureReason(int downloadStatusReason) {
switch (downloadStatusReason) {
diff --git a/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyService.java b/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyService.java
index 7edc35a..a71ff7c 100644
--- a/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyService.java
+++ b/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyService.java
@@ -61,7 +61,7 @@
dataStore,
new DownloadHelper(context),
new SignatureVerifier(context),
- new CertificateTransparencyLoggerImpl()),
+ new CertificateTransparencyLoggerImpl(dataStore)),
new CompatibilityVersion(
Config.COMPATIBILITY_VERSION,
Config.URL_SIGNATURE,
diff --git a/networksecurity/service/src/com/android/server/net/ct/LogListUpdateStatus.java b/networksecurity/service/src/com/android/server/net/ct/LogListUpdateStatus.java
index 0c75120..3d05857 100644
--- a/networksecurity/service/src/com/android/server/net/ct/LogListUpdateStatus.java
+++ b/networksecurity/service/src/com/android/server/net/ct/LogListUpdateStatus.java
@@ -24,6 +24,8 @@
import com.google.auto.value.AutoValue;
+import java.util.Optional;
+
/** Class to represent the signature verification status for Certificate Transparency. */
@AutoValue
public abstract class LogListUpdateStatus {
@@ -34,6 +36,10 @@
abstract long logListTimestamp();
+ abstract int httpErrorStatusCode();
+
+ abstract Optional<Integer> downloadStatus();
+
boolean isSignatureVerified() {
// Check that none of the signature verification failures have been set as the state
return state() != PUBLIC_KEY_NOT_FOUND
@@ -54,6 +60,10 @@
abstract Builder setLogListTimestamp(long timestamp);
+ abstract Builder setHttpErrorStatusCode(int httpStatusCode);
+
+ abstract Builder setDownloadStatus(Optional<Integer> downloadStatus);
+
abstract LogListUpdateStatus build();
}
@@ -63,6 +73,8 @@
return new AutoValue_LogListUpdateStatus.Builder()
.setState(CTLogListUpdateState.UNKNOWN_STATE)
.setSignature("")
- .setLogListTimestamp(0L);
+ .setLogListTimestamp(0L)
+ .setHttpErrorStatusCode(0)
+ .setDownloadStatus(Optional.empty());
}
}
diff --git a/networksecurity/tests/unit/src/com/android/server/net/ct/CertificateTransparencyDownloaderTest.java b/networksecurity/tests/unit/src/com/android/server/net/ct/CertificateTransparencyDownloaderTest.java
index fab28b7..08704d1 100644
--- a/networksecurity/tests/unit/src/com/android/server/net/ct/CertificateTransparencyDownloaderTest.java
+++ b/networksecurity/tests/unit/src/com/android/server/net/ct/CertificateTransparencyDownloaderTest.java
@@ -20,10 +20,6 @@
import static com.google.common.truth.Truth.assertThat;
import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.ArgumentMatchers.anyInt;
-import static org.mockito.ArgumentMatchers.anyString;
-import static org.mockito.ArgumentMatchers.eq;
-import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
@@ -50,6 +46,7 @@
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
+import org.mockito.ArgumentCaptor;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
@@ -67,6 +64,7 @@
import java.security.PublicKey;
import java.security.Signature;
import java.util.Base64;
+import java.util.Optional;
/** Tests for the {@link CertificateTransparencyDownloader}. */
@RunWith(JUnit4.class)
@@ -74,6 +72,8 @@
@Mock private DownloadManager mDownloadManager;
@Mock private CertificateTransparencyLogger mLogger;
+ private ArgumentCaptor<LogListUpdateStatus> mUpdateStatusCaptor =
+ ArgumentCaptor.forClass(LogListUpdateStatus.class);
private PrivateKey mPrivateKey;
private PublicKey mPublicKey;
@@ -207,14 +207,12 @@
mContext,
makePublicKeyDownloadFailedIntent(DownloadManager.ERROR_INSUFFICIENT_SPACE));
- assertThat(
- mDataStore.getPropertyInt(
- Config.LOG_LIST_UPDATE_FAILURE_COUNT, /* defaultValue= */ 0))
- .isEqualTo(1);
verify(mLogger, times(1))
- .logCTLogListUpdateStateChangedEventWithDownloadStatus(
- DownloadManager.ERROR_INSUFFICIENT_SPACE,
- /* failureCount= */ 1);
+ .logCTLogListUpdateStateChangedEvent(
+ LogListUpdateStatus.builder()
+ .setDownloadStatus(
+ Optional.of(DownloadManager.ERROR_INSUFFICIENT_SPACE))
+ .build());
}
@Test
@@ -256,14 +254,12 @@
makeMetadataDownloadFailedIntent(
mCompatVersion, DownloadManager.ERROR_INSUFFICIENT_SPACE));
- assertThat(
- mDataStore.getPropertyInt(
- Config.LOG_LIST_UPDATE_FAILURE_COUNT, /* defaultValue= */ 0))
- .isEqualTo(1);
verify(mLogger, times(1))
- .logCTLogListUpdateStateChangedEventWithDownloadStatus(
- DownloadManager.ERROR_INSUFFICIENT_SPACE,
- /* failureCount= */ 1);
+ .logCTLogListUpdateStateChangedEvent(
+ LogListUpdateStatus.builder()
+ .setDownloadStatus(
+ Optional.of(DownloadManager.ERROR_INSUFFICIENT_SPACE))
+ .build());
}
@Test
@@ -309,14 +305,12 @@
makeContentDownloadFailedIntent(
mCompatVersion, DownloadManager.ERROR_INSUFFICIENT_SPACE));
- assertThat(
- mDataStore.getPropertyInt(
- Config.LOG_LIST_UPDATE_FAILURE_COUNT, /* defaultValue= */ 0))
- .isEqualTo(1);
verify(mLogger, times(1))
- .logCTLogListUpdateStateChangedEventWithDownloadStatus(
- DownloadManager.ERROR_INSUFFICIENT_SPACE,
- /* failureCount= */ 1);
+ .logCTLogListUpdateStateChangedEvent(
+ LogListUpdateStatus.builder()
+ .setDownloadStatus(
+ Optional.of(DownloadManager.ERROR_INSUFFICIENT_SPACE))
+ .build());
}
@Test
@@ -352,30 +346,10 @@
mCertificateTransparencyDownloader.onReceive(
mContext, makeContentDownloadCompleteIntent(mCompatVersion, logListFile));
- assertThat(
- mDataStore.getPropertyInt(
- Config.LOG_LIST_UPDATE_FAILURE_COUNT, /* defaultValue= */ 0))
- .isEqualTo(1);
verify(mLogger, times(1))
- .logCTLogListUpdateStateChangedEvent(
- CTLogListUpdateState.PUBLIC_KEY_NOT_FOUND,
- /* failureCount= */ 1,
- "");
- verify(mLogger, never())
- .logCTLogListUpdateStateChangedEvent(
- eq(CTLogListUpdateState.SIGNATURE_NOT_FOUND),
- anyInt(),
- anyString());
- verify(mLogger, never())
- .logCTLogListUpdateStateChangedEvent(
- eq(CTLogListUpdateState.SIGNATURE_INVALID),
- anyInt(),
- anyString());
- verify(mLogger, never())
- .logCTLogListUpdateStateChangedEvent(
- eq(CTLogListUpdateState.SIGNATURE_VERIFICATION_FAILED),
- anyInt(),
- anyString());
+ .logCTLogListUpdateStateChangedEvent(mUpdateStatusCaptor.capture());
+ assertThat(mUpdateStatusCaptor.getValue().state())
+ .isEqualTo(CTLogListUpdateState.PUBLIC_KEY_NOT_FOUND);
}
@Test
@@ -398,30 +372,10 @@
mContext, makeContentDownloadCompleteIntent(mCompatVersion, logListFile));
// Assert
- assertThat(
- mDataStore.getPropertyInt(
- Config.LOG_LIST_UPDATE_FAILURE_COUNT, /* defaultValue= */ 0))
- .isEqualTo(1);
verify(mLogger, times(1))
- .logCTLogListUpdateStateChangedEvent(
- CTLogListUpdateState.SIGNATURE_INVALID,
- /* failureCount= */ 1,
- ""); // Should be empty b/c invalid key exception thrown
- verify(mLogger, never())
- .logCTLogListUpdateStateChangedEvent(
- eq(CTLogListUpdateState.SIGNATURE_VERIFICATION_FAILED),
- anyInt(),
- anyString());
- verify(mLogger, never())
- .logCTLogListUpdateStateChangedEvent(
- eq(CTLogListUpdateState.PUBLIC_KEY_NOT_FOUND),
- anyInt(),
- anyString());
- verify(mLogger, never())
- .logCTLogListUpdateStateChangedEvent(
- eq(CTLogListUpdateState.SIGNATURE_NOT_FOUND),
- anyInt(),
- anyString());
+ .logCTLogListUpdateStateChangedEvent(mUpdateStatusCaptor.capture());
+ assertThat(mUpdateStatusCaptor.getValue().state())
+ .isEqualTo(CTLogListUpdateState.SIGNATURE_INVALID);
}
@Test
@@ -444,31 +398,14 @@
mContext, makeContentDownloadCompleteIntent(mCompatVersion, logListFile));
// Assert
- assertThat(
- mDataStore.getPropertyInt(
- Config.LOG_LIST_UPDATE_FAILURE_COUNT, /* defaultValue= */ 0))
- .isEqualTo(1);
- verify(mLogger, never())
- .logCTLogListUpdateStateChangedEvent(
- eq(CTLogListUpdateState.SIGNATURE_NOT_FOUND),
- anyInt(),
- anyString());
- verify(mLogger, never())
- .logCTLogListUpdateStateChangedEvent(
- eq(CTLogListUpdateState.SIGNATURE_INVALID),
- anyInt(),
- anyString());
- verify(mLogger, never())
- .logCTLogListUpdateStateChangedEvent(
- eq(CTLogListUpdateState.PUBLIC_KEY_NOT_FOUND),
- anyInt(),
- anyString());
byte[] signatureBytes = Base64.getDecoder().decode(toByteArray(metadataFile));
verify(mLogger, times(1))
- .logCTLogListUpdateStateChangedEvent(
- CTLogListUpdateState.SIGNATURE_VERIFICATION_FAILED,
- /* failureCount= */ 1,
- new String(signatureBytes, StandardCharsets.UTF_8));
+ .logCTLogListUpdateStateChangedEvent(mUpdateStatusCaptor.capture());
+ LogListUpdateStatus statusValue = mUpdateStatusCaptor.getValue();
+ assertThat(statusValue.state())
+ .isEqualTo(CTLogListUpdateState.SIGNATURE_VERIFICATION_FAILED);
+ assertThat(statusValue.signature())
+ .isEqualTo(new String(signatureBytes, StandardCharsets.UTF_8));
}
@Test
@@ -485,16 +422,13 @@
mCertificateTransparencyDownloader.onReceive(
mContext, makeContentDownloadCompleteIntent(mCompatVersion, invalidLogListFile));
- assertThat(
- mDataStore.getPropertyInt(
- Config.LOG_LIST_UPDATE_FAILURE_COUNT, /* defaultValue= */ 0))
- .isEqualTo(1);
byte[] signatureBytes = Base64.getDecoder().decode(toByteArray(metadataFile));
verify(mLogger, times(1))
- .logCTLogListUpdateStateChangedEvent(
- CTLogListUpdateState.VERSION_ALREADY_EXISTS,
- /* failureCount= */ 1,
- new String(signatureBytes, StandardCharsets.UTF_8));
+ .logCTLogListUpdateStateChangedEvent(mUpdateStatusCaptor.capture());
+ LogListUpdateStatus statusValue = mUpdateStatusCaptor.getValue();
+ assertThat(statusValue.state()).isEqualTo(CTLogListUpdateState.VERSION_ALREADY_EXISTS);
+ assertThat(statusValue.signature())
+ .isEqualTo(new String(signatureBytes, StandardCharsets.UTF_8));
}
@Test