Bug 576429 - Log warning when none or only unsafe checksum

algorithms are used for an artifact

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

bundles/org.eclipse.equinox.p2.artifact.repository/plugin.xml

@@ -41,8 +41,9 @@
 	<extension
 			point="org.eclipse.equinox.p2.artifact.repository.artifactChecksums">
 		<artifactChecksum
-			algorithm="MD5"
-			id="md5">
+        algorithm="MD5"
+        id="md5"
+        warnInsecure="true">
 		</artifactChecksum>
 	</extension>
 	<extension

bundles/org.eclipse.equinox.p2.artifact.repository/schema/artifactChecksums.exsd

@@ -76,6 +76,14 @@ As other tools will rely on this id, consider using some well-defined value (i.e
                </documentation>
             </annotation>
          </attribute>
+         <attribute name="warnInsecure" type="boolean">
+            <annotation>
+               <documentation>
+                  Since 1.4.300
+Set to true if this algorithm is now considered as insecure. A warning will be logged when this algorithm is used. An artifact that has no checksums or checksums only for insecure algorithms will emit a warning to user.
+               </documentation>
+            </annotation>
+         </attribute>
       </complexType>
    </element>
 

bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/processors/checksum/ChecksumUtilities.java

@@ -19,12 +19,13 @@
 import java.security.NoSuchProviderException;
 import java.util.*;
 import java.util.Map.Entry;
+import java.util.stream.Collectors;
 import org.eclipse.core.runtime.*;
 import org.eclipse.equinox.internal.p2.artifact.repository.Messages;
+import org.eclipse.equinox.internal.p2.core.helpers.LogHelper;
 import org.eclipse.equinox.internal.p2.repository.Activator;
 import org.eclipse.equinox.internal.p2.repository.helpers.ChecksumHelper;
 import org.eclipse.equinox.internal.p2.repository.helpers.ChecksumProducer;
-import org.eclipse.equinox.internal.provisional.p2.artifact.repository.processing.ProcessingStep;
 import org.eclipse.equinox.p2.repository.artifact.IArtifactDescriptor;
 import org.eclipse.equinox.p2.repository.artifact.spi.ProcessingStepDescriptor;
 import org.eclipse.osgi.util.NLS;
@@ -43,8 +44,9 @@ public class ChecksumUtilities {
 	 * @throws IllegalArgumentException if property neither {@link IArtifactDescriptor#ARTIFACT_CHECKSUM} nor {@link IArtifactDescriptor#DOWNLOAD_CHECKSUM}
 	 * @see ChecksumHelper#getChecksums(IArtifactDescriptor, String)
 	 */
-	public static Collection<ProcessingStep> getChecksumVerifiers(IArtifactDescriptor descriptor, String property, Set<String> checksumsToSkip) throws IllegalArgumentException {
-		Collection<ProcessingStep> steps = new ArrayList<>();
+	public static Collection<ChecksumVerifier> getChecksumVerifiers(IArtifactDescriptor descriptor,
+			String property, Set<String> checksumsToSkip) throws IllegalArgumentException {
+		Collection<ChecksumVerifier> steps = new ArrayList<>();
 		Map<String, String> checksums = ChecksumHelper.getChecksums(descriptor, property);
 
 		IConfigurationElement[] checksumVerifierConfigurations = getChecksumComparatorConfigurations();
@@ -58,17 +60,25 @@ public static Collection<ProcessingStep> getChecksumVerifiers(IArtifactDescripto
 				if (checksumEntry.getKey().equals(checksumId)) {
 					String checksumAlgorithm = checksumVerifierConfiguration.getAttribute("algorithm"); //$NON-NLS-1$
 					String providerName = checksumVerifierConfiguration.getAttribute("providerName"); //$NON-NLS-1$
-					ChecksumVerifier checksumVerifier = new ChecksumVerifier(checksumAlgorithm, providerName, checksumId);
+					boolean insecure = Boolean.parseBoolean(checksumVerifierConfiguration.getAttribute("warnInsecure")); //$NON-NLS-1$
+					ChecksumVerifier checksumVerifier = new ChecksumVerifier(checksumAlgorithm, providerName, checksumId, insecure);
 					checksumVerifier.initialize(null, new ProcessingStepDescriptor(null, checksumEntry.getValue(), true), descriptor);
-					if (checksumVerifier.getStatus().isOK())
+					if (checksumVerifier.getStatus().isOK()) {
 						steps.add(checksumVerifier);
-					else
-						// TODO log something?
-						continue;
+					} else {
+						LogHelper.log(checksumVerifier.getStatus());
+					}
 				}
 			}
 		}
 
+		if (!steps.isEmpty() && steps.stream().allMatch(ChecksumVerifier::isInsecureAlgorithm)) {
+			LogHelper.log(new Status(IStatus.WARNING, Activator.ID,
+					NLS.bind(Messages.onlyInsecureDigestAlgorithmUsed,
+							steps.stream().map(ChecksumVerifier::getAlgorithmId).collect(Collectors.joining(",")), //$NON-NLS-1$
+							descriptor.getArtifactKey())));
+		}
+
 		return steps;
 	}
 

bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/processors/checksum/ChecksumVerifier.java

@@ -33,12 +33,14 @@ final public class ChecksumVerifier extends MessageDigestProcessingStep {
 	final private String algorithmName;
 	final private String providerName;
 	final private String algorithmId;
+	private final boolean insecureAlgorithm;
 
 	// public to access from tests
-	public ChecksumVerifier(String digestAlgorithm, String providerName, String algorithmId) {
+	public ChecksumVerifier(String digestAlgorithm, String providerName, String algorithmId, boolean insecure) {
 		this.algorithmName = digestAlgorithm;
 		this.providerName = providerName;
 		this.algorithmId = algorithmId;
+		this.insecureAlgorithm = insecure;
 		basicInitialize(null);
 	}
 
@@ -91,4 +93,7 @@ public String getAlgorithmId() {
 		return algorithmId;
 	}
 
+	public boolean isInsecureAlgorithm() {
+		return insecureAlgorithm;
+	}
 }

bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/processors/md5/MD5Verifier.java

@@ -70,7 +70,9 @@ private void basicInitialize(IProcessingStepDescriptor descriptor) {
 	@Override
 	protected void onClose(String digestString) {
 		// if the hashes don't line up set the status to error.
-		if (!digestString.equals(expectedMD5))
+		if (!digestString.equals(expectedMD5)) {
 			setStatus(new Status(IStatus.ERROR, Activator.ID, ProvisionException.ARTIFACT_MD5_NOT_MATCH, NLS.bind(Messages.Error_unexpected_hash, expectedMD5, digestString), null));
+		}
+		setStatus(new Status(IStatus.WARNING, Activator.ID, Messages.MD5_deprecated));
 	}
 }

bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/processors/md5/Messages.java

@@ -23,6 +23,8 @@ public class Messages extends NLS {
 	public static String Error_MD5_unavailable;
 	public static String Error_unexpected_hash;
 
+	public static String MD5_deprecated;
+
 	static {
 		// initialize resource bundle
 		NLS.initializeMessages(BUNDLE_NAME, Messages.class);

bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/processors/md5/messages.properties

@@ -15,3 +15,4 @@
 Error_invalid_hash=MD5 value not available or incorrect size, {0}.
 Error_MD5_unavailable=Could not create MD5 algorithm.
 Error_unexpected_hash=MD5 hash is not as expected. Expected: {0} and found {1}.
+MD5_deprecated=MD5 is a deprecated and unsafe checksum algorithm. Consider abandoning it in favor of safer checksums.

bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/Messages.java

@@ -71,6 +71,8 @@ public class Messages extends NLS {
 	public static String calculateChecksum_ok;
 	public static String calculateChecksum_error;
 	public static String calculateChecksum_providerError;
+	public static String onlyInsecureDigestAlgorithmUsed;
+	public static String noDigestAlgorithmToVerifyDownload;
 
 	static {
 		// initialize resource bundles

bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/RawMirrorRequest.java

@@ -20,7 +20,10 @@
 import java.util.Collections;
 import org.eclipse.core.runtime.*;
 import org.eclipse.equinox.internal.p2.artifact.processors.checksum.ChecksumUtilities;
+import org.eclipse.equinox.internal.p2.artifact.processors.checksum.ChecksumVerifier;
 import org.eclipse.equinox.internal.p2.artifact.repository.simple.SimpleArtifactRepository;
+import org.eclipse.equinox.internal.p2.core.helpers.LogHelper;
+import org.eclipse.equinox.internal.p2.repository.Activator;
 import org.eclipse.equinox.internal.p2.repository.Transport;
 import org.eclipse.equinox.internal.provisional.p2.artifact.repository.processing.ProcessingStep;
 import org.eclipse.equinox.internal.provisional.p2.artifact.repository.processing.ProcessingStepHandler;
@@ -57,7 +60,7 @@ public void perform(IArtifactRepository sourceRepository, IProgressMonitor monit
 		}
 		IStatus status = transfer(targetDescriptor, sourceDescriptor, monitor);
 
-		// if ok, cancelled or transfer has already been done with the canonical form return with status set 
+		// if ok, cancelled or transfer has already been done with the canonical form return with status set
 		if (status.getSeverity() == IStatus.CANCEL) {
 			setResult(status);
 			return;
@@ -85,9 +88,13 @@ public IArtifactDescriptor getArtifactDescriptor() {
 	// Perform the mirror operation without any processing steps
 	@Override
 	protected IStatus getArtifact(IArtifactDescriptor artifactDescriptor, OutputStream destination, IProgressMonitor monitor) {
-
 		if (SimpleArtifactRepository.CHECKSUMS_ENABLED) {
-			Collection<ProcessingStep> steps = ChecksumUtilities.getChecksumVerifiers(artifactDescriptor, IArtifactDescriptor.DOWNLOAD_CHECKSUM, Collections.emptySet());
+			Collection<ChecksumVerifier> steps = ChecksumUtilities.getChecksumVerifiers(artifactDescriptor,
+					IArtifactDescriptor.DOWNLOAD_CHECKSUM, Collections.emptySet());
+			if (steps.isEmpty()) {
+				LogHelper.log(new Status(IStatus.WARNING, Activator.ID,
+						NLS.bind(Messages.noDigestAlgorithmToVerifyDownload, artifactDescriptor.getArtifactKey())));
+			}
 			ProcessingStep[] stepArray = steps.toArray(new ProcessingStep[steps.size()]);
 			// TODO should probably be using createAndLink here
 			ProcessingStepHandler handler = new ProcessingStepHandler();

bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/messages.properties

@@ -62,4 +62,7 @@ calculateChecksum_providerError=Checksum provider id={0} algorithm={1} provider=
 exception_unableToCreateParentDir = Unable to create parent directory.
 folder_artifact_not_file_repo=Artifact {0} is a folder but the repository is an archive or remote location.
 retryRequest=Download of {0} failed on repository {1}. Retrying. 
-error_copying_local_file=An error occurred copying file {0}.
+error_copying_local_file=An error occurred copying file {0}.
+
+onlyInsecureDigestAlgorithmUsed = Only insecure digest algorithms ({0}) are being used to verify {1}. Consider using a safer digest algorithm.
+noDigestAlgorithmToVerifyDownload = No digest algorithm is available to verify download of {0}.

bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/SimpleArtifactRepository.java

@@ -29,10 +29,12 @@
 import org.eclipse.core.runtime.*;
 import org.eclipse.core.runtime.jobs.Job;
 import org.eclipse.equinox.internal.p2.artifact.processors.checksum.ChecksumUtilities;
+import org.eclipse.equinox.internal.p2.artifact.processors.checksum.ChecksumVerifier;
 import org.eclipse.equinox.internal.p2.artifact.processors.pgp.PGPSignatureVerifier;
 import org.eclipse.equinox.internal.p2.artifact.repository.*;
 import org.eclipse.equinox.internal.p2.artifact.repository.Messages;
 import org.eclipse.equinox.internal.p2.core.helpers.FileUtils;
+import org.eclipse.equinox.internal.p2.core.helpers.LogHelper;
 import org.eclipse.equinox.internal.p2.metadata.ArtifactKey;
 import org.eclipse.equinox.internal.p2.metadata.expression.CompoundIterator;
 import org.eclipse.equinox.internal.p2.metadata.index.IndexProvider;
@@ -502,7 +504,13 @@ private OutputStream addPreSteps(ProcessingStepHandler handler, IArtifactDescrip
 			steps.add(new ZipVerifierStep());
 
 		Set<String> skipChecksums = DOWNLOAD_MD5_CHECKSUM_ENABLED ? Collections.emptySet() : Collections.singleton(ChecksumHelper.MD5);
-		addChecksumVerifiers(descriptor, steps, skipChecksums, IArtifactDescriptor.DOWNLOAD_CHECKSUM);
+		ArrayList<ProcessingStep> downloadChecksumSteps = new ArrayList<>();
+		addChecksumVerifiers(descriptor, downloadChecksumSteps, skipChecksums, IArtifactDescriptor.DOWNLOAD_CHECKSUM);
+		if (downloadChecksumSteps.isEmpty()) {
+			LogHelper.log(new Status(IStatus.WARNING, Activator.ID,
+					NLS.bind(Messages.noDigestAlgorithmToVerifyDownload, descriptor.getArtifactKey())));
+		}
+		steps.addAll(downloadChecksumSteps);
 
 		// Add steps here if needed
 		if (steps.isEmpty())
@@ -514,7 +522,8 @@ private OutputStream addPreSteps(ProcessingStepHandler handler, IArtifactDescrip
 
 	private void addChecksumVerifiers(IArtifactDescriptor descriptor, ArrayList<ProcessingStep> steps, Set<String> skipChecksums, String property) {
 		if (CHECKSUMS_ENABLED) {
-			Collection<ProcessingStep> checksumVerifiers = ChecksumUtilities.getChecksumVerifiers(descriptor, property, skipChecksums);
+			Collection<ChecksumVerifier> checksumVerifiers = ChecksumUtilities.getChecksumVerifiers(descriptor,
+					property, skipChecksums);
 			steps.addAll(checksumVerifiers);
 		}
 	}

bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/artifact/processors/ChecksumUtilitiesTest.java

@@ -26,7 +26,6 @@
 import org.eclipse.equinox.internal.p2.artifact.processors.checksum.ChecksumVerifier;
 import org.eclipse.equinox.internal.p2.metadata.ArtifactKey;
 import org.eclipse.equinox.internal.p2.metadata.OSGiVersion;
-import org.eclipse.equinox.internal.provisional.p2.artifact.repository.processing.ProcessingStep;
 import org.eclipse.equinox.p2.repository.artifact.IArtifactDescriptor;
 import org.eclipse.equinox.p2.repository.artifact.spi.ArtifactDescriptor;
 import org.junit.Before;
@@ -70,10 +69,11 @@ public void buildArtifactDescriptor() {
 
 	@Test
 	public void testChecksumProperty() {
-		Collection<ProcessingStep> checksumVerifiers = ChecksumUtilities.getChecksumVerifiers(artifactDescriptor, propertyType, emptySet());
+		Collection<ChecksumVerifier> checksumVerifiers = ChecksumUtilities.getChecksumVerifiers(artifactDescriptor,
+				propertyType, emptySet());
 
 		assertEquals(format("Verifier for property=%s", property), 1, checksumVerifiers.size());
-		ChecksumVerifier verifier = (ChecksumVerifier) checksumVerifiers.iterator().next();
+		ChecksumVerifier verifier = checksumVerifiers.iterator().next();
 		assertEquals(digestAlgorithm, verifier.getAlgorithmName());
 		assertEquals(algorithmId, verifier.getAlgorithmId());
 		assertEquals(value, verifier.getExpectedChecksum());
@@ -82,7 +82,8 @@ public void testChecksumProperty() {
 
 	@Test
 	public void testChecksumsToSkip() {
-		Collection<ProcessingStep> checksumVerifiers = ChecksumUtilities.getChecksumVerifiers(artifactDescriptor, propertyType, singleton(algorithmId));
+		Collection<ChecksumVerifier> checksumVerifiers = ChecksumUtilities.getChecksumVerifiers(artifactDescriptor,
+				propertyType, singleton(algorithmId));
 
 		assertEquals(emptyList(), checksumVerifiers);
 	}

bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/artifact/processors/ChecksumVerifierTest.java

@@ -70,7 +70,7 @@ public void testInitialize() throws IOException, IllegalArgumentException, Secur
 		expect(processingStepDescriptor.getData()).andReturn(checksum);
 		replay(processingStepDescriptor);
 
-		ChecksumVerifier verifier = new ChecksumVerifier(digestAlgorithm, providerName, algorithmId);
+		ChecksumVerifier verifier = new ChecksumVerifier(digestAlgorithm, providerName, algorithmId, false);
 
 		verifier.initialize(null, processingStepDescriptor, null);
 
@@ -93,7 +93,7 @@ public void testInitialize_DownloadChecksum() throws IOException, IllegalArgumen
 		expect(artifactDescriptor.getProperties()).andReturn(properties);
 		replay(artifactDescriptor);
 
-		ChecksumVerifier verifier = new ChecksumVerifier(digestAlgorithm, providerName, algorithmId);
+		ChecksumVerifier verifier = new ChecksumVerifier(digestAlgorithm, providerName, algorithmId, false);
 
 		verifier.initialize(null, processingStepDescriptor, artifactDescriptor);
 
@@ -116,7 +116,7 @@ public void testInitialize_ArtifactChecksum() throws IOException, IllegalArgumen
 		expect(artifactDescriptor.getProperty(not(eq(artifactProperty)))).andReturn(null).times(1, 2);
 		replay(artifactDescriptor);
 
-		ChecksumVerifier verifier = new ChecksumVerifier(digestAlgorithm, providerName, algorithmId);
+		ChecksumVerifier verifier = new ChecksumVerifier(digestAlgorithm, providerName, algorithmId, false);
 
 		verifier.initialize(null, processingStepDescriptor, artifactDescriptor);