Added 2 member variable (optional in protobuf format) in FsServerDefaults:
1. namenodeSupportsProviderUri: to differentiate between empty keyProviderUri and namenode not supporting encrpytion zones and supporting backward compatibility.
2. keyProviderUri:

New namenode returns namenodeSupportsProviderUri as true.
If namenodeSupportsProviderUri is true, then the client will always trust whatever namenode returned in keyProviderUri.
If namenodeSupportsProviderUri is false (that means the namenode is not upgraded), then the client will use it own conf as before thereby supporting backwards compatibility.

  public boolean isHDFSEncryptionEnabled() {
    try {
      return DFSUtilClient.isHDFSEncryptionEnabled(getKeyProviderUri());
    } catch(IOException ioe) {
      return false;
    }
  }

  private String getKeyProviderUri() throws IOException {
    FsServerDefaults serverDefaults = getServerDefaults();
    return serverDefaults.getNamenodeSupportsProviderUri() ?
        serverDefaults.getKeyProviderUri() : conf.getTrimmed(
        CommonConfigurationKeysPublic.HADOOP_SECURITY_KEY_PROVIDER_PATH, "");
  }

DFSClient#getServerDefaults throws IOExcpetion (StandbyException)
If DFSUtilClient#isHDFSEncryptionEnabled throws IOException then I am returning false.
Not sure what should I return.
Comments are welcome.

Please review.

Thanks for posting the patch Rushabh! I like this idea, it'll simplify client configuration.

I took a quick look, few questions:

• Could you update the documentation and core-default.xml about this new feature?
• Do we actually need the PB boolean? PB can differentiate between not specified, empty string, and set. IIUC it's not specified, then it's an old NN, and we can fallback.

Andrew Wang: Thanks for quick review.

Could you update the documentation and core-default.xml about this new feature?

Will update in next revision.

Do we actually need the PB boolean? PB can differentiate between not specified, empty string, and set

The default value for string format in protobuf is "" (empty string).
If the client receives an empty string, it either means its talking to an old namenode or the namenode has provider path config not set.
Maybe if the namenode doesn't have security path set, then I can return null value and on the client side, I have to differentiate between null and empty string. But need to test if protobuf convert null string to empty string in response or not ?

PB can differentiate between not specified, empty string, and set.

Please ignore my previous comment. I understood what you actually meant.
I will update the patch by monday.

Canceling the patch to address review comments.
Will post a new patch shortly.

-1 overall

This message was automatically generated.

Attached a new patch which removed PB boolean.
Updated documentation and core-default.xml

Test Failures from the previous patch:
hadoop.security.TestKDiag – Ran fine on my node.
hadoop.hdfs.TestDFSUtil – Was actually a bug in my patch. Fixed in the latest revision.
Please review.

Thanks for working on this Rushabh, patch looks pretty close, just some nits:

• FtpConfigKeys and LocalConfigKeys, would still be nice to introduce a new variable for documentation purposes
• I dislike non-trivial ternary statements, mind rewriting DFSClient#getKeyProviderUri a bit? We can also dedupe the call to getKeyProviderUri for clarity.
• Still could use a doc update in TransparentEncryption.md about how this is fetched from server-side defaults.

Could you comment on potential additional overheads from invoking the NNto query this config value? I don't see getServerDefaults used much right now, so it looks like this will add another NN RPC to many client operations, for both unencrypted and encrypted clusters. This is concerning.

-1 overall

This message was automatically generated.

FtpConfigKeys and LocalConfigKeys, would still be nice to introduce a new variable for documentation purposes

Will update in the next revision.

I dislike non-trivial ternary statements, mind rewriting DFSClient#getKeyProviderUri a bit?

Will update in next revision.

We can also dedupe the call to getKeyProviderUri for clarity.

Can you please elaborate more ?

Still could use a doc update in TransparentEncryption.md about how this is fetched from server-side defaults.

Will update in next revision.

Could you comment on potential additional overheads from invoking the NNto query this config value? I don't see getServerDefaults used much right now, so it looks like this will add another NN RPC to many client operations, for both unencrypted and encrypted clusters. This is concerning.

The results of namenode#getServerDefaults on DFSClient are cached for 1 hour.
On the namenode side also, this rpc call doesn't need any lock. So I don't think this should add any significant load on Namenode.
For DFSInputStream, we already do call severDefaults to get DFSClient#newDataEncryptionKey

Hi Rushabh S Shah thanks much for your patch. Looks good overall but I still have a few questions.

According to current implementation of kms provider in client conf, there can only be one kms.

This is not a precise statement though, considering that multiple kms instances can be added for load balancing purposes. Would you mind to update the release note once this patch gets committed?

Preferably also state (in the doc, maybe?) that this only works if both client and namenodes are on a supported version, otherwise the client's local kms config is used.

Can we also add a test to ensure clients can access files in an encrypted remote cluster using the token obtained from the remote NameNode?

HI Rushabh S Shah,

Thanks much for working on this issue!

I had a few nits and question about the patch here:

1. Since this is public API, we should introduce a new constructor with the addiitional parameter keyProviderUri to be backward compatible, instead of just modifying the existing one.

