Bug 575541 - [pgp] P2 should not fail completely if a public key is not
found
Ignore signatures for which no corresponding key can be found.
If all signatures are ignored, the artifact will be effectively treated
the same as an unsigned artifact.
In the unsigned artifact details presented to the user, include the key
ID(s) of the ignored signature(s).
Update the test to expect the artifact request status to be OK.
Change-Id: If433a81144d2eafed51e12d38396e9ffa5b09787
Signed-off-by: Ed Merks <ed.merks@gmail.com>
Reviewed-on: https://git.eclipse.org/r/c/equinox/rt.equinox.p2/+/189588
Tested-by: Equinox Bot <equinox-bot@eclipse.org>
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 d752d1b..5a06ec3 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
@@ -21,7 +21,6 @@
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
@@ -76,19 +75,21 @@
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 signture of given keys, and at least 1 of this key is trusted
+// 2. verify artifact signature matches signature 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 {
- signaturesToVerify = getSignatures(context);
+ signatures = getSignatures(context);
} catch (Exception ex) {
setStatus(new Status(IStatus.ERROR, Activator.ID, Messages.Error_CouldNotLoadSignature, ex));
return;
}
- if (signaturesToVerify.isEmpty()) {
+ if (signatures.isEmpty()) {
setStatus(Status.OK_STATUS);
return;
}
@@ -96,18 +97,21 @@
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 : signaturesToVerify) {
+ for (PGPSignature signature : signatures) {
PGPPublicKey publicKey = KNOWN_KEYS.getKey(signature.getKeyID());
- 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;
+ 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;
+ }
}
}
}
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 92aee71..83aef67 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,6 +96,7 @@
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<>();
@@ -132,14 +133,26 @@
if (!signatures.isEmpty()) {
if (trustedKeysIds.isEmpty() && !trustedKeys.get().isEmpty()) {
trustedKeysIds.addAll(trustedKeys.get().all().stream()
- .map(PGPPublicKey::getKeyID).map(Long::valueOf).collect(Collectors.toSet()));
+ .map(PGPPublicKey::getKeyID).collect(Collectors.toSet()));
}
- 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()));
+ 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);
+ }
}
} else {
unsigned.put(artifact.getKey(), artifactFile);
@@ -196,7 +209,15 @@
}
String[] details = EngineActivator.UNSIGNED_ALLOW.equals(policy) || unsigned.isEmpty() ? null
- : unsigned.values().stream().map(Object::toString).toArray(String[]::new);
+ : 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(Objects::toString)
+ .collect(Collectors.joining(", ", " [", "]")); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
+ }
+ return detail;
+ }).toArray(String[]::new);
Certificate[][] unTrustedCertificateChains = untrustedCertificates.isEmpty() ? null
: untrustedChain.toArray(Certificate[][]::new);
// If there was no unsigned content, and nothing untrusted, no need to prompt.
@@ -326,8 +347,8 @@
}
// load from bundles providing capability
for (IConfigurationElement extension : RegistryFactory.getRegistry()
- .getConfigurationElementsFor(EngineActivator.ID + ".pgp")) {
- if ("trustedKeys".equals(extension.getName())) {
+ .getConfigurationElementsFor(EngineActivator.ID + ".pgp")) { //$NON-NLS-1$
+ if ("trustedKeys".equals(extension.getName())) { //$NON-NLS-1$
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 36b50a5..09d843f 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,13 +71,17 @@
@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.ERROR, status.getSeverity());
- assertTrue(status.getMessage().contains("Public key not found for"));
+ assertEquals(IStatus.OK, 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 c010af3..400ccda 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,9 +62,10 @@
@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");
- assertNotOK(mirrorStatus);
- assertTrue(mirrorStatus.toString().contains("Public key not found"));
+ assertOK(mirrorStatus);
}
@Override
