-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9771/
-----------------------------------------------------------

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

-----------------------------------------------------------
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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9771/#review17652
-----------------------------------------------------------
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?
src/docs/man/common-args.txt
<https://reviews.apache.org/r/9771/#comment37457>

    Would you mind changing the argument to --password-file? It seems to be more consistent with the other parameters.

src/docs/user/connecting.txt
<https://reviews.apache.org/r/9771/#comment37458>

    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.

src/java/org/apache/sqoop/mapreduce/db/DBConfiguration.java
<https://reviews.apache.org/r/9771/#comment37459>

    Can we put also login and entire JDBC url into the credentials object?

src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/9771/#comment37460>

    If you don't mind, I personally prefer explicit list of import classes than the wild card.
- Jarek Cecho
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
>
>

> 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

> 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>

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:

> 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?
>
> Jarek Cecho wrote:
>     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?

Metastore stores sqoop tool template? If so, then this would be insecure. Do folks use this? Two options come to mind:

* If metastore is widely used, disable storing passwords in metastore if file is specified
* If not, print a warning when metastore is used

Lemme know what 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!
>
> Jarek Cecho wrote:
>     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.

Aren't they already decoupled with the new tool parameter? They are also sufficiently validated so they are mutually exclusive, no?
> 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!
>
> Jarek Cecho wrote:
>     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.

Well, it depends. I'm no expert but IMHO JDBC URI's are always public, there is nothing secretive about it. Its also largely convention over configuration. Agree? May be once we get this base functionality in, we can have another jira to actually secure the entire URI along with username and password.

Makes sense?

I know where it is but its noise for me personally.  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:

> 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?
>
> Jarek Cecho wrote:
>     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?
>
> Seetharam Venkatesh wrote:
>     Metastore stores sqoop tool template? If so, then this would be insecure. Do folks use this? Two options come to mind:
>    
>     * If metastore is widely used, disable storing passwords in metastore if file is specified
>     * If not, print a warning when metastore is used
>    
>     Lemme know what you think?
>

I've seen people using the Sqoop 1 metastore, even though it's probably not very common. Nevertheless, I would prefer to secure that as it's potential security hole.

I think that solution to this is quite simple. Entire metastore serialization is happening in SqoopConfiguration.writeProperties() and the password is already handled in special if-else statements on rows 632-644. I think that we just need to add new branch to that part of the code. And similarly to the SqoopConfiguration.loadProperties that will deserialize the properties from the metastore.
> 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!
>
> Jarek Cecho wrote:
>     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.
>
> Seetharam Venkatesh wrote:
>     Well, it depends. I'm no expert but IMHO JDBC URI's are always public, there is nothing secretive about it. Its also largely convention over configuration. Agree? May be once we get this base functionality in, we can have another jira to actually secure the entire URI along with username and password.

I see what you're saying. Nor URL nor login are super secrets, but my point is that they are exposing information that might help possible attacker. I'm fine with creating follow up jira for this.

Och, please accept my apologies for the confusion. The code functionality is definitely decoupled. I was talking here only from the documentation perspective. E.g. to describe both features independently on each other.
- 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:

> 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?
>
> Jarek Cecho wrote:
>     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?
>
> Seetharam Venkatesh wrote:
>     Metastore stores sqoop tool template? If so, then this would be insecure. Do folks use this? Two options come to mind:
>    
>     * If metastore is widely used, disable storing passwords in metastore if file is specified
>     * If not, print a warning when metastore is used
>    
>     Lemme know what you think?
>
>
> Jarek Cecho wrote:
>     I've seen people using the Sqoop 1 metastore, even though it's probably not very common. Nevertheless, I would prefer to secure that as it's potential security hole.
>    
>     I think that solution to this is quite simple. Entire metastore serialization is happening in SqoopConfiguration.writeProperties() and the password is already handled in special if-else statements on rows 632-644. I think that we just need to add new branch to that part of the code. And similarly to the SqoopConfiguration.loadProperties that will deserialize the properties from the metastore.

Fixed. Thanks for your pointers in the code and your idea simplifies it quite a bit.
> 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!
>
> Jarek Cecho wrote:
>     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.
>
> Seetharam Venkatesh wrote:

I have taken a stab at reorganizing this part of the guide. Let me know if that makes sense.

Filed a followup Jira: SQOOP-948
- 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 13, 2013, 9:41 p.m.)
Review request for Sqoop and Jarek Cecho.
Changes
-------

