Merge pull request #909 from stefanneuhaus/improve-centralanalyzer-stability

Improve centralanalyzer stability

dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/ArchiveAnalyzer.java

@@ -217,20 +217,6 @@ public class ArchiveAnalyzer extends AbstractFileTypeAnalyzer {
         }
     }
 
-    /**
-     * Does not support parallel processing as it both modifies and iterates
-     * over the engine's list of dependencies.
-     *
-     * @return <code>true</code> if the analyzer supports parallel processing;
-     * otherwise <code>false</code>
-     * @see #analyzeDependency(Dependency, Engine)
-     * @see #findMoreDependencies(Engine, File)
-     */
-    @Override
-    public boolean supportsParallelProcessing() {
-        return true;
-    }
-
     /**
      * Analyzes a given dependency. If the dependency is an archive, such as a
      * WAR or EAR, the contents are extracted, scanned, and added to the list of

dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java

@@ -75,19 +75,16 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer {
     private static final String SUPPORTED_EXTENSIONS = "jar";
 
     /**
-     * The analyzer should be disabled if there are errors, so this is a flag to
+     * There may be temporary issues when connecting to MavenCentral.
-     * determine if such an error has occurred.
+     * In order to compensate for 99% of the issues, we perform a retry
+     * before finally failing the analysis.
      */
-    private volatile boolean errorFlag = false;
+    private static final int NUMBER_OF_TRIES = 5;
 
     /**
      * The searcher itself.
      */
