HBASE-29080 Validate Negotiated SASL QoP Against Requested #6601

stoty · 2025-01-14T09:56:16Z

No description provided.

anmolnar

lgtm, but number of tests added seems a bit small to me. Don't we already tests for Sasl negotiation that we could extend?

Apache9

Is it possible to add a test at rpc level?

Apache9 · 2025-02-01T14:00:31Z

hbase-client/src/test/java/org/apache/hadoop/hbase/security/TestSaslUtil.java

+    try {
+      SaslUtil.verifyNegotiatedQop(nullQop, nullQop);
+    } catch (Exception e) {
+      fail("Should not have thrown exception");


Just let the exception be thrown? In this way we could see more detailed failure message...

Apache9 · 2025-02-01T14:00:48Z

hbase-client/src/test/java/org/apache/hadoop/hbase/security/TestSaslUtil.java

+    // Auth requested, got confidentality.
+    try {
+      SaslUtil.verifyNegotiatedQop(authentication, confidentality);
+      fail("Should have thrown exception");


Use assertThrows

Apache9 · 2025-02-01T14:02:31Z

hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/SimpleServerRpcConnection.java

      if (saslServer.isComplete()) {
-        String qop = saslServer.getNegotiatedQop();
-        useWrap = qop != null && !"auth".equalsIgnoreCase(qop);
+        String negotiatedQop = saslServer.getNegotiatedQop();


So in the old code, SimpleServerRpcConnection does not call finishSaslNegotiation? Seems a bit strange...

That's a good point.
finishSaslNegotiation does the same thing, it's better to call that from here.

Refactored SimpleServerRpcConnection to use finishSaslNegotiation.

stoty · 2025-02-03T06:24:48Z

@anmolnar @Apache9
We have some integration tests that involve SASL, but none of them modify the SASL behviour.

Adding an integration test a that mocks/spies the SASL behaviour would be quite a task, and I don't really know how to go about that.

If you think that it is needed, and better yet if you have some suggestions on how to write it, I can attempt to write one.

Apache9 · 2025-02-03T10:24:20Z

@anmolnar @Apache9 We have some integration tests that involve SASL, but none of them modify the SASL behviour.

Adding an integration test a that mocks/spies the SASL behaviour would be quite a task, and I don't really know how to go about that.

If you think that it is needed, and better yet if you have some suggestions on how to write it, I can attempt to write one.

I think there are SASL related rpc tests? You can check the tests under o.a.h.h.security package in hbase-server's src/test.

Apache-HBase · 2025-02-03T20:54:02Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 35s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 1s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	hbaseanti	0m 0s		Patch does not have any anti-patterns.
			_ master Compile Tests _
+0 🆗	mvndep	0m 12s		Maven dependency ordering for branch
+1 💚	mvninstall	4m 4s		master passed
+1 💚	compile	4m 36s		master passed
+1 💚	checkstyle	1m 2s		master passed
+1 💚	spotbugs	2m 45s		master passed
+1 💚	spotless	0m 58s		branch has no errors when running spotless:check.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 12s		Maven dependency ordering for patch
+1 💚	mvninstall	3m 46s		the patch passed
+1 💚	compile	4m 20s		the patch passed
-0 ⚠️	javac	0m 50s	/results-compile-javac-hbase-client.txt	hbase-client generated 2 new + 101 unchanged - 2 fixed = 103 total (was 103)
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	1m 2s		the patch passed
+1 💚	spotbugs	3m 10s		the patch passed
+1 💚	hadoopcheck	13m 40s		Patch does not cause any errors with Hadoop 3.3.6 3.4.0.
+1 💚	spotless	0m 53s		patch has no errors when running spotless:check.
			_ Other Tests _
+1 💚	asflicense	0m 22s		The patch does not generate ASF License warnings.
		50m 6s

Subsystem	Report/Notes
Docker	ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6601/4/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#6601
Optional Tests	dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless
uname	Linux 6b3aa3e67a75 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `3c23b62`
Default Java	Eclipse Adoptium-17.0.11+9
Max. process+thread count	84 (vs. ulimit of 30000)
modules	C: hbase-client hbase-server U: .
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6601/4/console
versions	git=2.34.1 maven=3.9.8 spotbugs=4.7.3
Powered by	Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

Apache-HBase · 2025-02-04T00:46:17Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 33s		Docker mode activated.
-0 ⚠️	yetus	0m 3s		Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --author-ignore-list --blanks-eol-ignore-file --blanks-tabs-ignore-file --quick-hadoopcheck
			_ Prechecks _
			_ master Compile Tests _
+0 🆗	mvndep	0m 12s		Maven dependency ordering for branch
+1 💚	mvninstall	3m 40s		master passed
+1 💚	compile	1m 37s		master passed
+1 💚	javadoc	0m 53s		master passed
+1 💚	shadedjars	6m 33s		branch has no errors when building our shaded downstream artifacts.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 14s		Maven dependency ordering for patch
+1 💚	mvninstall	3m 39s		the patch passed
+1 💚	compile	1m 41s		the patch passed
+1 💚	javac	1m 41s		the patch passed
+1 💚	javadoc	0m 55s		the patch passed
+1 💚	shadedjars	7m 8s		patch has no errors when building our shaded downstream artifacts.
			_ Other Tests _
+1 💚	unit	2m 6s		hbase-client in the patch passed.
-1 ❌	unit	247m 45s	/patch-unit-hbase-server.txt	hbase-server in the patch failed.
		282m 0s

Subsystem	Report/Notes
Docker	ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6601/4/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR	#6601
Optional Tests	javac javadoc unit compile shadedjars
uname	Linux fd3a669236e7 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `3c23b62`
Default Java	Eclipse Adoptium-17.0.11+9
Test Results	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6601/4/testReport/
Max. process+thread count	4431 (vs. ulimit of 30000)
modules	C: hbase-client hbase-server U: .
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6601/4/console
versions	git=2.34.1 maven=3.9.8
Powered by	Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

anmolnar · 2025-02-10T18:34:01Z

@stoty Take a quick look at TestSecureIPC, AbstractTestSecureIPC and friends. I see a few tests related to negotiated QoP that you could extend maybe.

Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Andor Molnár <andor@apache.org> (cherry picked from commit 8a374dc)

Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Andor Molnár <andor@apache.org>

…6782) Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Andor Molnár <andor@apache.org>

…6782) Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Andor Molnár <andor@apache.org> (cherry picked from commit 03effb9)

…) (apache#6782) Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Andor Molnár <andor@apache.org> (cherry picked from commit 03effb9)

…6782) (#6791) Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Andor Molnár <andor@apache.org> (cherry picked from commit 03effb9)

…) (apache#6782) Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Andor Molnár <andor@apache.org>

This comment has been minimized.

Sign in to view

stoty added 2 commits January 20, 2025 12:58

HBASE-29080 Validate Negotiated SASL QoP Against Requested

c7fa19b

add test and fix logic

4bdcb2d

stoty force-pushed the HBASE-29080 branch from a6845a3 to 4bdcb2d Compare January 20, 2025 12:56

This comment has been minimized.

Sign in to view

stoty requested review from Apache9, anmolnar, apurtell and meszibalu January 20, 2025 18:06

anmolnar approved these changes Jan 31, 2025

View reviewed changes

Apache9 reviewed Feb 1, 2025

View reviewed changes

stoty added 2 commits February 3, 2025 06:34

make testVerifyQop less verbose

c86c5b0

use finishSaslNegotiation in SimpleServerRpcConnection

de8e6ce

This comment has been minimized.

Sign in to view

stoty added 2 commits February 3, 2025 07:32

spotless:apply

b6b00e8

fix useWrap regression in SimpleServerRpcConnection

3c23b62

Apache9 approved these changes Feb 3, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

apurtell approved these changes Mar 7, 2025

View reviewed changes

stoty merged commit 8a374dc into apache:master Mar 11, 2025
1 check failed

HBASE-29080 Validate Negotiated SASL QoP Against Requested #6601

HBASE-29080 Validate Negotiated SASL QoP Against Requested #6601

Uh oh!

Conversation

stoty commented Jan 14, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

anmolnar left a comment

Choose a reason for hiding this comment

Uh oh!

Apache9 left a comment

Choose a reason for hiding this comment

Uh oh!

Apache9 Feb 1, 2025

Choose a reason for hiding this comment

Uh oh!

stoty Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

Apache9 Feb 1, 2025

Choose a reason for hiding this comment

Uh oh!

stoty Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

Apache9 Feb 1, 2025

Choose a reason for hiding this comment

Uh oh!

stoty Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stoty Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

stoty commented Feb 3, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

Apache9 commented Feb 3, 2025

Uh oh!

Apache-HBase commented Feb 3, 2025

Uh oh!

Apache-HBase commented Feb 4, 2025

Uh oh!

anmolnar commented Feb 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

stoty Feb 3, 2025 •

edited

Loading