michael-o commented on a change in pull request #139:
URL: https://github.com/apache/maven-resolver/pull/139#discussion_r784760358
##########
File path:
maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/BasicRepositoryConnector.java
##########
@@ -571,25 +587,26 @@ private void uploadChecksums( File file, byte[] bytes )
}
try
{
- Set<String> algos = new HashSet<>();
- for ( RepositoryLayout.Checksum checksum : checksums )
+ ArrayList<ChecksumAlgorithmFactory> algorithms = new
ArrayList<>();
+ for ( RepositoryLayout.ChecksumLocation checksumLocation :
checksums )
{
- algos.add( checksum.getAlgorithm() );
+ algorithms.add(
checksumLocation.getChecksumAlgorithmFactory() );
}
- Map<String, Object> sumsByAlgo;
+ Map<String, String> sumsByAlgo;
if ( bytes != null )
{
- sumsByAlgo = ChecksumUtils.calc( bytes, algos );
+ sumsByAlgo = ChecksumAlgorithmHelper.calculate( bytes,
algorithms );
}
else
{
- sumsByAlgo = ChecksumUtils.calc( file, algos );
+ sumsByAlgo = ChecksumAlgorithmHelper.calculate( file,
algorithms );
}
- for ( RepositoryLayout.Checksum checksum : checksums )
+ for ( RepositoryLayout.ChecksumLocation checksum : checksums )
Review comment:
Same here
##########
File path:
maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/BasicRepositoryConnector.java
##########
@@ -495,16 +510,16 @@ protected void runTask()
}
class PutTaskRunner
- extends TaskRunner
+ extends TaskRunner
{
private final File file;
private final FileTransformer fileTransformer;
- private final Collection<RepositoryLayout.Checksum> checksums;
+ private final Collection<RepositoryLayout.ChecksumLocation> checksums;
Review comment:
`checksums` => `checksumLocations`?
##########
File path:
maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/BasicRepositoryConnector.java
##########
@@ -571,25 +587,26 @@ private void uploadChecksums( File file, byte[] bytes )
}
try
{
- Set<String> algos = new HashSet<>();
- for ( RepositoryLayout.Checksum checksum : checksums )
+ ArrayList<ChecksumAlgorithmFactory> algorithms = new
ArrayList<>();
+ for ( RepositoryLayout.ChecksumLocation checksumLocation :
checksums )
{
- algos.add( checksum.getAlgorithm() );
+ algorithms.add(
checksumLocation.getChecksumAlgorithmFactory() );
}
- Map<String, Object> sumsByAlgo;
+ Map<String, String> sumsByAlgo;
Review comment:
Since the value is not an object anymore `uploadChecksum()` cannot throw
with `checksum instanceof Exception`. Can you explain how this is supposed to
work?
##########
File path:
maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/ChecksumCalculator.java
##########
@@ -46,50 +47,32 @@
static class Checksum
{
- final String algorithm;
+ final ChecksumAlgorithmFactory checksumAlgorithmFactory;
- final MessageDigest digest;
+ ChecksumAlgorithm algorithm;
Exception error;
- Checksum( String algorithm )
+ Checksum( ChecksumAlgorithmFactory checksumAlgorithmFactory )
{
- this.algorithm = algorithm;
- MessageDigest digest = null;
- try
- {
- digest = MessageDigest.getInstance( algorithm );
- }
- catch ( NoSuchAlgorithmException e )
- {
- error = e;
- }
- this.digest = digest;
+ this.checksumAlgorithmFactory = requireNonNull(
checksumAlgorithmFactory );
+ this.algorithm = checksumAlgorithmFactory.getAlgorithm();
}
- public void update( ByteBuffer buffer )
+ public void reset()
{
- if ( digest != null )
- {
- digest.update( buffer );
- }
+ this.algorithm = checksumAlgorithmFactory.getAlgorithm();
Review comment:
So the `ChecksumAlgorithm` does not support a `reset()` directly and a
new object is required?
##########
File path:
maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/layout/RepositoryLayout.java
##########
@@ -36,45 +37,45 @@
{
/**
- * A descriptor for a checksum file. This descriptor simply associates the
location of a checksum file with the
- * underlying algorithm used to calculate/verify it. Checksum algorithms
are denoted by names as used with
- * {@link java.security.MessageDigest#getInstance(String)}, e.g. {@code
"SHA-1"} or {@code "MD5"}.
+ * A descriptor for a checksum location. This descriptor simply associates
the location of a checksum file with the
+ * underlying checksum algorithm used to calculate/verify it.
*/
- final class Checksum
+ final class ChecksumLocation
Review comment:
Reasonable rename since the class does not contain a checksum.
##########
File path:
maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/ChecksumCalculator.java
##########
@@ -116,16 +100,16 @@ public static ChecksumCalculator newInstance( File
targetFile, Collection<Reposi
return new ChecksumCalculator( targetFile, checksums );
}
- private ChecksumCalculator( File targetFile,
Collection<RepositoryLayout.Checksum> checksums )
+ private ChecksumCalculator( File targetFile,
+ Collection<RepositoryLayout.ChecksumLocation>
checksums )
{
this.checksums = new ArrayList<>();
Set<String> algos = new HashSet<>();
- for ( RepositoryLayout.Checksum checksum : checksums )
+ for ( RepositoryLayout.ChecksumLocation checksum : checksums )
Review comment:
Rename `checksum`
##########
File path:
maven-resolver-connector-basic/src/test/java/org/eclipse/aether/connector/basic/ChecksumCalculatorTest.java
##########
@@ -39,22 +42,16 @@
public class ChecksumCalculatorTest
{
- private static final String SHA512 = "SHA-512";
-
- private static final String SHA256 = "SHA-256";
-
- private static final String SHA1 = "SHA-1";
-
- private static final String MD5 = "MD5";
-
private File file;
+ private final TestChecksumAlgorithmSelector selector = new
TestChecksumAlgorithmSelector();
+
private ChecksumCalculator newCalculator( String... algos )
{
- List<RepositoryLayout.Checksum> checksums = new ArrayList<>();
+ List<RepositoryLayout.ChecksumLocation> checksums = new ArrayList<>();
Review comment:
Rename
##########
File path:
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/Maven2RepositoryLayoutFactory.java
##########
@@ -83,23 +106,33 @@ public RepositoryLayout newInstance(
RepositorySystemSession session, RemoteRepo
throw new NoRepositoryLayoutException( repository );
}
boolean forSignature = ConfigUtils.getBoolean( session, false,
CONFIG_PROP_SIGNATURE_CHECKSUMS );
- List<String> checksumsAlgorithms = Arrays.asList(
ConfigUtils.getString( session,
- DEFAULT_CHECKSUMS_ALGORITHMS, CONFIG_PROP_CHECKSUMS_ALGORITHMS
).split( "," ) );
+ // ensure order of (potentially user set) algorithm list is kept and
is unique
+ LinkedHashSet<String> checksumsAlgorithmNames = new LinkedHashSet<>(
Arrays.asList(
Review comment:
We don't do this for other config options. I don't know whether we
should do this just for one.
##########
File path:
maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/ChecksumValidator.java
##########
@@ -63,21 +62,23 @@ boolean fetchChecksum( URI remote, File local )
private final ChecksumPolicy checksumPolicy;
- private final Collection<Checksum> checksums;
+ private final Collection<ChecksumLocation> checksums;
Review comment:
Rename
##########
File path:
maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumAlgorithmFactorySelector.java
##########
@@ -0,0 +1,44 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+
+import java.util.Set;
+
+/**
+ * Component performing selection of {@link ChecksumAlgorithmFactory} based on
known factory names.
+ *
+ * @since TBD
+ */
+public interface ChecksumAlgorithmFactorySelector
+{
+ /**
+ * Returns factory for given algorithm name, or throws if algorithm not
supported.
+ *
+ * @throws IllegalArgumentException if asked algorithm name is not
supported.
+ */
+ ChecksumAlgorithmFactory select( String algorithmName ) throws
IllegalArgumentException;
+
+ /**
+ * Returns a set of supported algorithm names. This set represents ALL the
algorithms supported by Resolver, and is
+ * NOT in any relation to given repository layout used checksums, returned
by method {@link
+ *
org.eclipse.aether.spi.connector.layout.RepositoryLayout#getChecksumAlgorithmNames()}
(is super set of it).
+ */
+ Set<String> getChecksumAlgorithmNames();
Review comment:
Do we need the semantics of `Set` or a a read-only collection suffcient?
##########
File path:
maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumAlgorithmHelper.java
##########
@@ -0,0 +1,106 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+
+import java.io.BufferedInputStream;
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.Buffer;
+import java.nio.ByteBuffer;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Helper for checksum operations.
+ *
+ * @since TBD
+ */
+public final class ChecksumAlgorithmHelper
+{
+ private ChecksumAlgorithmHelper()
+ {
+ // nop
+ }
+
+ /**
+ * Calculates checksums for specified data.
+ *
+ * @param data The content for which to calculate checksums, must
not be {@code null}.
+ * @param factories The checksum algorithm factories to use, must not be
{@code null}.
+ * @return The calculated checksums, indexed by algorithm name, or the
exception that occurred while trying to
+ * calculate it, never {@code null}.
+ * @throws IOException In case of any problem.
+ */
+ public static Map<String, String> calculate( byte[] data,
List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ return calculate( new ByteArrayInputStream( data ), factories );
+ }
+
+ /**
+ * Calculates checksums for specified file.
+ *
+ * @param file The file for which to calculate checksums, must not
be {@code null}.
+ * @param factories The checksum algorithm factories to use, must not be
{@code null}.
+ * @return The calculated checksums, indexed by algorithm name, or the
exception that occurred while trying to
+ * calculate it, never {@code null}.
+ * @throws IOException In case of any problem.
+ */
+ public static Map<String, String> calculate( File file,
List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ return calculate( new BufferedInputStream( new FileInputStream( file )
), factories );
Review comment:
This should be rather in a try-with-resources clause.
##########
File path:
maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumAlgorithmHelper.java
##########
@@ -0,0 +1,106 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+
+import java.io.BufferedInputStream;
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.Buffer;
+import java.nio.ByteBuffer;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Helper for checksum operations.
+ *
+ * @since TBD
+ */
+public final class ChecksumAlgorithmHelper
+{
+ private ChecksumAlgorithmHelper()
+ {
+ // nop
+ }
+
+ /**
+ * Calculates checksums for specified data.
+ *
+ * @param data The content for which to calculate checksums, must
not be {@code null}.
+ * @param factories The checksum algorithm factories to use, must not be
{@code null}.
+ * @return The calculated checksums, indexed by algorithm name, or the
exception that occurred while trying to
+ * calculate it, never {@code null}.
+ * @throws IOException In case of any problem.
+ */
+ public static Map<String, String> calculate( byte[] data,
List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ return calculate( new ByteArrayInputStream( data ), factories );
+ }
+
+ /**
+ * Calculates checksums for specified file.
+ *
+ * @param file The file for which to calculate checksums, must not
be {@code null}.
+ * @param factories The checksum algorithm factories to use, must not be
{@code null}.
+ * @return The calculated checksums, indexed by algorithm name, or the
exception that occurred while trying to
+ * calculate it, never {@code null}.
+ * @throws IOException In case of any problem.
+ */
+ public static Map<String, String> calculate( File file,
List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ return calculate( new BufferedInputStream( new FileInputStream( file )
), factories );
+ }
+
+ private static Map<String, String> calculate( InputStream inputStream,
List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ LinkedHashMap<String, ChecksumAlgorithm> algorithms = new
LinkedHashMap<>();
+ factories.forEach( f -> algorithms.put( f.getName(), f.getAlgorithm()
) );
+ try ( InputStream in = inputStream )
Review comment:
I don't like this. We haven't opened the IS, we should not close this.
##########
File path:
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/named/DiscriminatingNameMapper.java
##########
@@ -115,25 +113,22 @@ private String createDiscriminator( final
RepositorySystemSession session )
String hostname = ConfigUtils.getString( session, this.hostname,
CONFIG_PROP_HOSTNAME );
File basedir = session.getLocalRepository().getBasedir();
discriminator = hostname + ":" + basedir;
- try
- {
- Map<String, Object> checksums = ChecksumUtils
- .calc( discriminator.getBytes( StandardCharsets.UTF_8
), Collections.singletonList( "SHA-1" ) );
- Object checksum = checksums.get( "SHA-1" );
-
- if ( checksum instanceof Exception )
- {
- throw (Exception) checksum;
- }
-
- return String.valueOf( checksum );
- }
- catch ( Exception e )
- {
- LOGGER.warn( "Failed to calculate discriminator digest, using
'{}'", DEFAULT_DISCRIMINATOR_DIGEST, e );
- return DEFAULT_DISCRIMINATOR_DIGEST;
- }
+ return sha1String( discriminator );
Review comment:
Can we leave this block as-is right now since it is not related for
artifact/metadata checksums at all? I actually need a fixed length nash to
properly map input data.
##########
File path:
maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumAlgorithmHelper.java
##########
@@ -0,0 +1,106 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+
+import java.io.BufferedInputStream;
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.Buffer;
+import java.nio.ByteBuffer;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Helper for checksum operations.
+ *
+ * @since TBD
+ */
+public final class ChecksumAlgorithmHelper
+{
+ private ChecksumAlgorithmHelper()
+ {
+ // nop
+ }
+
+ /**
+ * Calculates checksums for specified data.
+ *
+ * @param data The content for which to calculate checksums, must
not be {@code null}.
+ * @param factories The checksum algorithm factories to use, must not be
{@code null}.
+ * @return The calculated checksums, indexed by algorithm name, or the
exception that occurred while trying to
+ * calculate it, never {@code null}.
+ * @throws IOException In case of any problem.
+ */
+ public static Map<String, String> calculate( byte[] data,
List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ return calculate( new ByteArrayInputStream( data ), factories );
+ }
+
+ /**
+ * Calculates checksums for specified file.
+ *
+ * @param file The file for which to calculate checksums, must not
be {@code null}.
+ * @param factories The checksum algorithm factories to use, must not be
{@code null}.
+ * @return The calculated checksums, indexed by algorithm name, or the
exception that occurred while trying to
+ * calculate it, never {@code null}.
+ * @throws IOException In case of any problem.
+ */
+ public static Map<String, String> calculate( File file,
List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ return calculate( new BufferedInputStream( new FileInputStream( file )
), factories );
+ }
+
+ private static Map<String, String> calculate( InputStream inputStream,
List<ChecksumAlgorithmFactory> factories )
+ throws IOException
+ {
+ LinkedHashMap<String, ChecksumAlgorithm> algorithms = new
LinkedHashMap<>();
+ factories.forEach( f -> algorithms.put( f.getName(), f.getAlgorithm()
) );
+ try ( InputStream in = inputStream )
+ {
+ ByteBuffer byteBuffer = ByteBuffer.allocate( 32 * 1024 );
Review comment:
What is the benefit of a byte buffer compared to a pure byte array?
##########
File path: src/site/markdown/about-checksums.md
##########
@@ -0,0 +1,72 @@
+# About checksums
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you 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.
+-->
+
+Maven Resolver uses "checksums" to verify the consistency of the downloaded
+artifacts. Checksums are usually laid out in repositories next to the file in
question, with file
+extension telling the checksum algorithm that produced the given checksum file
content. Current Maven Repository
+layout contains SHA-1 and MD5 checksums by default (they are produced by
Resolver by default).
+
+Historically, Maven Resolver used `java.security.MessageDigest` to implement
checksums. So to say, secure one-way
+hashes provided by Java Cryptography Architecture were (mis)used to implement
checksums for transport integrity
+validation. There is no misunderstanding here, secure hashes MAY be used as
checksums, as there is quite some
+overlap between "checksums" and "hashes" in general. But this simplicity comes
at a price: cryptographically "safe"
+algorithms require way more CPU cycles to calculate checksum, while all their
purpose is just
+"integrity validation", nothing more. There is no "security", "trust" or
whatever else implied or expected from
Review comment:
Very good.
##########
File path: src/site/markdown/about-checksums.md
##########
@@ -0,0 +1,72 @@
+# About checksums
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you 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.
+-->
+
+Maven Resolver uses "checksums" to verify the consistency of the downloaded
Review comment:
Why in quotes?
##########
File path:
maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/layout/RepositoryLayout.java
##########
@@ -83,39 +84,34 @@ public static Checksum forLocation( URI location, String
algorithm )
{
throw new IllegalArgumentException( "resource location must
not have a fragment: " + location );
}
- String extension = '.' + algorithm.replace( "-", "" ).toLowerCase(
Locale.ENGLISH );
- return new Checksum( algorithm, URI.create( location.toString() +
extension ) );
+ return new ChecksumLocation( URI.create( location + "." +
checksumAlgorithmFactory.getExtension() ),
+ checksumAlgorithmFactory );
}
- private static void verify( String algorithm, URI location )
+ private static void verify( URI location, ChecksumAlgorithmFactory
checksumAlgorithmFactory )
{
- requireNonNull( algorithm, "checksum algorithm cannot be null" );
- if ( algorithm.length() == 0 )
- {
- throw new IllegalArgumentException( "checksum algorithm cannot
be empty" );
- }
requireNonNull( location, "checksum location cannot be null" );
if ( location.isAbsolute() )
{
throw new IllegalArgumentException( "checksum location must be
relative" );
}
+ requireNonNull( checksumAlgorithmFactory, "checksum algorithm
cannot be null" );
Review comment:
factory missing
##########
File path: src/site/markdown/about-checksums.md
##########
@@ -0,0 +1,72 @@
+# About checksums
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you 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.
+-->
+
+Maven Resolver uses "checksums" to verify the consistency of the downloaded
+artifacts. Checksums are usually laid out in repositories next to the file in
question, with file
+extension telling the checksum algorithm that produced the given checksum file
content. Current Maven Repository
+layout contains SHA-1 and MD5 checksums by default (they are produced by
Resolver by default).
+
+Historically, Maven Resolver used `java.security.MessageDigest` to implement
checksums. So to say, secure one-way
+hashes provided by Java Cryptography Architecture were (mis)used to implement
checksums for transport integrity
+validation. There is no misunderstanding here, secure hashes MAY be used as
checksums, as there is quite some
+overlap between "checksums" and "hashes" in general. But this simplicity comes
at a price: cryptographically "safe"
+algorithms require way more CPU cycles to calculate checksum, while all their
purpose is just
+"integrity validation", nothing more. There is no "security", "trust" or
whatever else implied or expected from
+them.
+
+If you are interested in "trust" in your artifacts, it is Artifact Signatures
(for example
+[GPG Signatures](https://maven.apache.org/plugins/maven-gpg-plugin/)) that you
should look for.
+
+Hence, the usual argument that "XXX algorithm is unsafe, deprecated, not
secure anymore" does not stand in use case
+of Maven Resolver: there is nothing "secure" being involved with checksums.
Moreover, this is true not only for SHA-1
+algorithm, but even for it's "elder brother" MD5. Both algorithms are still
widely used today as "transport integrity
Review comment:
its
##########
File path:
maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumAlgorithmFactory.java
##########
@@ -0,0 +1,48 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+
+/**
+ * A component representing a checksum factory: provides {@link
ChecksumAlgorithm} instances, name and extension to be
+ * used with this algorithm. While directly injecting components of this type
is possible, it is not recommended. To
+ * obtain factory instances use {@link ChecksumAlgorithmFactorySelector}
instead.
+ *
+ * @since TBD
+ */
+public interface ChecksumAlgorithmFactory
+{
+ /**
+ * Returns the algorithm name, usually used as key, never {@code null}
value. The name is a standard name of
+ * algorithm (if applicable) or any other designator that is algorithm
commonly referred with. Example: "SHA-1".
+ */
+ String getName();
+
+ /**
+ * Returns the file extension to be used for given checksum file (without
leading dot), never {@code null}. The
+ * extension should be file and URL path friendly, and usually differs
from value returned by {@link #getName()}.
+ * Example: "sha1".
+ */
+ String getExtension();
Review comment:
Maybe this should rather be `getFileExtension()`/`getPathExtension()`.
The context of "extension" isn't clear w/o reading the Javadoc.
##########
File path:
maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumAlgorithm.java
##########
@@ -0,0 +1,46 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+
+import java.nio.ByteBuffer;
+
+/**
+ * Implementation performing checksum calculation for specific algorithm.
Instances of this interface are stateful,
+ * non-thread safe, and should not be reused.
+ *
+ * @since TBD
+ */
+public interface ChecksumAlgorithm
+{
+ /**
+ * Updates the checksum algorithm inner state with input.
+ */
+ void update( ByteBuffer input );
Review comment:
Do we receive data as `ByteBuffer` or rather `byte[]`?
##########
File path:
maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/checksum/ChecksumAlgorithmFactorySelector.java
##########
@@ -0,0 +1,44 @@
+package org.eclipse.aether.spi.connector.checksum;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+
+import java.util.Set;
+
+/**
+ * Component performing selection of {@link ChecksumAlgorithmFactory} based on
known factory names.
+ *
+ * @since TBD
+ */
+public interface ChecksumAlgorithmFactorySelector
+{
+ /**
+ * Returns factory for given algorithm name, or throws if algorithm not
supported.
+ *
+ * @throws IllegalArgumentException if asked algorithm name is not
supported.
+ */
+ ChecksumAlgorithmFactory select( String algorithmName ) throws
IllegalArgumentException;
Review comment:
No need for `throws`. It is a runtime exception. No effect.
##########
File path:
maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/layout/RepositoryLayout.java
##########
@@ -36,45 +37,45 @@
{
/**
- * A descriptor for a checksum file. This descriptor simply associates the
location of a checksum file with the
- * underlying algorithm used to calculate/verify it. Checksum algorithms
are denoted by names as used with
- * {@link java.security.MessageDigest#getInstance(String)}, e.g. {@code
"SHA-1"} or {@code "MD5"}.
+ * A descriptor for a checksum location. This descriptor simply associates
the location of a checksum file with the
+ * underlying checksum algorithm used to calculate/verify it.
*/
- final class Checksum
+ final class ChecksumLocation
{
-
- private final String algorithm;
-
private final URI location;
+ private final ChecksumAlgorithmFactory checksumAlgorithmFactory;
+
/**
* Creates a new checksum file descriptor with the specified algorithm
and location. The method
- * {@link #forLocation(URI, String)} is usually more convenient though.
- *
- * @param algorithm The algorithm used to calculate the checksum, must
not be {@code null}.
- * @param location The relative URI to the checksum file within a
repository, must not be {@code null}.
+ * {@link #forLocation(URI, ChecksumAlgorithmFactory)} is usually more
convenient though.
+ *
+ * @param location The relative URI to the checksum
file within a repository, must not be {@code
+ * null}.
+ * @param checksumAlgorithmFactory The checksum type used to calculate
the checksum, must not be {@code null}.
*/
- public Checksum( String algorithm, URI location )
+ public ChecksumLocation( URI location, ChecksumAlgorithmFactory
checksumAlgorithmFactory )
{
- verify( algorithm, location );
- this.algorithm = algorithm;
+ verify( location, checksumAlgorithmFactory );
this.location = location;
+ this.checksumAlgorithmFactory = checksumAlgorithmFactory;
}
/**
- * Creates a checksum file descriptor for the specified
artifact/metadata location and algorithm. The location
+ * Creates a checksum descriptor for the specified artifact/metadata
location and algorithm. The location
* of the checksum file itself is derived from the supplied resource
URI by appending the file extension
* corresponding to the algorithm. The file extension in turn is
derived from the algorithm name by stripping
* out any hyphen ('-') characters and lower-casing the name, e.g.
"SHA-1" is mapped to ".sha1".
Review comment:
There two lines aren't correct anymore since most of it is handled of to
the chksum factory.
##########
File path:
maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/layout/RepositoryLayout.java
##########
@@ -126,55 +122,63 @@ public URI getLocation()
@Override
public String toString()
{
- return location + " (" + algorithm + ")";
+ return location + " (" + checksumAlgorithmFactory.getName() + ")";
}
-
}
+ /**
+ * Returns the collection of checksum names this instance of layout uses,
never {@code null}.
+ *
+ * @since TBD
+ */
+ List<String> getChecksumAlgorithmNames();
Review comment:
This one is a list, up earlier it was a set...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
