Skip to content

Replace hard-coded GCM tag length with named constant #13162

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

Closed
wants to merge 2 commits into from
Closed

Replace hard-coded GCM tag length with named constant #13162

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e505c4
Replace hard-coded GCM tag length with named constant
Oblivionsage Jul 10, 2025
1a10ce0
Replace hard-coded GCM tag length with named constant
Oblivionsage Jul 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions src/rust/src/backend/ciphers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ use pyo3::IntoPyObject;

use crate::backend::cipher_registry;
use crate::buf::{CffiBuf, CffiMutBuf};

// GCM authentication tag length: 16 bytes (128 bits) provides maximum security
// as recommended by NIST SP 800-38D. Smaller sizes (down to 12 bytes) are also
// acceptable but 16 bytes is the standard for this implementation.
const GCM_STANDARD_TAG_SIZE: usize = 16;

use crate::error::{CryptographyError, CryptographyResult};
use crate::{exceptions, types};

Expand Down Expand Up @@ -358,8 +364,8 @@ impl PyAEADEncryptionContext {
let ctx = get_mut_ctx(self.ctx.as_mut())?;
let result = ctx.finalize(py)?;

// XXX: do not hard code 16
let tag = pyo3::types::PyBytes::new_with(py, 16, |t| {
// Allocate buffer for GCM tag
let tag = pyo3::types::PyBytes::new_with(py, GCM_STANDARD_TAG_SIZE, |t| {
Comment on lines -361 to +368
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, but this isn't actually addressing the comment: the comment is saying that the PyAEADEncryptionContext should work for other tag sizes.

Copy link
Author

Choose a reason for hiding this comment

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

@alex Thanks for the feedback I focused too much on eliminating the hard-coded value and overlooked the broader intent of supporting variable tag sizes

I'm happy to work on a more comprehensive solution, but I'd love some guidance on the approach you'd prefer should I:

  • Research the tag size requirements for different AEAD algorithms(ChaCha20-Poly1305, AES-OCB, etc.) and make the allocation dynamic?
  • Or would it be better to revert this and tackle the broader architectural change in a separate discussion first?

This is a great learning experience for me thanks for pointing me in the right direction

Copy link
Member

Choose a reason for hiding this comment

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

I think it may just need to check against https://docs.rs/openssl/latest/openssl/cipher_ctx/struct.CipherCtx.html#method.tag_length

ctx.ctx.tag(t).map_err(CryptographyError::from)?;
Ok(())
})?;
Expand Down Expand Up @@ -491,17 +497,20 @@ impl PyAEADDecryptionContext {
.bind(py)
.getattr(pyo3::intern!(py, "_min_tag_length"))?
.extract()?;
// XXX: Do not hard code 16
// Validate tag length against GCM standards
if tag.len() < min_tag_length {
return Err(CryptographyError::from(
pyo3::exceptions::PyValueError::new_err(format!(
"Authentication tag must be {min_tag_length} bytes or longer.",
)),
));
} else if tag.len() > 16 {
} else if tag.len() > GCM_STANDARD_TAG_SIZE {
return Err(CryptographyError::from(
pyo3::exceptions::PyValueError::new_err(
"Authentication tag cannot be more than 16 bytes.",
// Defensive error handling - rarely triggered in normal usage
format!(
"Authentication tag cannot be more than {GCM_STANDARD_TAG_SIZE} bytes."
),
),
));
}
Expand Down