[GitHub] [nifi] jfrazee commented on a change in pull request #4788: NIFI-8132 Replaced framework uses of MD5 with SHA-256

[GitBox <gi...@apache.org>]

jfrazee commented on a change in pull request #4788:
URL: https://github.com/apache/nifi/pull/4788#discussion_r576380776



##########
File path: nifi-commons/nifi-utils/src/main/java/org/apache/nifi/util/file/classloader/ClassLoaderUtils.java
##########
@@ -137,25 +137,17 @@ private static boolean isNotBlank(final String value) {
         return additionalClasspath.toArray(new URL[additionalClasspath.size()]);
     }
 
-    public static String generateAdditionalUrlsFingerprint(Set<URL> urls) {
-        List<String> listOfUrls = urls.stream().map(Object::toString).collect(Collectors.toList());
-        StringBuffer urlBuffer = new StringBuffer();
+    public static String generateAdditionalUrlsFingerprint(final Set<URL> urls) {

Review comment:
       Similar comment as above. This changes the behavior of a public method.

##########
File path: nifi-commons/nifi-utils/src/main/java/org/apache/nifi/util/file/FileUtils.java
##########
@@ -541,57 +540,30 @@ public static void syncWithRestore(final File primaryFile, final File restoreFil
     }
 
     /**
-     * Returns true if the given files are the same according to their MD5 hash.
+     * Returns true if the given files are the same according to their hash.
      *
      * @param file1 a file
      * @param file2 a file
      * @return true if the files are the same; false otherwise
-     * @throws IOException if the MD5 hash could not be computed
+     * @throws IOException if the hash could not be computed
      */
     public static boolean isSame(final File file1, final File file2) throws IOException {
-        return Arrays.equals(computeMd5Digest(file1), computeMd5Digest(file2));
+        return Arrays.equals(computeDigest(file1), computeDigest(file2));
     }
 
     /**
-     * Returns the MD5 hash of the given file.
+     * Returns the hash of the given file using default digest algorithm
      *
      * @param file a file
-     * @return the MD5 hash
-     * @throws IOException if the MD5 hash could not be computed
+     * @return Digest Hash Bytes
+     * @throws IOException if the hash could not be computed
      */
-    public static byte[] computeMd5Digest(final File file) throws IOException {
+    public static byte[] computeDigest(final File file) throws IOException {
         try (final FileInputStream fis = new FileInputStream(file)) {
-            return computeMd5Digest(fis);
+            return MessageDigestUtils.getDigest(fis);
         }
     }
 
-    /**
-     * Returns the MD5 hash of the given stream.
-     *
-     * @param stream an input stream
-     * @return the MD5 hash
-     * @throws IOException if the MD5 hash could not be computed
-     */
-    public static byte[] computeMd5Digest(final InputStream stream) throws IOException {

Review comment:
       This is public in a package users could be using in custom code. It should probably be marked `@Deprecated` for a cycle or two before removing. 

##########
File path: nifi-commons/nifi-utils/src/main/java/org/apache/nifi/util/file/classloader/ClassLoaderUtils.java
##########
@@ -137,25 +137,17 @@ private static boolean isNotBlank(final String value) {
         return additionalClasspath.toArray(new URL[additionalClasspath.size()]);
     }
 
-    public static String generateAdditionalUrlsFingerprint(Set<URL> urls) {
-        List<String> listOfUrls = urls.stream().map(Object::toString).collect(Collectors.toList());
-        StringBuffer urlBuffer = new StringBuffer();
+    public static String generateAdditionalUrlsFingerprint(final Set<URL> urls) {
+        final StringBuilder fingerprintBuilder = new StringBuilder();
 
         //Sorting so that the order is maintained for generating the fingerprint
-        Collections.sort(listOfUrls);
-        try {
-            MessageDigest md = MessageDigest.getInstance("MD5");
-            listOfUrls.forEach(url -> {
-                urlBuffer.append(url).append("-").append(getLastModified(url)).append(";");
-            });
-            byte[] bytesOfAdditionalUrls = urlBuffer.toString().getBytes(StandardCharsets.UTF_8);
-            byte[] bytesOfDigest = md.digest(bytesOfAdditionalUrls);
-
-            return DatatypeConverter.printHexBinary(bytesOfDigest);
-        } catch (NoSuchAlgorithmException e) {
-            LOGGER.error("Unable to generate fingerprint for the provided additional resources {}", new Object[]{urls, e});
-            return null;
-        }
+        final List<String> sortedUrls = urls.stream().map(Object::toString).collect(Collectors.toList());
+        Collections.sort(sortedUrls);

Review comment:
       I think we can just use the `sorted()` on the stream.
   ```suggestion
           final List<String> sortedUrls = urls.stream()
               .map(Object::toString)
               .sorted()
               .collect(Collectors.toList());
   ```




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org