Skip to content

[CDAP-20872] Add AeadCipher SPI for Encryption in CDAP#15425

Merged
dli357 merged 1 commit intodevelopfrom
feature/CDAP-20648-cipher
Nov 18, 2023
Merged

[CDAP-20872] Add AeadCipher SPI for Encryption in CDAP#15425
dli357 merged 1 commit intodevelopfrom
feature/CDAP-20648-cipher

Conversation

@dli357
Copy link
Copy Markdown
Contributor

@dli357 dli357 commented Nov 13, 2023

This PR adds a new SPI for supporting encryption using Tink. The following encryption implementations are supported:

  • Tink Cleartext: Uses a file-based Tink key read from SConfiguration.
  • Tink GCP Envelope: Uses Tink client-side envelope encryption with GCP Cloud KMS as the key management service.

Additionally, the PR also uses the new encryption SPI in the following ways:

  • Adds data storage encryption to CredentialIdentityStore and CredentialProfileStore
  • Refactors the existing user credential encryption in AuthenticationHandler and NettyRouter to use the new SPI

Finally, this PR also removes the Tink dependency from cdap-security, along with protobuf and tink shading.

@dli357 dli357 added build Triggers github actions build 6.10 credential_provider Label for tracking the credential provider feature. namespaced_service_accounts Label for tracking namespaced service accounts feature labels Nov 13, 2023
@dli357 dli357 self-assigned this Nov 13, 2023
@dli357 dli357 force-pushed the feature/CDAP-20648-cipher branch 9 times, most recently from 693c7a5 to ecb7d92 Compare November 14, 2023 19:11
@dli357 dli357 requested review from chtyim, rmstar and tivv November 14, 2023 19:22
albertshau
albertshau reviewed Nov 16, 2023
View reviewed changes
Comment thread cdap-encryption-ext-tink/src/main/java/io/cdap/cdap/encryption/tink/CloudKmsClient.java Outdated
Comment thread cdap-encryption-ext-tink/src/main/java/io/cdap/cdap/encryption/tink/CloudKmsClient.java Outdated
Comment thread cdap-encryption-ext-tink/src/main/java/io/cdap/cdap/encryption/tink/CloudKmsClient.java Outdated
Comment thread cdap-encryption-ext-tink/src/main/java/io/cdap/cdap/encryption/tink/CloudKmsClient.java Outdated
Comment thread cdap-security/src/main/java/io/cdap/cdap/security/encryption/NoOpAeadCipher.java Outdated
Comment thread ...-app-fabric/src/main/java/io/cdap/cdap/internal/credential/store/CredentialProfileStore.java Outdated
chtyim
chtyim reviewed Nov 16, 2023
View reviewed changes
Comment thread cdap-security/src/main/java/io/cdap/cdap/security/encryption/AeadCipher.java
chtyim
chtyim reviewed Nov 16, 2023
View reviewed changes
Comment thread cdap-security/src/main/java/io/cdap/cdap/security/encryption/AeadCipher.java Outdated
@dli357 dli357 force-pushed the feature/CDAP-20648-cipher branch 2 times, most recently from b2f31c1 to df4ae52 Compare November 16, 2023 04:37
@dli357 dli357 requested a review from chtyim November 16, 2023 22:18
@dli357 dli357 requested a review from albertshau November 16, 2023 22:18
chtyim
chtyim reviewed Nov 16, 2023
View reviewed changes
Comment thread cdap-app-fabric/src/main/java/io/cdap/cdap/internal/credential/CredentialIdentityStore.java Outdated
chtyim
chtyim reviewed Nov 16, 2023
View reviewed changes
Comment thread cdap-app-fabric/src/main/java/io/cdap/cdap/internal/credential/CredentialProfileStore.java Outdated
chtyim
chtyim reviewed Nov 16, 2023
View reviewed changes
Comment thread cdap-encryption-ext-tink/pom.xml Outdated
chtyim
chtyim approved these changes Nov 18, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@chtyim chtyim left a comment

Choose a reason for hiding this comment

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

LGTM, just couple minor comments left.

@dli357
[CDAP-20872] Create AeadCipher SPI for supporting extensible encrypti…
61df5b6 
…on in CDAP

[CDAP-20872] Add data encryption for CredentialIdentityStore and CredentialProfileStore

[CDAP-20872] Refactor user credential encryption to use new SPI

[CDAP-20872] Remove Tink dependency from cdap-security

[CDAP-20872] Addressed comments, renamed AeadCipher SPI to AeadCipherCryptor

[CDAP-20872] Change CipherException to extend RuntimeException and address comments

[CDAP-20872] Fix checktstyle
@dli357 dli357 force-pushed the feature/CDAP-20648-cipher branch from 628ccea to 61df5b6 Compare November 18, 2023 01:57
@dli357
Copy link
Copy Markdown
Contributor Author

dli357 commented Nov 18, 2023

Thank you for the reviews!

@dli357 dli357 merged commit 9a89c3e into develop Nov 18, 2023
@dli357 dli357 deleted the feature/CDAP-20648-cipher branch November 18, 2023 20:19
@dli357 dli357 mentioned this pull request Nov 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chtyim chtyim chtyim approved these changes

@itsankit-google itsankit-google Awaiting requested review from itsankit-google

@tivv tivv Awaiting requested review from tivv

@rmstar rmstar Awaiting requested review from rmstar

@albertshau albertshau Awaiting requested review from albertshau

Assignees

@dli357 dli357

Labels

6.10 build Triggers github actions build credential_provider Label for tracking the credential provider feature. namespaced_service_accounts Label for tracking namespaced service accounts feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dli357 @chtyim @albertshau