> On March 10, 2013, 12:18 a.m., Jarek Cecho wrote: > > Hi Venkatesh, > > thank you very much for working on this and please accept my late review. > > The changes looks good me the except few nits highlighted below. > > > > I do have just one bigger concern regarding saved jobs. It seems to me that > > the code will load the password from file and store it inside the > > metastore, which might be seen as a security hole. What about explicitly > > not saving the password in metastore and instead loading it from the file > > each subsequent execution?
Jarek, Thanks for your time reviewing this. I'm not sure about your suspicion on storing clear text passwords in the metastore. It isn't. How are you arriving at that? I'm only reading password from a file which should be under user'e home directory with 400 permissions which implies no one except the user running sqoop will have access to. Makes sense? > On March 10, 2013, 12:18 a.m., Jarek Cecho wrote: > > src/docs/man/common-args.txt, line 42 > > <https://reviews.apache.org/r/9771/diff/1/?file=267018#file267018line42> > > > > Would you mind changing the argument to --password-file? It seems to be > > more consistent with the other parameters. Sure, makes sense. > On March 10, 2013, 12:18 a.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/tool/BaseSqoopTool.java, line 21 > > <https://reviews.apache.org/r/9771/diff/1/?file=267025#file267025line21> > > > > If you don't mind, I personally prefer explicit list of import classes > > than the wild card. This is mostly an IDEA thing, anything more than 4, it would put a * to avoid noise. Will do. > On March 10, 2013, 12:18 a.m., Jarek Cecho wrote: > > src/docs/user/connecting.txt, lines 63-68 > > <https://reviews.apache.org/r/9771/diff/1/?file=267020#file267020line63> > > > > I would recommend rephrasing that a bit. As the connectors are > > responsible for creating mapreduce job, they might override the default > > behavior. I would prefer to have this properly documented as it's about > > potential security hole. Any hint of what I might be missing would largely help. I'd take a shot at it but can greatly help if I know what should be improved. Thanks! > On March 10, 2013, 12:18 a.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/mapreduce/db/DBConfiguration.java, lines 71-72 > > <https://reviews.apache.org/r/9771/diff/1/?file=267024#file267024line71> > > > > Can we put also login and entire JDBC url into the credentials object? We could but KISS. Only passwords are secure credentials but not sure why the entire JDBC URI should be in there? Appreciate if you could elaborate. Thanks! - Seetharam ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9771/#review17652 ----------------------------------------------------------- On March 6, 2013, 7:24 a.m., Seetharam Venkatesh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9771/ > ----------------------------------------------------------- > > (Updated March 6, 2013, 7:24 a.m.) > > > Review request for Sqoop and Jarek Cecho. > > > Description > ------- > > An attempt to add a way for users to pass credentials to sqoop in a secure > way by storing the password in a file under user's home directory with 400 > permissions. > > > This addresses bug https://issues.apache.org/jira/browse/SQOOP-914. > > https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/SQOOP-914 > > > Diffs > ----- > > src/docs/man/common-args.txt cf9c0c3 > src/docs/user/common-args.txt 0554f81 > src/docs/user/connecting.txt 44a5111 > src/docs/user/help.txt 24fbddc > src/docs/user/tools.txt 96bf777 > src/java/org/apache/sqoop/SqoopOptions.java addc889 > src/java/org/apache/sqoop/mapreduce/db/DBConfiguration.java d270bc8 > src/java/org/apache/sqoop/tool/BaseSqoopTool.java 684d4a5 > src/test/org/apache/sqoop/credentials/TestPassingSecurePassword.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/9771/diff/ > > > Testing > ------- > > Adds unit tests. All unit tests pass and checkstyle issues fixed. > > > Thanks, > > Seetharam Venkatesh > >
