nodece commented on code in PR #20542: URL: https://github.com/apache/pulsar/pull/20542#discussion_r1228211888
########## bouncy-castle/bc-common/src/test/java/org/apache/pulsar/client/impl/crypto/WrappingVersusEncryptionCrossCompatibilityTestBase.java: ########## @@ -0,0 +1,236 @@ +/* + * 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.pulsar.client.impl.crypto; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.testng.AssertJUnit.assertEquals; +import com.google.common.collect.ImmutableMap; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Paths; +import java.security.NoSuchAlgorithmException; +import java.security.NoSuchProviderException; +import java.security.PrivateKey; +import java.security.PublicKey; +import java.security.spec.InvalidKeySpecException; +import java.util.Optional; +import javax.crypto.KeyGenerator; +import javax.crypto.SecretKey; +import javax.crypto.SecretKeyFactory; +import javax.crypto.spec.SecretKeySpec; +import org.apache.commons.io.FileUtils; +import org.apache.pulsar.client.api.PulsarClientException; +import org.apache.pulsar.common.util.SecurityUtility; +import org.hamcrest.Matcher; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; + +public abstract class WrappingVersusEncryptionCrossCompatibilityTestBase { + + //from https://stackoverflow.com/a/140861 + //TODO use java.format.HexFormat after we move to java17 language features + protected static byte[] hexStringToByteArray(String s/* s must be an even-length string. */) { Review Comment: You can use `Hex.decodeHex(s)` instead. ########## pulsar-client-messagecrypto-bc/src/main/java/org/apache/pulsar/client/impl/crypto/MessageCryptoBc.java: ########## @@ -106,28 +80,64 @@ public class MessageCryptoBc implements MessageCrypto<MessageMetadata, MessageMe private static final SecureRandom secureRandom; + + private static final String providerName; + private static BcVersionSpecificCryptoUtility bcVersionSpecificCryptoUtilityDelegate; + static { - SecureRandom rand = null; - try { - rand = SecureRandom.getInstance("NativePRNGNonBlocking"); - } catch (NoSuchAlgorithmException nsa) { - rand = new SecureRandom(); - } - secureRandom = rand; + providerName = SecurityUtility.getProvider().getName(); + + switch (providerName) { + case SecurityUtility.BC: + SecureRandom rand = null; + try { + rand = SecureRandom.getInstance("NativePRNGNonBlocking", providerName); + } catch (NoSuchAlgorithmException | NoSuchProviderException nsa) { + rand = new SecureRandom(); + } + secureRandom = rand; + break; + case SecurityUtility.BC_FIPS: + try { + secureRandom = SecureRandom.getInstance("NONCEANDIV", providerName); + } catch (NoSuchAlgorithmException | NoSuchProviderException nsa) { + throw new RuntimeException( + "In BC FIPS mode, we expect the specific Random generators to be available!"); + } + break; Review Comment: IMO, if the "NONCEANDIV" doesn't work, we should consider the "DEFAULT" because we will end up using the "DEFAULT". And then you can remove the following code: ``` SecureRandom secureRandomForKeygen = null; switch (providerName) { case SecurityUtility.BC: { //TODO is this prediction resistant for generating keys? secureRandomForKeygen = secureRandom; break; } case SecurityUtility.BC_FIPS: { try { //prediction resistant secureRandomForKeygen = SecureRandom.getInstance("DEFAULT", providerName); } catch (NoSuchAlgorithmException | NoSuchProviderException nsa) { throw new RuntimeException( "In BC FIPS mode, we expect the specific Random generators to be available!"); } break; } default: throw new IllegalStateException("Provider not handled for encryption: " + providerName); } ``` ########## bouncy-castle/bc-common/src/test/java/org/apache/pulsar/client/impl/crypto/WrappingVersusEncryptionCrossCompatibilityTestBase.java: ########## @@ -0,0 +1,236 @@ +/* + * 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.pulsar.client.impl.crypto; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.testng.AssertJUnit.assertEquals; +import com.google.common.collect.ImmutableMap; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Paths; +import java.security.NoSuchAlgorithmException; +import java.security.NoSuchProviderException; +import java.security.PrivateKey; +import java.security.PublicKey; +import java.security.spec.InvalidKeySpecException; +import java.util.Optional; +import javax.crypto.KeyGenerator; +import javax.crypto.SecretKey; +import javax.crypto.SecretKeyFactory; +import javax.crypto.spec.SecretKeySpec; +import org.apache.commons.io.FileUtils; +import org.apache.pulsar.client.api.PulsarClientException; +import org.apache.pulsar.common.util.SecurityUtility; +import org.hamcrest.Matcher; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; + +public abstract class WrappingVersusEncryptionCrossCompatibilityTestBase { + + //from https://stackoverflow.com/a/140861 + //TODO use java.format.HexFormat after we move to java17 language features + protected static byte[] hexStringToByteArray(String s/* s must be an even-length string. */) { + int len = s.length(); + byte[] data = new byte[len / 2]; + for (int i = 0; i < len; i += 2) { + data[i / 2] = (byte) ((Character.digit(s.charAt(i), 16) << 4) + + Character.digit(s.charAt(i + 1), 16)); + } + return data; + } + + //from https://stackoverflow.com/a/9855338 + private static final byte[] HEX_ARRAY = "0123456789ABCDEF".getBytes(StandardCharsets.US_ASCII); + + //TODO use java.format.HexFormat after we move to java17 language features + protected static String bytesToHex(byte[] bytes) { + byte[] hexChars = new byte[bytes.length * 2]; Review Comment: Yan can use `Hex.encodeHexString(bytes)` instead. ########## bouncy-castle/bcfips/pom.xml: ########## @@ -51,6 +58,12 @@ <artifactId>bcpkix-fips</artifactId> <version>${bouncycastle.bcpkix-fips.version}</version> </dependency> + <dependency> + <groupId>${project.groupId}</groupId> Review Comment: Duplicated. See line 43-48. ########## bouncy-castle/bc-common/src/main/java/org/apache/pulsar/client/impl/crypto/BcVersionSpecificCryptoUtilityFactory.java: ########## @@ -0,0 +1,48 @@ +/* + * 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.pulsar.client.impl.crypto; + +import java.lang.reflect.InvocationTargetException; +import org.apache.pulsar.common.util.SecurityUtility; + +class BcVersionSpecificCryptoUtilityFactory { + + static BcVersionSpecificCryptoUtility createNewInstance() { + String providerName = SecurityUtility.getProvider().getName(); + try { + switch (providerName) { + case SecurityUtility.BC: + return (BcVersionSpecificCryptoUtility) BcVersionSpecificCryptoUtility.class.getClassLoader() + .loadClass("org.apache.pulsar.client.impl.crypto.bc.BCNonFipsSpecificUtility") + .getConstructor() + .newInstance(); + case SecurityUtility.BC_FIPS: + return (BcVersionSpecificCryptoUtility) BcVersionSpecificCryptoUtility.class.getClassLoader() + .loadClass("org.apache.pulsar.client.impl.bcfips.BCFipsSpecificUtility") + .getConstructor().newInstance(); + default: + throw new IllegalStateException( + "Provider not handled for BC Specific delegate creation: " + providerName); + } + } catch (InstantiationException | IllegalAccessException | InvocationTargetException | NoSuchMethodException Review Comment: I suggest using the same package name, so like: - org.apache.pulsar.client.impl.crypto.bc.BCNonFipsSpecificUtility - org.apache.pulsar.client.impl.crypto.bcfips.BCFipsSpecificUtility ########## bouncy-castle/bc-common/src/main/java/org/apache/pulsar/client/impl/crypto/BcVersionSpecificCryptoUtility.java: ########## @@ -0,0 +1,53 @@ +/* + * 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.pulsar.client.impl.crypto; + +import static org.apache.pulsar.client.impl.crypto.BcVersionSpecificCryptoUtilityFactory.createNewInstance; +import java.security.PrivateKey; +import java.security.PublicKey; +import java.util.Optional; +import javax.crypto.SecretKey; +import org.apache.pulsar.client.api.PulsarClientException; + +/** + * Facade to various crypto functions implemented by BouncyCastle FIPS and Non-fips module. + */ +public interface BcVersionSpecificCryptoUtility { + + BcVersionSpecificCryptoUtility INSTANCE = createNewInstance(); + + String RSA = "RSA"; + + // Ideally the transformation should also be part of the message property. This will prevent client + // from assuming hardcoded value. However, it will increase the size of the message even further. + String RSA_TRANS = "RSA/NONE/OAEPWithSHA1AndMGF1Padding"; + + + byte[] encryptDataKey(String logCtx, String keyName, PublicKey publicKey, SecretKey dataKey) + throws PulsarClientException.CryptoException; + + Optional<SecretKey> deCryptDataKey(String datakeyAlgorithm, String logCtx, String keyName, PrivateKey privateKey, + byte[] encryotedDataKey); Review Comment: ```suggestion byte[] encryptedDataKey); ``` ########## bouncy-castle/bc-common/src/test/java/org/apache/pulsar/client/impl/crypto/EncKeyReader.java: ########## @@ -0,0 +1,77 @@ +/* + * 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.pulsar.client.impl.crypto; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Paths; +import java.util.Map; +import java.util.Optional; +import org.apache.pulsar.client.api.CryptoKeyReader; +import org.apache.pulsar.client.api.EncryptionKeyInfo; +import org.testng.Assert; + +public class EncKeyReader implements CryptoKeyReader { + + public enum KeyType { + PUBLIC("public"), PRIVATE("private"); + private final String id; + + KeyType(String id) { + this.id = id; + } + } + + public static byte[] getKeyAsPEM(KeyType keyType, String keyName, Map<String, String> keyMeta) { + String CERT_FILE_PATH = "../src/test/resources/certificate/" + keyType.id + "-key." + keyName; + if (Files.isReadable(Paths.get(CERT_FILE_PATH))) { Review Comment: It seems that you can directly return `Files.readAllBytes(Paths.get(CERT_FILE_PATH))` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
