implementation + test

Could this use java.security.KeyStore?

Are these stores expected to pre-encrypted or do they do the encryption themselves? I sort of echo what Tom says: are we building something custom that we shouldn't be?

The intent is to have a key-value store for security credentials. It is the equivalent of a secure job configuration. It needs to be passable over RPC and therefore should be implemented as a writable.

I don't see a good way (or justification) to use the KeyStore.

They will only be stored in the JobTracker's system directory and so they don't need to be encrypted themselves.

I think this actually belongs over in map/reduce rather than in common.

The use case is: when the user submits a job, they need to include credentials. Currently this would go into the job conf, but that is visible from the web. We are better off factoring this out into a separate map.

Note that the credentials will be disjoint for different HDFS clusters (or other filesystems). So it will look like:

"hdfs://nn1/" -> binary blob1
"hdfs://nn2/" -> binary blob2
"mapred.job.token" -> binary blob3

This key storage should be included in the call to submitJob.

Closing it. We used JobTokens class to store shuffle key.

I think we should leave this issue open. We will need a solution to support the use case of arbitrary user supplied security credentials...

Note that the credentials will be disjoint for different HDFS clusters (or other filesystems). So it will look like:

"hdfs://nn1/" -> binary blob1
"hdfs://nn2/" -> binary blob2
"mapred.job.token" -> binary blob3

This key storage should be included in the call to submitJob.

If the keys are given on a command line - how can we pass it to the job. All the command lines arguments are passed thru config, and we want to avoid id.

Moved this issue from HADOOP-6325 to this one. It is mapreduce related.

preliminary patch,
test not complete,
debug info

Added TokenCache class.
It keeps static mapping between jobId and TokenStorage for this job.

Some comments after a quick look:
1) The json parser should be used from the jackson one
2) The loading of tokens in the child should happen more or less in the same way as it happens in the trunk (reading the filename via an env var, etc.)
3) The user API can be simplified. I would prefer something like TokenCache.getSecretKey(alias) rather than TokenCache.getTokenStorage(jobId).getSecretKey(alias)..

addressed comments by Devaraj.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12430080/MAPREDUCE-1338-4.patch
against trunk revision 898486.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 7 new or modified tests.

-1 patch. The patch command could not apply the patch.

Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/380/console

This message is automatically generated.

Looks good overall. Some comments:
1) Rename the shuffletoken to jobtoken (TokenStorage.setShuffle, etc.)
2) Rename loadJobToken to loadTokens
3) The javadoc at the beginning of the TokenCache file is misleading
3) Ideally the alias for the secrets in TokenStorage should be a Text object instead of String
4) Is there any reason why TestTokenCache is in mapred package whereas TestTokenStorage is in the security package?
5) I think there should be an API to get all the tokens (TokenCache.getAllTokens()). That will be helpful for HADOOP-6299.
6) Also, i think there should be an API to add tokens (TokenCache.addToken(Token)). That will be helpful for MAPREDUCE-1383.

One other thing that I realized while reviewing this patch was that the persistence of the token cache should happen in the submitJob method in the JobTracker, rather than in the Job's initialization, in order to better support job recovery. For example, we could have a case where the user submits a job, and then before the job initialization, the JobTracker gets restarted for some reason. If job recovery is enabled, the job submitted earlier will be considered for initialization at some point of time after the restart. But the job's tokencache wouldn't be there then.. But I am okay to handle this issue in a follow up jira.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12430473/MAPREDUCE-1338-7.patch
against trunk revision 899844.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 7 new or modified tests.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 findbugs. The patch does not introduce any new Findbugs warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed core unit tests.

-1 contrib tests. The patch failed contrib unit tests.

Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/392/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/392/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/392/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/392/console

This message is automatically generated.

addressed comments by Devaraj.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12430684/MAPREDUCE-1338-8.patch
against trunk revision 900159.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 13 new or modified tests.

-1 javadoc. The javadoc tool appears to have generated 1 warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 findbugs. The patch does not introduce any new Findbugs warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed core unit tests.

-1 contrib tests. The patch failed contrib unit tests.

Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/394/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/394/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/394/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/394/console

This message is automatically generated.

fixed contrib test and warning

+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12430786/MAPREDUCE-1338-9.patch
against trunk revision 900815.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 13 new or modified tests.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 findbugs. The patch does not introduce any new Findbugs warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed core unit tests.

+1 contrib tests. The patch passed contrib unit tests.

Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/277/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/277/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/277/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/277/console

This message is automatically generated.

Apologies for not being thorough enough earlier. But I think the API needs a little bit more of tweaking.

I think in TokenCache, all APIs should be static (the addDelegationToken is not).

The TokenStorage class should be annotated with InterfaceAudience Private. The TokenCache should be annotated with InterfaceStability Evolving.

As far as possible, TokenCache should be the one that should be used by the framework and the applications. TokenStorage should be a helper class that is used internally by the TokenCache. In that sense, some of the APIs like TokenCache.getTokenStorage could be package private (so that you can still use it in your new tests to inspect the fields). TokenCache.setStorage should be annotated with InterfaceAudience Private (since you use it in LocalJobRunner, and hence the method cannot be made package private).

Can we move the loadJobToken calls to TokenStorage. The TokenCache internally checks for whether the tokenStorage field is null or not on every API invocation. If the field is null, it invokes the static method TokenStorage.loadJobToken, and gets an initialized TokenStorage object.

The setJobToken/getJobToken could be moved to TokenCache but annotated with InterfaceAudience Private.

Doing the above would make the TokenCache as the single point of entry to the token storage and make the APIs more clear IMO.

Does these make sense? Again, apologies for not commenting on these earlier.

addressed some issues brought up by Devaraj

+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12430928/MAPREDUCE-1338-10.patch
against trunk revision 901350.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 13 new or modified tests.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 findbugs. The patch does not introduce any new Findbugs warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed core unit tests.

+1 contrib tests. The patch passed contrib unit tests.

Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/279/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/279/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/279/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/279/console

This message is automatically generated.

I guess I am okay with the current patch. I would have been happier if the TokenStorage was not exposed at all, but since the user facing API in TokenCache seems right, I am okay to have a follow up jira to factor out the TokenStorage class as per my earlier comment ("As far as possible...").
I intend to commit this patch soon.

Devaraj, please open a Jira on this issue, so we don't loos track of it.

attaching patch for backport

I just committed this. Thanks, Boris!

Integrated in Hadoop-Mapreduce-trunk #213 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/213/)
. Introduces the notion of token cache using which tokens and secrets can be sent by the Job client to the JobTracker. Contributed by Boris Shkolnik.

Patch for earlier version of Hadoop. Not for commit here.

removed comments

Integrated in Hadoop-Mapreduce-trunk-Commit #225 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/225/)