Revert "Bug 575541 - [pgp] P2 should not fail completely if a public key is not"
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>
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 0f2569b..9c8caf9 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.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 @@
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 @@
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 03656bf..34aa50f 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 @@
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 @@
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 @@
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 @@
}
// 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 09d843f..36b50a5 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 @@
@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 400ccda..c010af3 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 @@
@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
