Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adding a validate version error handling as discussed in issue #113 #116

Merged
merged 14 commits into from
Aug 16, 2019
Merged

Adding a validate version error handling as discussed in issue #113 #116

merged 14 commits into from
Aug 16, 2019

Conversation

josecorella
Copy link
Contributor

Issue #, if available:

Description of changes:

Issue #113
Added the version error handling.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

lizroth
lizroth reviewed Jul 9, 2019
View reviewed changes
Copy link
Contributor

@lizroth lizroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look! A couple of comments so far.

aws-encryption-sdk-java.iml Outdated
@@ -0,0 +1,215 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably don't want to commit this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure where this file came from, but if this is an IDE/build artifact, let's also add it to the .gitignore.

src/main/java/com/amazonaws/encryptionsdk/model/CiphertextHeaders.java Outdated
@@ -179,6 +179,9 @@ public Boolean isComplete() {
*/
private int parseVersion(final byte[] b, final int off) throws ParseException {
version_ = PrimitivesParser.parseByte(b, off);
if(CiphertextType.deserialize(version_) == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some tests

mattsb42-aws
mattsb42-aws reviewed Jul 9, 2019
View reviewed changes
src/main/java/com/amazonaws/encryptionsdk/model/CiphertextHeaders.java Outdated
@@ -179,6 +179,9 @@ public Boolean isComplete() {
*/
private int parseVersion(final byte[] b, final int off) throws ParseException {
version_ = PrimitivesParser.parseByte(b, off);
if(CiphertextType.deserialize(version_) == null) {
Copy link
Member

@mattsb42-aws mattsb42-aws Jul 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the correct check.

We currently only have one valid version: (hex) 01. Any other value MUST throw an error.

mattsb42-aws
mattsb42-aws previously requested changes Jul 12, 2019
View reviewed changes
Copy link
Member

@mattsb42-aws mattsb42-aws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we need tests that actually trigger this failure mode.

.gitignore Outdated
@@ -5,3 +5,4 @@ target/
.classpath
/bin/
.idea/
*.iml/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good, but aws-encryption-sdk-java.iml is still in the diff.

src/main/java/com/amazonaws/encryptionsdk/model/CiphertextHeaders.java Outdated
@@ -179,6 +180,9 @@ public Boolean isComplete() {
*/
private int parseVersion(final byte[] b, final int off) throws ParseException {
version_ = PrimitivesParser.parseByte(b, off);
if(CiphertextType.deserialize(version_).getValue() != Byte.parseByte(VersionInfo.VERSION_NUM, 16)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the major version of this implementation, not the ciphertext message format version. You want CURRENT_CIPHERTEXT_VERSION.

Suggested change
if(CiphertextType.deserialize(version_).getValue() != Byte.parseByte(VersionInfo.VERSION_NUM, 16)) {
if(CiphertextType.deserialize(version_).getValue() != Byte.parseByte(VersionInfo.CURRENT_CIPHERTEXT_VERSION, 16)) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please actually change as follows: (note lack of parsing and space after if

         if (CiphertextType.deserialize(version_).getValue() != VersionInfo.CURRENT_CIPHERTEXT_VERSION) {

SalusaSecondus
SalusaSecondus suggested changes Jul 16, 2019
View reviewed changes
src/main/java/com/amazonaws/encryptionsdk/model/CiphertextHeaders.java Outdated
@@ -179,6 +180,9 @@ public Boolean isComplete() {
*/
private int parseVersion(final byte[] b, final int off) throws ParseException {
version_ = PrimitivesParser.parseByte(b, off);
if(CiphertextType.deserialize(version_).getValue() != Byte.parseByte(VersionInfo.VERSION_NUM, 16)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please actually change as follows: (note lack of parsing and space after if

         if (CiphertextType.deserialize(version_).getValue() != VersionInfo.CURRENT_CIPHERTEXT_VERSION) {

src/main/java/com/amazonaws/encryptionsdk/model/CiphertextHeaders.java Outdated
@@ -179,6 +180,9 @@ public Boolean isComplete() {
*/
private int parseVersion(final byte[] b, final int off) throws ParseException {
version_ = PrimitivesParser.parseByte(b, off);
if(CiphertextType.deserialize(version_).getValue() != VersionInfo.CURRENT_CIPHERTEXT_VERSION ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our style requires a space between the if and before the (.

SalusaSecondus
SalusaSecondus previously approved these changes Jul 16, 2019
View reviewed changes
Copy link
Contributor

@SalusaSecondus SalusaSecondus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@josecorella
There was a bug in the parseVersion code. Before it was converting th…
444146a 
…e version to a ciphertext type and deserialzing it

when all it needed to do was compare the bytes.
@josecorella
There was a bug with the test I wrote. With the conditions that I pas…
0a4653e 
…sed to parseVersion it would never throw the error

because the value was the same as CURRENT_CIPHERTEXT_VERSION. The test would pass but the error would not be thrown.
It is now fixed and it throws the right error when it receives a wrong version.
SalusaSecondus
SalusaSecondus previously approved these changes Jul 25, 2019
View reviewed changes
@josecorella
Modified my test in the invalidVersion Test. Before it would overwrit…
8be7db8 
…e the byte that didn't contain the version type.

However, test still passed because the previous version of the test would offset to where the byte was changed. That has been fixed and the test now overrides the byte that holds the version number.
@lizroth lizroth dismissed mattsb42-aws’s stale review August 15, 2019 23:45

Requested test has been added and requested constant has been used. Matt is on vacation.

@lizroth lizroth merged commit 325bf21 into aws:master Aug 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lizroth lizroth lizroth approved these changes

@SalusaSecondus SalusaSecondus SalusaSecondus left review comments

@mattsb42-aws mattsb42-aws mattsb42-aws left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@josecorella @SalusaSecondus @mattsb42-aws @lizroth