-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add support for explicit field encryption (sync client) #4302
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.
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
Conversation
typically only supported in automatic schema but neat to have it here as well. eg. for customer data cyper based on eg. username. Also make sure to translate decryption exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments from an initial review. I like the direction these changes took.
@Target(ElementType.FIELD) | ||
@Encrypted | ||
@ValueConverter | ||
public @interface ExplicitlyEncrypted { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe ExplicitEncrypted
and having a reference in the documentation above that points/explains the difference to automatic encryption.
* @author Christoph Strobl | ||
* @since 4.1 | ||
*/ | ||
public interface EncryptingConverter<S, T> extends MongoValueConverter<S, T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether we should split the encryption
package and move the Converter
-related bits into convert.encryption
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
valid point, we need to untangle bits here to avoid cycle between the convert
and encrypt
package. Basically we need to make sure EncryptionContext
is not a ValueConversionContext
any longer
if (BsonBinarySubType.isUuid(value.getType())) { | ||
String representation = value.asUuid().toString(); | ||
if (representation.length() > 6) { | ||
return String.format("KeyId('%s***')", representation.substring(0, 6)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the feeling that we should avoid rendering the key, even subparts in toString
. We could rather replace each character with a *
and retain the dashes if we wanted to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had looked at EncryptionKey
and KeyImpl
from javax.security.auth.kerberos
which do render the bits right away. DESKey
does not. P11Key
tells you the type and length. So I'm not sure. Maybe @rwinch has an opinion.
if (value().length() <= 3) { | ||
return "AltKeyName('***')"; | ||
} | ||
return String.format("AltKeyName('%s***')", value.substring(0, 3)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same topic about rendering the key name like KeyId.toString()
/** | ||
* @return {@literal true} if refreshed, {@literal false} otherwise. | ||
*/ | ||
public boolean refresh() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering whether we should rather show an Lazy
example for singleton usage and always call Supplier.get()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had thought about using Lazy
but decided against it to have more control over the refresh. Also calling the supplier may lead to unnecessary creation of ClientEncryption
instances that all hold connections to the database.
… generic type info loss.
Remove caching variant of MongoClientEncryption. Rename types for consistent key alt name scheme. Rename annotation to ExplicitEncrypted. Add package-info. Improve documentation wording. Reduce visibility of KeyId and KeyAltName to package-private.
We now support explicit field encryption using mapped entities through the `@ExplicitEncrypted` annotation. class Person { ObjectId id; @ExplicitEncrypted(algorithm = AEAD_AES_256_CBC_HMAC_SHA_512-Deterministic, altKeyName = "my-secret-key") String socialSecurityNumber; } Encryption is applied transparently to all mapped entities leveraging the existing converter infrastructure. Original pull request: #4302 Closes: #4284
That's merged and polished now. |
Closes: #4284