Replace hard-coded GCM tag length with named constant #13162
Conversation
- Add GCM_STANDARD_TAG_SIZE constant (16 bytes) with NIST SP 800-38D reference - Replace magic numbers in tag allocation and validation logic - Update error message to use constant value for consistency - Improves code maintainability and follows crypto best practices Tested: GCM functionality preserved, no regressions Resolves TODO comments about hard-coded GCM tag length values
- Add GCM_STANDARD_TAG_SIZE constant (16 bytes) with NIST SP 800-38D reference - Replace magic numbers in tag allocation and validation logic - Update error message with defensive handling comment - Apply Rust formatting and linting standards Tested: GCM functionality preserved, no regressions Resolves TODO comments about hard-coded GCM tag length values
Oblivionsage
force-pushed
the
fix-gcm-tag-length-constants
branch
from
29a2cc3 to
1a10ce0
Compare
| // 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| { |
There was a problem hiding this comment.
Sorry, but this isn't actually addressing the comment: the comment is saying that the PyAEADEncryptionContext should work for other tag sizes.
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
I think it may just need to check against https://docs.rs/openssl/latest/openssl/cipher_ctx/struct.CipherCtx.html#method.tag_length
|
Were you interested in continuing to work on this? |
|
@alex Yes absolutely , sorry for the delay , I'm definitely still interested in working on this I was looking into the OpenSSL |
|
Updating this PR is fine.
…On Mon, Jul 28, 2025 at 7:49 AM Oblivionsage ***@***.***> wrote:
*Oblivionsage* left a comment (pyca/cryptography#13162)
<#13162 (comment)>
@alex <https://github.com/alex> Yes, absolutely sorry for the delay , I'm
definitely still interested in working on this I was looking into the
OpenSSL CipherCtx::tag_length() approach you suggested before I implement
it, I want to make sure I understand correctly . Should I update the
existing PR or would you prefer a fresh one? I'm ready to implement this
cleaner solution.
—
Reply to this email directly, view it on GitHub
<#13162 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAGBBKQHJVXV4IYGPFOYL3KYE6LAVCNFSM6AAAAACBGRUPUWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTCMRWHA3DKMRSGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
All that is necessary for evil to succeed is for good people to do nothing.
|
Tested: GCM functionality preserved, no regressions Resolves TODO comments about hard-coded GCM tag length values