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.

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

Jump to bottom

cipher: do not reuse cipher ctx for certain operations #146

Merged
merged 1 commit into from
May 24, 2024
Merged

cipher: do not reuse cipher ctx for certain operations #146

merged 1 commit into from
May 24, 2024

Conversation

derekparker
Copy link
Contributor

Fixes golang-fips/go#187

CBC and CTR are left as-is, as we also cached the context in the v1 version, so technically not a regression.

@derekparker
Copy link
Contributor Author

Tested locally against Go 1.22.3 (golang-fips/go tip).

ueno
ueno approved these changes May 21, 2024
View reviewed changes
Copy link
Collaborator

@ueno ueno left a comment

Choose a reason for hiding this comment

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

LGTM

@derekparker
Copy link
Contributor Author

@lyoung-confluent I've tested against your reproducer but could you verify this fix works for you as well?

Also @dagood or @qmuntal if you can take a look as well that would be greatly appreciated.

@qmuntal
Copy link
Collaborator

qmuntal commented May 23, 2024

This looks like an important issue to fix. @dagood we should port this PR to the CNG backend, ideally before go1.23 is released.

Can't properly review it myself, I'm on paternity leave. Will defer the review to @dagood. Fyi @karianna.

@dagood
Copy link
Collaborator

dagood commented May 23, 2024

Thanks @qmuntal for the ping, and especially the note about CNG. This is definitely near the top of my TODO list, sorry it's been hanging a bit.

@karianna offered to help with an initial review for the PR on behalf the Microsoft team to get this through, so I've added him as a collaborator.

karianna
karianna approved these changes May 24, 2024
View reviewed changes
Copy link
Collaborator

@karianna karianna left a comment

Choose a reason for hiding this comment

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

LGTM

@derekparker derekparker merged commit 61a53ab into golang-fips:v2 May 24, 2024
10 checks passed
@derekparker derekparker deleted the wip/restore-threadsafe-aead-gcm branch May 24, 2024 01:11
@derekparker derekparker mentioned this pull request May 24, 2024
Closed
@dagood dagood mentioned this pull request Jun 18, 2024
Closed
qmuntal pushed a commit that referenced this pull request Sep 2, 2024
@derekparker @qmuntal
cipher: do not reuse cipher ctx for certain operations (#146)
fc9ef60 
Fixes golang-fips/go#187

Co-authored-by: Derek Parker <deparker@redhat.com>
xnox pushed a commit to xnox/golang-fips-openssl that referenced this pull request Sep 2, 2024
@derekparker @xnox
cipher: do not reuse cipher ctx for certain operations (golang-fips#146)
fb9c9e3 
Fixes golang-fips/go#187

Co-authored-by: Derek Parker <deparker@redhat.com>
(cherry picked from commit 61a53ab)
dagood pushed a commit that referenced this pull request Sep 3, 2024
@xnox @derekparker
cipher: do not reuse cipher ctx for certain operations (#146) (#157)
db0a77f 
Fixes golang-fips/go#187

Co-authored-by: Derek Parker <deparker@redhat.com>
(cherry picked from commit 61a53ab)

Co-authored-by: Derek Parker <parkerderek86@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karianna karianna karianna approved these changes

@ueno ueno ueno approved these changes

@dagood dagood Awaiting requested review from dagood

@qmuntal qmuntal Awaiting requested review from qmuntal

@dbenoit17 dbenoit17 Awaiting requested review from dbenoit17

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[AES-GCM] cipher.AEAD is no-longer safe for concurrent use
5 participants
@derekparker @qmuntal @dagood @ueno @karianna