Resolving issue: MNG-339

o Added checksumPolicy to artifact repository construction, which meant changing all the places where the factory was called.

o Added two command-line switches (-C=strict-checksum-checking, -c=lax-checksum-checking, or warning)

o Added checksum policy to all repository definitions (profiles.mdo, settings.mdo, maven.mdo)

o Changed the maven-artifact-ant stuff to use a Repository definition with checksumPolicy added to it

NOTE: I just realized that I haven't touched the inheritance/conversion of repository stuff from profiles/settings.xml to the model. I'll do this, and commit the additional changes.

Currently, the default checksum policy is to warn, since there are still bad checksums out there that prevent bootstrapping. Once we chase these down, we can change to default-strict checking. When verifying checksums, SHA-1 is attempted first, with MD5 acting as a backup verification method. If the checksum verification fails legitimately (not related to the process of retrieving/reading the checksum file), then the verification process is repeated ONCE ONLY.



git-svn-id: https://svn.apache.org/repos/asf/maven/components/trunk@191536 13f79535-47bb-0310-9956-ffa450edef68

maven-artifact-ant/src/main/java/org/apache/maven/artifact/ant/AbstractArtifactTask.java

@@ -18,6 +18,7 @@
 
 import org.apache.maven.artifact.manager.WagonManager;
 import org.apache.maven.artifact.repository.ArtifactRepository;
+import org.apache.maven.artifact.repository.ArtifactRepositoryFactory;
 import org.apache.maven.artifact.repository.layout.ArtifactRepositoryLayout;
 import org.apache.maven.artifact.Artifact;
 import org.apache.maven.artifact.factory.ArtifactFactory;
@@ -31,6 +32,7 @@
 import org.apache.tools.ant.Project;
 import org.apache.tools.ant.Task;
 import org.codehaus.plexus.PlexusContainerException;
+import org.codehaus.plexus.component.repository.exception.ComponentLifecycleException;
 import org.codehaus.plexus.component.repository.exception.ComponentLookupException;
 import org.codehaus.plexus.embed.Embedder;
 import org.codehaus.plexus.util.IOUtil;
@@ -97,16 +99,33 @@ protected ArtifactRepository createRemoteArtifactRepository( RemoteRepository re
                               proxy.getPassword(), proxy.getNonProxyHosts() );
         }
         
+        ArtifactRepositoryFactory repositoryFactory = null;
+        
         ArtifactRepository artifactRepository;
-        if ( repository.getSnapshotPolicy() != null )
+        
+        try
         {
-            artifactRepository = new ArtifactRepository( "remote", repository.getUrl(), repositoryLayout,
+            repositoryFactory = (ArtifactRepositoryFactory) lookup( ArtifactRepositoryFactory.ROLE );
-                                                         repository.getSnapshotPolicy() );
+            
+            String snapshotPolicy = repository.getSnapshotPolicy();
+            String checksumPolicy = repository.getChecksumPolicy();
+            
+            artifactRepository = repositoryFactory.createArtifactRepository( "remote", repository.getUrl(), 
+                                                                             repositoryLayout, snapshotPolicy, 
+                                                                             checksumPolicy );
         }
-        else
+        finally
         {
-            artifactRepository = new ArtifactRepository( "remote", repository.getUrl(), repositoryLayout );
+            try
+            {
+                getEmbedder().release( repositoryFactory );
+            }
+            catch ( ComponentLifecycleException e )
+            {
+                // TODO: Warn the user, or not?
+            }
         }
+
         return artifactRepository;
     }
 

maven-artifact-ant/src/main/java/org/apache/maven/artifact/ant/RemoteRepository.java

@@ -31,6 +31,8 @@ public class RemoteRepository
 
     private String snapshotPolicy;
     
+    private String checksumPolicy;
+    
     private Proxy proxy;
 
     public String getUrl()
@@ -72,4 +74,14 @@ public Proxy getProxy()
     {
         return proxy;
     }
+
+    public String getChecksumPolicy()
+    {
+        return checksumPolicy;
+    }
+
+    public void setChecksumPolicy( String checksumPolicy )
+    {
+        this.checksumPolicy = checksumPolicy;
+    }
 }

maven-artifact/src/main/java/org/apache/maven/artifact/ChecksumFailedException.java

@@ -30,4 +30,9 @@ public ChecksumFailedException( String s )
     {
         super( s );
     }
+    
+    public ChecksumFailedException( String message, Throwable cause )
+    {
+        super( message, cause );
+    }
 }

maven-artifact/src/main/java/org/apache/maven/artifact/manager/DefaultWagonManager.java

@@ -25,6 +25,7 @@
 import org.apache.maven.wagon.TransferFailedException;
 import org.apache.maven.wagon.UnsupportedProtocolException;
 import org.apache.maven.wagon.Wagon;
+import org.apache.maven.wagon.WagonException;
 import org.apache.maven.wagon.authentication.AuthenticationException;
 import org.apache.maven.wagon.authentication.AuthenticationInfo;
 import org.apache.maven.wagon.authorization.AuthorizationException;
@@ -77,8 +78,8 @@ public Wagon getWagon( String protocol )
         }
         catch ( ComponentLookupException e )
         {
-            throw new UnsupportedProtocolException( "Cannot find wagon which supports the requested protocol: " +
+            throw new UnsupportedProtocolException( "Cannot find wagon which supports the requested protocol: "
-                                                    protocol, e );
+                + protocol, e );
         }
 
         return wagon;
@@ -98,7 +99,7 @@ public void putArtifactMetadata( File source, ArtifactMetadata artifactMetadata,
     }
 
     private void putRemoteFile( ArtifactRepository repository, File source, String remotePath,
-                                TransferListener downloadMonitor )
+                               TransferListener downloadMonitor )
         throws TransferFailedException
     {
         String protocol = repository.getProtocol();
@@ -241,7 +242,7 @@ public void getArtifactMetadata( ArtifactMetadata metadata, ArtifactRepository r
     }
 
     private void getRemoteFile( ArtifactRepository repository, File destination, String remotePath,
-                                TransferListener downloadMonitor )
+                               TransferListener downloadMonitor )
         throws TransferFailedException, ResourceDoesNotExistException, ChecksumFailedException
     {
         // TODO: better excetpions - transfer failed is not enough?
@@ -270,11 +271,15 @@ private void getRemoteFile( ArtifactRepository repository, File destination, Str
         }
 
         // TODO: configure on repository
-        ChecksumObserver checksumObserver;
+        ChecksumObserver md5ChecksumObserver;
+        ChecksumObserver sha1ChecksumObserver;
         try
         {
-            checksumObserver = new ChecksumObserver( "MD5" );
+            md5ChecksumObserver = new ChecksumObserver( "MD5" );
-            wagon.addTransferListener( checksumObserver );
+            wagon.addTransferListener( md5ChecksumObserver );
+
+            sha1ChecksumObserver = new ChecksumObserver( "SHA-1" );
+            wagon.addTransferListener( sha1ChecksumObserver );
         }
         catch ( NoSuchAlgorithmException e )
         {
@@ -288,36 +293,92 @@ private void getRemoteFile( ArtifactRepository repository, File destination, Str
         {
             wagon.connect( repository, getAuthenticationInfo( repository.getId() ), getProxy( protocol ) );
 
-            // This should take care of creating destination directory now on
+            boolean firstRun = true;
-            wagon.get( remotePath, temp );
+            boolean retry = false;
             
-            try
+            // this will run at most twice. The first time, the firstRun flag is turned off, and if the retry flag
+            // is set on the first run, it will be turned off and not re-set on the second try. This is because the
+            // only way the retry flag can be set is if ( firstRun == true ).
+            while( firstRun || retry )
             {
-                // grab it first, because it's about to change...
+                // reset the retry flag.
-                String actualChecksum = checksumObserver.getActualChecksum();
+                retry = false;
                 
-                File checksumFile = new File( destination + ".md5" );
+                // This should take care of creating destination directory now on
-                wagon.get( remotePath + ".md5", checksumFile );
+                wagon.get( remotePath, temp );
                 
-                String expectedChecksum = FileUtils.fileRead( checksumFile );
+                // keep the checksum files from showing up on the download monitor...
-                if ( !expectedChecksum.equals( actualChecksum ) )
+                if ( downloadMonitor != null )
                 {
-                    getLogger().warn(
+                    wagon.removeTransferListener( downloadMonitor );
-                        "*** CHECKSUM MISMATCH - currently disabled fail due to bad repository checksums ***" );
+                }
-
+
-                    // TODO: optionally retry?
+                // try to verify the SHA-1 checksum for this file.
-                    /*                   throw new ChecksumFailedException( "Checksum failed on download: local = '" + actualChecksum +
+                try
-                     "'; remote = '" + expectedChecksum + "'" );
+                {
-                     */
+                    verifyChecksum( sha1ChecksumObserver, destination, remotePath, ".sha1", wagon );
+                }
+                catch ( WagonException sha1TryException )
+                {
+                    // if we catch a ChecksumFailedException, it means the transfer/read succeeded, but the checksum
+                    // doesn't match. This could be a problem with the server (ibiblio HTTP-200 error page), so we'll
+                    // try this up to two times. On the second try, we'll handle it as a bona-fide error, based on the
+                    // repository's checksum checking policy.
+                    if ( sha1TryException instanceof ChecksumFailedException )
+                    {
+                        // if this is the second try, handle the problem...otherwise, let it try again.
+                        if( firstRun )
+                        {
+                            retry = true;
+                        }
+                        else
+                        {
+                            handleChecksumFailure( repository, sha1TryException.getMessage(), sha1TryException.getCause() );
+                        }
+                    }
+                    // if this IS NOT a ChecksumFailedException, it was a problem with transfer/read of the checksum
+                    // file...we'll try again with the MD5 checksum.
+                    else
+                    {
+                        try
+                        {
+                            verifyChecksum( md5ChecksumObserver, destination, remotePath, ".md5", wagon );
+                        }
+                        catch ( WagonException md5TryException )
+                        {
+                            // if we also fail to verify based on the MD5 checksum, and the checksum transfer/read
+                            // succeeded, then we need to determine whether to retry or handle it as a failure.
+                            if( md5TryException instanceof ChecksumFailedException )
+                            {
+                                // only retry once.
+                                if( firstRun )
+                                {
+                                    retry = true;
+                                }
+                                else
+                                {
+                                    handleChecksumFailure( repository, md5TryException.getMessage(), md5TryException.getCause() );
+                                }
+                            }
+                            // otherwise, this was a failed transfer, and we don't want to retry.
+                            else
+                            {
+                                handleChecksumFailure( repository, "Error retrieving checksum file for " + destination, md5TryException );
+                            }
+                        }
+                    }
+                }
+                finally
+                {
+                    // reinstate the download monitor...
+                    if ( downloadMonitor != null )
+                    {
+                        wagon.addTransferListener( downloadMonitor );
+                    }
+                    
+                    // unset the firstRun flag, so we don't get caught in an infinite loop...
+                    firstRun = false;
                 }
-            }
-            catch ( ResourceDoesNotExistException e )
-            {
-                getLogger().warn( "No checksum exists - assuming a valid download" );
-            }
-            catch ( IOException e )
-            {
-                getLogger().error( "Unable to read checksum - assuming a valid download", e );
             }
         }
         catch ( ConnectionException e )
@@ -365,6 +426,47 @@ private void getRemoteFile( ArtifactRepository repository, File destination, Str
         }
     }
 
+    private void handleChecksumFailure( ArtifactRepository repository, String message, Throwable cause )
+        throws ChecksumFailedException
+    {
+        if( ArtifactRepository.CHECKSUM_POLICY_FAIL.equals( repository.getChecksumPolicy() ) )
+        {
+            throw new ChecksumFailedException( message, cause );
+        }
+        else
+        {
+            getLogger().warn( "*** CHECKSUM FAILED - " + message + " - IGNORING" );
+        }
+    }
+
+    private void verifyChecksum( ChecksumObserver checksumObserver, File destination, String remotePath,
+                                String checksumFileExtension, Wagon wagon )
+        throws WagonException
+    {
+        try
+        {
+            // grab it first, because it's about to change...
+            String actualChecksum = checksumObserver.getActualChecksum();
+
+            File checksumFile = new File( destination + ".sha1" );
+            wagon.get( remotePath + ".sha1", checksumFile );
+
+            String expectedChecksum = FileUtils.fileRead( checksumFile );
+            if ( !expectedChecksum.equals( actualChecksum ) )
+            {
+                //                getLogger().warn(
+                //                    "*** CHECKSUM MISMATCH - currently disabled fail due to bad repository checksums ***" );
+
+                throw new ChecksumFailedException( "Checksum failed on download: local = '" + actualChecksum
+                    + "'; remote = '" + expectedChecksum + "'" );
+            }
+        }
+        catch ( IOException e )
+        {
+            throw new TransferFailedException( "Invalid SHA-1 checksum file", e );
+        }
+    }
+
     private void disconnectWagon( Wagon wagon )
     {
         try
@@ -416,8 +518,7 @@ public Repository getMirror( String mirrorOf )
      * property format: <code>*.foo.com|localhost</code>.
      * @todo [BP] would be nice to configure this via plexus in some way
      */
-    public void addProxy( String protocol, String host, int port, String username, String password,
+    public void addProxy( String protocol, String host, int port, String username, String password, String nonProxyHosts )
-                          String nonProxyHosts )
     {
         ProxyInfo proxyInfo = new ProxyInfo();
         proxyInfo.setHost( host );
@@ -445,7 +546,7 @@ public void setDownloadMonitor( TransferListener downloadMonitor )
     }
 
     public void addAuthenticationInfo( String repositoryId, String username, String password, String privateKey,
-                                       String passphrase )
+                                      String passphrase )
     {
         AuthenticationInfo authInfo = new AuthenticationInfo();
 

maven-artifact/src/main/java/org/apache/maven/artifact/repository/ArtifactRepository.java

@@ -33,6 +33,8 @@ public class ArtifactRepository
 {
     private final String snapshotPolicy;
     
+    private final String checksumPolicy;
+
     private final ArtifactRepositoryLayout layout;
 
     public static final String SNAPSHOT_POLICY_NEVER = "never";
@@ -43,18 +45,28 @@ public class ArtifactRepository
 
     public static final String SNAPSHOT_POLICY_INTERVAL = "interval";
     
+    public static final String CHECKSUM_POLICY_FAIL = "fail";
+    
+    public static final String CHECKSUM_POLICY_WARN = "warn";
+    
+    public static final String CHECKSUM_ALGORITHM_SHA1 = "SHA-1";
+    
+    public static final String CHECKSUM_ALGORITHM_MD5 = "MD5";
+
     public ArtifactRepository( String id, String url, ArtifactRepositoryLayout layout )
     {
-        this( id, url, layout, SNAPSHOT_POLICY_NEVER );
+        this( id, url, layout, SNAPSHOT_POLICY_NEVER, CHECKSUM_POLICY_WARN );
     }
 
-    public ArtifactRepository( String id, String url, ArtifactRepositoryLayout layout, String snapshotPolicy )
+    public ArtifactRepository( String id, String url, ArtifactRepositoryLayout layout, String snapshotPolicy, String checksumPolicy )
     {
         super( id, url );
 
         this.layout = layout;
 
         this.snapshotPolicy = snapshotPolicy;
+        
+        this.checksumPolicy = checksumPolicy;
     }
 
     public String pathOf( Artifact artifact )
@@ -72,8 +84,18 @@ public String getSnapshotPolicy()
         return snapshotPolicy;
     }
     
+    public String getChecksumPolicy()
+    {
+        return checksumPolicy;
+    }
+    
+    public boolean failOnChecksumMismatch()
+    {
+        return CHECKSUM_POLICY_FAIL.equals( checksumPolicy );
+    }
+    
     public ArtifactRepository createMirror( Repository mirror )
     {
-        return new ArtifactRepository( mirror.getId(), mirror.getUrl(), layout, snapshotPolicy );
+        return new ArtifactRepository( mirror.getId(), mirror.getUrl(), layout, snapshotPolicy, checksumPolicy );
     }
 }

maven-artifact/src/main/java/org/apache/maven/artifact/repository/ArtifactRepositoryFactory.java

@@ -28,7 +28,9 @@ public interface ArtifactRepositoryFactory
 
     public ArtifactRepository createArtifactRepository( String id, String url,
                                                         ArtifactRepositoryLayout repositoryLayout,
-                                                        String snapshotPolicy );
+                                                        String snapshotPolicy, String checksumPolicy );
 
     void setGlobalSnapshotPolicy( String snapshotPolicy );
