You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ranger.apache.org by Endre Zoltan Kovacs via Review Board <no...@reviews.apache.org> on 2018/01/19 16:19:26 UTC
Review Request 65242: service passwords and its encrypt configuration
improvement
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65242/
-----------------------------------------------------------
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
Re: Review Request 65242: service passwords and its encrypt
configuration improvement
Posted by Endre Zoltan Kovacs via Review Board <no...@reviews.apache.org>.
> 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
>
>
Re: Review Request 65242: service passwords and its encrypt
configuration improvement
Posted by Ramesh Mani <rm...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65242/#review195848
-----------------------------------------------------------
agents-common/src/main/resources/service-defs/ranger-servicedef-atlas.json
Lines 195 (patched)
<https://reviews.apache.org/r/65242/#comment275191>
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.
- Ramesh Mani
On Jan. 19, 2018, 4: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, 4: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
>
>