Skip to content

Field Level Encryption Support. #1546

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 29, 2022
Merged

Field Level Encryption Support. #1546

merged 1 commit into from
Aug 29, 2022

Conversation

mikereiche
Copy link
Collaborator

Note that an @Encrypted property object that contains an @Encrypted property is
not supported and will result in an exception.

Closes #763.

  • You have read the Spring Data contribution guidelines.
  • There is a ticket in the bug tracker for the project in our JIRA.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

@mikereiche mikereiche requested review from daschl and dnault August 26, 2022 20:44
@mikereiche mikereiche force-pushed the datacouch_763_fle_support branch 3 times, most recently from 9ac4347 to 0f02d5f Compare August 26, 2022 22:11
@mikereiche
Field Level Encryption Support.
5490b21 
Note that an @Encrypted property object that contains an @Encrypted property is
not supported and will result in an exception.

Closes #763.
@mikereiche mikereiche force-pushed the datacouch_763_fle_support branch from 0f02d5f to 5490b21 Compare August 26, 2022 22:36
dnault
dnault reviewed Aug 27, 2022
View reviewed changes
.../src/main/java/org/springframework/data/couchbase/config/AbstractCouchbaseConfiguration.java
public CustomConversions customConversions() {
return new CouchbaseCustomConversions(Collections.emptyList());
public CustomConversions customConversions(CryptoManager... cryptoManagerOptional) {
assert (cryptoManagerOptional == null || cryptoManagerOptional.length <= 1);
Copy link
Contributor

@dnault dnault Aug 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What do you think of these alternatives suggested by StackOverflow users?

public CustomConversions customConversions(
    Optional<CryptoManager> cryptoManagerOptional
)

Or this:

public CustomConversions customConversions(
    @Autowired(required = false) CryptoManager cryptoManager
)

Reference: https://stackoverflow.com/questions/46704950/how-to-define-optional-parameters-dependencies-in-the-bean-method-in-spring

Copy link
Collaborator Author

@mikereiche mikereiche Aug 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The first one changes a public API so is not possible.
I have doubts about using @Autowired in the configuration class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Changing foo() to foo(T...) is a source-compatible change, but I suspect it's not binary-compatible. (Maybe binary compatibility is not a concern?)

@mikereiche mikereiche merged commit 921b8c2 into main Aug 29, 2022
dnault
dnault reviewed Aug 29, 2022
View reviewed changes
Copy link
Contributor

@dnault dnault left a comment

Choose a reason for hiding this comment

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

Looks like I missed the deadline -- sorry!

...rc/main/java/org/springframework/data/couchbase/core/convert/EncryptingWritingConverter.java
com.couchbase.client.java.encryption.annotation.Encrypted ann = sourceType
.getAnnotation(com.couchbase.client.java.encryption.annotation.Encrypted.class);
Map<Object, Object> result = new HashMap<>();
result.putAll(cryptoManager.encrypt(source.toString().getBytes(), ann.encrypter()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Specify charset?

.getBytes(StandardCharsets.UTF_8)

...rc/main/java/org/springframework/data/couchbase/core/convert/DecryptingReadingConverter.java

@Override
public Object convert(Object source, TypeDescriptor sourceType, TypeDescriptor targetType) {
return source == null? null : new String(cryptoManager.decrypt(((CouchbaseDocument) source).getContent()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Specify charset?

new String(..., StandardCharsets.UTF_8)

...rc/main/java/org/springframework/data/couchbase/core/convert/EncryptingWritingConverter.java
com.couchbase.client.java.encryption.annotation.Encrypted ann = sourceType
.getAnnotation(com.couchbase.client.java.encryption.annotation.Encrypted.class);
Map<Object, Object> result = new HashMap<>();
result.putAll(cryptoManager.encrypt(source.toString().getBytes(), ann.encrypter()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Does source.toString() always return JSON? (Asking because I'm wondering about interoperability with apps that don't use SDC)

...rc/main/java/org/springframework/data/couchbase/core/convert/EncryptingWritingConverter.java
@Override
public Object convert(Object source, TypeDescriptor sourceType, TypeDescriptor targetType) {
if (source == null) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean null values don't get encrypted? Shouldn't a JSON "null" should get encrypted like any other value?

.../src/main/java/org/springframework/data/couchbase/config/AbstractCouchbaseConfiguration.java
public CustomConversions customConversions() {
return new CouchbaseCustomConversions(Collections.emptyList());
public CustomConversions customConversions(CryptoManager... cryptoManagerOptional) {
assert (cryptoManagerOptional == null || cryptoManagerOptional.length <= 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing foo() to foo(T...) is a source-compatible change, but I suspect it's not binary-compatible. (Maybe binary compatibility is not a concern?)

mikereiche added a commit that referenced this pull request Sep 8, 2022
@mikereiche
Revert "Field Level Encryption Support. (#1546)"
c0305b3 
This reverts commit 921b8c2.

Closes #1551.
mikereiche added a commit that referenced this pull request Sep 8, 2022
@mikereiche
Revert fle using generic converter (#1552)
b7d1654 
* Revert "Fix up FLE support. (#1550)"

Closes #1551.

This reverts commit 5fcb8ed.

* Revert "Field Level Encryption Support. (#1546)"

This reverts commit 921b8c2.

Closes #1551.
@mikereiche mikereiche deleted the datacouch_763_fle_support branch February 27, 2023 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnault dnault dnault left review comments

@daschl daschl Awaiting requested review from daschl

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Field Level Encryption [DATACOUCH-455]
2 participants
@mikereiche @dnault