+    
+    void setGlobalChecksumPolicy( String checksumPolicy );
 }

maven-artifact/src/main/java/org/apache/maven/artifact/repository/DefaultArtifactRepositoryFactory.java

@@ -29,18 +29,39 @@ public class DefaultArtifactRepositoryFactory
     // TODO: use settings?
     private String globalSnapshotPolicy = null;
     
+    private String globalChecksumPolicy = null;
+
     public ArtifactRepository createArtifactRepository( String id, String url,
                                                         ArtifactRepositoryLayout repositoryLayout,
-                                                        String snapshotPolicy )
+                                                        String snapshotPolicy, String checksumPolicy )
     {
         ArtifactRepository repo = null;
         
+        String snapPolicy = snapshotPolicy;
+
         if ( globalSnapshotPolicy != null )
         {
-            snapshotPolicy = globalSnapshotPolicy;
+            snapPolicy = globalSnapshotPolicy;
         }
         
-        repo = new ArtifactRepository( id, url, repositoryLayout, snapshotPolicy );
+        if ( snapPolicy == null )
+        {
+            snapPolicy = ArtifactRepository.SNAPSHOT_POLICY_NEVER;
+        }
+        
+        String csumPolicy = checksumPolicy;
+        
+        if ( globalChecksumPolicy != null )
+        {
+            csumPolicy = globalChecksumPolicy;
+        }
+        
+        if ( csumPolicy == null )
+        {
+            csumPolicy = ArtifactRepository.CHECKSUM_POLICY_FAIL;
+        }
+        
+        repo = new ArtifactRepository( id, url, repositoryLayout, snapPolicy, csumPolicy );
 
         return repo;
     }
