You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2022/11/28 06:08:57 UTC
[GitHub] [commons-compress] Dougniel opened a new pull request, #332: feat: Encyrption support for Seven7
Dougniel opened a new pull request, #332:
URL: https://github.com/apache/commons-compress/pull/332
ππΌ The purpose is to provide password based encryption for 7z compression in the same way of decryption already supported, so go forward of one know limitation
βοΈIn this way, I would like to submit my contribution based on the existing implementation of decryption and the C++ implementation of 7z
β
I added one unit test
Implementation of password-based encryption for 7z compressor
Let me know what do you think about this implementation π
https://issues.apache.org/jira/browse/COMPRESS-633
--
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
[GitHub] [commons-compress] garydgregory commented on a diff in pull request #332: [COMPRESS-633] Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1038170771
##########
src/main/java/org/apache/commons/compress/archivers/sevenz/AES256SHA256Decoder.java:
##########
@@ -126,4 +115,109 @@ public void close() throws IOException {
}
};
}
+
+ @Override
+ OutputStream encode(OutputStream out, Object options) throws IOException {
+ final AES256Options opts = (AES256Options) options;
+
+ return new OutputStream() {
+ private final CipherOutputStream cipherOutputStream = new CipherOutputStream(out, opts.getCipher());
+
+ // Ensures that data are encrypt in respect of cipher block size and pad with '0' if smaller
+ // NOTE: As "AES/CBC/PKCS5Padding" is weak and should not be used, we use "AES/CBC/NoPadding" with this
+ // manual implementation for padding possible thanks to the size of the file stored separately
+ private final int cipherBlockSize = opts.getCipher().getBlockSize();
+ private final byte[] cipherBlockBuffer = new byte[cipherBlockSize];
+ private int count = 0;
+
+ @Override
+ public void write(int b) throws IOException {
+ cipherBlockBuffer[count++] = (byte) b;
+ if (count == cipherBlockSize) {
+ flushBuffer();
+ }
+ }
+
+ @Override
+ public void write(byte[] b, int off, int len) throws IOException {
+ int gap = len + count > cipherBlockSize ? cipherBlockSize - count : len;
+ System.arraycopy(b, off, cipherBlockBuffer, count, gap);
+ count += gap;
+
+ if (count == cipherBlockSize) {
+ flushBuffer();
+
+ if (len - gap >= cipherBlockSize) {
+ // skip buffer to encrypt data chunks big enought to fit cipher block size
+ int multipleCipherBlockSizeLen = (len - gap) / cipherBlockSize * cipherBlockSize;
+ cipherOutputStream.write(b, off + gap, multipleCipherBlockSizeLen);
+ gap += multipleCipherBlockSizeLen;
+ }
+ System.arraycopy(b, off + gap, cipherBlockBuffer, 0, len - gap);
+ count = len - gap;
+ }
+ }
+
+ private void flushBuffer() throws IOException {
+ cipherOutputStream.write(cipherBlockBuffer);
+ count = 0;
+ Arrays.fill(cipherBlockBuffer, (byte) 0);
+ }
+
+ @Override
+ public void flush() throws IOException {
+ cipherOutputStream.flush();
+ }
+
+ @Override
+ public void close() throws IOException {
+ if (count > 0) {
+ cipherOutputStream.write(cipherBlockBuffer);
+ }
+ cipherOutputStream.close();
+ }
+ };
+ }
+
+ static byte[] sha256Password(final byte[] password, final int numCyclesPower, final byte[] salt) {
+ final MessageDigest digest;
+ try {
+ digest = MessageDigest.getInstance("SHA-256");
+ } catch (final NoSuchAlgorithmException noSuchAlgorithmException) {
+ throw new IllegalStateException("SHA-256 is unsupported by your Java implementation", noSuchAlgorithmException);
+ }
+ final byte[] extra = new byte[8];
+ for (long j = 0; j < (1L << numCyclesPower); j++) {
+ digest.update(salt);
+ digest.update(password);
+ digest.update(extra);
+ for (int k = 0; k < extra.length; k++) {
+ ++extra[k];
+ if (extra[k] != 0) {
+ break;
+ }
+ }
+ }
+ return digest.digest();
+ }
+
+ @Override
+ byte[] getOptionsAsProperties(Object options) throws IOException {
+ AES256Options opts = (AES256Options) options;
Review Comment:
Use final where you can.
--
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
[GitHub] [commons-compress] garydgregory commented on pull request #332: [COMPRESS-633] Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #332:
URL: https://github.com/apache/commons-compress/pull/332#issuecomment-1343395856
Just rebase on git master to test with the latest, that will help us get closer ;-)
--
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
[GitHub] [commons-compress] Dougniel commented on a diff in pull request #332: Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
Dougniel commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1037684209
##########
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;
+ byte[] salt = new byte[0];
+ byte[] iv = new byte[16];
+ int numCyclesPower = 19;
+
+ public AES256Options(byte[] password) {
+ this.password = password;
Review Comment:
I consider myself not enought skilled in security concerns, but I asked myself about that too when I saw this mechanism that [clears password as soon as possible](https://github.com/apache/commons-compress/blob/0cbe431fe13e40735ccba054bab8fe4a83062b37/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java#L386) π€.
But I have chosen to implement the encryption like decryption [`SevenZFile.java`](https://github.com/apache/commons-compress/blob/0cbe431fe13e40735ccba054bab8fe4a83062b37/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java#L102) (that stores password too) for homogeneous reasons π€·.
By the way, as the password is required to decrypt each file independently (the sevenZ archive is not entirely encrypted - we can access metadata like file names without password - only files are encrypted)_. I don't know how to manage that without storing it somewhere in memory - asking user to fulfill it for each file to compress/decompress ? Even in this way it will be stored inside the Input/OutputStream used by the library user.
ππΌ What I could try is to store it in less places (I stored it in `AES256Options` as in `SevenZOutputFile`
##########
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;
+ byte[] salt = new byte[0];
+ byte[] iv = new byte[16];
+ int numCyclesPower = 19;
+
+ public AES256Options(byte[] password) {
+ this.password = password;
Review Comment:
I consider myself not enought skilled in security concerns, but I asked myself about that too when I saw this mechanism that [clears password as soon as possible](https://github.com/apache/commons-compress/blob/0cbe431fe13e40735ccba054bab8fe4a83062b37/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java#L386) π€.
But I have chosen to implement the encryption like decryption [`SevenZFile.java`](https://github.com/apache/commons-compress/blob/0cbe431fe13e40735ccba054bab8fe4a83062b37/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java#L102) (that stores password too) for homogeneous reasons π€·.
By the way, as the password is required to decrypt each file independently (the sevenZ archive is not entirely encrypted - we can access metadata like file names without password - only files are encrypted)_. I don't know how to manage that without storing it somewhere in memory - asking user to fulfill it for each file to compress/decompress ? Even in this way it will be stored inside the Input/OutputStream used by the library user.
ππΌ What I could try is to store it in less places (I stored it in `AES256Options` as in `SevenZOutputFile`)
--
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
[GitHub] [commons-compress] Dougniel commented on pull request #332: Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
Dougniel commented on PR #332:
URL: https://github.com/apache/commons-compress/pull/332#issuecomment-1334843789
@kinow, @garydgregory,
Thanks for your reviews, I just pushed corrections that you suggested and I changed the place where `Cipher` is initialized in order to not store password in a clear way (following @kinow [advice](https://github.com/apache/commons-compress/pull/332#discussion_r1037484243)), let me know what do you think about that.
With these changes, the implementation between decryption and encryption is less homogeneous. In my opinion the decryption could be aligned in the encryption way for the same concern about password storage, but also to reduce the complexity of [decode](https://github.com/apache/commons-compress/blob/0cbe431fe13e40735ccba054bab8fe4a83062b37/src/main/java/org/apache/commons/compress/archivers/sevenz/AES256SHA256Decoder.java#L36) method _(ex: replace `coder` and `password` parameters with `options` as done in `encode` method). But as theses changes will introduce a lot of changes, maybe it could be easier to do it in another PR
Cheers,
Daniel
--
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
[GitHub] [commons-compress] Dougniel commented on a diff in pull request #332: Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
Dougniel commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1037684209
##########
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;
+ byte[] salt = new byte[0];
+ byte[] iv = new byte[16];
+ int numCyclesPower = 19;
+
+ public AES256Options(byte[] password) {
+ this.password = password;
Review Comment:
I consider myself not enought skilled in security concerns, but I asked myself about that too when I saw this mechanism that [clears password as soon as possible](https://github.com/apache/commons-compress/blob/0cbe431fe13e40735ccba054bab8fe4a83062b37/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java#L386) π€.
I have chosen to implement the encryption like decryption done in [`SevenZFile.java`](https://github.com/apache/commons-compress/blob/0cbe431fe13e40735ccba054bab8fe4a83062b37/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java#L102) (that stores password too) for homogeneous reasons π€·.
By the way, as the password is required to decrypt each file independently _(the sevenZ archive is not entirely encrypted - we can access metadata like file names without password - only files are encrypted)_. I don't know how to manage that without storing it somewhere in memory - asking user to fulfill it for each file to compress/decompress ? ~Even in this way it will be stored inside the Input/OutputStream used by the library user.~ (Update: I checked through my debugger and I'm not so sure about that π
)
ππΌ What I could try is to store it in less places (I stored it in `AES256Options` as in `SevenZOutputFile`) and even try to initialise a [`Cipher`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/javax/crypto/Cipher.html) early as it seems a secure place for it
--
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
[GitHub] [commons-compress] Dougniel commented on a diff in pull request #332: Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
Dougniel commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1037684209
##########
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;
+ byte[] salt = new byte[0];
+ byte[] iv = new byte[16];
+ int numCyclesPower = 19;
+
+ public AES256Options(byte[] password) {
+ this.password = password;
Review Comment:
I consider myself not enought skilled in security concerns, but I asked myself about that too when I saw this mechanism that [clears password as soon as possible](https://github.com/apache/commons-compress/blob/0cbe431fe13e40735ccba054bab8fe4a83062b37/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java#L386) π€.
I have chosen to implement the encryption like decryption done in [`SevenZFile.java`](https://github.com/apache/commons-compress/blob/0cbe431fe13e40735ccba054bab8fe4a83062b37/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java#L102) (that stores password too) for homogeneous reasons π€·.
By the way, as the password is required to decrypt each file independently _(the sevenZ archive is not entirely encrypted - we can access metadata like file names without password - only files are encrypted)_. I don't know how to manage that without storing it somewhere in memory - asking user to fulfill it for each file to compress/decompress ? ~Even in this way it will be stored inside the Input/OutputStream used by the library user.~ (Update: I checked through my debugger and I'm not so sure about that π
)
ππΌ What I could try is to store it in less places (I stored it in `AES256Options` as in `SevenZOutputFile`) and even try to initialise a [`Cipher`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/javax/crypto/Cipher.html) sooner that seems a secure place for it
--
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
[GitHub] [commons-compress] github-code-scanning[bot] commented on a diff in pull request #332: feat: Encyrption support for Seven7
Posted by GitBox <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1033168787
##########
src/main/java/org/apache/commons/compress/archivers/sevenz/AES256SHA256Decoder.java:
##########
@@ -126,4 +114,99 @@
}
};
}
+
+ @Override
+ OutputStream encode(OutputStream out, Object options) throws IOException {
+ return new OutputStream() {
+ private boolean isInitialized;
+ private CipherOutputStream cipherOutputStream;
+
+ private CipherOutputStream init() throws IOException {
+ if (isInitialized) {
+ return cipherOutputStream;
+ }
+
+ AES256Options opts = (AES256Options) options;
+ final byte[] aesKeyBytes = sha256Password(opts.password, opts.numCyclesPower, opts.salt);
+
+ final SecretKey aesKey = new SecretKeySpec(aesKeyBytes, "AES");
+ try {
+ final Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding"); // padding required for very small files (< 16 bytes)
Review Comment:
## Use of a broken or risky cryptographic algorithm
Cryptographic algorithm [AES/CBC/PKCS5Padding](1) is weak and should not be used.
[Show more details](https://github.com/apache/commons-compress/security/code-scanning/104)
--
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
[GitHub] [commons-compress] garydgregory commented on pull request #332: [COMPRESS-633] Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #332:
URL: https://github.com/apache/commons-compress/pull/332#issuecomment-1359219490
Nope, we have other components to work through and Commons Net just had a release.
--
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
[GitHub] [commons-compress] garydgregory commented on a diff in pull request #332: [COMPRESS-633] Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1037641840
##########
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 Random instance allocated each time?
--
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
[GitHub] [commons-compress] kinow commented on pull request #332: Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
kinow commented on PR #332:
URL: https://github.com/apache/commons-compress/pull/332#issuecomment-1334495833
> @kinow It seems like you have tagged the wrong person.
Oh, apologies @tcurdt . No idea why GitHub suggested your user. it's normally pretty clever to show the handle of the opener of the issue first at the list. Fixed!
--
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
[GitHub] [commons-compress] Dougniel commented on a diff in pull request #332: Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
Dougniel commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1037684209
##########
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;
+ byte[] salt = new byte[0];
+ byte[] iv = new byte[16];
+ int numCyclesPower = 19;
+
+ public AES256Options(byte[] password) {
+ this.password = password;
Review Comment:
I consider myself not enought skilled in security concerns, but I asked myself about that too when I saw this mechanism that [clears password as soon as possible](https://github.com/apache/commons-compress/blob/0cbe431fe13e40735ccba054bab8fe4a83062b37/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java#L386) π€.
But I have chosen to implement the encryption like decryption done
in [`SevenZFile.java`](https://github.com/apache/commons-compress/blob/0cbe431fe13e40735ccba054bab8fe4a83062b37/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java#L102) (that stores password too) for homogeneous reasons π€·.
By the way, as the password is required to decrypt each file independently (the sevenZ archive is not entirely encrypted - we can access metadata like file names without password - only files are encrypted)_. I don't know how to manage that without storing it somewhere in memory - asking user to fulfill it for each file to compress/decompress ? Even in this way it will be stored inside the Input/OutputStream used by the library user.
ππΌ What I could try is to store it in less places (I stored it in `AES256Options` as in `SevenZOutputFile`)
##########
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;
+ byte[] salt = new byte[0];
+ byte[] iv = new byte[16];
+ int numCyclesPower = 19;
+
+ public AES256Options(byte[] password) {
+ this.password = password;
Review Comment:
I consider myself not enought skilled in security concerns, but I asked myself about that too when I saw this mechanism that [clears password as soon as possible](https://github.com/apache/commons-compress/blob/0cbe431fe13e40735ccba054bab8fe4a83062b37/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java#L386) π€.
But I have chosen to implement the encryption like decryption done in [`SevenZFile.java`](https://github.com/apache/commons-compress/blob/0cbe431fe13e40735ccba054bab8fe4a83062b37/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java#L102) (that stores password too) for homogeneous reasons π€·.
By the way, as the password is required to decrypt each file independently (the sevenZ archive is not entirely encrypted - we can access metadata like file names without password - only files are encrypted)_. I don't know how to manage that without storing it somewhere in memory - asking user to fulfill it for each file to compress/decompress ? Even in this way it will be stored inside the Input/OutputStream used by the library user.
ππΌ What I could try is to store it in less places (I stored it in `AES256Options` as in `SevenZOutputFile`)
--
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
[GitHub] [commons-compress] Dougniel commented on a diff in pull request #332: Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
Dougniel commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1037850366
##########
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:
I rework the implementation and now I only use SecureRandom one time
As in Java 8 it's not clear that [SecureRandom is thread safe or not](https://docs.oracle.com/javase/8/docs/api/java/security/SecureRandom.html), I didn't put In a static member _(even if the JavaDocs 9 seems to tell that [it could be ThreadSafe](https://docs.oracle.com/javase/9/docs/api/java/security/SecureRandom.html))_
What do you think ?
--
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
[GitHub] [commons-compress] Dougniel commented on a diff in pull request #332: Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
Dougniel commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1037684209
##########
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;
+ byte[] salt = new byte[0];
+ byte[] iv = new byte[16];
+ int numCyclesPower = 19;
+
+ public AES256Options(byte[] password) {
+ this.password = password;
Review Comment:
I consider myself not enought skilled in security concerns, but I asked myself about that too when I saw this mechanism that [clears password as soon as possible](https://github.com/apache/commons-compress/blob/0cbe431fe13e40735ccba054bab8fe4a83062b37/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java#L386) π€.
I have chosen to implement the encryption like decryption done in [`SevenZFile.java`](https://github.com/apache/commons-compress/blob/0cbe431fe13e40735ccba054bab8fe4a83062b37/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java#L102) (that stores password too) for homogeneous reasons π€·.
By the way, as the password is required to decrypt each file independently _(the sevenZ archive is not entirely encrypted - we can access metadata like file names without password - only files are encrypted)_. I don't know how to manage that without storing it somewhere in memory - asking user to fulfill it for each file to compress/decompress ? ~Even in this way it will be stored inside the Input/OutputStream used by the library user.~ (Update: I checked through my debugger and I'm not so sure about that π
)
ππΌ What I could try is to store it in less places (I stored it in `AES256Options` as in `SevenZOutputFile`) and even try to initialise a cipher sooner that seems a secure place for it
--
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
[GitHub] [commons-compress] kinow commented on a diff in pull request #332: Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
kinow commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1037477537
##########
src/main/java/org/apache/commons/compress/utils/ByteUtils.java:
##########
@@ -266,4 +270,17 @@ private static void checkReadLength(final int length) {
throw new IllegalArgumentException("Can't read more than eight bytes into a long value");
}
}
+
+ public static byte[] utf16Decode(final char[] chars) {
Review Comment:
Missing javadoc for this public method. See other public methods for examples.
##########
src/test/java/org/apache/commons/compress/archivers/sevenz/SevenZOutputFileTest.java:
##########
@@ -468,6 +473,40 @@ public void testArchiveWithMixedMethods() throws Exception {
}
}
+ /**
+ * Test password-baed encryption
Review Comment:
s/baed/based
##########
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;
+ byte[] salt = new byte[0];
+ byte[] iv = new byte[16];
+ int numCyclesPower = 19;
+
+ public AES256Options(byte[] password) {
+ this.password = password;
Review Comment:
I don't know the Commons Compress API well enough, but I guess it'd be better if the password were never stored in an object in memory or serialized, if possible. This means instead of storing the object in the class, instead trying to design a method `public void decompress(byte[] data, byte[] password) {...}` that never stores the password.
But if other classes are already doing that, then I guess that should be fine. (Didn't have time to check, sorry)
##########
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;
+ byte[] salt = new byte[0];
+ byte[] iv = new byte[16];
+ int numCyclesPower = 19;
+
+ public AES256Options(byte[] password) {
+ this.password = password;
+ new Random(Arrays.hashCode(password)).nextBytes(salt);
+ new Random(Arrays.hashCode(password)).nextBytes(iv);
Review Comment:
Do we need to worry about the `Random` instead of `SecureRandom` or some other random class from Commons Rng/etc?
##########
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:
Missing javadoc for public class. Worth documenting its members (properties, methods) too, whenever possible (something short is fine).
##########
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:
Unnecessary string concatenation?
##########
src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZOutputFile.java:
##########
@@ -68,17 +72,34 @@ public class SevenZOutputFile implements Closeable {
private Iterable<? extends SevenZMethodConfiguration> contentMethods =
Collections.singletonList(new SevenZMethodConfiguration(SevenZMethod.LZMA2));
private final Map<SevenZArchiveEntry, long[]> additionalSizes = new HashMap<>();
+ private byte[] password;
/**
* Opens file to write a 7z archive to.
*
* @param fileName the file to write to
+ * @param password optional password if the archive have to be encrypted
Review Comment:
s/if the archive have/if the archive has
##########
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:
s/1.13/1.23 (see `pom.xml` for the current snapshot, i.e. next release)
##########
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.
Review Comment:
I think it needs a `<p>` wrapping it to be a separate paragraph.
##########
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
Review Comment:
s/the archive have/the archive has
##########
src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java:
##########
@@ -2054,19 +2054,6 @@ public String getDefaultName() {
return lastSegment + "~";
}
- private static byte[] utf16Decode(final char[] chars) {
- if (chars == null) {
- return null;
- }
- final ByteBuffer encoded = UTF_16LE.encode(CharBuffer.wrap(chars));
- if (encoded.hasArray()) {
- return encoded.array();
- }
- final byte[] e = new byte[encoded.remaining()];
- encoded.get(e);
- return e;
- }
-
Review Comment:
:+1: I think it makes sense to move this method to `ByteUtils`, thanks!
##########
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.
Review Comment:
And s/some padding logic have/some padding logic has
--
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
[GitHub] [commons-compress] Dougniel commented on pull request #332: [COMPRESS-633] Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
Dougniel commented on PR #332:
URL: https://github.com/apache/commons-compress/pull/332#issuecomment-1358641774
Hi @garydgregory, an idea of next release date ?
--
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
[GitHub] [commons-compress] Dougniel commented on pull request #332: [COMPRESS-633] Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
Dougniel commented on PR #332:
URL: https://github.com/apache/commons-compress/pull/332#issuecomment-1343364000
It seems that there is an issue with `actions/setup-java` : https://github.com/actions/setup-java/issues/422
--
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
[GitHub] [commons-compress] Dougniel commented on pull request #332: [COMPRESS-633] Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
Dougniel commented on PR #332:
URL: https://github.com/apache/commons-compress/pull/332#issuecomment-1345669609
> @Dougniel
> Merged. Thank you for your work and patience. Please verify.
Thank you for your rigor, all is ok π
I noticed that you switched `AES256Options` to package-private, I hesitated because, as public, it offers more possibilities to advanced uses (per file Initialization Vector, per file password.. when setting it at `SevenZArchiveEntry` level). But this increases the public API surface and is not documented
--
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
[GitHub] [commons-compress] Dougniel commented on a diff in pull request #332: [COMPRESS-633] Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
Dougniel commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1038884999
##########
src/main/java/org/apache/commons/compress/utils/ByteUtils.java:
##########
@@ -266,4 +270,24 @@ private static void checkReadLength(final int length) {
throw new IllegalArgumentException("Can't read more than eight bytes into a long value");
}
}
+
Review Comment:
Right, I move it on `AES256SHA256Decoder` because it is used to convert password from char to byte for encryption/decryption
--
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
[GitHub] [commons-compress] Dougniel commented on a diff in pull request #332: Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
Dougniel commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1037684209
##########
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;
+ byte[] salt = new byte[0];
+ byte[] iv = new byte[16];
+ int numCyclesPower = 19;
+
+ public AES256Options(byte[] password) {
+ this.password = password;
Review Comment:
I consider myself not enought skilled in security concerns, but I asked myself about that too when I saw this mechanism that [clears password as soon as possible](https://github.com/apache/commons-compress/blob/0cbe431fe13e40735ccba054bab8fe4a83062b37/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java#L386) π€.
I have chosen to implement the encryption like decryption done in [`SevenZFile.java`](https://github.com/apache/commons-compress/blob/0cbe431fe13e40735ccba054bab8fe4a83062b37/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java#L102) (that stores password too) for homogeneous reasons π€·.
By the way, as the password is required to decrypt each file independently (the sevenZ archive is not entirely encrypted - we can access metadata like file names without password - only files are encrypted)_. I don't know how to manage that without storing it somewhere in memory - asking user to fulfill it for each file to compress/decompress ? Even in this way it will be stored inside the Input/OutputStream used by the library user.
ππΌ What I could try is to store it in less places (I stored it in `AES256Options` as in `SevenZOutputFile`)
--
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
[GitHub] [commons-compress] Dougniel commented on a diff in pull request #332: [COMPRESS-633] Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
Dougniel commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1043827496
##########
.vscode/settings.json:
##########
@@ -0,0 +1,3 @@
+{
+ "java.configuration.updateBuildConfiguration": "automatic"
+}
Review Comment:
Right, It was a mistake π
BTW, there is a `.idea` directory pushed before, so I will remove it too as it seems a mistake because there is a `.idea` on `.gitignore`
--
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
[GitHub] [commons-compress] garydgregory merged pull request #332: [COMPRESS-633] Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
garydgregory merged PR #332:
URL: https://github.com/apache/commons-compress/pull/332
--
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
[GitHub] [commons-compress] Dougniel commented on a diff in pull request #332: Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
Dougniel commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1037631958
##########
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;
+ byte[] salt = new byte[0];
+ byte[] iv = new byte[16];
+ int numCyclesPower = 19;
+
+ public AES256Options(byte[] password) {
+ this.password = password;
+ new Random(Arrays.hashCode(password)).nextBytes(salt);
+ new Random(Arrays.hashCode(password)).nextBytes(iv);
Review Comment:
Good suggestion, as `SecureRandom` doc says :
> This class provides a cryptographically strong random number generator (RNG)
ππΌ So I will switch to it
--
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
[GitHub] [commons-compress] Dougniel commented on a diff in pull request #332: Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
Dougniel commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1037632987
##########
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:
Right, a stupid copy/paste that I made. Maybe it was done that way for formatting purposes
--
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
[GitHub] [commons-compress] Dougniel commented on a diff in pull request #332: Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
Dougniel commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1037825356
##########
src/main/java/org/apache/commons/compress/utils/ByteUtils.java:
##########
@@ -266,4 +270,17 @@ private static void checkReadLength(final int length) {
throw new IllegalArgumentException("Can't read more than eight bytes into a long value");
}
}
+
+ public static byte[] utf16Decode(final char[] chars) {
+ if (chars == null) {
+ return null;
+ }
+ final ByteBuffer encoded = UTF_16LE.encode(CharBuffer.wrap(chars));
Review Comment:
Hi @garydgregory,
I don't have the answer, but the existing implementation of decryption use this π€·
--
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
[GitHub] [commons-compress] kinow commented on pull request #332: [COMPRESS-633] Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
kinow commented on PR #332:
URL: https://github.com/apache/commons-compress/pull/332#issuecomment-1334883648
> > (Also needs a JIRA issue for the changelog)
>
> Hi @kinow,
>
> Thank you for your rigorous and interesting review, I will correct these and respond
>
> The JIRA in description is not right for the purpose of changelog ? (https://issues.apache.org/jira/browse/COMPRESS-633)
>
> Cheers, Daniel
Hi Daniel,\\My bad, I hadn't seen that there was already a JIRA created for this issue. Thanks for updating the PR!
--
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
[GitHub] [commons-compress] garydgregory commented on a diff in pull request #332: [COMPRESS-633] Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1036054969
##########
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
[GitHub] [commons-compress] garydgregory commented on a diff in pull request #332: [COMPRESS-633] Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1038173634
##########
src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java:
##########
@@ -18,6 +18,7 @@
package org.apache.commons.compress.archivers.sevenz;
import static java.nio.charset.StandardCharsets.UTF_16LE;
+import static org.apache.commons.compress.utils.ByteUtils.utf16Decode;
Review Comment:
I think we only use this style of method references for assertions in tests.
--
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
[GitHub] [commons-compress] garydgregory commented on pull request #332: [COMPRESS-633] Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #332:
URL: https://github.com/apache/commons-compress/pull/332#issuecomment-1345293519
@Dougniel
Thank you for your work and patience. Please verify.
--
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
[GitHub] [commons-compress] Dougniel commented on a diff in pull request #332: [COMPRESS-633] Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
Dougniel commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1043827496
##########
.vscode/settings.json:
##########
@@ -0,0 +1,3 @@
+{
+ "java.configuration.updateBuildConfiguration": "automatic"
+}
Review Comment:
Right, It was a mistake π
--
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
[GitHub] [commons-compress] Dougniel commented on pull request #332: Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
Dougniel commented on PR #332:
URL: https://github.com/apache/commons-compress/pull/332#issuecomment-1334543846
> (Also needs a JIRA issue for the changelog)
Hi @kinow,
Thank you for your rigorous and interesting review, I will correct these and respond
The JIRA in description is not right for the purpose of changelog ? (https://issues.apache.org/jira/browse/COMPRESS-633)
Cheers,
Daniel
--
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
[GitHub] [commons-compress] garydgregory commented on a diff in pull request #332: Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1037734771
##########
src/test/java/org/apache/commons/compress/archivers/sevenz/SevenZOutputFileTest.java:
##########
@@ -546,8 +603,16 @@ private Boolean verifyFile(final SevenZFile archive, final int index,
if (entry.getSize() == 0) {
return Boolean.FALSE;
}
- assertEquals(1, entry.getSize());
- assertEquals('A', archive.read());
+ assertEquals(size, entry.getSize());
+
+ byte[] actual = new byte[size];
+ int count = 0;
+ while(count < size) {
Review Comment:
Fix formatting `while (`
--
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
[GitHub] [commons-compress] tcurdt commented on pull request #332: Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
tcurdt commented on PR #332:
URL: https://github.com/apache/commons-compress/pull/332#issuecomment-1334489603
@kinow It seems like you have tagged the wrong person.
--
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
[GitHub] [commons-compress] Dougniel commented on a diff in pull request #332: Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
Dougniel commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1037684209
##########
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;
+ byte[] salt = new byte[0];
+ byte[] iv = new byte[16];
+ int numCyclesPower = 19;
+
+ public AES256Options(byte[] password) {
+ this.password = password;
Review Comment:
I consider myself not enought skilled in security concerns, but I asked myself about that too when I saw this mechanism that [clears password as soon as possible](https://github.com/apache/commons-compress/blob/0cbe431fe13e40735ccba054bab8fe4a83062b37/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java#L386) π€.
I have chosen to implement the encryption like decryption done in [`SevenZFile.java`](https://github.com/apache/commons-compress/blob/0cbe431fe13e40735ccba054bab8fe4a83062b37/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java#L102) (that stores password too) for homogeneous reasons π€·.
By the way, as the password is required to decrypt each file independently _(the sevenZ archive is not entirely encrypted - we can access metadata like file names without password - only files are encrypted)_. I don't know how to manage that without storing it somewhere in memory - asking user to fulfill it for each file to compress/decompress ? Even in this way it will be stored inside the Input/OutputStream used by the library user.
ππΌ What I could try is to store it in less places (I stored it in `AES256Options` as in `SevenZOutputFile`)
--
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
[GitHub] [commons-compress] garydgregory commented on a diff in pull request #332: Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1037738230
##########
src/main/java/org/apache/commons/compress/utils/ByteUtils.java:
##########
@@ -266,4 +270,17 @@ private static void checkReadLength(final int length) {
throw new IllegalArgumentException("Can't read more than eight bytes into a long value");
}
}
+
+ public static byte[] utf16Decode(final char[] chars) {
+ if (chars == null) {
+ return null;
+ }
+ final ByteBuffer encoded = UTF_16LE.encode(CharBuffer.wrap(chars));
Review Comment:
Hm, why little-endian?
--
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
[GitHub] [commons-compress] Dougniel commented on pull request #332: feat: Encryption support for Seven7
Posted by GitBox <gi...@apache.org>.
Dougniel commented on PR #332:
URL: https://github.com/apache/commons-compress/pull/332#issuecomment-1330195038
I push an alternate implementation to throw out use of `AES/CBC/PKCS5Padding` raised by _Code scanning
/ CodeQL_ as weak
--
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
[GitHub] [commons-compress] kinow commented on pull request #332: Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
kinow commented on PR #332:
URL: https://github.com/apache/commons-compress/pull/332#issuecomment-1334253189
(Also needs a JIRA issue for the changelog)
--
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
[GitHub] [commons-compress] codecov-commenter commented on pull request #332: feat: Encyrption support for Seven7
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #332:
URL: https://github.com/apache/commons-compress/pull/332#issuecomment-1328608516
# [Codecov](https://codecov.io/gh/apache/commons-compress/pull/332?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#332](https://codecov.io/gh/apache/commons-compress/pull/332?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b55996c) into [master](https://codecov.io/gh/apache/commons-compress/commit/1ffc89eea058148db939e99033f8ad45a7491a55?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1ffc89e) will **increase** coverage by `0.03%`.
> The diff coverage is `81.70%`.
```diff
@@ Coverage Diff @@
## master #332 +/- ##
============================================
+ Coverage 80.12% 80.15% +0.03%
- Complexity 6608 6620 +12
============================================
Files 339 340 +1
Lines 25195 25250 +55
Branches 4146 4153 +7
============================================
+ Hits 20187 20239 +52
Misses 3411 3411
- Partials 1597 1600 +3
```
| [Impacted Files](https://codecov.io/gh/apache/commons-compress/pull/332?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Ξ | |
|---|---|---|
| [.../commons/compress/archivers/sevenz/SevenZFile.java](https://codecov.io/gh/apache/commons-compress/pull/332/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29tcHJlc3MvYXJjaGl2ZXJzL3NldmVuei9TZXZlblpGaWxlLmphdmE=) | `75.96% <ΓΈ> (+0.20%)` | :arrow_up: |
| [...a/org/apache/commons/compress/utils/ByteUtils.java](https://codecov.io/gh/apache/commons-compress/pull/332/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29tcHJlc3MvdXRpbHMvQnl0ZVV0aWxzLmphdmE=) | `94.20% <50.00%> (-5.80%)` | :arrow_down: |
| [...compress/archivers/sevenz/AES256SHA256Decoder.java](https://codecov.io/gh/apache/commons-compress/pull/332/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29tcHJlc3MvYXJjaGl2ZXJzL3NldmVuei9BRVMyNTZTSEEyNTZEZWNvZGVyLmphdmE=) | `72.94% <76.59%> (+3.12%)` | :arrow_up: |
| [...mmons/compress/archivers/sevenz/AES256Options.java](https://codecov.io/gh/apache/commons-compress/pull/332/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29tcHJlc3MvYXJjaGl2ZXJzL3NldmVuei9BRVMyNTZPcHRpb25zLmphdmE=) | `100.00% <100.00%> (ΓΈ)` | |
| [...ns/compress/archivers/sevenz/SevenZOutputFile.java](https://codecov.io/gh/apache/commons-compress/pull/332/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29tcHJlc3MvYXJjaGl2ZXJzL3NldmVuei9TZXZlblpPdXRwdXRGaWxlLmphdmE=) | `96.98% <100.00%> (+0.54%)` | :arrow_up: |
| [...compress/harmony/unpack200/bytecode/Attribute.java](https://codecov.io/gh/apache/commons-compress/pull/332/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29tcHJlc3MvaGFybW9ueS91bnBhY2syMDAvYnl0ZWNvZGUvQXR0cmlidXRlLmphdmE=) | `65.62% <0.00%> (+6.25%)` | :arrow_up: |
:mega: Weβre building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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
[GitHub] [commons-compress] garydgregory commented on a diff in pull request #332: [COMPRESS-633] Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1038175628
##########
src/main/java/org/apache/commons/compress/utils/ByteUtils.java:
##########
@@ -266,4 +270,24 @@ private static void checkReadLength(final int length) {
throw new IllegalArgumentException("Can't read more than eight bytes into a long value");
}
}
+
Review Comment:
It looks like this method is only used from the `sevenz` package so let's keep it package-private there and avoid increasing the public API surface.
--
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
[GitHub] [commons-compress] garydgregory commented on a diff in pull request #332: Add encryption support for SevenZ
Posted by 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
[GitHub] [commons-compress] garydgregory commented on a diff in pull request #332: [COMPRESS-633] Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1044525053
##########
src/main/java/org/apache/commons/compress/archivers/sevenz/AES256Options.java:
##########
@@ -0,0 +1,100 @@
+/*
+ * 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.security.GeneralSecurityException;
+import java.security.NoSuchAlgorithmException;
+import java.security.SecureRandom;
+import javax.crypto.Cipher;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.IvParameterSpec;
+import javax.crypto.spec.SecretKeySpec;
+
+/**
+ * Options for {@link SevenZMethod#AES256SHA256} encoder
+ *
+ * @since 1.23
+ * @see AES256SHA256Decoder
+ */
+public class AES256Options {
+
+ private final byte[] salt;
+ private final byte[] iv;
+ private final int numCyclesPower;
+ private final Cipher cipher;
+
+ /**
Review Comment:
Please complete the Javadoc comments. You need a starting sentence.
--
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
[GitHub] [commons-compress] kinow commented on a diff in pull request #332: [COMPRESS-633] Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
kinow commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1043444381
##########
.vscode/settings.json:
##########
@@ -0,0 +1,3 @@
+{
+ "java.configuration.updateBuildConfiguration": "automatic"
+}
Review Comment:
This file does not need to be added. I believe the repository does not include anything specific to Eclipse/IntelliJ/etc. If you use this file locally, you can add a global `.gitignore` to prevent Git from including it with your changes (or just ignore it when committing your changes).
--
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
[GitHub] [commons-compress] garydgregory commented on a diff in pull request #332: Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1037737611
##########
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?)",
+ generalSecurityException
+ );
+ }
+ return new OutputStream() {
+ final CipherOutputStream cipherOutputStream = new CipherOutputStream(out, cipher);
+
+ // Ensures that data are encrypt in respect of cipher block size and pad with '0' if smaller
+ // NOTE: As "AES/CBC/PKCS5Padding" is weak and should not be used, we use "AES/CBC/NoPadding" with this
+ // manual implementation for padding possible thanks to the size of the file stored separately
+ final int cipherBlockSize = cipher.getBlockSize();
+ final byte[] cipherBlockBuffer = new byte[cipherBlockSize];
+ private int count = 0;
+
+ @Override
+ public void write(int b) throws IOException {
+ cipherBlockBuffer[count++] = (byte) b;
+ if (count == cipherBlockSize) {
+ flushBuffer();
+ }
+ }
+
+ @Override
+ public void write(byte[] b, int off, int len) throws IOException {
+ int gap = len + count > cipherBlockSize ? cipherBlockSize - count : len;
+ System.arraycopy(b, off, cipherBlockBuffer, count, gap);
+ count += gap;
+
+ if (count == cipherBlockSize) {
+ flushBuffer();
+
+ if (len - gap >= cipherBlockSize) {
+ // skip buffer to encrypt data chunks big enought to fit cipher block size
+ int multipleCipherBlockSizeLen = (len - gap) / cipherBlockSize * cipherBlockSize;
+ cipherOutputStream.write(b, off + gap, multipleCipherBlockSizeLen);
+ gap += multipleCipherBlockSizeLen;
+ }
+ System.arraycopy(b, off + gap, cipherBlockBuffer, 0, len - gap);
+ count = len - gap;
+ }
+ }
+
+ private void flushBuffer() throws IOException {
+ cipherOutputStream.write(cipherBlockBuffer);
+ count = 0;
+ Arrays.fill(cipherBlockBuffer, (byte) 0);
+ }
+
+ @Override
+ public void flush() throws IOException {
+ cipherOutputStream.flush();
+ }
+
+ @Override
+ public void close() throws IOException {
+ if (count > 0) {
+ cipherOutputStream.write(cipherBlockBuffer);
+ }
+ cipherOutputStream.close();
+ }
+ };
+ }
+
+ private byte[] sha256Password(final byte[] password, final int numCyclesPower, final byte[] salt) throws IOException {
+ final MessageDigest digest;
+ try {
+ digest = MessageDigest.getInstance("SHA-256");
+ } catch (final NoSuchAlgorithmException noSuchAlgorithmException) {
+ throw new IOException("SHA-256 is unsupported by your Java implementation", noSuchAlgorithmException);
Review Comment:
This is not possible since SHA-256 is required by the JRE specification, see https://docs.oracle.com/javase/8/docs/api/java/security/MessageDigest.html, therefore this could be an `IllegalStateException`.
--
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
[GitHub] [commons-compress] garydgregory commented on pull request #332: [COMPRESS-633] Add encryption support for SevenZ
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #332:
URL: https://github.com/apache/commons-compress/pull/332#issuecomment-1345706828
@Dougniel
Right. We can always document it and make it public later. But also then make sure the API is right.
--
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