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

disallow implicit tag truncation with finalize_with_tag #4342

Merged
merged 1 commit into from Jul 17, 2018

Conversation

reaperhulk
Copy link
Member

No description provided.

@@ -190,7 +190,7 @@ def finalize(self):
self._backend.openssl_assert(res == 1)
return self._backend._ffi.buffer(buf)[:outlen[0]]

def finalize_with_tag(self, tag):
def finalize_with_tag(self, tag, min_tag_length=16):
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we can't just use the value from teh GCM constructor?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's pretty ugly to say you need to put the min_tag_length into the GCM constructor, but not the tag (which you may not have yet), and then later on enforce that tag length when calling finalize_with_tag.

On the other hand it's also confusing and potentially error prone that min_tag_length is on the GCM constructor and also here. In an ideal world I'd make the "pass the tag in the constructor" API go away entirely as I think finalize_with_tag is a much saner API, but I'm not allowed to have nice things.

Copy link
Member

Choose a reason for hiding this comment

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

I'd favor having us expose the API in one place; and I think it's somewhat natural to include tag length with the "algorithm configuration"

@alex alex merged commit d4378e4 into pyca:master Jul 17, 2018
@reaperhulk reaperhulk deleted the gcm-fix branch July 17, 2018 14:57
amauryfa pushed a commit to amauryfa/cryptography that referenced this pull request Jul 22, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants