Skip to main content
aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorMickael Istria2022-01-18 14:40:22 +0000
committerMickael Istria2022-01-18 18:01:31 +0000
commit0353431e350e6991acf324f32fe00f153ccfcb06 (patch)
treef21960deeef1e8e9b8a637b990ffd6d8df1c3d9b
parentd4e59da60ecdc597f7bbb8041d2e1a25b798a2dd (diff)
downloadrt.equinox.p2-Y20220127-0600.tar.gz
rt.equinox.p2-Y20220127-0600.tar.xz
rt.equinox.p2-Y20220127-0600.zip
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>
-rw-r--r--bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/processors/pgp/PGPSignatureVerifier.java34
-rw-r--r--bundles/org.eclipse.equinox.p2.engine/src/org/eclipse/equinox/internal/p2/engine/phases/CertificateChecker.java36
-rw-r--r--bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/artifact/processors/PGPSignatureVerifierTest.java8
-rw-r--r--bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/artifact/repository/PGPVerifierTest.java5
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

Back to the top