> On March 23, 2016, 8:27 p.m., Jarek Cecho wrote: > > common/src/main/java/org/apache/sqoop/model/MMasterKey.java, line 36 > > <https://reviews.apache.org/r/44984/diff/5/?file=1312405#file1312405line36> > > > > Do we want to be leaking the secret when serializing to String?
this is the same encrypted secret that is stored in the database (i'll rename some things so it is clearer). so i dont think that it is a problem. i can change it if you feel strongly about printing out the encrypted data. > On March 23, 2016, 8:27 p.m., Jarek Cecho wrote: > > core/src/main/java/org/apache/sqoop/repository/RepositoryEncryptionManager.java, > > line 46 > > <https://reviews.apache.org/r/44984/diff/5/?file=1312410#file1312410line46> > > > > I'm wondering - what is the value of introducing a new manager over > > merging that functionality to RepositoryManager itself? I was thinking about this. I think it may be better to rename RepositoryEncryptionManager to MasterKeyManager (or something like that). It seemed like a reasonable separation of concerns to keep it apart from RepositoryManager (due to all of its state). > On March 23, 2016, 8:27 p.m., Jarek Cecho wrote: > > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java, > > lines 439-443 > > <https://reviews.apache.org/r/44984/diff/5/?file=1312415#file1312415line439> > > > > I would probably recommend to use the same query to retrieve both > > encrypted/unecrypted variant - it will be less headache to maintain all > > those queries. this is not retrieving inputs, it is inserting inputs > On March 23, 2016, 8:27 p.m., Jarek Cecho wrote: > > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java, > > line 2277 > > <https://reviews.apache.org/r/44984/diff/5/?file=1312415#file1312415line2277> > > > > For data verification I would use normal exception rathern then assert. > > We're using assert more to verify code rather then data. forgot to make a change from the proof of concept phase. good catch - Abraham ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44984/#review125114 ----------------------------------------------------------- On March 23, 2016, 11:49 p.m., Abraham Fine wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44984/ > ----------------------------------------------------------- > > (Updated March 23, 2016, 11:49 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-2887 > https://issues.apache.org/jira/browse/SQOOP-2887 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > proof of concept regarding sqoop repository encryption, exceptions are not > handled properly and there is plenty of cleanup to do. but you should get the > basic idea > > > Diffs > ----- > > > common-test/src/main/java/org/apache/sqoop/common/test/db/DerbyProvider.java > 839e5618e3cd86a1df24b53b04aae3721b56ed27 > common/src/main/java/org/apache/sqoop/error/code/CommonRepositoryError.java > 37eb04aaa30f24577ff755e361e6d0a9d47be37c > common/src/main/java/org/apache/sqoop/model/MMasterKey.java PRE-CREATION > core/src/main/java/org/apache/sqoop/core/SqoopServer.java > 80a7b88ac4704972f1de0ddaf21869feedba4aa8 > core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java > 5b70f951c98dec7d20ff446f52418234fd98e454 > core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java > feab2ad8360b7d978b263dc0da1d2ff2578f0555 > core/src/main/java/org/apache/sqoop/repository/Repository.java > 03989a3c053edfc51364e9fb368ec970c1dfecee > > core/src/main/java/org/apache/sqoop/repository/RepositoryEncryptionManager.java > PRE-CREATION > core/src/main/java/org/apache/sqoop/security/SecurityConstants.java > 0241c8692fef65866160e0bec53cc5c2da1c17e5 > core/src/main/java/org/apache/sqoop/security/SecurityError.java > 988e425a041a5829ad95508a1e9b20d0992c868e > > core/src/test/java/org/apache/sqoop/repository/TestRepositoryEncryptionManager.java > PRE-CREATION > dev-support/upload-patch.py 5c17e587bd64bc8fa98ae90058d99da2bd4230a5 > dist/src/main/conf/sqoop.properties > 767d3f2b3ddf8ca978830e37a321d9defbafc57d > docs/src/site/sphinx/user/examples/S3Import.rst > d75b23e06da29f16ffe35d4e18043e10add8ce62 > > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java > 15cc41b83adc59c266cb899a19edfcc5fc2ee343 > > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryInsertUpdateDeleteSelectQuery.java > ae16b8517daf4d40fd2ebf4a473e16e9083740d6 > > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositorySchemaConstants.java > d1940e82c02039c144c58a80b3da07f6d2e38b27 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java > ee5e8d10f16365fa55574a819501b6ea48ffe30a > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaCreateQuery.java > 177003671863244a094abdb6f4a0184d38e79823 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaUpgradeQuery.java > 5081b82ae2cbbd06cef9061b333dd4ec0b61b815 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java > e4cca07fd062a64ac161929943dbd70b23a01c7f > > repository/repository-mysql/src/main/java/org/apache/sqoop/repository/mysql/MySqlRepositoryHandler.java > 2c74c323bf64f5c4b120aa745b1e7208e4ddfb3d > > repository/repository-mysql/src/main/java/org/apache/sqoop/repository/mysql/MySqlSchemaCreateQuery.java > 47f12fe457bc84fc36444e9aeb0b5fbc5a16356f > > repository/repository-mysql/src/main/java/org/apache/sqoop/repository/mysql/MysqlRepositoryInsertUpdateDeleteSelectQuery.java > 4c295c017195bd3b5ee79a6961814f3d8c8f9e17 > > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepositoryHandler.java > 400d706c76bbb3e45f535556e925d1debf557928 > > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlSchemaCreateQuery.java > 8358df0d90ef31fad02cf48df198f5c263a3915e > > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlSchemaUpgradeQuery.java > 52954e6946772d4cff268d88aa0bfbc4a9d11aa9 > test/src/main/java/org/apache/sqoop/test/minicluster/SqoopMiniCluster.java > c7a4db871c2e147f81455585aaeef71660c45922 > > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java > 44f4a5bcc3bfd45bcfd3c5a8fecb665b6a34b450 > > Diff: https://reviews.apache.org/r/44984/diff/ > > > Testing > ------- > > > Thanks, > > Abraham Fine > >
