Fixed issue with encrypting and decrypting maps #12
Conversation
|
Added change to map handling so the same map is used in encryption and decryption. This causes the original argument to the method to be modified and is not ideal. I would prefer to see 16d1d1f reverted once we find a better way to work with DynamoDBMapper. |
…o avoid encapsulation issue for now.
|
I don't really understand the described bug right now. My read of the code says that maps are correctly being encrypted and the result is ByteBuffer (which is correct). If there is a bug there, we can fix it. However, we should not be exposing as plaintext any internal information about these maps including the key names or the number of elements. Can you provide me with information to reproduce the reported bug? (Perhaps in the form of a unit test which currently fails but shouldn't? BTW, the provided unit tests still don't work. I'll add below a patch to AttrMatcher will will be checked in later for you to use.) Expected (and correct) behavior:
public static boolean attrEquals(AttributeValue e, AttributeValue a) {
if (!isEqual(e.getB(), a.getB()) ||
!isEqual(e.getN(), a.getN()) ||
!isEqual(e.getS(), a.getS()) ||
!isEqual(e.getBS(), a.getBS()) ||
!isEqual(e.getNS(), a.getNS()) ||
!isEqual(e.getSS(), a.getSS()) ||
!isEqual(e.getBOOL(), a.getBOOL()) ||
!isLEqual(e.getL(), a.getL()) ||
!isMEqual(e.getM(), a.getM()) ||
!isEqual(e.getNULL(), a.getNULL())) {
return false;
}
return true;
}
private static boolean isMEqual(Map<String, AttributeValue> e, Map<String, AttributeValue> a) {
if (e == null && a == null) {
return true;
} else if (e == null || a == null) {
return false;
} else if (e.size() != a.size()) {
return false;
}
return isEqual(e.entrySet(), a.entrySet());
}
private static boolean isLEqual(List<AttributeValue> e, List<AttributeValue> a) {
if (e == null && a == null) {
return true;
} else if (e == null || a == null) {
return false;
} else if (e.size() != a.size()) {
return false;
}
for (int x = 0; x < e.size(); x++) {
if (!attrEquals(e.get(x), a.get(x))) {
return false;
}
}
return true;
} |
|
Sorry for the lack of details. I'll work on providing a better description and more details on the issue I was experiencing so we can sort out if this a bug or just an issue specific to my application. Thanks for your patience. |
|
Thanks. That said, I do need to add better tests around these new data types. The few that I added in investigating this are all passing right now, which just adds to my confusion. |
|
My use case is to preserve the map structure and encrypt each field. So I think we have different goals in mind. Also, you're right, I don't think there is an issue with this encryption library. The problem is with the way it is interacting with the DynamoDBMapper in the current version of aws-java-sdk-dynamodb. Before my commits, this was the error that occurred when I attempted to save an entity containing a map. For reference, I was attempting to persist the following entity. @DynamoDBTable(tableName="entity")
public class Entity {
@DynamoDBHashKey
private String id;
private Settings settings;
public String getId() {
return id;
}
public void setId(String id) {
this.id = id;
}
public Settings getSettings() {
return settings;
}
public void setSettings(Settings settings) {
this.settings = settings;
}
}@DynamoDBDocument
public class Settings {
private String string1;
private String string2;
public String getString1() {
return string1;
}
public void setString1(String string1) {
this.string1 = string1;
}
public String getString2() {
return string2;
}
public void setString2(String string2) {
this.string2 = string2;
}
}The transformAttributeUpdates() method in the DynamoDBMapper class relies on the scanned getters of the entity class to know which fields to persist on an update. This causes the map field to be populated as the update request is being generated. By the time transformAttributeUpdates() returns, the settings attribute has both a B value and M value which triggers the exception. I don't know an easy way to solve that problem while also encrypting the entire map (keys and values). Since that is your intended functionality, I'm closing my pull request and will use my fork for my application. Let me know if I can provide any more information about the issue that I had. Thanks again for your help! |
|
Hmm, that is an interesting case. While we may disagree about the serialization, the library should definitely be able to handle that. I'll create an issue and try to get it fixed. Thanks for pointing it out. |
|
I am looking for a resolution for this issue as well. We make use of DynamoDBDocument quite a bit and switching over to a custom marshaller for those objects just to make the library work is awkward. Does the proposed approach need some rework? Is there an alternative that better aligns with the serialization design of the library? |
|
We've got a fix rolling out soon for this. (Though I can't predict the actual timing.) The source of this bug was actually inside DynamoDBMarshaller which made it take a bit longer to track down and fix. |
|
Excellent news! Do you have a link to the issue/ticket for the problem in DynamoDBMarshaller? I would really like to include that fix in the near term so I want to lend a +1 to it or whatever magic I can to help get it fixed up 😄 |
|
@richardwalsh Fixed as of 1.11.0 |
|
Fantastic, thanks! |
Previously maps would contain both an encrypted B-value and a M-value which contained attributes with their own encrypted values. This fixes the encryption and decryption methods so they preserve the map elements, but the map does not have it's own B-value.