Revert "Bug 575541 - [pgp] P2 should not fail completely if a public key is not"Y20220127-0600Y20220126-0600Y20220125-0600Y20220124-0600Y20220123-0600Y20220122-0600Y20220121-0600Y20220120-0600Y20220119-0600I20220126-1800I20220126-0520I20220125-1800I20220125-0320I20220124-0510I20220123-1800I20220122-1800I20220121-1800I20220120-1800I20220120-0720I20220120-0220I20220119-1800I20220119-1440I20220119-1320I20220119-0540I20220118-1800

Later steps of verification assume all signatures got verified, so chaning this
invariant can invalidate the whole process.
Instead, work must be placed on providers of signatures to ensure they also provide
the pgp.publicKeys to verify the signatures.

This reverts commit c193e5197535846f3f546f0e9ce210660c190484.

Change-Id: I1dc2cacd044f1ab878001ffef29167867d63c0fe
Reviewed-on: https://git.eclipse.org/r/c/equinox/rt.equinox.p2/+/189738
Tested-by: Equinox Bot <equinox-bot@eclipse.org>
Reviewed-by: Mickael Istria <mistria@redhat.com>

4 files changed, 30 insertions, 53 deletions

diff --git a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/processors/pgp/PGPSignatureVerifier.java b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/processors/pgp/PGPSignatureVerifier.java
index 0f2569b87..9c8caf9f9 100644
--- a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/processors/pgp/PGPSignatureVerifier.java
+++ b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/processors/pgp/PGPSignatureVerifier.java
@@ -22,6 +22,7 @@ import org.eclipse.equinox.internal.p2.artifact.repository.Activator;
 import org.eclipse.equinox.internal.provisional.p2.artifact.repository.processing.ProcessingStep;
 import org.eclipse.equinox.p2.core.IProvisioningAgent;
 import org.eclipse.equinox.p2.repository.artifact.*;