@@ -49,4 +70,9 @@ public void setGlobalSnapshotPolicy( String snapshotPolicy )
     {
         this.globalSnapshotPolicy = snapshotPolicy;
     }
+    
+    public void setGlobalChecksumPolicy( String checksumPolicy )
+    {
+        this.globalChecksumPolicy = checksumPolicy;
+    }
 }

maven-artifact/src/test/java/org/apache/maven/artifact/ArtifactComponentTestCase.java

@@ -90,7 +90,7 @@ protected ArtifactRepository remoteRepository()
         ArtifactRepositoryLayout repoLayout = (ArtifactRepositoryLayout) lookup( ArtifactRepositoryLayout.ROLE,
                                                                                  "legacy" );
 
-        ArtifactRepository repository = new ArtifactRepository( "test", "file://" + f.getPath(), repoLayout );
+        ArtifactRepository repository = new ArtifactRepository( "test", "file://" + f.getPath(), repoLayout, ArtifactRepository.SNAPSHOT_POLICY_NEVER, ArtifactRepository.CHECKSUM_POLICY_WARN );
 
         return repository;
     }

maven-core/src/main/java/org/apache/maven/cli/MavenCli.java

@@ -392,6 +392,20 @@ private static ArtifactRepository createLocalRepository( Embedder embedder, Sett
         {
             artifactRepositoryFactory.setGlobalSnapshotPolicy( ArtifactRepository.SNAPSHOT_POLICY_ALWAYS );
         }
+        
+        if ( commandLine.hasOption( CLIManager.CHECKSUM_FAILURE_POLICY ) )
+        {
+            System.out.println( "+ Enabling strict checksum verification on all artifact downloads.");
+            
+            artifactRepositoryFactory.setGlobalChecksumPolicy( ArtifactRepository.CHECKSUM_POLICY_FAIL );
+        }
+        else if ( commandLine.hasOption( CLIManager.CHECKSUM_WARNING_POLICY ) )
+        {
+            System.out.println( "+ Disabling strict checksum verification on all artifact downloads.");
+            
+            artifactRepositoryFactory.setGlobalChecksumPolicy( ArtifactRepository.CHECKSUM_POLICY_WARN );
+        }
+        
         return localRepository;
     }
 
