You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nifi.apache.org by miquillo <gi...@git.apache.org> on 2016/04/19 14:36:50 UTC
[GitHub] nifi pull request: NIFI-1769: added support for SSE-KMS and signat...
GitHub user miquillo opened a pull request:
https://github.com/apache/nifi/pull/362
NIFI-1769: added support for SSE-KMS and signature s3v4 authentication
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/miquillo/nifi master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/362.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #362
----
commit a130d4257120ee547c4f38d6ef9da8143aa36d3b
Author: Michiel Moonen <mi...@wartsila.com>
Date: 2016-04-19T12:30:55Z
NIFI-1769: added support for SSE-KMS and signature s3v4 authentication
----
---
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.
---
[GitHub] nifi issue #362: NIFI-1769: added support for SSE-KMS and signature s3v4 aut...
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/362
@miquillo I'm happy to review and test the contributions you have made, and I would be happy to put some code where my comments with an implementation of the signature version controls. But I am hoping you will continue to work us on this PR. I would very much like your help in either writing the code or reviewing and testing the changes, as you have both experience with SSE-KMS in NiFi and knowledge of the driving use case. What would you feel most comfortable with?
---
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.
---
[GitHub] nifi issue #362: NIFI-1769: added support for SSE-KMS and signature s3v4 aut...
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/362
I now better understand that we do not need to do anything to the FetchS3Object processor to read KMS encrypted files. Server-Side Encryption is, after all, on the server. Strike that point.
---
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.
---
[GitHub] nifi pull request #362: NIFI-1769: added support for SSE-KMS and signature s...
Posted by miquillo <gi...@git.apache.org>.
Github user miquillo closed the pull request at:
https://github.com/apache/nifi/pull/362
---
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.
---
[GitHub] nifi issue #362: NIFI-1769: added support for SSE-KMS and signature s3v4 aut...
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/362
@miquillo , what do you think about using [ClientConfiguration::setSignerOverride()](http://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/ClientConfiguration.html#setSignerOverride(java.lang.String)) to control the signature version? One of the advantages I see to it is better isolation for the processor vs. any other NiFi AWS processors. I'm a bit worried that one PutS3Object processor using SSE-KMS would change the settings other processors running at the same time. I believe an appropriate location to do this would be in AbstractS3Processor::createClient(). That would allow the configuration code to be shared, while the configured value would remain specific to individual processors.
But I'm not sure I agree with configuring the version as a true/false setting for signature version 4. I would recommend a list of values:
* AWS SDK default (as the default selection)
* Signature v2
* Signature v4
That leaves room for the AWS SDK default to change if/when we upgrade to a newer SDK, and it would allow for users to explicitly request either v4 or v2 to match whatever features and endpoint they are using. What do you think?
---
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.
---
[GitHub] nifi issue #362: NIFI-1769: added support for SSE-KMS and signature s3v4 aut...
Posted by miquillo <gi...@git.apache.org>.
Github user miquillo commented on the issue:
https://github.com/apache/nifi/pull/362
@jvwing So it has been a bit silent here, but we continued last week and now we've merged the current master branch into this project. This is where I realized you've implemented the S3v4 (along with v2) already in master, so I've removed our implementation from this branch. The KMS feature is still in, merged with your implementation of S3V4 [using SetSignerOverride() ].
Furthermore we've included a new processor! It's the Kinesis Streams processor, basically the streaming version of firehose. Hope you like it :)
Will check Jira and update accordingly. I was unable to built the current master succesfully b/o failing tests in other packages, but it should be working.
---
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.
---
[GitHub] nifi issue #362: NIFI-1769: added support for SSE-KMS and signature s3v4 aut...
Posted by jvwing <gi...@git.apache.org>.
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.
---
[GitHub] nifi issue #362: NIFI-1769: added support for SSE-KMS and signature s3v4 aut...
Posted by miquillo <gi...@git.apache.org>.
Github user miquillo commented on the issue:
https://github.com/apache/nifi/pull/362
@jvwing Thanks for picking this up!
- I would 100% apply this to the AbstractS3processor. By the time I figured it myself, I already had implemented it (in the wrong class). Should be easy to move to the AbstractS3processor, not much code is affected by this PR.
- Definitely make this a combobox / selectable option. It was just a quick fix to us to make it a true/false flag as we are only using Signature v4. Indeed makes it futureproof.
- I am completely unaware of the ClientConfiguration::setSignerOverride() and I do also dislike the system.setproperty(..). If it works the same way; definitely go for it.
---
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.
---
[GitHub] nifi issue #362: NIFI-1769: added support for SSE-KMS and signature s3v4 aut...
Posted by miquillo <gi...@git.apache.org>.
Github user miquillo commented on the issue:
https://github.com/apache/nifi/pull/362
@jvwing OUCH! that's painfull to see it's exactly similar to what we've build.. :(
I reviewed the code and it seems that it is more mature than our solution and VERY similar :) I'll leave my comments there. I can see if can fix/remove the code we've committed concerning the putkinesis, so this PR will only contain fixes for SSE-KMS.
---
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.
---
[GitHub] nifi issue #362: NIFI-1769: added support for SSE-KMS and signature s3v4 aut...
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/362
@miquillo, I'll take another look at the SSE change, but I would like to keep this PR focused on just SSE. We'll need additional tickets and PRs for additional changes.
Also, you might take a look at NiFi 1540 / PR #239, an existing effort underway for Kinesis Streams. I'm not sure how your approach compares, but input and feedback would certainly be appreciated.
---
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.
---