nifi-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jvwing <...@git.apache.org>
Subject [GitHub] nifi issue #362: NIFI-1769: added support for SSE-KMS and signature s3v4 aut...
Date Mon, 20 Jun 2016 15:27:20 GMT
Github user jvwing commented on the issue:

    https://github.com/apache/nifi/pull/362
  
    @miquillo, thanks for putting together this PR, Server-Side Encryption with customer keys
is a great feature for NiFi to have.  I'm doing some review, and considering the following
topics:
    
    * **NiFi's AWS SDK Version** - Since you submitted this PR, Amazon appears to have made
signature version 4 the default for S3 in their Java SDK v1.11.0, May 13, 2016.  NiFi's current
SDK version is 1.10.32 from Nov 3, 2015.  Upgrading the SDK would apply much broader than
this feature, but maybe not out of line for the v1.0 release.  If we chose not to upgrade
now, we might consider that in a future upgrade the signature version defaults will change.
    * **Controlling Signature Versions** - Regardless of the SDK default, you are absolutely
right to make it optional.  I'm a bit baffled by why Amazon uses a System property to control
this setting rather than a client or request property, and a bit concerned about multiple
processors fighting over the signature version.  I agree that `System.setProperty(...)` is
the [documented method of setting the signature version](http://docs.aws.amazon.com/AmazonS3/latest/dev/UsingAWSSDK.html).
 But I don't like it, I would prefer to set this more concisely than a global setting.  Are
you familiar with [`ClientConfiguration::setSignerOverride()`](http://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/ClientConfiguration.html#setSignerOverride(java.lang.String))?
 It appears to allow this, if not so well documented.
    * **SSE KMS for FetchS3Object** - We might also want to apply this feature to the FetchS3Object
processor, or at least to allow for that to happen in the future.  Have you considered moving
some of the KMS logic to the AbstractS3Processor class? 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message