Merge "Modify CTLogger to take in the LogListUpdateStatus object instead of individually passed parameters" into main
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