Skip to content

Proposals for traits for symmetric ciphers#13

Open
ounsworth wants to merge 2 commits into
bcgit:release/0.1.2alphafrom
ounsworth:feature/mikeo/sym_cipher_traits
Open

Proposals for traits for symmetric ciphers#13
ounsworth wants to merge 2 commits into
bcgit:release/0.1.2alphafrom
ounsworth:feature/mikeo/sym_cipher_traits

Conversation

@ounsworth
Copy link
Copy Markdown
Contributor

No description provided.

Comment thread crypto/core-test-framework/src/symmetric_ciphers.rs
SecurityStrength::_256bit,
];
for ss in security_strengths.iter() {
key.set_security_strength(ss.clone()).unwrap();
Copy link
Copy Markdown

@officialfrancismendoza officialfrancismendoza Jun 3, 2026

Choose a reason for hiding this comment

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

Confused: for security strength tests, the loop mutates key, but calls &mac_key? Failure here may be due to wrong key type rather than weak strength as intending.

Comment thread crypto/core-test-framework/src/symmetric_ciphers.rs
Comment thread crypto/core-test-framework/src/symmetric_ciphers.rs
C::aead_encrypt_out(&key, aad, msg, &mut ct).unwrap();
let (nonce2, _ct_bytes_written, _tag) =
C::aead_encrypt_out(&key, aad, msg, &mut ct).unwrap();
assert_ne!(nonce1, nonce2);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Wouldn't this fail for nonce.len() != 0l which was declared earlier?

Comment thread crypto/core/src/traits.rs
ciphertext: &mut [u8],
) -> Result<([u8; INIT_DATA_LEN], usize), SymmetricCipherError>;
#[cfg(std)]
/// A one-shot API to decrypt some ciphertext with the given key.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why does decrypt have &self and no init data? It's inconsistent with encrypt, which is static. If we want a one-shot decrypt, shouldn't we also have decrypt as a static?

Comment thread crypto/core/src/traits.rs
SymmetricCipher<KEY_LEN, NONCE_LEN> + Sized
{
#[cfg(std)]
/// A one-shot API to encrypt some plaintext with the given key.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For a secure one-shot API, why are we taking in a manual nonce? Shouldn't it be automatic? Or at least manual nonce generation API separated/marked hazardous?

Comment thread crypto/core-test-framework/src/symmetric_ciphers.rs
Copy link
Copy Markdown

@officialfrancismendoza officialfrancismendoza left a comment

Choose a reason for hiding this comment

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

Wanted to check possible bugs in tests and the AEAD tampering test potentially allowing successful corrupted decrypts.

@officialfrancismendoza
Copy link
Copy Markdown

@ounsworth merge conflict in QUALITY_AND_STYLE.md. Also had a few unresolved comments about presumably mismatched key usage (ie: mac_key instead of key and vice versa), permuting over a different key, etc.

Comment thread crypto/core/src/traits.rs
}

pub trait Hash : Default {
/// The basic one-shat encrypt and decrypt that all types of symmetric ciphers must implement.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Funny typo ? Or intentionally sassy?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

lol

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.

3 participants