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

ZOOKEEPER-3905: Race condition causes sessions to be created for clients even though their certificate authentication has failed (3.6) #1427

Closed
wants to merge 2 commits into from

Conversation

anmolnar
Copy link
Contributor

Backport to branch-3.6

Netty channel doesn't get closed if authentication fails after a successful SSL handshake. We need a custom authentication provider in order to trigger this, because the default implementation does the same check as for the SSL handshake. Hence it never fails.

Unit test added to make sure client is not able to connect.

…nts even though their certificate authentication has failed

Netty channel doesn't get closed if authentication fails after a successful SSL handshake. We need a custom authentication provider in order to trigger this, because the default implementation does the same check as for the SSL handshake. Hence it never fails.

Unit test added to make sure client is not able to connect.

Target branches: master, 3.6, 3.5 (will create separate PR for 3.5)

Author: Andor Molnar <andor@apache.org>

Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mate Szalay-Beko <symat@apache.org>

Closes apache#1422 from anmolnar/ZOOKEEPER-3905
@anmolnar
Copy link
Contributor Author

@eolivelli @symat PTAL.

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.

Looks good, thanks for the quick backport!
(according to Jenkins there is a small checkstyle error.)

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.

LGTM

Can't merge it now.
I will do tomorrow.

We should check CI as well

@nkalmar
Copy link
Contributor

nkalmar commented Aug 11, 2020

Wrong importorder:
'[ERROR] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-server/src/test/java/org/apache/zookeeper/test/ClientSSLTest.java:26: Import org.junit.Assert.assertTrue appears after other imports that it should precede [ImportOrder]'

@@ -22,7 +22,9 @@

package org.apache.zookeeper.test;

import java.io.IOException;
import static org.junit.Assert.assertTrue;
Copy link
Contributor

Choose a reason for hiding this comment

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

checkstyle don't like the order for this... yikes.
I guess Assert.fail should be before? Not sure why.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, nevermind, static imports should be first, that's the problem.

@anmolnar
Copy link
Contributor Author

Thanks @nkalmar . Just fixed it.

@anmolnar
Copy link
Contributor Author

@eolivelli all good now.

@anmolnar
Copy link
Contributor Author

anmolnar commented Aug 11, 2020

@eolivelli @symat @nkalmar Please merge.

@symat
Copy link
Contributor

symat commented Aug 12, 2020

I'm merging it

asfgit pushed a commit that referenced this pull request Aug 12, 2020
…nts even though their certificate authentication has failed (3.6)

Backport to branch-3.6

Netty channel doesn't get closed if authentication fails after a successful SSL handshake. We need a custom authentication provider in order to trigger this, because the default implementation does the same check as for the SSL handshake. Hence it never fails.

Unit test added to make sure client is not able to connect.

Author: Andor Molnar <andor@apache.org>

Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mate Szalay-Beko <symat@apache.org>, Norbert Kalmar <nkalmar@apache.org>

Closes #1427 from anmolnar/ZOOKEEPER-3905_36
asfgit pushed a commit that referenced this pull request Aug 12, 2020
…nts even though their certificate authentication has failed (3.5)

Backport to branch-3.5

Netty channel doesn't get closed if authentication fails after a successful SSL handshake. We need a custom authentication provider in order to trigger this, because the default implementation does the same check as for the SSL handshake. Hence it never fails.

Unit test added to make sure client is not able to connect.

Author: Andor Molnar <andor@apache.org>

Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mate Szalay-Beko <symat@apache.org>, Norbert Kalmar <nkalmar@apache.org>

Closes #1427 from anmolnar/ZOOKEEPER-3905_36

(cherry picked from commit 8bcaf7b)
Signed-off-by: Mate Szalay-Beko <symat@apache.org>
@symat
Copy link
Contributor

symat commented Aug 12, 2020

Thanks @anmolnar!
I pushed the PR to branch-3.6 and also cherry-picked it to branch-3.5 (resolving conflicts + run unit tests).

@anmolnar
Copy link
Contributor Author

Great, thanks @symat !

@anmolnar anmolnar closed this Aug 12, 2020
@anmolnar anmolnar deleted the ZOOKEEPER-3905_36 branch August 22, 2020 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants