[GitHub] [commons-compress] garydgregory commented on a diff in pull request #332: Add encryption support for SevenZ

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

garydgregory commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1036055562


##########
src/main/java/org/apache/commons/compress/archivers/sevenz/AES256Options.java:
##########
@@ -0,0 +1,34 @@
+/*
+ *  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.
+ *
+ */
+package org.apache.commons.compress.archivers.sevenz;
+
+import java.util.Arrays;
+import java.util.Random;
+
+public class AES256Options {
+    byte[] password;

Review Comment:
   Instance variables should be private and final when possible.



##########
src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZOutputFile.java:
##########
@@ -93,8 +114,27 @@ public SevenZOutputFile(final File fileName) throws IOException {
      * @since 1.13
      */
     public SevenZOutputFile(final SeekableByteChannel channel) throws IOException {
+        this(channel, null);
+    }
+
+    /**
+     * Prepares channel to write a 7z archive to.
+     *
+     * <p>{@link
+     * org.apache.commons.compress.utils.SeekableInMemoryByteChannel}
+     * allows you to write to an in-memory archive.</p>
+     *
+     * @param channel the channel to write to
+     * @param password optional password if the archive have to be encrypted
+     * @throws IOException if the channel cannot be positioned properly
+     * @since 1.13

Review Comment:
   The next version will be 1.23, not 1.13.



##########
src/test/java/org/apache/commons/compress/archivers/sevenz/SevenZOutputFileTest.java:
##########
@@ -468,6 +473,40 @@ public void testArchiveWithMixedMethods() throws Exception {
         }
     }
 
+    /**
+     * Test password-baed encryption
+     * 
+     * As AES/CBC Cipher requires a minimum of 16 bytes file data to be encrypted, some padding logic have been implemented.
+     * This test checks different file sizes (1, 16..) to ensure code coverage
+     */
+    @Test 
+    public void testEncrypt() throws Exception {
+        output = new File(dir, "encrypted.7z");
+        try (SevenZOutputFile outArchive = new SevenZOutputFile(output, "foo".toCharArray())) {
+            addFile(outArchive, 0, 1, null);
+            addFile(outArchive, 1, 16, null);
+            addFile(outArchive, 2, 32, null);
+            addFile(outArchive, 3, 33, null);
+            addFile(outArchive, 4, 10000, null);
+        }
+
+        // Is archive really password-based encrypted ?
+        try (SevenZFile archive = new SevenZFile(output)) {
+            verifyFile(archive, 0);
+            fail("A password should be needed");

Review Comment:
   Use assertThrows()



##########
src/main/java/org/apache/commons/compress/archivers/sevenz/AES256SHA256Decoder.java:
##########
@@ -126,4 +115,121 @@ public void close() throws IOException {
             }
         };
     }
+
+    @Override
+    OutputStream encode(OutputStream out, Object options) throws IOException {
+        AES256Options opts = (AES256Options) options;
+        final byte[] aesKeyBytes = sha256Password(opts.password, opts.numCyclesPower, opts.salt);
+        final SecretKey aesKey = new SecretKeySpec(aesKeyBytes, "AES");
+
+        final Cipher cipher;
+        try {
+            cipher = Cipher.getInstance("AES/CBC/NoPadding");
+            cipher.init(Cipher.ENCRYPT_MODE, aesKey, new IvParameterSpec(opts.iv));
+        } catch (final GeneralSecurityException generalSecurityException) {
+            throw new IOException(
+                "Encryption error " + "(do you have the JCE Unlimited Strength Jurisdiction Policy Files installed?)",

Review Comment:
   Why is a new Randon instance allocated each time?



##########
src/main/java/org/apache/commons/compress/archivers/sevenz/AES256Options.java:
##########
@@ -0,0 +1,34 @@
+/*
+ *  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.
+ *
+ */
+package org.apache.commons.compress.archivers.sevenz;
+
+import java.util.Arrays;
+import java.util.Random;
+
+public class AES256Options {

Review Comment:
   New public and protected elements need Javadocs with @since tags.



-- 
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: issues-unsubscribe@commons.apache.org

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