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

ZOOKEEPER-4889: Fallback to DIGEST-MD5 auth mech should be disabled in Fips mode #2213

Merged
merged 12 commits into from
Nov 25, 2024
Merged

ZOOKEEPER-4889: Fallback to DIGEST-MD5 auth mech should be disabled in Fips mode #2213

merged 12 commits into from
Nov 25, 2024

Conversation

anmolnar
Copy link
Contributor

@anmolnar anmolnar commented Nov 24, 2024 •
edited
Loading

Disable DIGEST-MD5 mech in Fips mode. I had to modify lots of unit tests, because fips mode is enabled by default on the master branch and unit tests heavily rely on using DIGEST-MD5 for Sasl authentication.

Scary number of files, but mostly refactoring in the tests.

@anmolnar
Copy link
Contributor Author

We should have a common base class for SASL auth tests.

eolivelli
eolivelli approved these changes Nov 25, 2024
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Perfect !

Lgtm

@eolivelli eolivelli merged commit fe64596 into apache:master Nov 25, 2024
14 checks passed
symat
symat reviewed Nov 25, 2024
View reviewed changes
Copy link
Contributor

@symat symat left a comment

Choose a reason for hiding this comment

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

LGTM

shouldn't we mention this to the documentation somewhere?
(I don't think this would really be a backward compatibility issue, as I guess noone is considering digest MD5 in a FIPS environment - yet still this is a change in the current behaviour)

@anmolnar
Copy link
Contributor Author

LGTM

shouldn't we mention this to the documentation somewhere? (I don't think this would really be a backward compatibility issue, as I guess noone is considering digest MD5 in a FIPS environment - yet still this is a change in the current behaviour)

That's very good point. You submitted a bit fast, but let me create an addendum patch.
It also needs to be backported to other branches branch-3.9 and branch-3.8, I'm not sure what to do with the addendum in this case, can I just backport in a single patch?

@anmolnar anmolnar deleted the ZOOKEEPER-4889 branch November 25, 2024 14:47
@anmolnar
Copy link
Contributor Author

@eolivelli @symat Please see the addendum here: #2214

@anmolnar
Copy link
Contributor Author

shouldn't we mention this to the documentation somewhere? (I don't think this would really be a backward compatibility issue, as I guess noone is considering digest MD5 in a FIPS environment - yet still this is a change in the current behaviour)

It's not about consideration, FIPS doesn't allow using MD5 anywhere in the running code. If ever the code runs on that code patch, runtime exception will be thrown and JVM halts. This change is backward compatible.

asfgit pushed a commit that referenced this pull request Nov 25, 2024
@anmolnar
ZOOKEEPER-4889: Fallback to DIGEST-MD5 auth mech should be disabled i…
555d08d 
…n Fips mode (#2213)

(cherry picked from commit fe64596)
Signed-off-by: Andor Molnar <andor@apache.org>
@anmolnar
Copy link
Contributor Author

I've backported the patch to branch-3.9, will open separate PR for branch-3.8.

@anmolnar anmolnar mentioned this pull request Nov 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@symat symat symat left review comments

@eolivelli eolivelli eolivelli approved these changes

@kezhuw kezhuw Awaiting requested review from kezhuw

@afine afine Awaiting requested review from afine

@tisonkun tisonkun Awaiting requested review from tisonkun

@li4wang li4wang Awaiting requested review from li4wang

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

Successfully merging this pull request may close these issues.
3 participants
@anmolnar @eolivelli @symat