+import org.eclipse.osgi.util.NLS;
 
 /**
  * This processing step verifies PGP signatures are correct (ie artifact was not
@@ -125,21 +126,19 @@ public final class PGPSignatureVerifier extends ProcessingStep {
 			IArtifactDescriptor context) {
 		super.initialize(agent, descriptor, context);
 //		1. verify declared public keys have signature from a trusted key, if so, add to KeyStore
-//		2. verify artifact signature matches signature of given keys, and at least 1 of this key is trusted
+//		2. verify artifact signature matches signture of given keys, and at least 1 of this key is trusted
 		String signatureText = unnormalizedPGPProperty(context.getProperty(PGP_SIGNATURES_PROPERTY_NAME));
 		if (signatureText == null) {
 			setStatus(Status.OK_STATUS);
 			return;
 		}
-
-		Collection<PGPSignature> signatures;
 		try {
-			signatures = getSignatures(context);
+			signaturesToVerify = getSignatures(context);
 		} catch (Exception ex) {
 			setStatus(new Status(IStatus.ERROR, Activator.ID, Messages.Error_CouldNotLoadSignature, ex));
 			return;
 		}
-		if (signatures.isEmpty()) {
+		if (signaturesToVerify.isEmpty()) {
 			setStatus(Status.OK_STATUS);
 			return;
 		}
@@ -147,21 +146,18 @@ public final class PGPSignatureVerifier extends ProcessingStep {
 		IArtifactRepository repository = context.getRepository();
 		KNOWN_KEYS.addKeys(context.getProperty(PGP_SIGNER_KEYS_PROPERTY_NAME),
 				repository != null ? repository.getProperty(PGP_SIGNER_KEYS_PROPERTY_NAME) : null);
-		for (PGPSignature signature : signatures) {
+		for (PGPSignature signature : signaturesToVerify) {
 			PGPPublicKey publicKey = KNOWN_KEYS.getKey(signature.getKeyID());
-			if (publicKey != null) {
-				// Signatures without known a corresponding key will be treated like unsigned
-				// content.
-				try {
-					if (signaturesToVerify == null) {
-						signaturesToVerify = new ArrayList<>();
-					}
-					signature.init(new BcPGPContentVerifierBuilderProvider(), publicKey);
-					signaturesToVerify.add(signature);
-				} catch (PGPException ex) {
-					setStatus(new Status(IStatus.ERROR, Activator.ID, ex.getMessage(), ex));
-					return;
-				}
+			if (publicKey == null) {
+				setStatus(new Status(IStatus.ERROR, Activator.ID,
+						NLS.bind(Messages.Error_publicKeyNotFound, Long.toHexString(signature.getKeyID()))));
+				return;
+			}
+			try {
+				signature.init(new BcPGPContentVerifierBuilderProvider(), publicKey);
+			} catch (PGPException ex) {
+				setStatus(new Status(IStatus.ERROR, Activator.ID, ex.getMessage(), ex));
+				return;
 			}
 		}
 	}
diff --git a/bundles/org.eclipse.equinox.p2.engine/src/org/eclipse/equinox/internal/p2/engine/phases/CertificateChecker.java b/bundles/org.eclipse.equinox.p2.engine/src/org/eclipse/equinox/internal/p2/engine/phases/CertificateChecker.java
index 03656bfbc..34aa50fc9 100644
--- a/bundles/org.eclipse.equinox.p2.engine/src/org/eclipse/equinox/internal/p2/engine/phases/CertificateChecker.java
+++ b/bundles/org.eclipse.equinox.p2.engine/src/org/eclipse/equinox/internal/p2/engine/phases/CertificateChecker.java
@@ -96,7 +96,6 @@ public class CertificateChecker {
 	private IStatus checkCertificates(SignedContentFactory verifierFactory) {
 		UIServices serviceUI = agent.getService(UIServices.class);
 		ArrayList<Certificate> untrustedCertificates = new ArrayList<>();
-		Map<IArtifactDescriptor, Collection<Long>> untrustedPGPKeyIDArtifacts = new HashMap<>();
 		Map<IArtifactDescriptor, Collection<PGPPublicKey>> untrustedPGPArtifacts = new HashMap<>();
 		Map<IArtifactDescriptor, File> unsigned = new HashMap<>();
 		ArrayList<Certificate[]> untrustedChain = new ArrayList<>();
@@ -133,26 +132,14 @@ public class CertificateChecker {
 					if (!signatures.isEmpty()) {
 						if (trustedKeysIds.isEmpty() && !trustedKeys.get().isEmpty()) {
 							trustedKeysIds.addAll(trustedKeys.get().all().stream()
-									.map(PGPPublicKey::getKeyID).collect(Collectors.toSet()));
+									.map(PGPPublicKey::getKeyID).map(Long::valueOf).collect(Collectors.toSet()));
 						}
-						Set<Long> untrustedKeyIds = signatures.stream().map(PGPSignature::getKeyID)
-								.collect(Collectors.toCollection(LinkedHashSet::new));
-						// If any key is already trusted, then we don't need to prompt for any of the
-						// other keys.
-						if (!untrustedKeyIds.removeAll(trustedKeysIds)) {
-							untrustedPGPKeyIDArtifacts.put(artifact.getKey(), untrustedKeyIds);
-							List<PGPPublicKey> untrustedKeys = untrustedKeyIds.stream()
-									.map(id -> findKey(id, artifact.getKey()))
-									.filter(Objects::nonNull)
-									.collect(Collectors.toList());
-							if (untrustedKeys.isEmpty()) {
-								// If no keys can be found for any of the signatures, treat the artifact like
-								// unsigned content because none of the signatures can actually be verified.
-								unsigned.put(artifact.getKey(), artifactFile);
-							} else {
-								// Otherwise, one of these keys needs to be trusted.
-								untrustedPGPArtifacts.put(artifact.getKey(), untrustedKeys);
-							}
+						if (signatures.stream().map(PGPSignature::getKeyID).noneMatch(trustedKeysIds::contains)) {
+							untrustedPGPArtifacts.put(artifact.getKey(),
+									signatures.stream().map(PGPSignature::getKeyID)
+											.map(id -> findKey(id, artifact.getKey()))
+											.filter(Objects::nonNull)
+											.collect(Collectors.toList()));
 						}
 					} else {
 						unsigned.put(artifact.getKey(), artifactFile);
@@ -211,9 +198,8 @@ public class CertificateChecker {
 		String[] details = EngineActivator.UNSIGNED_ALLOW.equals(policy) || unsigned.isEmpty() ? null
 				: unsigned.entrySet().stream().map(entry -> {
 					String detail = entry.getValue().toString();
-					Collection<Long> unknownIds = untrustedPGPKeyIDArtifacts.get(entry.getKey());
-					if (unknownIds != null) {
-						return detail + unknownIds.stream().map(Long::toHexString)
+					if (untrustedPGPKeys != null && !untrustedPGPKeys.isEmpty()) {
+						return detail + untrustedPGPKeys.stream().map(PGPPublicKey::getKeyID).map(Long::toHexString)
 								.collect(Collectors.joining(", ", " [", "]")); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
 					}
 					return detail;
@@ -347,8 +333,8 @@ public class CertificateChecker {
 		}
 		// load from bundles providing capability
 		for (IConfigurationElement extension : RegistryFactory.getRegistry()
-				.getConfigurationElementsFor(EngineActivator.ID + ".pgp")) { //$NON-NLS-1$
-			if ("trustedKeys".equals(extension.getName())) { //$NON-NLS-1$
+				.getConfigurationElementsFor(EngineActivator.ID + ".pgp")) {
+			if ("trustedKeys".equals(extension.getName())) {
 				String pathInBundle = extension.getAttribute("path"); //$NON-NLS-1$
 				if (pathInBundle != null) {
 					Stream.of(EngineActivator.getContext().getBundles())
diff --git a/bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/artifact/processors/PGPSignatureVerifierTest.java b/bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/artifact/processors/PGPSignatureVerifierTest.java
index 09d843f8c..36b50a545 100644
--- a/bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/artifact/processors/PGPSignatureVerifierTest.java
+++ b/bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/artifact/processors/PGPSignatureVerifierTest.java
@@ -71,17 +71,13 @@ public class PGPSignatureVerifierTest {
 
 	@Test
 	public void testNoPublicKeyFound() throws Exception {
-		// To address https://bugs.eclipse.org/bugs/show_bug.cgi?id=575541 a signature
-		// for which no key can be found is ignored.
-		// Such an artifact will be treated the same as an unsigned artifact.
-		// The missing key information will be in the details presented to the user.
 		IProcessingStepDescriptor processingStepDescriptor = new ProcessingStepDescriptor(null, null, false);
 		IArtifactDescriptor artifact = createArtifact("signed_by_signer_1", "public_signer2.pgp");
 		try (PGPSignatureVerifier verifier = new PGPSignatureVerifier()) {
 			verifier.initialize(null, processingStepDescriptor, artifact);
 			IStatus status = verifier.getStatus();
-			assertEquals(IStatus.OK, status.getSeverity());
-			// assertTrue(status.getMessage().contains("Public key not found for"));
+			assertEquals(IStatus.ERROR, status.getSeverity());
+			assertTrue(status.getMessage().contains("Public key not found for"));
 		}
 	}
 
diff --git a/bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/artifact/repository/PGPVerifierTest.java b/bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/artifact/repository/PGPVerifierTest.java
index 400ccdace..c010af306 100644
--- a/bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/artifact/repository/PGPVerifierTest.java
+++ b/bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/artifact/repository/PGPVerifierTest.java
@@ -62,10 +62,9 @@ public class PGPVerifierTest extends AbstractProvisioningTest {
 
 	@Test
 	public void testMissingPublicKey() throws Exception {
-		// See https://bugs.eclipse.org/bugs/show_bug.cgi?id=575541
-		// Missing PGP keys are ignored and the content is treated as unsigned.
 		IStatus mirrorStatus = performMirrorFrom("repoMissingPublicKey");
-		assertOK(mirrorStatus);
+		assertNotOK(mirrorStatus);
+		assertTrue(mirrorStatus.toString().contains("Public key not found"));
 	}
 
 	@Override