Thanks Jarek for your time in reviewing the patch. Very good catch on the metastore storing passwords and I had inadvertently missed this feature. This revision incorporates all the review comments so far from Jarek. Thanks again!
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 (updated)
-----

  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/java/org/apache/sqoop/util/CredentialsUtil.java PRE-CREATION
  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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9771/#review17850
-----------------------------------------------------------
Hi Venkatesh,
thank you for updating the patch. I have just two small nits. I'll take a few days to see if I'll be able to think of a way to crack it. I'll go ahead and commit it in case that I won't find any issues.

src/docs/user/connecting.txt
<https://reviews.apache.org/r/9771/#comment37809>

    Technically speaking the parameter -P is the most secure way of entering password, right? It's not stored anywhere and thus it can't leak in any way.

src/java/org/apache/sqoop/SqoopOptions.java
<https://reviews.apache.org/r/9771/#comment37810>

    Can we put "return" after this statement?
Jarcec

- Jarek Cecho
On March 13, 2013, 9:41 p.m., Seetharam Venkatesh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9771/
> -----------------------------------------------------------
>
> (Updated March 13, 2013, 9:41 p.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/java/org/apache/sqoop/util/CredentialsUtil.java PRE-CREATION
>   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
>
>

> On March 14, 2013, 4:30 a.m., Jarek Cecho wrote:
> > Hi Venkatesh,
> > thank you for updating the patch. I have just two small nits. I'll take a few days to see if I'll be able to think of a way to crack it. I'll go ahead and commit it in case that I won't find any issues.
> >

Thanks for your time Jarek. Will update the patch after incorporating your comments.
> On March 14, 2013, 4:30 a.m., Jarek Cecho wrote:
> > src/docs/user/connecting.txt, lines 63-70
> > <https://reviews.apache.org/r/9771/diff/2/?file=270255#file270255line63>
> >
> >     Technically speaking the parameter -P is the most secure way of entering password, right? It's not stored anywhere and thus it can't leak in any way.

Well, a file under user's home dir with 400 permissions is also secure but I see your point. Anyway you enter password, its stored in the credential store and cant be seen in job conf.
> On March 14, 2013, 4:30 a.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/SqoopOptions.java, line 596
> > <https://reviews.apache.org/r/9771/diff/2/?file=270258#file270258line596>
> >
> >     Can we put "return" after this statement?

Good catch.
- Seetharam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9771/#review17850
-----------------------------------------------------------
On March 13, 2013, 9:41 p.m., Seetharam Venkatesh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9771/
> -----------------------------------------------------------
>
> (Updated March 13, 2013, 9:41 p.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/java/org/apache/sqoop/util/CredentialsUtil.java PRE-CREATION
>   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
>
>

> On March 14, 2013, 4:30 a.m., Jarek Cecho wrote:
> > Hi Venkatesh,
> > thank you for updating the patch. I have just two small nits. I'll take a few days to see if I'll be able to think of a way to crack it. I'll go ahead and commit it in case that I won't find any issues.
> >
>
> Seetharam Venkatesh wrote:
>     Thanks for your time Jarek. Will update the patch after incorporating your comments.

I did not found any other look hole, so please update your patch and I'll commit it!
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9771/#review17850
-----------------------------------------------------------
On March 13, 2013, 9:41 p.m., Seetharam Venkatesh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9771/
> -----------------------------------------------------------
>
> (Updated March 13, 2013, 9:41 p.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/java/org/apache/sqoop/util/CredentialsUtil.java PRE-CREATION
>   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
>
>

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9771/
-----------------------------------------------------------

(Updated March 19, 2013, 8:30 a.m.)
Review request for Sqoop and Jarek Cecho.
Changes
-------

Incorporating review comments from Jarek. Thanks again for your valuable time. I did reword the help and also took care of the return statement.
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 (updated)
-----

  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/java/org/apache/sqoop/util/CredentialsUtil.java PRE-CREATION
  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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9771/#review18169
-----------------------------------------------------------

Ship it!
Ship It!

- Jarek Cecho
On March 19, 2013, 8:30 a.m., Seetharam Venkatesh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9771/
> -----------------------------------------------------------
>
> (Updated March 19, 2013, 8:30 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/java/org/apache/sqoop/util/CredentialsUtil.java PRE-CREATION
>   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
>
>