@InterfaceAudience.Public
@InterfaceStability.Evolving
public class FsServerDefaults implements Writable {
......
  public FsServerDefaults(long blockSize, int bytesPerChecksum,
      int writePacketSize, short replication, int fileBufferSize,
      boolean encryptDataTransfer, long trashInterval,
      DataChecksum.Type checksumType,
      String keyProviderUri) {

2. Suggest to add a KEY_PROVIDER_URI_DEFAULT to replce the "" here (in both FtpConfigKeys.java and LocalConfigKeys.java):

  protected static FsServerDefaults getServerDefaults() throws IOException {
    return new FsServerDefaults(
        BLOCK_SIZE_DEFAULT,
        ......
        CHECKSUM_TYPE_DEFAULT,
        "");

3. the following method swallows IOException and return false. Suggest to remove the @throws IOException and add a comment in the catch block about why it can be iegnored and false should be returned here. And a
dd a space after the word catrch.

  /**
   * Probe for encryption enabled on this filesystem.
   * See {@link DFSUtilClient#isHDFSEncryptionEnabled(Configuration)}
   * @return true if encryption is enabled
   * @throws IOException
   */
  public boolean isHDFSEncryptionEnabled() {
    try {
      return DFSUtilClient.isHDFSEncryptionEnabled(getKeyProviderUri());
    } catch(IOException ioe) {
      return false;
    }
  }

4. Line 84 of KeyProviderCache.java (not introduced by your changei)

LOG.error("Could not create KeyProvider for DFSClient !!", e.getCause());

suggest to replace e.getCause() with e, so we can see the full stack.

5. Currently getServerDefaults() contact NN every hour, to find if there is any update of keyprovider. If keyprovider changed within the hour,
client code may get into exception, wonder if we have mechanism to handle the exception and update the keyprovider and try again?

Thanks.

Looked closer, I think #5 of my prior comment is indeed a potential problem because I don't see the kind of handling I was looking for in the current implementation.

Thanks for reviewing Wei-chiu and Yongjun, let me reply to a few comments of Rushabh:

This is not a precise statement though, considering that multiple kms instances can be added for load balancing purposes. Would you mind to update the release note once this patch gets committed?

By this, I think you mean we should update the JIRA summary and related to say "for the KeyProvider URI"?

Can we also add a test to ensure clients can access files in an encrypted remote cluster using the token obtained from the remote NameNode?

What constitutes "remote" here? Clients fetch delegation tokens based on the specified filesystem(s), so I don't think it matters where the client or cluster are located.

5. Currently getServerDefaults() contact NN every hour, to find if there is any update of keyprovider. If keyprovider changed within the hour, client code may get into exception, wonder if we have mechanism to handle the exception and update the keyprovider and try again?

What kind of error handling do you recommend here? I do think at least some a log message would be nice.

Some new questions of my own:

• I like that getServerDefaults is lock-free, but I'm still worried about the overhead. MR tasks are short lived and thus don't benefit from the caching. This also affects all clients, on both encrypted and unencrypted clusters. I think getServerDefault is also currently only called when SASL is enabled. Have you done any performance testing of this RPC?
• Another option is to put this behind a config key which defaults off, and file a new JIRA to track defaulting it on in a later release.

Thanks Andrew Wang.

I gave some more thoughts, I think a better solution instead of the period polling is, just like how client handle namenode HA, we can do the same for KeyProvider. Say, if we specify keyproviderservice in config file to associate with a list of KeyProviders, if one keyProvider is down, the client can try to access the next one in the list (client failover). This is essentially KeyProvider HA. But this would be a larger scope solution. Does this make sense to you?

Hi Yongjun, what you describe is how the existing KMS HA already works.

One thing I might recommend is that we don't query getServerDefaults after we get the KP initially. This way we don't need to worry about the value changing later (or an exception being thrown).

... how client handle namenode HA ... we specify keyproviderservice in config file ...

The need for configs is the problem, so more configs is not the answer (aside: the NN HA token handling is an example of exactly what not to do).

One thing I might recommend is that we don't query getServerDefaults after we get the KP initially.

Enabling EZ on a cluster must not require a restart of all daemon and proxy services that communicate with said cluster. It can't be cached forever.

––

I reviewed Rushabh's approach with him this morning. The main goal should be a config-free token acquisition and selection. How do we get there?

The first challenge is how does a client intelligently request a kms token, when needed, and from the right kms? The NN is the authoritative and dynamic source for the correct kms, ala this patch.. Token acquisition should use the kp uri provided by the NN, and I'm not too worried about caching when a typical cluster has a few dozen app submits/sec (equaling token requests) vs 10s of thousand of NN ops/sec. This is only a small part of the problem.

The second challenge is how does a client select the correct kms for a given NN? The client could again ask the NN but you stumble into the morass of caching. However as soon as the NN reports a different key provider than when a job launched, clients won't be able to find a token for the new kms - even when the old one is still legit. Now jobs fail that should/could have completed. It's very messy. The simpler answer is a client should always use the key provider for a given NN as it existed when the token was acquired (ie. job submit).

So how do we implement a config-free mapping of NN to key provider? When the hdfs and kms tokens are acquired we need a way to later associate them as a pair. I think the cleanest/most-compatible way is leveraging the Credentials instead of the config. We could inject a mapping of filesystem uri to kms uri via the secrets map. So now when the client needs to talk to the kms it can check the map, else fallback to getServerDefaults.

Discussed with Wei-Chiu Chuang and John Zhuge, we looked at the code together and saw KMS provider already support multiple key provider servers in its configuration, for example:

 <property>
    <name>hadoop.security.key.provider.path</name>
    <value>kms://https@kms01.example.com;kms02.example.com:16000/kms</value>
  </property>

and

     * If multiple hosts are provider, the Factory will create a
     * {@link LoadBalancingKMSClientProvider} that round-robins requests
     * across the provided list of hosts.

This is a form of KeyProvider HA handling (also handles load balancing).

Andrew Wang,

I like that getServerDefaults is lock-free, but I'm still worried about the overhead. MR tasks are short lived and thus don't benefit from the caching. This also affects all clients, on both encrypted and unencrypted clusters. I think getServerDefault is also currently only called when SASL is enabled. Have you done any performance testing of this RPC?

getServerDefaults mechanism has been there and the patch here just included an additional field. Calling it once an hour should not be a problem to me, at least not a regression. It's just that if things changed within the hour, the errors may not be handled correctly (for example, all old key provider hosts are replaced with new ones), but it's less of a concern assuming that's rare. I don't see that sasl is checked when getServerDefaults is called.

Thanks.

Hm, my jira window was not refreshed so I missed updates from you guys when I did my last one. Thanks for further discussions.

Hi Daryn, thanks for commenting! Looks like you have a more ambitious implementation in mind, since your usecases include dynamic configuration changes without client restarts (something not possible with the current config-based approach).

Generally speaking, I think it's pretty rare to change the KMS URI. I think the two situations are:

• Enabling HDFS encryption for the first time. This currently requires a client restart.
• Enabling KMS HA. As long as the old KMS is part of the HA group, then clients with the old value will still work.

Since the KMS is just a proxy, you can swap out the backing KeyProvider implementation without changing the URI.

I'm not familiar with the Credentials APIs, but I like the sound of your proposal. It lets most clients avoid calling getServerDefaults, which was my main concern about the current patch.

Since we're very interested in a NN-specified KMS URI but less interested in dynamic refresh, so if it's reasonable to do refresh as a follow-on JIRA, that'd be optimal from our perspective.

Thanks Andrew Wang, good comments.

Hi Daryn Sharp,

I like the sound of your proposal too

I think the cleanest/most-compatible way is leveraging the Credentials instead of the config. We could inject a mapping of filesystem uri to kms uri via the secrets map. So now when the client needs to talk to the kms it can check the map, else fallback to getServerDefaults.

Did you mean to use the following UserProvider method

  @Override
  public synchronized CredentialEntry createCredentialEntry(String name, char[] credential) 
      throws IOException {
    Text nameT = new Text(name);
    if (credentials.getSecretKey(nameT) != null) {
      throw new IOException("Credential " + name + 
          " already exists in " + this);
    }
    credentials.addSecretKey(new Text(name), 
        new String(credential).getBytes("UTF-8"));
    return new CredentialEntry(name, credential);
  }

to add <fs-uri, keyProvider> mapping to the credential map? This mapping info for a remote cluster need to come from either the remote cluster conf, or the NN of the remote cluster, what's your thinking here?

Would you please elaborate this approach? Is there any in-compatibility here?

Thanks.

Goal is naturally no incompatibility but expanded functionality. To that end, the general principal when acquiring tokens:

1. If getServerDefaults doesn't return a keyprovider field: it's a previous version to the client, don't know if kms is applicable, so fallback to config just as today.
2. If getServerDefaults does return a keyprovider: it's authoritative, add a kvp to the credentials mapping the fs uri to the kms uri.

When the dfsclient needs a key provider, check credentials:

1. If fs/kms provider mapping is present, use the value as authoritative since a token in the credentials was obtained from that provider.
2. If no mapping, check getServerDefaults, fallback to config.

Thanks Daryn Sharp!

Some follow-up questions:

My understanding is that the credential map exists at the server side (NN of a cluster), thus NN should populate the fs/kms provider mapping, is that understanding correct? If so, where and when does NN populate the mapping?

How the client gets the credential map?

Does this work only for kerberized cluster, but not non-kerberized cluster? If that's the case, how we solve the problem of non-kerberized cluster with encryption zone?

Thanks much.

Thanks Daryn Sharp Andrew Wang Yongjun Zhang Wei-Chiu Chuang for the discussion and reviews.
I will put up a patch in couple of days with test cases implementing Daryn's idea from this comment.
We can then discuss further ahead.

Credentials are simply a container in the ugi for tokens and/or secrets. There is no notion of client, server, etc. Credentials are the mechanism by which tokens are propagated throughout a job.

I may be understanding the EZ w/o security question (which seems an entirely contrived use case), but regardless: if tokens are available, so are the secrets since they are both packaged in the credentials object. If this use case works today then it will continue to work with the mappings based approach.

Thank you guys very much Daryn Sharp and Rushabh Shah!

Much appreciated that you will provide the revised patch Rushabh!

Hi Daryn Sharp,

Took a further look, my understanding is:

Credentials is an object stored in UGI, and it is passed around to various components such as mappers and reducers to get a job done. The Credentials object contains two maps:

  private  Map<Text, byte[]> secretKeysMap = new HashMap<Text, byte[]>();
  private  Map<Text, Token<? extends TokenIdentifier>> tokenMap =
      new HashMap<Text, Token<? extends TokenIdentifier>>();

When initializing the Credentials object for the client, the token map is populated by asking NN for the tokens with FSNamesystem#getDelegationToken(Text renewer)

The secretKeysMap is populated by UserProvider.

To add fs/keyProvider entries to secretKeysMap, we call getServerDefaults once to get back the keyProvider info, and update secretKeysMap with entries <fs, keyProvider>.

Is that understanding correct?

Thanks.

--Yongjun

This patch addresses most of the previous comments.
What changed in this patch compared to previous ?
1. During job submission, added a new secret in the credential's secret map.
DFS-KMS-<namenodeUri> --> keyproviderUri
This mapping captures the namenode's keyProviderUri at the time of job submission.
Every task that is scheduled to run will contact this key provider uri for decrypting EDEK's.
2. The key provider uri will be searched in the following order.

• from the credentials secrets map.
• Query the namenode via server defaults.
• Local conf.

Previous concerns:
1. From Andrew Wang

I like that getServerDefaults is lock-free, but I'm still worried about the overhead.

The namenode#getServerDefaults will be queried only once at the time of job submission.

2. From Yongjun Zhang

Currently getServerDefaults() contact NN every hour, to find if there is any update of keyprovider. If keyprovider changed within the hour,
client code may get into exception, wonder if we have mechanism to handle the exception and update the keyprovider and try again?

This was a very good question which I didn't think while writing previous patch. Thanks !
We glue the namenode uri to key provider uri at the time of job submission and persist in ugi's credentials object.
The task will find it in credentials object and no longer need to contact namenode.
If there is an update(hardware update or put in maintenance mode ) for key provider, we plan to keep in decommission mode for 7 days.
So all the tokens which were given out while it was still active will be valid for 7 days and then new key provider will issue tokens for newly submitted jobs.

I tried to incorporate all the previous comments in the current patch (v2) but let me know if I missed any.

I need one suggestion.

  public boolean isHDFSEncryptionEnabled() {
    try {
      return DFSUtilClient.isHDFSEncryptionEnabled(getKeyProviderUri());
    } catch (IOException ioe) {
      // This means the ClientProtocol#getServerDefautls threw StandbyException
      return false;
    }
  }

getKeyProviderUri is calling NamenodeRpcServer#getServerDefaults and it can throw an Standby Exception in which case, I am returning false.
I don't know what is the right thing to do.
DFSClient.isHDFSEncryptionEnabled() is being called by DistributedFileSystem.getTrashRoot(Path path) which doesn't throw any IOException so I need to take some decision if an Exception is encountered.
Your help is much appreciated.
Please review.

Thanks much for your quick turnaround, Rushabh Shah, I will review it tonight.

-1 overall

This message was automatically generated.

HI Rushabh Shah,

Thanks again for your updated patch. Below are my comments:

1. Currently we do

      final CryptoCodec codec = getCryptoCodec(conf, feInfo);
      KeyVersion decrypted = decryptEncryptedDataEncryptionKey(feInfo);

in createWrappedOutputStream(, where conf is the configuration of local cluster. There is a possibility that the local configuration is different than remote cluster's. So it's possible to fail here.

2. suggest to introduce

public satic final String HADOOP_SECURITY_KEY_PROVIDER_PATH_DEFAULT = "";

in parallel to HADOOP_SECURITY_KEY_PROVIDER_PATH in CommonConfigurationKeysPublic.java (@awang would you please confirm if it's ok to do so since this class is public), and use this constant at multiple places that current uses "".

3. Notice that "dfs.encryption.key.provider.uri" is deprecated and replaced with hadoop.security.key.provider.path (see HDFS-10489). So suggest to replace variable name keyProviderUri with keyProviderPath

4. Suggest to add two methods of package scope in DFSClient

  void addKmsKeyProviderPath(...)
  String getKmsKeyProviderPath(...)

and call them from needed places.

5.The uri used in DistributedFileSystem and DFSClient may be different, see DistributedFileSystem#initialize below

  public void initialize(URI uri, Configuration conf) throws IOException {
    ...
    this.dfs = new DFSClient(uri, conf, statistics);
    this.uri = URI.create(uri.getScheme()+"://"+uri.getAuthority());
    this.workingDir = getHomeDirectory();

So the update and retrieve of fs/keyProvider from the credentials may be mismatching, because uri.toString() is used as the key in the credentials map. We may just use (uri.getScheme()+"://"+uri.getAuthority() as the key and encapsulate this in the add/get methods I proposed in 4.

6. Seems we need a similar change in WebHdfsFileSystem when calling addDelegationTokens

7. About your question w.r.t. public boolean isHDFSEncryptionEnabled() throwing StandbyException. There is a solution, that is, we need to incorporate remote's cluster's nameservices configurations in the client (distcp for example) configuration, and let the client handle the NN failover and retry. We need to document this.

Hi Daryn Sharp and Andrew Wang, would you please help doing a review too? and see if my above comments make sense to you?

Thanks.

HI Rushabh Shah,

We need a solution soon, so I hope you don't mind I just uploaded a patch to address most of my comments.

I also included some changes to avoid overhead for cluster that doesn't have encryption enabled, by inserting an empty entry into the secret map.

Would you please take a look?

Hi Daryn Sharp and Andrew Wang, would you please help taking a look? thanks a lot.

-1 overall

This message was automatically generated.

final CryptoCodec codec = getCryptoCodec(conf, feInfo);
in createWrappedOutputStream(, where conf is the configuration of local cluster. There is a possibility that the local configuration is different than remote cluster's. So it's possible to fail here.

We are not reading the key provider path from conf within getCryptoCodec method. So I don't think my change (form v2 patch) will fail anything.

public satic final String HADOOP_SECURITY_KEY_PROVIDER_PATH_DEFAULT = "";

Before the v2 of patch, there was no default value for HADOOP_SECURITY_KEY_PROVIDER_PATH_DEFAULT and I kept it the same way.
I think this change is out of the scope of this jira. Suggest you to create a new jira for this change. Let's not mix multiple things in one jira.

3. Notice that "dfs.encryption.key.provider.uri" is deprecated and replaced with hadoop.security.key.provider.path (see HDFS-10489). So suggest to replace variable name keyProviderUri with keyProviderPath

I didn't even know this config used to be called as dfs.encryption.key.provider.uri.
But this hadoop.security.key.provider.path on the client side is just an URI and not path by any way. We convert this to a path via KMSClientProvider(URI uri, Configuration conf) where we extract the path via KMSClientProvider#extractKMSPath(URI uri). Thats why I named it keyProviderUri.
But if you feel that strongly about the variable name, I can change it to provider path in my next revision.

4. Suggest to add two methods of package scope in DFSClient
void addKmsKeyProviderPath(...)
String getKmsKeyProviderPath(...)

I think we use add method if that data structure is going to contain more than one entry. I don't think the provider uri on the client side is going to contain more than one entry.
I already have getKeyProviderUri in v2 of patch.

5.The uri used in DistributedFileSystem and DFSClient may be different, see DistributedFileSystem#initialize below

This is very good observation. I think its safe to flip these 2 statements in DistributedFilesystem class.
Will do it in next revision.

  this.dfs = new DFSClient(uri, conf, statistics);
    this.uri = URI.create(uri.getScheme()+"://"+uri.getAuthority());

6. Seems we need a similar change in WebHdfsFileSystem when calling addDelegationTokens

I don't think key provider is used by WebHDFSFileSystem. Maybe I'm missing something.
Can you please elaborate your comment ?

7. About your question w.r.t. public boolean isHDFSEncryptionEnabled() throwing StandbyException. There is a solution, that is, we need to incorporate remote's cluster's nameservices configurations in the client (distcp for example) configuration, and let the client handle the NN failover and retry. We need to document this.

I didn't understand this comment also. Please elaborate.

We need a solution soon, so I hope you don't mind I just uploaded a patch to address most of my comments.

I don't see any sense of urgency from the community since providing key provider path via conf was the standard way since the feature was introduced since hadoop 2.6 (Aug 2014).
Having said that I was waiting for comments from Daryn Sharp and Andrew Wang before putting up a new patch.
I know Daryn Sharp was out of office for last couple of days and Andrew Wang was involved in 2.8.0 release work so I thought to wait for few days before pinging them.
Yongjun Zhang: Since I am the assignee of the jira and I am updating patches regularly, please allow me to upload the patches henceforth. I would like to get valuable review comments from you.
I hope you don't mind that.

Andrew Wang Daryn Sharp: Just FYI, please review HADOOP-14104-trunk-v2.patch patch.

Thanks for the update Rushabh Shah, sorry about that, but please be assured that I did not mean to intrude, my sincere apology if you felt so.

I should have given some background, I was looking into HDFS-9868 earlier because we need a solution very soon to let distcp to be able to see the keyProvider of the remote cluster, then we found that HADOOP-14104 may be a better solution. As far as I know, the existing "providing key provider path via conf " implementation doesn't support external cluster, we could do an implementation to extend the conf support for external cluster for keyprovider, as an alternative solution for HADOOP-14104.

Will comment on your other points soon.

Here's my review on v2, along with responses to some of Yongjun's questions. Thanks for working on this Rushabh!

in createWrappedOutputStream(, where conf is the configuration of local cluster. There is a possibility that the local configuration is different than remote cluster's. So it's possible to fail here.

The conf is only used to configure the CryptoCodec, which does not need a KMS URI. I think this is okay, all the CC parameters are included in the feInfo.

@awang would you please confirm if it's ok to do so since this class is public), and use this constant at multiple places that current uses ""

Yea it's fine. I would like to improve this in this patch if possible, since it removes redundancies.

3. Notice that "dfs.encryption.key.provider.uri" is deprecated and replaced with hadoop.security.key.provider.path (see HDFS-10489). So suggest to replace variable name keyProviderUri with keyProviderPath

I think we can ignore this for now like Rushabh said, can handle it in another JIRA if necessary.

Seems we need a similar change in WebHdfsFileSystem when calling addDelegationTokens

The DN does the encryption/decryption in WebHDFS, so the client doesn't need to do any KMS communication.

It does bring up a question regarding the DN DFSClient though. It looks like WebHdfsHandler creates a new DFSClient each time, which means we won't benefit from getServerDefaults caching.

Is the fix to make config preferred over getServerDefaults?

<StandbyException>

Does this exception actually make it to clients? The HA RPC proxy normally catches the StandbyException and fails over to the other NN. We can write a unit test for this to verify if we're unsure.

My additional review comments:

• typo nameonde
• ServerDefaults#getKeyProviderUri needs javadoc explaining how to interpret null and empty (IIUC null means not set, empty means means set but not enabled)
• In docs, "DFSClients" is an internal name. Please rename to say "HDFS clients" or similar. Same for "dfs clients" in core-default.xml
• There are a lot of whitespace errors, please take a look at what's flagged by checkstyle. Recommend using IDE autoformatting in the future.
• An actual mini-cluster test that mimics an MR job submitter and task's call pattern would also be good.

TestEncryptionZones:

• Would like to see the test reversed so it covers the fallback behavior, i.e.
• set client config with kp1, check that it returns kp1
• mock getServerDefaults() with kp2, check it returns kp2
• set Credentials with kp3, check it returns kp3
• typo originalNameodeUri
• String lookUpKey = DFSClient.DFS_KMS_PREFIX + originalNameodeUri.toString(); should this be a getKey helper method in DFSClient rather than having the test code also construct the key?

HI Rushabh Shah,

Thanks again for your work. Below are my comments in reply to yours. Just saw Andrew Wang's, thanks Andrew!

1.

final FileEncryptionInfo feInfo = dfsos.getFileEncryptionInfo();
final CryptoCodec codec = getCryptoCodec(conf, feInfo);
in createWrappedOutputStream, where conf is the configuration of local cluster. There is a possibility that the local configuration is different than remote cluster's. So it's possible to fail here.

Sorry I was a bit unclear earlier, this comment is not about the change of this jira, but about the existing implementation. My concern is about Codec rather than keyProvider here. We get feInfo from the target file, getting Codec based on conf and feInfo. The conf here is the configuration of the local cluster. When the target is the remote cluster, could we fail to find Codec info or wrong codec info because we don't have remote cluster's configuration? That's what wanted to say. So I hope Andrew Wang who was involved in the original development of encryption feature could comment. (Andrew, saw you above comment, but would you please look at my comment here again and see if it makes sense?

2. It looks having a HADOOP_SECURITY_KEY_PROVIDER_PATH_DEFAULT constant would help making the code more consistent, even for the patch here, but having a separate jira works for me too.

3. Keeping the name keyProviderUri instead of keyProviderPath is actually fine. I did some study before creating patch rev3, the format appears to be Uri. I wish HDFS-10489 made it uri instead of path. So I did not change uri to path in rev3.

4. About adding two methods to add/get from credentials, it's a way of encapsulating how it's handled in one place, and share the way how the key in the map is generated (e.g. uri.getScheme()+"://"+uri.getAuthority()). You can see my example in rev3. These methodd are also called in Test code.

5.

I don't think key provider is used by WebHDFSFileSystem. Maybe I'm missing something.
Can you please elaborate your comment ?

I was just guessing, but I'm not so sure, but hope Daryn Sharp can comment.

6.

7. About your question w.r.t. public boolean isHDFSEncryptionEnabled() throwing StandbyException. There is a solution, that is, we need to incorporate remote's cluster's nameservices configurations in the client (distcp for example) configuration, and let the client handle the NN failover and retry. We need to document this.

For HA cluster, we can access NN via nameservice (for example, hadoop distcp hdfs://nameservice1:/xyz hdfs://nameservice2:/abc) , so the StandbyException can be detected and different NN will be tried automatically. See https://issues.apache.org/jira/browse/HDFS-6376. We actually see a non-intrusive way to do that without using dfs.internal.nameservices", that is, we copy the local cluster's conf to a new dir, and append the nameservice portion of the remote cluster's conf to the hdfs-site.xml in the new dir. Then pass the new dir to distcp.

Hi Yongjun,

When the target is the remote cluster, could we fail to find Codec info or wrong codec info because we don't have remote cluster's configuration?

The FileEncryptionInfo has a cipher type field. If the client doesn't support the cipher, then it can't read/write the file, and will throw an exception. If it does support that cipher, it uses its local config to determine the correct codec implementation to use (i.e. java or native).

From a safety point of view, we're okay to use the local config. If the client is too old and doesn't understand a new cipher type, it'll abort. Supporting a new cipher necessarily requires upgrading the client (and potentially also installing native libraries), so I think this behavior is okay.

Thanks Andrew Wang.

The FileEncryptionInfo has a cipher type field. If the client doesn't support the cipher, then it can't read/write the file, and will throw an exception

Does the exception get thrown when we do final CryptoCodec codec = getCryptoCodec(conf, feInfo); or later when we write file? I prefer the former.

BTW Rushabh Shah,

In rev2, DistributedFileSyste#addDelegationTokens put the keyProvider entry to scretMap only when dfs.isHDFSEncryptionEnabled() is true. dfs.isHDFSEncryptionEnabled() calls getServerDefaults to find out whether encryption is enabled when there is no keyProvider entry in the secretMap. This means, If encryption is not enabled, then we don't put any entry into the secretMap, and each task of mapreduce job will always call getServerDefaults, which we try to avoid.

I suggest to add an empty entry to the secretMap after we call getServerDefaults for the first time and find encryption is disabled. Then later if we find an entry in the map, and if it's empty, we know encryption is not enabled, if it's non-empty, it's enabled. This is what I tried to do in rev3 to avoid the extra getServerDefaults call for the case when encryption is disabled. For your reference.

Thanks.

HI Rushabh Shah,

To avoid confusions to other reviewers, I'm taking out rev3. Thanks.

This means, If encryption is not enabled, then we don't put any entry into the secretMap, and each task of mapreduce job will always call getServerDefaults, which we try to avoid.

I don't follow this.
DFSClient#isHDFSEncryptionEnabled is being only called by DistributedFileSystem#addDelegationTokens (job submission) and DistributedFileSystem.getTrashRoot (FSShell commands).
DFSClient#getKeyProvider is being called by DistributedFileSystem#addDelegationTokens (again job submission) and DFSClient#decryptEncryptedDataEncryptionKey.
When calling via DFSClient#decryptEncryptedDataEncryptionKey, we already know that this directory or parent directory is already encrypted.
So I don't understand why each mapreduce task which is accessing non-encrypted directory will call getKeyProvider and in turn call serverDefaults ?
Am I missing something ?

Hi Rushabh Shah,

Thanks for pointing it out. Because DFSClient#isHDFSEncryptionEnabled is a public method of DFSClient, I thought anyone could call it including any mapreduce task.

Not an ask for this jira, - we can do it later - If this method is only intended to be called by DistributedFileSystem#addDelegationTokens and DistributedFileSystem.getTrashRoot, maybe we can add a javadoc to indicate that. BTW, One could create FsShell object and call shell from any java code too, we can worry about that later.

Thanks.

Sorry for the delay, I've been OOO on and off the past week. Reviewing this morning.

Still scrutinizing, initial comments:

Regarding need for constants, in general, would be preferable for “null” to mean no provider instead of both empty string and null meaning no provider – in which case a constant is not necessary.

Regarding concerns over getServerDefaults being called via webhdfs: if you are using EZ with webhdfs, one little getServerDefaults call is the least of your worries... You should terrified that apparently one compromised DN renders encryption moot. At any rate, we'll see what we can do to reduce the number of calls from webhdfs.

DFSClient

1. get/setKeyProviderUri methods should use URI instances, not strings, as name implies. The caller should have already done the string to uri conversions.
2. getServerDefaults never returns null, so don’t null check. Just assign keyProviderUri and check for null.

KMSUtil

1. If createKeyProviderFromUri(Configuration, String) fails to create a client, it throws with a hardcoded CommonConfigurationKeysPublic.HADOOP_SECURITY_KEY_PROVIDER_PATH which may not be the key that was used. Although see comments in KeyProviderCache that may negate need for these changes at all.

KeyProviderCache

1. The new get(Configuration,String) should take a URI, not String, and just look it up. No need to perform all the String -> URI logic for every lookup since DFSClient should call with a computed URI.
2. Consider just calling KeyProviderFactory.get(uri, conf) directly instead of roundabout trip through DFSUtilClient/KMSUtil. Then you probably don’t need to change those classes since their methods are all about getting the key provider from the config. We know what the URI is.
3. Minor, in get, leave the unwrapping of the ExecutionException in place.

DFSClient

1. In isHDFSEncryptionEnabled, it’s completely broken to swallow exceptions and return false… It’s not just a StandbyException, per the comment, that may bubble out. It could be the retry proxy giving up, connect refused, etc. Change this method to allow IOE because DFSClient is a private interface. Then the filesystem’s getTrashRoot can do the wrong thing by swallowing the exception.

DistributedFileSystem

1. I’d rather see addDelegationTokens not call the problematic isHDFSEncryptionEnabled method, but instead call dfs.getKeyProvider(). If not null, do the block of code to get a kms token.

I'll double check that token selection works correctly.

Thanks Daryn Sharp a lot for the review and comments.

Hi Rushabh S Shah,

Wonder if you have chance to look at Daryn's comments?

Many thanks!

Wonder if you have chance to look at Daryn's comments?

I was out last week and before that busy working on internal stuff.
I should be able to upload a new patch today.

An actual mini-cluster test that mimics an MR job submitter and task's call pattern would also be good.

I was able to address all the review comments except the above one.
Only way I can think to mimic job submitter is by creating MiniYARNCluster. I don't have any experience writing yarn tests. I may take additional time if you want me to write miniyarn cluster test.
Yongjun Zhang, Andrew Wang: Any pointers would be appreciated.

Thanks much Rushabh S Shah!

Hi Andrew Wang, about your comment Rushabh was asking, would you please elaborate? seems a more comprehensive test, do you agree that we could address it in later jira? Thanks.

I think a full YARN job is a bit much; basically I wanted a unit test that did an encrypted read/write using the KP URI from the credentials.

This might be partially addressed by an existing HDFS encryption test, but I'd like to see the same verification as in the newly added unit tests that the value from the credentials overrides the value in the config.

Thanks Daryn Sharp Andrew Wang Yongjun Zhang for your valuable reviews.
I tried to address most of the review comments.
For the following comment:

basically I wanted a unit test that did an encrypted read/write using the KP URI from the credentials.

I added another unit test TestEncryptionZones#testEncryptedReadWriteUsingDiffKeyProvider
This test tried to read/write a file from/to encrypted zone.
The test adds key provider uri to credentials object and then unsets the local conf.
While reading the file, it gets the key provider uri from the credentials object.
The way we try to resolve the provider uri is first credentials map, then from namenode and then from conf.
So among this chain, the testcase can get the key provider uri from namenode but I verified by adding a log line that it got from credentials map.

Note: I haven't resolved the checkstyle warnings from the previous patch since the precommit build logs were removed from jenkins server.
Once the precommit build runs, I will fix all the checkstyle warnings and then upload another patch.
Request for reviews.

-1 overall

This message was automatically generated.

Removing the patch to address checkstyle comments.

Addressed some of the checkstyle comments.
Since the config key name "CommonConfigurationKeysPublic.HADOOP_SECURITY_KEY_PROVIDER_PATH" is so big, it can't fit in a line within 80 characters and remaining are not added by this patch.
Test failures:
1. TestWebHdfsTimeouts - Fails consistently with and without the patch.
Tracked via HDFS-11043
2. TestDataNodeVolumeFailureReporting - Passes locally on my box. Ran multiple times. Seems like a flaky test.
3. TestDFSZKFailoverController - Passes locally on my box. Proven to be a flaky test. We tried to fix it via HDFS-9333 but still failing. Left a comment on HDFS-9333.

-1 overall

This message was automatically generated.

Hi Rushabh, thanks for revving. Yongjun and I reviewed this together, posted here are our combined review comments. Looks really good overall!

Nits:

• Change DFS_KMS_PREFIX to private
• Rename getKmsSecretKey to getKeyProviderMapKey (included in item below), since "SecretKey" sounds like an encryption key, a javadoc would also help

Bigger things:

In DistributedFileSystem, this changes the uri passed to DFSClient:

   this.dfs = new DFSClient(uri, conf, statistics);
   this.uri = URI.create(uri.getScheme()+"://"+uri.getAuthority());

to

   this.uri = URI.create(uri.getScheme()+"://"+uri.getAuthority());
   this.dfs = new DFSClient(uri, conf, statistics);

To be safe, I'd suggest that we don't change the order of the above code, and instead change the method in DFSClient.java to just grab the scheme and authority:

 public Text getKmsSecretKey() {
    return new Text(DFS_KMS_PREFIX + namenodeUri.toString());
  }

to

  public Text getKeyProviderMapKey() {
    return new Text(DFS_KMS_PREFIX + nnUri.getScheme()
        + "://" + nnUri.getAuthority());
  }

Thanks Andrew Wang and Yongjun Zhang for reviewing and for your valubale feedback.
Will upload a new patch by EOB tomorrow addressing your review comments.

Hopefully the last patch for trunk.
Andrew Wang Yongjun Zhang: I really appreciate your feedback.
Thanks for being extremely patient with me.
Attaching a new patch addressing your comments and fixing some checkstyle issues.
Please review.
If this patch is good for trunk, I will then attach branch-2 and branch-2.8 patch immediately.

Test failures from previous precommit build. Both tests ran fine on my machine.
1. hadoop.security.TestKDiag: Known to be flaky. Tracked via HADOOP-14030
2. hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting:
This also is known to be flaky. Tried to fix via HDFS-11316, HDFS-11398. But still flaky.

-1 overall

This message was automatically generated.

+1 LGTM, Yongjun also pinged me to say it looked good to him. I'll commit this shortly.

Committed to trunk, leaving this open for the branch-2 and branch-2.8 patches. Thanks again Rushabh!

SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11521 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11521/)
HADOOP-14104. Client should always ask namenode for kms provider path. (wang: rev 18432130a7f580f206adf023507678c534487f2e)

• (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/KMSUtil.java
• (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ftp/FtpConfigKeys.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/site/markdown/TransparentEncryption.md
• (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/proto/hdfs.proto
• (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/local/LocalConfigKeys.java
• (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
• (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestKeyProviderCache.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestEncryptionZones.java
• (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestPBHelper.java
• (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
• (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FsServerDefaults.java
• (edit) hadoop-common-project/hadoop-common/src/main/resources/core-default.xml
• (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/KeyProviderCache.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java

Thanks Andrew Wang for committing. Nice work Rushabh S Shah!

Attaching branch-2 and branch-2.8 patch.
No code change compared to trunk.

Andrew Wang Thanks for reviewing and committing to trunk.
Yongjun Zhang Daryn Sharp: Thanks for reviews.

Hi Rushabh S Shah, I think you attached a wrong branch-2 patch which has 0.8 K only?

+1 overall

This message was automatically generated.

Hi Rushabh S Shah, I think you attached a wrong branch-2 patch which has 0.8 K only?

Thanks Junping Du !
I attached branch-2 version of another jira.
Attached the right branch-2 patch now.

-1 overall

This message was automatically generated.

Committed to branch-2 and branch-2.8 as well, thanks again for working on this Rushabh, as well as Yongjun and Daryn for reviews.

Thanks all !

SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11591 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11591/)
HADOOP-14104. Client should always ask namenode for kms provider path. (wang: rev 18432130a7f580f206adf023507678c534487f2e)

• (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
• (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/KMSUtil.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestPBHelper.java
• (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FsServerDefaults.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestEncryptionZones.java
• (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/local/LocalConfigKeys.java
• (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/site/markdown/TransparentEncryption.md
• (edit) hadoop-common-project/hadoop-common/src/main/resources/core-default.xml
• (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/proto/hdfs.proto
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestKeyProviderCache.java
• (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/KeyProviderCache.java
• (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java
• (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
• (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ftp/FtpConfigKeys.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java

HI Rushabh Shah, Andrew Wang, Daryn Sharp,

Just noticed that he change of adding "throws" in the public API of DFSClient breaks hive, which accesses DFSClient, thought it should not because DFSClient is private.

public boolean isHDFSEncryptionEnabled() throws IOException{

 * Hadoop DFS users should obtain an instance of 
 * DistributedFileSystem, which uses DFSClient to handle
 * filesystem tasks.
 *
 ********************************************************/
@InterfaceAudience.Private
public class DFSClient implements java.io.Closeable, RemotePeerFactory,
    DataEncryptionKeyFactory {

Since the impact is wide (many different versions of Hive), we can remove the throws from this API as a temporary solution. And simply return false when it throws, do you guys agree?

We should also let hive to fix their code not to access private interface of Hadoop.

Thanks.

Yongjun Zhang thanks for reporting !
I will discuss with Daryn Sharp and will update this jira soon.

Since the impact is wide (many different versions of Hiv

Yongjun Zhang which version of hive is calling ?
I tried to search on grepcode but couldn't find any hit.

Thanks a lot Rushabh S Shah.

I created HADOOP-14333. This is a temp solution, and we are requesting hive to make changes to use public API, we can add public APIs if they are not there. I hope we can get this in asap today.

Hive code

private boolean isEncryptionEnabled(DFSClient client, Configuration conf) {
  try {
    DFSClient.class.getMethod("isHDFSEncryptionEnabled");
  } catch (NoSuchMethodException e) {
    // the method is available since Hadoop-2.7.1
    // if we run with an older Hadoop, check this ourselves
    return !conf.getTrimmed(DFSConfigKeys.DFS_ENCRYPTION_KEY_PROVIDER_URI, "").isEmpty();
  }
  return client.isHDFSEncryptionEnabled();
}

2.8.1 became a security release. Moving fix-version to 2.8.2 after the fact.