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.

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

[#105] Fixes decryption of data with size 0 bytes and 1 byte #108

Merged
merged 5 commits into from
Jan 24, 2020

Conversation

clintfred
Copy link
Contributor

@clintfred clintfred commented Jan 23, 2020

See #105

Decryption had a check to explicitly disallow "empty" encrypted data. The check was also mistakenly disallowing 1 byte. Both restrictions were relaxed and tests introduced.

src/internal/document_api/mod.rs Outdated Show resolved Hide resolved
@@ -302,6 +303,33 @@ mod tests {
assert_eq!(*decrypted_plaintext, plaintext[..]);
}

// Even very small documents of 0 and 1 bytes should roundtrip between AesEncryptedValue and bytes
#[test]
fn test_roundtrip_aesencryptedvalue_zero_one_bytes() -> Result<(), IronOxideErr> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test name seems like it could be improved or the test changed because you're not really doing a "roundtrip" test right? I would expect a roundtrip to do an encrypt and decrypt and assert you got the same value out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intention is that the AesEncryptedValue is roundtripping to/from a bytes representation. This is narrower than a full AES encrypt/decrypt. I only included the encrypt step to avoid referencing a the AES_IV_LEN and AES_GCM_TAG_LEN consts. That was probably a bad idea. I have reworked this test to remove encrypt so hopefully it's more clear what's being tested.

updated: doc_meta.0.updated,
decrypted_data: decrypted_doc.to_vec(),
}
})?,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, is this a more Rust-preferred way of doing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would say yes, so long as you are just calling into() on an error and not inspecting the error at all.

@clintfred clintfred merged commit 78cf22d into master Jan 24, 2020
@clintfred clintfred deleted the bug-encrypt-empty-bytes branch January 24, 2020 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants