> 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? > > Seetharam Venkatesh wrote: > 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?
Hi Venkatesh, you're welcome. I'm glad to help get this in! I believe that I do understand proposed functionality correctly. I'm trying to look for potential corner cases and one possible issue that I was able to think of is with saved jobs. Imagine following example (real one): $ sqoop job --create jarcec -- import --connect jdbc:mysql://mysql/clusterdb --username sqoop --passwordPath sqoop-password --table texts $ sqoop job --show jarcec | grep password db.password = secret-password It's important to mention that this will work only with sqoop.metastore.client.record.password set to true. Nevertheless, I believe that the passwords shouldn't be serialized into the metastore in case that --password-file is enabled in any case. What do you think? > 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. > > Seetharam Venkatesh wrote: > 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! What about decoupling the two new features that are introduced by this patch - One is to load password from file instead of command line and second is to not store the password in job configuration object - and document them separately? This way we can properly document what will happen in each feature and how (if even) it can be bypassed. For example loading from file can't be bypassed, but storing password in job configuration object can be bypassed by insecure connector. > 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? > > Seetharam Venkatesh wrote: > 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! I personally see this functionality more as securing Sqoop rather than just scratching the password. Thinking from the attacker's point of view having valid username and valid database URL is something that will significantly help me to compromise remote system. For example, knowing login will allow me to attack one specific account on the remote database box, whereas otherwise it's a tuple username-password that I need to guess. Similarly for the URL - it's exposing where the database is physically running which is information that can be abused. > 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. > > Seetharam Venkatesh wrote: > This is mostly an IDEA thing, anything more than 4, it would put a * to > avoid noise. Will do. I was actually fighting with this in IDA for quite some time. It can be configured in "Settings" -> "Code Style" -> "Java", tab "Imports", input "Class count to use import with '*'". I'm personally setting this to something quite huge like 666, but your configuration is up to you :-) - Jarek ----------------------------------------------------------- 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 > >
