diff options
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 |