@@ -509,6 +523,10 @@ static class CLIManager
         
         public static final char FORCE_PLUGIN_UPDATES = 'F';
         
+        public static final char CHECKSUM_FAILURE_POLICY = 'C';
+        
+        public static final char CHECKSUM_WARNING_POLICY = 'c';
+
         public CLIManager()
         {
             options = new Options();
@@ -536,6 +554,8 @@ public CLIManager()
                 "Comma-delimited list of profiles to activate").hasArg().create( ACTIVATE_PROFILES ) );
             options.addOption( OptionBuilder.withLongOpt( "batch-mode" ).withDescription( "Run in non-interactive (batch) mode" ).create( BATCH_MODE ) );
             options.addOption( OptionBuilder.withLongOpt( "update-plugins" ).withDescription( "Force upToDate check for any relevant registered plugins" ).create( FORCE_PLUGIN_UPDATES ) );
+            options.addOption( OptionBuilder.withLongOpt( "strict-checksums" ).withDescription( "Fail the build if checksums don't match" ).create( CHECKSUM_FAILURE_POLICY ) );
+            options.addOption( OptionBuilder.withLongOpt( "lax-checksums" ).withDescription( "Warn if checksums don't match" ).create( CHECKSUM_WARNING_POLICY ) );
         }
 
         public CommandLine parse( String[] args )

maven-model/maven.mdo

@@ -1999,6 +1999,13 @@
           <type>String</type>
           <defaultValue>default</defaultValue>
         </field>
+        <field>
+          <name>checksumPolicy</name>
+          <version>4.0.0</version>
+          <description>What to do when verification of an artifact checksum fails - warn, fail, etc. Valid values are "fail" or "warn"</description>
+          <type>String</type>
+          <defaultValue>warn</defaultValue>
+        </field>
       </fields>
       <codeSegments>
         <codeSegment>

maven-profile/profiles.mdo

@@ -181,6 +181,13 @@
           <type>String</type>
           <defaultValue>default</defaultValue>
         </field>
+        <field>
+          <name>checksumPolicy</name>
+          <version>1.0.0</version>
+          <description>What to do when verification of an artifact checksum fails - warn, fail, etc. Valid values are "fail" or "warn"</description>
+          <type>String</type>
+          <defaultValue>warn</defaultValue>
+        </field>
       </fields>
       <codeSegments>
         <codeSegment>

maven-project/src/main/java/org/apache/maven/project/ProjectUtils.java

@@ -66,9 +66,12 @@ public static ArtifactRepository buildArtifactRepository( Repository repo,
             String id = repo.getId();
             String url = repo.getUrl();
             String snapshotPolicy = repo.getSnapshotPolicy();
+            String checksumPolicy = repo.getChecksumPolicy();
+            
             // TODO: make this a map inside the factory instead, so no lookup needed
             ArtifactRepositoryLayout layout = getRepositoryLayout( repo, container );
-            return artifactRepositoryFactory.createArtifactRepository( id, url, layout, snapshotPolicy );
+            
+            return artifactRepositoryFactory.createArtifactRepository( id, url, layout, snapshotPolicy, checksumPolicy );
         }
         else
         {

maven-settings/settings.mdo

@@ -612,6 +612,13 @@
           <type>String</type>
           <defaultValue>default</defaultValue>
         </field>
+        <field>
+          <name>checksumPolicy</name>
+          <version>1.0.0</version>
+          <description>What to do when verification of an artifact checksum fails - warn, fail, etc. Valid values are "fail" or "warn"</description>
+          <type>String</type>
+          <defaultValue>warn</defaultValue>
+        </field>
       </fields>
       <codeSegments>
         <codeSegment>