[sync] Introduce AgileSymmetricKeySet [chromium/src : main]
Maksim Moskvitin (Gerrit)
Maksim Moskvitin added 6 comments![]( "Open in Gerrit")
continue;It feels a bit odd that we tolerate corrupted key, but not duplicates and the rest of violations below.
if (proto.primary_key_id() == 0 ||Maybe check has_primary_key_id() instead and avoid contract (id == 0 -> id is empty)?
Doesn't have to match each other, but related: primary_key_id_ could be optional instead of special casing 0.
std::vector<uint8_t> raw_ciphertext = primary_key.Encrypt(plaintext);nit: const?
// 1. Try modern path if modern blob is present.Looks like a good way to split this function (DecryptModern() vs DecryptLegacy()).
if (encrypted_data.has_key_id_v2()) {Looks a bit asymmetric: we don't require `key_id_v2`, but we do require it to be correct if preset. Is there any legit case, when `key_id_v2` is missing?
Related details
- Mikel Astiz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikel Astiz (Gerrit)
Mikel Astiz added 6 comments![]( "Open in Gerrit")
Thanks, PTAL! I only tackled some comments as others warrant some discussion.
continue;It feels a bit odd that we tolerate corrupted key, but not duplicates and the rest of violations below.
Two scenarios that concern me a bit are:
1) Unsupported future algorithms or fields.
2) No-longer-supported algorithms.
As long as they are not the primary key, it seems sensible to tolerate them.
Other scenarios like ID duplicates are just malformed data, so I don't see any reasonable way to recover from that.
if (proto.primary_key_id() == 0 ||Maybe check has_primary_key_id() instead and avoid contract (id == 0 -> id is empty)?
Doesn't have to match each other, but related: primary_key_id_ could be optional instead of special casing 0.
I don't think we should allow zero as key ID because it is most likely malformed data / a bug. See also how `RotatePrimaryToNewlyGeneratedRandomKey()` would avoid zero. From this PoV I think it's best to compare against zero here too. WDYT?
std::vector<uint8_t> raw_ciphertext = primary_key.Encrypt(plaintext);Mikel Astiznit: const?
Done
Looks like a good way to split this function (DecryptModern() vs DecryptLegacy()).
Done
if (encrypted_data.has_key_id_v2()) {Looks a bit asymmetric: we don't require `key_id_v2`, but we do require it to be correct if preset. Is there any legit case, when `key_id_v2` is missing?
There is a specific unit-test about the `key_id_v2` missing case. This follows Tink's approach, in that the ID is only an optimization and not required for decryption, with a fallback that tries all keys. It seems most sensible (future-proof) to do the same here.
Related details
- Maksim Moskvitin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Moskvitin (Gerrit)
Maksim Moskvitin voted and added 4 comments![]( "Open in Gerrit")
Votes added by Maksim Moskvitin
| Code-Review | +1 |
4 comments
Some additions, but LGTM now given the upcoming OOO, I trust your judgement on this!
continue;Mikel AstizIt feels a bit odd that we tolerate corrupted key, but not duplicates and the rest of violations below.
Two scenarios that concern me a bit are:
1) Unsupported future algorithms or fields.
2) No-longer-supported algorithms.As long as they are not the primary key, it seems sensible to tolerate them.
Other scenarios like ID duplicates are just malformed data, so I don't see any reasonable way to recover from that.
Ack, makes sense. Worth documenting though.
if (proto.primary_key_id() == 0 ||Mikel AstizMaybe check has_primary_key_id() instead and avoid contract (id == 0 -> id is empty)?
Doesn't have to match each other, but related: primary_key_id_ could be optional instead of special casing 0.
I don't think we should allow zero as key ID because it is most likely malformed data / a bug. See also how `RotatePrimaryToNewlyGeneratedRandomKey()` would avoid zero. From this PoV I think it's best to compare against zero here too. WDYT?
Partially agreed, banning zero makes sense to me, at the same time I still prefer using more elaborative has_() for proto and optional for class field.
Either way, please document this special case in proto file and around the class member.
if (encrypted_data.has_key_id_v2()) {Mikel AstizLooks a bit asymmetric: we don't require `key_id_v2`, but we do require it to be correct if preset. Is there any legit case, when `key_id_v2` is missing?
There is a specific unit-test about the `key_id_v2` missing case. This follows Tink's approach, in that the ID is only an optimization and not required for decryption, with a fallback that tries all keys. It seems most sensible (future-proof) to do the same here.
- Mikel Astiz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikel Astiz (Gerrit)
Mikel Astiz added 3 comments![]( "Open in Gerrit")
I'm not sure why but your +1 was lost :-/
continue;Mikel AstizIt feels a bit odd that we tolerate corrupted key, but not duplicates and the rest of violations below.
Maksim MoskvitinTwo scenarios that concern me a bit are:
1) Unsupported future algorithms or fields.
2) No-longer-supported algorithms.As long as they are not the primary key, it seems sensible to tolerate them.
Other scenarios like ID duplicates are just malformed data, so I don't see any reasonable way to recover from that.
Ack, makes sense. Worth documenting though.
Added comments.
if (proto.primary_key_id() == 0 ||Mikel AstizMaybe check has_primary_key_id() instead and avoid contract (id == 0 -> id is empty)?
Doesn't have to match each other, but related: primary_key_id_ could be optional instead of special casing 0.
Maksim MoskvitinI don't think we should allow zero as key ID because it is most likely malformed data / a bug. See also how `RotatePrimaryToNewlyGeneratedRandomKey()` would avoid zero. From this PoV I think it's best to compare against zero here too. WDYT?
Partially agreed, banning zero makes sense to me, at the same time I still prefer using more elaborative has_() for proto and optional for class field.
Either way, please document this special case in proto file and around the class member.
Added a comment and sticking to current approach, as it seems like a optional request. Thanks!
Related details
- Maksim Moskvitin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Moskvitin (Gerrit)
Maksim Moskvitin voted Code-Review+1![]( "Open in Gerrit")
- Mikel Astiz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikel Astiz (Gerrit)
Mikel Astiz added 1 comment![]( "Open in Gerrit")
+treib@ after maksim's LGTM got lost during rebase, thanks.
Related details
- Maksim Moskvitin
- Marc Treib
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |