> On Jan. 19, 2018, 8:28 p.m., Ramesh Mani wrote: > > agents-common/src/main/resources/service-defs/ranger-servicedef-atlas.json > > Lines 195 (patched) > > <https://reviews.apache.org/r/65242/diff/1/?file=1942691#file1942691line195> > > > > Why the ServiceDef is changed to have these new configs? > > We can add those as part of Key/Value in "Add New Configurations". In > > that way you can just populate it when needed. > > If we change the ServiceDef its going to be on the UI > > Do you meant to have this on the UI by default? > > > > > > Also I see a comment that you are using the password field for the Key > > / Value and split it? Why do you do so as this key / Value will be part of > > the service def config.
@rmani the serivceDef change was intentional. Here is my reasoning for it: I made it so i can enforce the separate storing of the: 'encrpyted/decrypted password' and all its meta data: it's crypto algorithm, along with its configurational paramters (salt, iterationcount, secret_key, InitialVector). Assigning them each, to their own ServiceDef key, value. No more coma separation, no more fragile coma spliting, scattered parser code. Previously such was the content of the password field: PBEWithHMACSHA512ANDAES_128,tzL1AKl5uc4NKYaoQ4P3WLGIBFPXWPWdu1fRm9004jtQiV,f77aLYLo,1000,9f3vNL0ijeHF4RWN/yUo0A==,Bnq1uhBssC8mBpfUhjSYww== or in its decrypted form: PBEWithHMACSHA512ANDAES_128,tzL1AKl5uc4NKYaoQ4P3WLGIBFPXWPWdu1fRm9004jtQiV,f77aLYLo,1000,9f3vNL0ijeHF4RWN/yUo0A==,asd,123 by storing these values into a single string joint by comma separation, we put unneccesary fragility on the code which tries to parse this password field, and guess if there are enough commas in the string, then possibly the first couple of tokens are algo and its parameters, and the remaining tokens are the password which is to be encrypted/decrypted. This fragility was made even worse when the password: started with / contained / ended / or only consist of: comas. Not to mention, that the fragility of the parse code was exponentiated as the parse code was scattered with copy-paste to the caller sites. Lets think this way: what do we gain by / what forces us to storing it the old way? There is no PRO imho, but only cons: much risk of encr/decr failure (i think this was the reason in the first place this ticket was opened) when an unanticipated password was used by the user, besides the fragile parser code. Also what forces us to encode multiple things into a single string joint by commas? When we can have 3-5 additional XXServiceConfigMap at which, for literally no cost we can store key-values without this CSV-ing. So after all, it becomes: In encrypted form: password:'Bnq1uhBssC8mBpfUhjSYww==' crypto.algorithm:'PBEWithHMACSHA512ANDAES_128' crypto.encryptionKey: 'tzL1AKl5uc4NKYaoQ4P3WLGIBFPXWPWdu1fRm9004jtQiV' crypto.salt:'f77aLYLo' crypto.iv:'9f3vNL0ijeHF4RWN/yUo0A==' crypto.iteration_count:'1000' In decrypted form: password:'asd,123' crypto.algorithm:'PBEWithHMACSHA512ANDAES_128' crypto.encryptionKey: 'tzL1AKl5uc4NKYaoQ4P3WLGIBFPXWPWdu1fRm9004jtQiV' crypto.salt:'f77aLYLo' crypto.iv:'9f3vNL0ijeHF4RWN/yUo0A==' crypto.iteration_count:'1000' After the separation, i could get rid of all fragile parser code, and simply just look up the specific value by the specific key. One more thing i think could think of to further improve my solution, is to provide a button on the edit/create service page, to generate strong: secretKey, salt, IV's (and base64 encode them as well). Endre - Endre Zoltan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65242/#review195848 ----------------------------------------------------------- On Jan. 19, 2018, 5:19 p.m., Endre Zoltan Kovacs wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65242/ > ----------------------------------------------------------- > > (Updated Jan. 19, 2018, 5:19 p.m.) > > > Review request for ranger. > > > Bugs: RANGER-1643 > https://issues.apache.org/jira/browse/RANGER-1643 > > > Repository: ranger > > > Description > ------- > > A user provided password and its crpytographic parameters (e.g.: encryption > algorithm, iterationcount, salt, encryptionkey, iv) > should not encode into a single string with coma separation, but should > rather each have their own service configuration key to store their value. my > patch removed each scattered place, where this coma separated decoding of > algo params happened, and added each their own key/value at the > service#getConfigs > > > Diffs > ----- > > agents-common/src/main/java/org/apache/ranger/plugin/client/BaseClient.java > cb170c2c1 > > agents-common/src/main/java/org/apache/ranger/plugin/util/PasswordUtils.java > 6ba42d475 > agents-common/src/main/resources/service-defs/ranger-servicedef-atlas.json > 4a550c640 > agents-common/src/main/resources/service-defs/ranger-servicedef-hbase.json > 71fae66d4 > agents-common/src/main/resources/service-defs/ranger-servicedef-hdfs.json > 2e5d07c2f > agents-common/src/main/resources/service-defs/ranger-servicedef-hive.json > b0f877a01 > agents-common/src/main/resources/service-defs/ranger-servicedef-kafka.json > 839d7806d > agents-common/src/main/resources/service-defs/ranger-servicedef-kms.json > f96cb9cd1 > agents-common/src/main/resources/service-defs/ranger-servicedef-knox.json > 495a69913 > agents-common/src/main/resources/service-defs/ranger-servicedef-solr.json > 2f12721e1 > agents-common/src/main/resources/service-defs/ranger-servicedef-storm.json > 03c1574ff > agents-common/src/main/resources/service-defs/ranger-servicedef-yarn.json > a32c08d93 > > agents-common/src/test/java/org/apache/ranger/plugin/util/PasswordUtilsTest.java > 4e135aaa7 > > hive-agent/src/main/java/org/apache/ranger/services/hive/client/HiveClient.java > 265c01575 > > knox-agent/src/main/java/org/apache/ranger/services/knox/client/KnoxClient.java > 0c83ef9bb > > knox-agent/src/main/java/org/apache/ranger/services/knox/client/KnoxConnectionMgr.java > 55bc2381d > > knox-agent/src/main/java/org/apache/ranger/services/knox/client/KnoxResourceMgr.java > e887b1157 > > plugin-atlas/src/main/java/org/apache/ranger/services/atlas/client/AtlasClient.java > ea05ad0fe > > plugin-kms/src/main/java/org/apache/ranger/services/kms/client/KMSClient.java > af0ac71f0 > > plugin-kms/src/main/java/org/apache/ranger/services/kms/client/KMSConnectionMgr.java > b81d7b857 > > plugin-kms/src/main/java/org/apache/ranger/services/kms/client/KMSResourceMgr.java > fe54723df > > plugin-solr/src/main/java/org/apache/ranger/services/solr/client/ServiceSolrClient.java > 5875a298b > security-admin/src/main/java/org/apache/ranger/biz/KmsKeyMgr.java 03bcb609e > security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java > 9d8f5d2a9 > > security-admin/src/main/java/org/apache/ranger/patch/PatchPasswordParameterStoring_J100013.java > PRE-CREATION > > security-admin/src/main/java/org/apache/ranger/service/RangerServiceService.java > df3fdb57c > security-admin/src/main/java/org/apache/ranger/service/XAssetService.java > 132879a63 > security-admin/src/main/resources/conf.dist/ranger-admin-default-site.xml > 9dfc03df1 > security-admin/src/test/java/org/apache/ranger/biz/TestServiceDBStore.java > f7eb0d430 > > storm-agent/src/main/java/org/apache/ranger/services/storm/client/StormClient.java > 363a6561c > > storm-agent/src/main/java/org/apache/ranger/services/storm/client/StormConnectionMgr.java > ab1b409a4 > > storm-agent/src/main/java/org/apache/ranger/services/storm/client/StormResourceMgr.java > 0dd550793 > > > Diff: https://reviews.apache.org/r/65242/diff/1/ > > > Testing > ------- > > by hand: > 1. on ranger admin ui: i edited hive service and added the config keys and > values that was comaseparated into the password previously. > 2. removed the same configs and comas from the password (in ranger database) > 3. hit test connection on the ranger admin ui, edit hive service > 4. connection was successful > > > What i chould not test: > the patch class i included > org.apache.ranger.patch.PatchPasswordParameterStoring_J100013.java > > I did not find any documentation on how this patch classes are set up to run, > please point me towards how i can test the automatic updating of the password > fields in the database! > > > Thanks, > > Endre Zoltan Kovacs > >
