test(Dafny): BKSA HV1 w/ Emptyish EC fails Mut to HV2 #1542
Conversation
texastony
changed the title
texastony
force-pushed
the
texastony/hv-2/hv-2-test-migration-empty-ec
branch
from
ac6f03f to
6ba3279
Compare
|
What if you change this code to |
rishav-karanjit
left a comment
rishav-karanjit
left a comment
There was a problem hiding this comment.
I have a blocking comment and a question. All other looks good.
| encryptionContext = Collections.unmodifiableMap(_encryptionContext); | ||
| } | ||
|
|
||
| protected String createHV1WithAnEmptyEncryptionContextKey() { |
There was a problem hiding this comment.
| protected String createHV1WithAnEmptyEncryptionContextKey() { | |
| private String createHV1WithAnEmptyEncryptionContextKey() { |
| return testBranchKeyId; | ||
| } | ||
|
|
||
| protected void checkBranchKey(final String branchKeyId) { |
There was a problem hiding this comment.
| protected void checkBranchKey(final String branchKeyId) { | |
| private void checkBranchKey(final String branchKeyId) { |
| ); | ||
| } | ||
|
|
||
| protected void mutateToHV2(final String branchKeyId) { |
There was a problem hiding this comment.
| protected void mutateToHV2(final String branchKeyId) { | |
| private void mutateToHV2(final String branchKeyId) { |
There was a problem hiding this comment.
I made this protected, because I am thinking about creating a super class for these sorts of tests that are of the pattern:
- create BK
- validate created BK
- Mutate BK, maybe handle exception
- validate mutated BK
- clean up
| const MD_DIGEST_SHA_NOT_MATCHED := | ||
| "This branch key item has failed the authentication check. Either it has been tampered with or the wrong 'Logical Key Store Name' has been provided." | ||
|
|
||
| const INVALID_EC_FOUND := |
There was a problem hiding this comment.
Let's add a TODO for this for hv-2 fast follow. I don't want us to forget about rejecting empty key and I think customer should be getting a KMS error instead of our error.
There was a problem hiding this comment.
They cannot get a KMS Error unless we actually call KMS.
And that KMS Call may be hard to construct with our Dafny tooling... I am not sure.
I am also not certain that the KMS exception will be well understood by a customer going from HV-1 to HV-2.
But I can take a TODO; we can then evaluate if this is a launch blocker in an ORR.
There was a problem hiding this comment.
Yes, having TODO and then evaluating in ORR is fine.
I am also not sure about the KMS call with empty EC in Dafny. The reason why I like to give them KMS error is because let's say KMS changes and start accepting empty EC after few years, at that time no one in the team will realize that we are rejecting empty EC saying KMS will reject it until someone works on this code space or a user complains. Returning KMS error instead of our error makes me feel like the code that we write will survive for next 5 years.
There was a problem hiding this comment.
I hear you, but our customers will need to understand why the KMS error was thrown.
On Branch Key creation, this is trivial.
But on Mutation, particularly one that changes HV, I fear that customers will not be able to relate the KMS error to the root cause.
2 participants
Issue #, if available:
Description of changes:
Refactor the Create Branch Key example to consume Nullable Encryption Context
Error message details Invalid EC for HV-2 can be due to Emptyish OR Modified ECJava Integ Test that ensures HV-1 with emptyish EC keys cannot be mutated to HV-2
Squash/merge commit message, if applicable:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.