-
Notifications
You must be signed in to change notification settings - Fork 19
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
Outline Proof Correctness Message Body #237
Conversation
Includes all requirements, expected function definitions. Avoids arithmetic problems in EncryptMessageBody method
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Great job so far interpreting the spec! The one big issue that needs to be fixed is the fact that frameLength isn't the length of the serialized frame (I know, bad name), but instead the length of the plaintext that is encrypted by that frame. That has some implications on what we are able to verify here because we can't assume that the encrypted content will have the same length as the plaintext.
If there is anywhere in the spec that is misleading on that point, please point it out so we can fix it.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Export does not support exporting predicates from datatypes. This will cause errors if the methods are called outside of the module.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
feat: Added draft of postconditions DecryptFrame
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
fix: Finished the proof of DecryptFrame method
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
feat: Proved all post conditions of MessageBody
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
feat: Small strengtening encrypt post conditions
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
fix: Moved EncryptMock and DecryptMock yo AESEncryption
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
fix:Added export back
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Finished review of Frame Decrypt operations.
Good job so far! The major things are I think we can still improve on readability by thinking about the names of ghosts a bit more. Finally, I just noticed that we are lacking unit tests here. Since this PR will change behavior (even if very slightly), it shouldn't go in until we have the framed encrypt/decrypt behavior covered by unit tests. I will ask team how we want to go forward with this.
return Success(plaintext); | ||
} | ||
|
||
datatype FrameWithGhostSeq = FrameWithGhostSeq(frame: Frame, ghost ciphertext: seq<uint8>) |
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.
Creating a new datatype to pair these together makes sense, but we should think about the name a bit. This is a thing that contains the ghost Frame representation of the data, and then that same data, but serialized and compiled into a seq. @RustanLeino Is there any idiom for naming a thing which is paired with some ghost representation of itself?
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.
There's no such naming convention that I know of. Whatever the name, we could add such a type to the standard library, in a generic way. For example:
datatype ExtendWithGhost<A, B> = ExtendWithGhost(a: A, ghost b: B)
The use of this type in Wouter's code gives us a way to return some ghost value along with the result. With language support, a better solution would be for Dafny to allow :-
to be used with more than one out-parameter. I just submitted an Issue to request such a feature, dafny-lang/dafny#603.
src/SDK/MessageBody.dfy
Outdated
var decryptedFrame := frameWithGhostSeq.frame; | ||
var ciphertext := frameWithGhostSeq.ciphertext; | ||
var final := decryptedFrame.FinalFrameConstructor?; |
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 would expect these to line up with the &&
s. @RustanLeino do you have preferences on this styling?
chore: Added extra comments chore: General refactoring
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
file .vs/EncryptionSDK/v16/.suo should be removed from this PR
and one small fix
This reverts commit 5b3959b.
Chore: Updated comments and removed unwanted commit Chore: Changed sloppy error message removed unwanted file changes
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
lgtm! Thanks for the great work here. Make sure that @acioc takes a look at the updated documentation/comments and his concerns are addressed before merging.
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.
Overall, looks good. A few np's but blocking change is the expect
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
LGTM! Thanks!
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
feat(MessageBody): Added outline proving correctness EncryptMessageBody body. Includes all requirements, expected function definitions. Avoids arithmetic problems in EncryptMessageBody method