-    private CentralSearch searcher;
+    protected CentralSearch searcher;
-    /**
-     * Field indicating if the analyzer is enabled.
-     */
-    private boolean enabled = true;
 
     /**
      * Initializes the analyzer with the configured settings.
@@ -97,17 +94,7 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer {
     @Override
     public void initialize(Settings settings) {
         super.initialize(settings);
-        enabled = checkEnabled();
+        setEnabled(checkEnabled());
-    }
-
-    /**
-     * Determine whether to enable this analyzer or not.
-     *
-     * @return whether the analyzer should be enabled
-     */
-    @Override
-    public boolean isEnabled() {
-        return enabled;
     }
 
     /**
@@ -202,17 +189,13 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer {
      * Performs the analysis.
      *
      * @param dependency the dependency to analyze
-     * @param engine the engine
+     * @param engine     the engine
      * @throws AnalysisException when there's an exception during analysis
      */
     @Override
     public void analyzeDependency(Dependency dependency, Engine engine) throws AnalysisException {
-        if (errorFlag || !isEnabled()) {
-            return;
-        }
-
         try {
-            final List<MavenArtifact> mas = searcher.searchSha1(dependency.getSha1sum());
+            final List<MavenArtifact> mas = fetchMavenArtifacts(dependency);
             final Confidence confidence = mas.size() > 1 ? Confidence.HIGH : Confidence.HIGHEST;
             for (MavenArtifact ma : mas) {
                 LOGGER.debug("Central analyzer found artifact ({}) for dependency ({})", ma, dependency.getFileName());
@@ -256,8 +239,52 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer {
         } catch (FileNotFoundException fnfe) {
             LOGGER.debug("Artifact not found in repository: '{}", dependency.getFileName());
         } catch (IOException ioe) {
-            LOGGER.debug("Could not connect to Central search", ioe);
+            final String message = "Could not connect to Central search. Analysis failed.";
-            errorFlag = true;
+            LOGGER.error(message, ioe);
+            throw new AnalysisException(message, ioe);
         }
     }
+
+    /**
+     * Downloads the corresponding list of MavenArtifacts of the given
+     * dependency from MavenCentral.
+     * <p>
+     * As the connection to MavenCentral is known to be unreliable, we implement
+     * a simple retry logic in order to compensate for 99% of the issues.
+     *
+     * @param dependency the dependency to analyze
+     * @return the downloaded list of MavenArtifacts
+     * @throws FileNotFoundException if the specified artifact is not found
+     * @throws IOException           if connecting to MavenCentral finally failed
+     */
+    protected List<MavenArtifact> fetchMavenArtifacts(Dependency dependency) throws IOException {
+        IOException lastException = null;
+        long sleepingTimeBetweenRetriesInMillis = 1000;
+        int triesLeft = NUMBER_OF_TRIES;
+        while (triesLeft-- > 0) {
+            try {
+                return searcher.searchSha1(dependency.getSha1sum());
+            } catch (FileNotFoundException fnfe) {
+                // retry does not make sense, just throw the exception
+                throw fnfe;
+            } catch (IOException ioe) {
+                LOGGER.debug("Could not connect to Central search (tries left: {}): {}",
+                        triesLeft, ioe.getMessage());
+                lastException = ioe;
+
+                if (triesLeft > 0) {
+                    try {
+                        Thread.sleep(sleepingTimeBetweenRetriesInMillis);
+                        sleepingTimeBetweenRetriesInMillis *= 2;
+                    } catch (InterruptedException e) {
+                        Thread.currentThread().interrupt();
+                    }
+                }
+            }
+        }
+
+        final String message = "Finally failed connecting to Central search." +
+                " Giving up after " + NUMBER_OF_TRIES + " tries.";
+        throw new IOException(message, lastException);
+    }
 }

dependency-check-core/src/main/java/org/owasp/dependencycheck/data/central/CentralSearch.java

@@ -74,7 +74,7 @@ public class CentralSearch {
      *
      * @param settings the configured settings
      * @throws MalformedURLException thrown if the configured URL is
-     * invalid
+     *                               invalid
      */
     public CentralSearch(Settings settings) throws MalformedURLException {
         this.settings = settings;
@@ -101,8 +101,9 @@ public class CentralSearch {
      *
      * @param sha1 the SHA-1 hash string for which to search
      * @return the populated Maven GAV.
-     * @throws IOException if it's unable to connect to the specified repository
+     * @throws FileNotFoundException if the specified artifact is not found
-     * or if the specified artifact is not found.
+     * @throws IOException           if it's unable to connect to the specified
+     *                               repository
      */
     public List<MavenArtifact> searchSha1(String sha1) throws IOException {
         if (null == sha1 || !sha1.matches("^[0-9A-Fa-f]{40}$")) {
@@ -178,9 +179,8 @@ public class CentralSearch {
                 throw new FileNotFoundException("Artifact not found in Central");
             }
         } else {
-            LOGGER.debug("Could not connect to Central received response code: {} {}",
+            String errorMessage = "Could not connect to MavenCentral (" + conn.getResponseCode() + "): " + conn.getResponseMessage();
-                    conn.getResponseCode(), conn.getResponseMessage());
+            throw new IOException(errorMessage);
-            throw new IOException("Could not connect to Central");
         }
         return result;
     }

dependency-check-core/src/main/java/org/owasp/dependencycheck/dependency/Dependency.java

@@ -164,7 +164,7 @@ public class Dependency extends EvidenceCollection implements Serializable, Comp
      * Constructs a new Dependency object.
      */
     public Dependency() {
-        //empty contructor
+        //empty constructor
     }
 
     /**

dependency-check-core/src/main/java/org/owasp/dependencycheck/dependency/EvidenceCollection.java

@@ -25,8 +25,6 @@ import javax.annotation.concurrent.ThreadSafe;
 import org.apache.commons.lang3.builder.EqualsBuilder;
 import org.apache.commons.lang3.builder.HashCodeBuilder;
 import org.owasp.dependencycheck.utils.Filter;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 /**
  * Used to maintain a collection of Evidence.
@@ -40,10 +38,6 @@ class EvidenceCollection implements Serializable {
      * The serial version UID for serialization.
      */
     private static final long serialVersionUID = 1L;
-    /**
-     * The logger.
-     */
-    private static final Logger LOGGER = LoggerFactory.getLogger(EvidenceCollection.class);
     /**
      * A collection of vendor evidence.
      */

dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java

@@ -0,0 +1,172 @@
+/*
+ * This file is part of dependency-check-core.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * Copyright (c) 2017 Jeremy Long. All Rights Reserved.
+ */
+package org.owasp.dependencycheck.analyzer;
+
+import mockit.Expectations;
+import mockit.Mock;
+import mockit.MockUp;
+import mockit.Mocked;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.owasp.dependencycheck.analyzer.exception.AnalysisException;
+import org.owasp.dependencycheck.data.central.CentralSearch;
+import org.owasp.dependencycheck.data.nexus.MavenArtifact;
+import org.owasp.dependencycheck.dependency.Dependency;
+
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.util.Collections;
+import java.util.List;
+
+import static org.junit.Assert.assertEquals;
+
+/**
+ * Tests for the CentralAnalyzer.
+ */
+public class CentralAnalyzerTest {
+
+    private static final String SHA1_SUM = "my-sha1-sum";
+
+    @BeforeClass
+    public static void beforeClass() {
+        doNotSleepBetweenRetries();
+    }
+
+    @Test
+    @SuppressWarnings("PMD.NonStaticInitializer")
+    public void testFetchMavenArtifactsWithoutException(@Mocked final CentralSearch centralSearch,
+                                                        @Mocked final Dependency dependency)
+            throws IOException {
+
+        CentralAnalyzer instance = new CentralAnalyzer();
+        instance.searcher = centralSearch;
+        specifySha1SumFor(dependency);
+
+        final List<MavenArtifact> expectedMavenArtifacts = Collections.emptyList();
+        new Expectations() {{
+            centralSearch.searchSha1(SHA1_SUM);
+            returns(expectedMavenArtifacts);
+        }};
+
+        final List<MavenArtifact> actualMavenArtifacts = instance.fetchMavenArtifacts(dependency);
+
+        assertEquals(expectedMavenArtifacts, actualMavenArtifacts);
+    }
+
+    @Test
+    @SuppressWarnings("PMD.NonStaticInitializer")
+    public void testFetchMavenArtifactsWithSporadicIOException(@Mocked final CentralSearch centralSearch,
+                                                               @Mocked final Dependency dependency)
+            throws IOException {
+
+        CentralAnalyzer instance = new CentralAnalyzer();
+        instance.searcher = centralSearch;
+        specifySha1SumFor(dependency);
+
+        final List<MavenArtifact> expectedMavenArtifacts = Collections.emptyList();
+        new Expectations() {{
+            centralSearch.searchSha1(SHA1_SUM);
+            result = new IOException("Could not connect to MavenCentral (500): Internal Server Error");
+            result = new IOException("Could not connect to MavenCentral (500): Internal Server Error");
+            result = expectedMavenArtifacts;
+        }};
+
+        final List<MavenArtifact> actualMavenArtifacts = instance.fetchMavenArtifacts(dependency);
+
+        assertEquals(expectedMavenArtifacts, actualMavenArtifacts);
+    }
+
+    @Test(expected = FileNotFoundException.class)
+    @SuppressWarnings("PMD.NonStaticInitializer")
+    public void testFetchMavenArtifactsRethrowsFileNotFoundException(@Mocked final CentralSearch centralSearch,
+                                                                     @Mocked final Dependency dependency)
+            throws IOException {
+
+        CentralAnalyzer instance = new CentralAnalyzer();
+        instance.searcher = centralSearch;
+        specifySha1SumFor(dependency);
+
+        new Expectations() {{
+            centralSearch.searchSha1(SHA1_SUM);
+            result = new FileNotFoundException("Artifact not found in Central");
+        }};
+
+        instance.fetchMavenArtifacts(dependency);
+    }
+
+    @Test(expected = IOException.class)
+    @SuppressWarnings("PMD.NonStaticInitializer")
+    public void testFetchMavenArtifactsAlwaysThrowsIOException(@Mocked final CentralSearch centralSearch,
+                                                               @Mocked final Dependency dependency)
+            throws IOException {
+
+        CentralAnalyzer instance = new CentralAnalyzer();
+        instance.searcher = centralSearch;
+        specifySha1SumFor(dependency);
+
+        new Expectations() {{
+            centralSearch.searchSha1(SHA1_SUM);
+            result = new IOException("no internet connection");
+        }};
+
+        instance.fetchMavenArtifacts(dependency);
+    }
+
+    @Test(expected = AnalysisException.class)
+    @SuppressWarnings("PMD.NonStaticInitializer")
+    public void testFetchMavenArtifactsAlwaysThrowsIOExceptionLetsTheAnalysisFail(@Mocked final CentralSearch centralSearch,
+                                                                                  @Mocked final Dependency dependency)
+            throws AnalysisException, IOException {
+
+        CentralAnalyzer instance = new CentralAnalyzer();
+        instance.searcher = centralSearch;
+        specifySha1SumFor(dependency);
+
+        new Expectations() {{
+            centralSearch.searchSha1(SHA1_SUM);
+            result = new IOException("no internet connection");
+        }};
+
+        instance.analyze(dependency, null);
+    }
+
+    /**
+     * We do not want to waste time in unit tests.
+     */
+    private static void doNotSleepBetweenRetries() {
+        new MockUp<Thread>() {
+            @Mock
+            void sleep(long millis) {
+                // do not sleep
+            }
+        };
+    }
+
+    /**
+     * Specifies the mock dependency's SHA1 sum.
+     *
+     * @param dependency then dependency
+     */
+    @SuppressWarnings("PMD.NonStaticInitializer")
+    private void specifySha1SumFor(final Dependency dependency) {
+        new Expectations() {{
+            dependency.getSha1sum();
+            returns(SHA1_SUM);
+        }};
+    }
+}