From 9a143c78a4e89bdfb59863772085f43a593d5341 Mon Sep 17 00:00:00 2001 From: Chris Wynne Date: Tue, 29 Oct 2024 11:05:48 +0000 Subject: [PATCH 1/2] PYIC-7571: Track verifier use Lots of VCs were failing signature verification in staging. Turns out they were signed with an unexpected key. It would be useful to understand how many VCs are signed with this key (which has been added to config). This would also potentially be interesting when we recondile in production to understand the proportion of passport VCs signed with the old and new keys. --- .../ReconcileMigratedVcsHandler.java | 26 ++++++++++++------- .../domain/ReconciliationReport.java | 3 +++ .../domain/VerifierAndUseCount.java | 16 ++++++++++++ 3 files changed, 35 insertions(+), 10 deletions(-) create mode 100644 lambdas/reconcile-migrated-vcs/src/main/java/uk/gov/di/ipv/core/reconcilemigratedvcs/domain/VerifierAndUseCount.java diff --git a/lambdas/reconcile-migrated-vcs/src/main/java/uk/gov/di/ipv/core/reconcilemigratedvcs/ReconcileMigratedVcsHandler.java b/lambdas/reconcile-migrated-vcs/src/main/java/uk/gov/di/ipv/core/reconcilemigratedvcs/ReconcileMigratedVcsHandler.java index 10c7517bb0..fbeaccbac2 100644 --- a/lambdas/reconcile-migrated-vcs/src/main/java/uk/gov/di/ipv/core/reconcilemigratedvcs/ReconcileMigratedVcsHandler.java +++ b/lambdas/reconcile-migrated-vcs/src/main/java/uk/gov/di/ipv/core/reconcilemigratedvcs/ReconcileMigratedVcsHandler.java @@ -5,7 +5,6 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.nimbusds.jose.JOSEException; -import com.nimbusds.jose.JWSVerifier; import com.nimbusds.jose.crypto.ECDSAVerifier; import com.nimbusds.jose.crypto.RSASSAVerifier; import com.nimbusds.jose.crypto.impl.ECDSA; @@ -33,6 +32,7 @@ import uk.gov.di.ipv.core.library.service.ConfigService; import uk.gov.di.ipv.core.reconcilemigratedvcs.domain.ReconciliationReport; import uk.gov.di.ipv.core.reconcilemigratedvcs.domain.Request; +import uk.gov.di.ipv.core.reconcilemigratedvcs.domain.VerifierAndUseCount; import java.text.ParseException; import java.util.ArrayList; @@ -42,6 +42,7 @@ import java.util.concurrent.ConcurrentSkipListSet; import java.util.concurrent.ExecutionException; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; import static com.nimbusds.jose.JWSAlgorithm.ES256; import static com.nimbusds.jose.jwk.KeyType.EC; @@ -73,7 +74,7 @@ public class ReconcileMigratedVcsHandler implements RequestHandler processedIdentities; private AtomicBoolean timeoutClose; private Map criIssuersMap; - private Map> historicSigningKeyVerifiers; + private Map> historicSigningKeyVerifiers; public ReconcileMigratedVcsHandler( ConfigService configService, @@ -174,6 +175,7 @@ public ReconciliationReport handleRequest(Request request, Context context) { unprocessedIdentities.removeAll(processedIdentities); reconciliationReport.setUnprocessedHashedUserIds( unprocessedIdentities.stream().map(DigestUtils::sha256Hex).toList()); + reconciliationReport.setVerifierUse(historicSigningKeyVerifiers); logReport(); } @@ -325,9 +327,9 @@ private synchronized void refreshEvcsClientIfNeeded() { private boolean validSignature(VerifiableCredential vc, String hashedUserId) throws JOSEException, ParseException { // try all known keys until VC is validated, or return false - for (var verifier : historicSigningKeyVerifiers.get(vc.getCri())) { + for (var verifierAndUseCount : historicSigningKeyVerifiers.get(vc.getCri())) { SignedJWT formattedVc; - if (verifier instanceof ECDSAVerifier) { + if (verifierAndUseCount.getVerifier() instanceof ECDSAVerifier) { try { formattedVc = transcodeSignatureIfDerFormat(vc.getSignedJwt()); } catch (JOSEException | ParseException e) { @@ -338,7 +340,8 @@ private boolean validSignature(VerifiableCredential vc, String hashedUserId) formattedVc = vc.getSignedJwt(); } - if (formattedVc.verify(verifier)) { + if (formattedVc.verify(verifierAndUseCount.getVerifier())) { + verifierAndUseCount.getUseCount().incrementAndGet(); return true; } } @@ -430,9 +433,9 @@ private void logReport() { } } - private Map> getHistoricSigningKeyVerifiers() + private Map> getHistoricSigningKeyVerifiers() throws ParseException, JOSEException { - Map> verifiers = new EnumMap<>(Cri.class); + Map> verifiers = new EnumMap<>(Cri.class); for (var cri : Cri.values()) { try { for (var publicKey : configService.getHistoricSigningKeys(cri.getId())) { @@ -440,9 +443,12 @@ private Map> getHistoricSigningKeyVerifiers() verifiers .computeIfAbsent(cri, k -> new ArrayList<>()) .add( - parsedJwk.getKeyType() == EC - ? new ECDSAVerifier(parsedJwk.toECKey()) - : new RSASSAVerifier(parsedJwk.toRSAKey())); + new VerifierAndUseCount( + parsedJwk.getKeyType() == EC + ? new ECDSAVerifier(parsedJwk.toECKey()) + : new RSASSAVerifier(parsedJwk.toRSAKey()), + publicKey, + new AtomicInteger(0))); } } catch (ConfigParameterNotFoundException e) { LOGGER.warn( diff --git a/lambdas/reconcile-migrated-vcs/src/main/java/uk/gov/di/ipv/core/reconcilemigratedvcs/domain/ReconciliationReport.java b/lambdas/reconcile-migrated-vcs/src/main/java/uk/gov/di/ipv/core/reconcilemigratedvcs/domain/ReconciliationReport.java index aed3d19322..af1ff90940 100644 --- a/lambdas/reconcile-migrated-vcs/src/main/java/uk/gov/di/ipv/core/reconcilemigratedvcs/domain/ReconciliationReport.java +++ b/lambdas/reconcile-migrated-vcs/src/main/java/uk/gov/di/ipv/core/reconcilemigratedvcs/domain/ReconciliationReport.java @@ -3,10 +3,12 @@ import lombok.Getter; import lombok.Setter; import uk.gov.di.ipv.core.library.annotations.ExcludeFromGeneratedCoverageReport; +import uk.gov.di.ipv.core.library.domain.Cri; import uk.gov.di.ipv.core.library.domain.VerifiableCredential; import java.util.ArrayList; import java.util.List; +import java.util.Map; @Getter @ExcludeFromGeneratedCoverageReport @@ -37,6 +39,7 @@ public class ReconciliationReport { private final List failedToAttainP2HashedUserIds = new ArrayList<>(); private final List differenceInVcsHashedUserIds = new ArrayList<>(); @Setter private List unprocessedHashedUserIds = new ArrayList<>(); + @Setter private Map> verifierUse; public ReconciliationReport(Request request) { batchId = request.batchId(); diff --git a/lambdas/reconcile-migrated-vcs/src/main/java/uk/gov/di/ipv/core/reconcilemigratedvcs/domain/VerifierAndUseCount.java b/lambdas/reconcile-migrated-vcs/src/main/java/uk/gov/di/ipv/core/reconcilemigratedvcs/domain/VerifierAndUseCount.java new file mode 100644 index 0000000000..f7655fae50 --- /dev/null +++ b/lambdas/reconcile-migrated-vcs/src/main/java/uk/gov/di/ipv/core/reconcilemigratedvcs/domain/VerifierAndUseCount.java @@ -0,0 +1,16 @@ +package uk.gov.di.ipv.core.reconcilemigratedvcs.domain; + +import com.fasterxml.jackson.annotation.JsonIgnore; +import com.nimbusds.jose.JWSVerifier; +import lombok.AllArgsConstructor; +import lombok.Getter; + +import java.util.concurrent.atomic.AtomicInteger; + +@Getter +@AllArgsConstructor +public class VerifierAndUseCount { + @JsonIgnore private final JWSVerifier verifier; + private final String publicKey; + private final AtomicInteger useCount; +} From a968b1d44563530edf30d074c3afca14ee901dde Mon Sep 17 00:00:00 2001 From: Chris Wynne Date: Tue, 29 Oct 2024 11:16:53 +0000 Subject: [PATCH 2/2] PYIC-7571: Track individual VC matched count It would be good to be able to easily say that we've matchec the same number of VCs as expected, rather than just the number of identities. --- .../ReconcileMigratedVcsHandler.java | 4 +-- .../domain/ReconciliationReport.java | 20 +++++++------- .../ReconcileMigratedVcsHandlerTest.java | 26 ++++++++++--------- 3 files changed, 27 insertions(+), 23 deletions(-) diff --git a/lambdas/reconcile-migrated-vcs/src/main/java/uk/gov/di/ipv/core/reconcilemigratedvcs/ReconcileMigratedVcsHandler.java b/lambdas/reconcile-migrated-vcs/src/main/java/uk/gov/di/ipv/core/reconcilemigratedvcs/ReconcileMigratedVcsHandler.java index fbeaccbac2..9cead430a8 100644 --- a/lambdas/reconcile-migrated-vcs/src/main/java/uk/gov/di/ipv/core/reconcilemigratedvcs/ReconcileMigratedVcsHandler.java +++ b/lambdas/reconcile-migrated-vcs/src/main/java/uk/gov/di/ipv/core/reconcilemigratedvcs/ReconcileMigratedVcsHandler.java @@ -222,12 +222,12 @@ private void reconcileIdentity(String userId, Context context) { // Compare VCs if (!evcsVcsStrings.equals(tacticalVcsStrings)) { logVcDifferences(evcsVcsStrings, tacticalVcsStrings, hashedUserId); - reconciliationReport.incrementDifferenceInVcs(hashedUserId); + reconciliationReport.incrementIdentitiesWithDifferentVcs(hashedUserId); reconciliationReport.incrementIdentitiesFullyProcessed(); processedIdentities.add(userId); return; } else { - reconciliationReport.incrementVcsMatch(); + reconciliationReport.incrementIdentitiesWithMatchingVcs(evcsVcsStrings.size()); } if (reconciliationReport.isCheckSignatures() || reconciliationReport.isCheckP2()) { diff --git a/lambdas/reconcile-migrated-vcs/src/main/java/uk/gov/di/ipv/core/reconcilemigratedvcs/domain/ReconciliationReport.java b/lambdas/reconcile-migrated-vcs/src/main/java/uk/gov/di/ipv/core/reconcilemigratedvcs/domain/ReconciliationReport.java index af1ff90940..ae169a8e43 100644 --- a/lambdas/reconcile-migrated-vcs/src/main/java/uk/gov/di/ipv/core/reconcilemigratedvcs/domain/ReconciliationReport.java +++ b/lambdas/reconcile-migrated-vcs/src/main/java/uk/gov/di/ipv/core/reconcilemigratedvcs/domain/ReconciliationReport.java @@ -18,8 +18,9 @@ public class ReconciliationReport { private final boolean checkP2; private final int batchSize; private int identitiesFullyProcessed; + private int identitiesWithMatchingVcs; private int vcsMatchedCount; - private int vcsDifferentCount; + private int identitiesWithDifferentVcsCount; private int identityWithValidSignaturesCount; private int identityWithInvalidSignaturesCount; private int invalidVcSignatureCount; @@ -37,7 +38,7 @@ public class ReconciliationReport { private final List identityWithAnyInvalidSignaturesHashedUserIds = new ArrayList<>(); private final List invalidVcSignatureHashedUserIdAndCri = new ArrayList<>(); private final List failedToAttainP2HashedUserIds = new ArrayList<>(); - private final List differenceInVcsHashedUserIds = new ArrayList<>(); + private final List identitiesWithDifferentVcsHashedUserIds = new ArrayList<>(); @Setter private List unprocessedHashedUserIds = new ArrayList<>(); @Setter private Map> verifierUse; @@ -52,8 +53,14 @@ public synchronized void incrementIdentitiesFullyProcessed() { identitiesFullyProcessed++; } - public synchronized void incrementVcsMatch() { - vcsMatchedCount++; + public synchronized void incrementIdentitiesWithMatchingVcs(int vcCount) { + identitiesWithMatchingVcs++; + vcsMatchedCount += vcCount; + } + + public synchronized void incrementIdentitiesWithDifferentVcs(String hashedUserId) { + identitiesWithDifferentVcsCount++; + identitiesWithDifferentVcsHashedUserIds.add(hashedUserId); } public synchronized void incrementFailedEvcsRead(String hashedUserId) { @@ -102,9 +109,4 @@ public synchronized void incrementFailedToAttainP2(String hashedUserId) { public synchronized void incrementSuccessfullyMatchedP2Count() { successfullyAttainedP2Count++; } - - public synchronized void incrementDifferenceInVcs(String hashedUserId) { - vcsDifferentCount++; - differenceInVcsHashedUserIds.add(hashedUserId); - } } diff --git a/lambdas/reconcile-migrated-vcs/src/test/java/uk/gov/di/ipv/core/reconcilemigratedvcs/ReconcileMigratedVcsHandlerTest.java b/lambdas/reconcile-migrated-vcs/src/test/java/uk/gov/di/ipv/core/reconcilemigratedvcs/ReconcileMigratedVcsHandlerTest.java index 0ef1776d09..6264228fda 100644 --- a/lambdas/reconcile-migrated-vcs/src/test/java/uk/gov/di/ipv/core/reconcilemigratedvcs/ReconcileMigratedVcsHandlerTest.java +++ b/lambdas/reconcile-migrated-vcs/src/test/java/uk/gov/di/ipv/core/reconcilemigratedvcs/ReconcileMigratedVcsHandlerTest.java @@ -91,8 +91,9 @@ void handlerShouldHandleMatchingVcs() throws Exception { assertEquals("vcs-match-batch-id", reconciliationReport.getBatchId()); assertEquals(1, reconciliationReport.getBatchSize()); assertEquals(1, reconciliationReport.getIdentitiesFullyProcessed()); - assertEquals(1, reconciliationReport.getVcsMatchedCount()); - assertEquals(0, reconciliationReport.getVcsDifferentCount()); + assertEquals(1, reconciliationReport.getIdentitiesWithMatchingVcs()); + assertEquals(3, reconciliationReport.getVcsMatchedCount()); + assertEquals(0, reconciliationReport.getIdentitiesWithDifferentVcsCount()); assertFalse(reconciliationReport.isCheckSignatures()); assertFalse(reconciliationReport.isCheckP2()); } @@ -122,7 +123,7 @@ void handlerShouldHandleNonMatchingVcs() throws Exception { assertEquals(1, reconciliationReport.getBatchSize()); assertEquals(1, reconciliationReport.getIdentitiesFullyProcessed()); assertEquals(0, reconciliationReport.getVcsMatchedCount()); - assertEquals(1, reconciliationReport.getVcsDifferentCount()); + assertEquals(1, reconciliationReport.getIdentitiesWithDifferentVcsCount()); assertFalse(reconciliationReport.isCheckSignatures()); assertFalse(reconciliationReport.isCheckP2()); } @@ -151,7 +152,7 @@ void handlerShouldHandleMissingVcs() throws Exception { assertEquals(1, reconciliationReport.getBatchSize()); assertEquals(1, reconciliationReport.getIdentitiesFullyProcessed()); assertEquals(0, reconciliationReport.getVcsMatchedCount()); - assertEquals(1, reconciliationReport.getVcsDifferentCount()); + assertEquals(1, reconciliationReport.getIdentitiesWithDifferentVcsCount()); assertFalse(reconciliationReport.isCheckSignatures()); assertFalse(reconciliationReport.isCheckP2()); } @@ -193,8 +194,9 @@ void handlerShouldHandleValidateSignatures() throws Exception { assertEquals("vcs-valid-sig-batch-id", reconciliationReport.getBatchId()); assertEquals(1, reconciliationReport.getBatchSize()); assertEquals(1, reconciliationReport.getIdentitiesFullyProcessed()); - assertEquals(1, reconciliationReport.getVcsMatchedCount()); - assertEquals(0, reconciliationReport.getVcsDifferentCount()); + assertEquals(1, reconciliationReport.getIdentitiesWithMatchingVcs()); + assertEquals(2, reconciliationReport.getVcsMatchedCount()); + assertEquals(0, reconciliationReport.getIdentitiesWithDifferentVcsCount()); assertEquals(1, reconciliationReport.getIdentityWithValidSignaturesCount()); assertEquals(0, reconciliationReport.getIdentityWithInvalidSignaturesCount()); assertEquals(0, reconciliationReport.getInvalidVcSignatureCount()); @@ -224,7 +226,7 @@ void handlerShouldHandleInvalidSignatures() throws Exception { assertEquals(1, reconciliationReport.getBatchSize()); assertEquals(1, reconciliationReport.getIdentitiesFullyProcessed()); assertEquals(1, reconciliationReport.getVcsMatchedCount()); - assertEquals(0, reconciliationReport.getVcsDifferentCount()); + assertEquals(0, reconciliationReport.getIdentitiesWithDifferentVcsCount()); assertEquals(0, reconciliationReport.getIdentityWithValidSignaturesCount()); assertEquals(1, reconciliationReport.getIdentityWithInvalidSignaturesCount()); assertEquals(1, reconciliationReport.getInvalidVcSignatureCount()); @@ -254,7 +256,7 @@ void handlerShouldCheckSignaturesWithAllKeys() throws Exception { assertEquals(1, reconciliationReport.getBatchSize()); assertEquals(1, reconciliationReport.getIdentitiesFullyProcessed()); assertEquals(1, reconciliationReport.getVcsMatchedCount()); - assertEquals(0, reconciliationReport.getVcsDifferentCount()); + assertEquals(0, reconciliationReport.getIdentitiesWithDifferentVcsCount()); assertEquals(1, reconciliationReport.getIdentityWithValidSignaturesCount()); assertEquals(0, reconciliationReport.getIdentityWithInvalidSignaturesCount()); assertEquals(0, reconciliationReport.getInvalidVcSignatureCount()); @@ -287,7 +289,7 @@ void handlerShouldHandleIdentityWithP2() throws Exception { assertEquals(1, reconciliationReport.getBatchSize()); assertEquals(1, reconciliationReport.getIdentitiesFullyProcessed()); assertEquals(1, reconciliationReport.getVcsMatchedCount()); - assertEquals(0, reconciliationReport.getVcsDifferentCount()); + assertEquals(0, reconciliationReport.getIdentitiesWithDifferentVcsCount()); assertEquals(1, reconciliationReport.getIdentityWithValidSignaturesCount()); assertEquals(0, reconciliationReport.getIdentityWithInvalidSignaturesCount()); assertEquals(0, reconciliationReport.getInvalidVcSignatureCount()); @@ -317,7 +319,7 @@ void handlerShouldHandleIdentityWithNonP2() throws Exception { assertEquals(1, reconciliationReport.getBatchSize()); assertEquals(1, reconciliationReport.getIdentitiesFullyProcessed()); assertEquals(1, reconciliationReport.getVcsMatchedCount()); - assertEquals(0, reconciliationReport.getVcsDifferentCount()); + assertEquals(0, reconciliationReport.getIdentitiesWithDifferentVcsCount()); assertEquals(0, reconciliationReport.getIdentityWithValidSignaturesCount()); assertEquals(0, reconciliationReport.getIdentityWithInvalidSignaturesCount()); assertEquals(0, reconciliationReport.getInvalidVcSignatureCount()); @@ -344,8 +346,8 @@ void handlerShouldHandleLambdaTimeout() throws Exception { assertEquals("timeout-batch-id", reconciliationReport.getBatchId()); assertEquals(2, reconciliationReport.getBatchSize()); assertEquals(2, reconciliationReport.getIdentitiesFullyProcessed()); - assertEquals(2, reconciliationReport.getVcsMatchedCount()); - assertEquals(0, reconciliationReport.getVcsDifferentCount()); + assertEquals(2, reconciliationReport.getIdentitiesWithMatchingVcs()); + assertEquals(0, reconciliationReport.getIdentitiesWithDifferentVcsCount()); assertEquals("Lambda close to timeout", reconciliationReport.getExitReason()); }