----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69985/#review212827 -----------------------------------------------------------
kms/src/main/java/org/apache/hadoop/crypto/key/DBToKeySecure.java Lines 82 (patched) <https://reviews.apache.org/r/69985/#comment298708> I would expect that if the import was failed, the process ends with a non-zero exit code. kms/src/main/java/org/apache/hadoop/crypto/key/RangerSafenetKeySecure.java Lines 50 (patched) <https://reviews.apache.org/r/69985/#comment298711> You can mark all variable as final kms/src/main/java/org/apache/hadoop/crypto/key/RangerSafenetKeySecure.java Lines 52 (patched) <https://reviews.apache.org/r/69985/#comment298712> This variable shouldn't be static. kms/src/main/java/org/apache/hadoop/crypto/key/RangerSafenetKeySecure.java Lines 59 (patched) <https://reviews.apache.org/r/69985/#comment298710> Unnecessary constructor kms/src/main/java/org/apache/hadoop/crypto/key/RangerSafenetKeySecure.java Lines 87 (patched) <https://reviews.apache.org/r/69985/#comment298709> Why don't you simply re-throw the exception(s)? Having a non-usable RangerSafenetKeySecure object for the caller doesn't make too much sense. So later, you don't need to check that myStore is not null kms/src/main/java/org/apache/hadoop/crypto/key/RangerSafenetKeySecure.java Lines 115 (patched) <https://reviews.apache.org/r/69985/#comment298713> Why the e.printStackTrace(), could you just add that 'e' to the logger.error call? kms/src/main/java/org/apache/hadoop/crypto/key/RangerSafenetKeySecure.java Lines 117 (patched) <https://reviews.apache.org/r/69985/#comment298714> It's not an issue with your code, but I think RangerKMSKI is a bit confusing, what's the reason for having a 'Throwable' in the method declaration, and returning a boolean=false. One of them is unnecessary. kms/src/main/java/org/apache/hadoop/crypto/key/RangerSafenetKeySecure.java Lines 135 (patched) <https://reviews.apache.org/r/69985/#comment298715> If 'key' is null, then it will throw an NPE from here, get catched in the 'catch (Exception e)' and returned null later. Maybe it's simpler to return null in the if: if (key == null) { logger.warn('getMasterKey(pw) returned null!'); return null; } kms/src/main/java/org/apache/hadoop/crypto/key/RangerSafenetKeySecure.java Lines 154 (patched) <https://reviews.apache.org/r/69985/#comment298716> I don't get, why it throws NoSuchAlgorithmException, CertificateException, and IOException, but catch KeyStoreException ? - Zsombor Gegesy On Feb. 14, 2019, 9:59 a.m., Gautam Borad wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69985/ > ----------------------------------------------------------- > > (Updated Feb. 14, 2019, 9:59 a.m.) > > > Review request for ranger, Ankita Sinha, Don Bosco Durai, Abhay Kulkarni, > Madhan Neethiraj, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, Sailaja > Polavarapu, and Velmurugan Periasamy. > > > Bugs: RANGER-2331 > https://issues.apache.org/jira/browse/RANGER-2331 > > > Repository: ranger > > > Description > ------- > > User story: As a security admin, I want to manage encryption keys for > securing my Hadoop cluster files in Ranger KMS service with Safenet KeySecure > crypto platform. > > > For Safenet KeySecure overview refer to: > https://safenet.gemalto.com/data-encryption/enterprise-key-management/key-secure/ > > > Acceptance Criteria: > > > 1) Ranger KMS has ability to configure Safenet KeySecure platform to be used > for key offload > > > 2) Ranger KMS provides ability to provide key management functions (create > keys, manage keys, retrieve keys, rollover) using Safenet KeySecure platform > > > 3) Ranger KMS UI panel on Ambari can be used to configure Safenet KeySecure > platform > > > Diffs > ----- > > kms/config/kms-webapp/dbks-site.xml 0e0f2ec > kms/scripts/DBMKTOKEYSECURE.sh PRE-CREATION > kms/scripts/KEYSECUREMKTOKMSDB.sh PRE-CREATION > kms/scripts/install.properties ddc779d > kms/scripts/setup.sh 2db05b8 > kms/src/main/java/org/apache/hadoop/crypto/key/DBToKeySecure.java > PRE-CREATION > kms/src/main/java/org/apache/hadoop/crypto/key/JKS2RangerUtil.java 22dce0f > > kms/src/main/java/org/apache/hadoop/crypto/key/KeySecureToRangerDBMKUtil.java > PRE-CREATION > kms/src/main/java/org/apache/hadoop/crypto/key/Ranger2JKSUtil.java 1abbf8e > kms/src/main/java/org/apache/hadoop/crypto/key/RangerKeyStoreProvider.java > 267fcf0 > kms/src/main/java/org/apache/hadoop/crypto/key/RangerMasterKey.java 5614c16 > kms/src/main/java/org/apache/hadoop/crypto/key/RangerSafenetKeySecure.java > PRE-CREATION > src/main/assembly/kms.xml fca6a32 > > > Diff: https://reviews.apache.org/r/69985/diff/1/ > > > Testing > ------- > > Verified below scenario: > > > 1) Fresh Installation Of Ranger KMS with Safenet Key Secure (NAE-XML Protocol) > 2) DB to Key Secure (NAE-XML) master key Migration utility > 3) Key Secure (NAE-XML) to DB master key Migration utility > > > Thanks, > > Gautam Borad > >
