Skip to content

NIFI-11360 Support Client-Side Encryption for Azure Blob v12 Processors#7182

Closed
mkalavala wants to merge 4 commits intoapache:mainfrom
mkalavala:NIFI-11360
Closed

NIFI-11360 Support Client-Side Encryption for Azure Blob v12 Processors#7182
mkalavala wants to merge 4 commits intoapache:mainfrom
mkalavala:NIFI-11360

Conversation

@mkalavala
Copy link
Copy Markdown
Contributor

@mkalavala mkalavala commented Apr 17, 2023

Summary

NIFI-11360

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

  • Build completed using mvn clean install -P contrib-check
    • JDK 11
    • JDK 17

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

@nandorsoma nandorsoma self-requested a review April 24, 2023 12:46
exceptionfactory
exceptionfactory requested changes Apr 25, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this new feature @mkalavala!

Although the approach is similar to the legacy implementation, this is a good opportunity to make some improvements. I noted some naming and implementation recommendations in several places. I also recommend removing the integration tests since they are not run automatically and can get stale quickly.

...a/org/apache/nifi/processors/azure/storage/utils/AzureBlobClientSideEncryptionUtils_v12.java Outdated
Comment on lines +44 to +45
.name("cse-key-type")
.displayName("Client-Side Encryption Key Type")
Copy link
Copy Markdown
Contributor

@exceptionfactory exceptionfactory Apr 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although the conventions are not consistent, in this case, the new properties can have the same name and displayName.

Suggested change
.name("cse-key-type")
.displayName("Client-Side Encryption Key Type")
.name("Client-Side Encryption Key Type")
.displayName("Client-Side Encryption Key Type")

@turcsanyip @exceptionfactory
NIFI-11475 Fixed missing jackson-dataformat-xml in nifi-azure-service…
14f86a2 
…s-api

This closes apache#7186

Signed-off-by: David Handermann <exceptionfactory@apache.org>
(cherry picked from commit 8586ac5)
exceptionfactory
exceptionfactory requested changes May 1, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @mkalavala! Functional behavior works as expected, and these changes interoperate with the earlier versions of the Processors. I noted some additional wording and style adjustments, but otherwise this looks close to completion.

...e-processors/src/main/java/org/apache/nifi/processors/azure/ClientSideEncryptionSupport.java Outdated
Comment on lines +99 to +101
byte[] keyBytes;
try {
keyBytes = Hex.decodeHex(keyHex);
Copy link
Copy Markdown
Contributor

@exceptionfactory exceptionfactory May 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The declaration and assignment can be merged.

Suggested change
byte[] keyBytes;
try {
keyBytes = Hex.decodeHex(keyHex);
try {
final byte[] keyBytes = Hex.decodeHex(keyHex);

...e-processors/src/main/java/org/apache/nifi/processors/azure/ClientSideEncryptionSupport.java Outdated
keyBytes = Hex.decodeHex(keyHex);
if (getKeyWrapAlgorithm(keyBytes) == null) {
validationResults.add(new ValidationResult.Builder().subject(CSE_LOCAL_KEY_HEX.getDisplayName())
.explanation("the local key must be 128, 192, 256, 384 or 512 bits of data.").build());
Copy link
Copy Markdown
Contributor

@exceptionfactory exceptionfactory May 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend adjusting the wording and including the actual length.

Suggested change
.explanation("the local key must be 128, 192, 256, 384 or 512 bits of data.").build());
.explanation(String.format("Key size in bits must be one of [128, 192, 256, 384, 512] instead of [%d]", keyBytes.length * 8)).build());

...e-processors/src/main/java/org/apache/nifi/processors/azure/ClientSideEncryptionSupport.java Outdated
}
} catch (DecoderException e) {
validationResults.add(new ValidationResult.Builder().subject(CSE_LOCAL_KEY_HEX.getDisplayName())
.explanation("the local key must be a valid hexadecimal string.").build());
Copy link
Copy Markdown
Contributor

@exceptionfactory exceptionfactory May 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.explanation("the local key must be a valid hexadecimal string.").build());
.explanation("Key must be a valid hexadecimal string").build());

...e-processors/src/main/java/org/apache/nifi/processors/azure/ClientSideEncryptionSupport.java Outdated
final List<ValidationResult> validationResults = new ArrayList<>();
if (StringUtils.isBlank(keyHex)) {
validationResults.add(new ValidationResult.Builder().subject(CSE_LOCAL_KEY_HEX.getDisplayName())
.explanation("a local key must be set when client-side encryption is enabled with local encryption.").build());
Copy link
Copy Markdown
Contributor

@exceptionfactory exceptionfactory May 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.explanation("a local key must be set when client-side encryption is enabled with local encryption.").build());
.explanation("a local key must be set when client-side encryption is enabled").build());

...e-processors/src/main/java/org/apache/nifi/processors/azure/ClientSideEncryptionSupport.java Outdated
final String cseLocalKeyHex = validationContext.getProperty(CSE_LOCAL_KEY_HEX).getValue();
if (cseKeyType != ClientSideEncryptionMethod.NONE && StringUtils.isBlank(cseKeyId)) {
validationResults.add(new ValidationResult.Builder().subject(CSE_KEY_ID.getDisplayName())
.explanation("a key ID must be set when client-side encryption is enabled.").build());
Copy link
Copy Markdown
Contributor

@exceptionfactory exceptionfactory May 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.explanation("a key ID must be set when client-side encryption is enabled.").build());
.explanation("a key ID must be set when client-side encryption is enabled").build());

...e-processors/src/main/java/org/apache/nifi/processors/azure/ClientSideEncryptionSupport.java
.dependsOn(CSE_KEY_TYPE, ClientSideEncryptionMethod.LOCAL.name())
.build();

PropertyDescriptor CSE_LOCAL_KEY_HEX = new PropertyDescriptor.Builder()
Copy link
Copy Markdown
Contributor

@exceptionfactory exceptionfactory May 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
PropertyDescriptor CSE_LOCAL_KEY_HEX = new PropertyDescriptor.Builder()
PropertyDescriptor CSE_LOCAL_KEY = new PropertyDescriptor.Builder()

...e-processors/src/main/java/org/apache/nifi/processors/azure/ClientSideEncryptionSupport.java Outdated
.displayName("Client-Side Encryption Key ID")
.description("Specifies the ID of the key to use for client-side encryption.")
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
.required(false)
Copy link
Copy Markdown
Contributor

@exceptionfactory exceptionfactory May 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be changed to required(true) since it depends on the Local Type, which should remove the need for custom validation.

...e-processors/src/main/java/org/apache/nifi/processors/azure/ClientSideEncryptionSupport.java Outdated
.name("cse-local-key-hex")
.displayName("Client-Side Encryption Local Key")
.description("When using local client-side encryption, this is the raw key, encoded in hexadecimal")
.required(false)
Copy link
Copy Markdown
Contributor

@exceptionfactory exceptionfactory May 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.required(false)
.required(true)

...ocessors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureBlobStorage_v12.java Outdated
Comment on lines +133 to +134
CSE_KEY_ID,
CSE_KEY_TYPE,
Copy link
Copy Markdown
Contributor

@exceptionfactory exceptionfactory May 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Key Type should be listed before the Key ID:

Suggested change
CSE_KEY_ID,
CSE_KEY_TYPE,
CSE_KEY_TYPE,
CSE_KEY_ID,

...essors/src/main/java/org/apache/nifi/processors/azure/storage/FetchAzureBlobStorage_v12.java Outdated
Comment on lines +123 to +124
CSE_KEY_ID,
CSE_KEY_TYPE,
Copy link
Copy Markdown
Contributor

@exceptionfactory exceptionfactory May 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
CSE_KEY_ID,
CSE_KEY_TYPE,
CSE_KEY_TYPE,
CSE_KEY_ID,

@exceptionfactory
Copy link
Copy Markdown
Contributor

exceptionfactory commented May 1, 2023

@mkalavala I also cherry-picked the fix from NIFI-11475 on to the pull request branch to correct a missing runtime dependency on jackson-dataformat-xml, which is already corrected in the main branch.

exceptionfactory
exceptionfactory approved these changes May 3, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @mkalavala, the latest version looks good! +1 merging

exceptionfactory pushed a commit that referenced this pull request May 3, 2023
@mkalavala @exceptionfactory
NIFI-11360 Added Client-Side Encryption for Azure Blob v12 Processors
66b2e2f 
This closes #7182

Signed-off-by: David Handermann <exceptionfactory@apache.org>
(cherry picked from commit aad7b40)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@exceptionfactory exceptionfactory exceptionfactory approved these changes

@nandorsoma nandorsoma Awaiting requested review from nandorsoma

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mkalavala @exceptionfactory @turcsanyip