Skip to content

PHOENIX-6750 Bump Avatica version to 1.21.0 in queryserver#104

Merged
richardantal merged 2 commits into
apache:masterfrom
richardantal:PHOENIX-6750
Sep 6, 2022
Merged

PHOENIX-6750 Bump Avatica version to 1.21.0 in queryserver#104
richardantal merged 2 commits into
apache:masterfrom
richardantal:PHOENIX-6750

Conversation

@richardantal

Copy link
Copy Markdown
Contributor

No description provided.

@richardantal

Copy link
Copy Markdown
Contributor Author

I created this pull request to ask your opinion about it.

Let me give a little background for this.
I started working by bumping the avatica and the jetty version, ran some test and got the following exception in HttpParamImpersonationQueryServerIT:

java.lang.IllegalArgumentException: Illegal principal name securecluster/localhost: org.apache.hadoop.security.authentication.util.KerberosName$NoMatchingRule: No rules applied to securecluster/localhost
	at org.apache.hadoop.security.User.<init>(User.java:51)
	at org.apache.hadoop.security.UserGroupInformation.createProxyUser(UserGroupInformation.java:1347)
	at org.apache.phoenix.queryserver.server.QueryServer$PhoenixDoAsCallback.lambda$createProxyUser$0(QueryServer.java:615)
	at java.util.concurrent.ConcurrentHashMap.computeIfAbsent(ConcurrentHashMap.java:1660)
	at org.apache.phoenix.util.SimpleLRUCache.computeIfAbsent(SimpleLRUCache.java:56)
	at org.apache.phoenix.queryserver.server.QueryServer$PhoenixDoAsCallback.createProxyUser(QueryServer.java:615)
	at org.apache.phoenix.queryserver.server.QueryServer$PhoenixDoAsCallback.doAsRemoteUser(QueryServer.java:598)

Then I checked the remoteUserName here: https://github.com/apache/phoenix-queryserver/blob/master/phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/server/QueryServer.java#L598

Turns out, that before CALCITE-4152 we had securecluster/localhost@EXAMPLE.COM and no we have securecluster/localhost
Which will not match later and cause exception.

So I trimmed the hostname because in hadoop we would set it to null if there is no realm anyway:
https://github.com/apache/hadoop/blob/branch-3.1.4/hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/KerberosName.java#L125

Does this change makes sense?

Also @joshelser could you give us a little explanation why was the realm trimmed off in calcite?

@joshelser joshelser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just a word of warning to test this from the Phoenix POV since you'll be getting in the new SPNEGO implementation from Avatica+Jetty.

If you turn up logging to DEBUG or TRACE for the PQS client or server, the thing you want to not see is repeated calls that look like:

  1. (client) POST http://pqs:8765
  2. (server) http/401 "WWW-Authenticate: Negotiate"
  3. (client) POST http://pqs:8765 "WWW-Authenticate: ...."
  4. (server) http/200 , set

You should only see steps 2 and 3 the first time the client connects to PQS in that JVM. After step 4, the cookie will be used to authenticate the client (replacing the need for steps 2 and 3). Actually, even the first time, the Avatica http client should proactively send the WWW-Authenticate header+value to prevent the need for the (useless) www-authenticate: negotiate response from the server. If the client knows it needs to authenticate, it can preemptively send the authentication data.

Finally, it's all the more important that we also have PQS running with TLS on. A bad actor listening on the network could steal the cookie the server sends back much more easily than a SPNEGO replay attack. If TLS is on, then this problem goes away (because bad actors can't see what the cookies' value was).

@richardantal

Copy link
Copy Markdown
Contributor Author

Thank you Josh and Istvan for the review, and @joshelser for sharing some background.

I've set debug logs and with this change there are less WWW-Authenticate: Negotiate messages (96 instead of 384)
I see Set-Cookie: JSESSIONID... lines in the log and the cookies being used.

@joshelser

Copy link
Copy Markdown
Member

I see Set-Cookie: JSESSIONID... lines in the log and the cookies being used.

Awesome, that sounds right.

could you give us a little explanation why was the realm trimmed off in calcite?

Sorry, I missed this question yesterday :)

Calcite isn't pulling it off, but it's actually being done by Jetty. There was a pretty fundamental change in the spnego code that we used to use in Avatica/Phoenix (now, deprecated in Jetty) and the spnego code that Avatica is now using. I'm going off of memory here... but it was something like the old version of Jetty would let you define "realms" that were allowed to talk to that Jetty server. So you could say "SALES.COM" and "FINANCE.COM" could talk to the service but not "HR.COM".

The newer version of the spnego impl did away with that logic. If you have a valid krb auth'n, then you're good to go.

@stoty stoty left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 LGTM

@stoty

stoty commented Aug 22, 2022

Copy link
Copy Markdown
Contributor

We should probably re-test the Mac Kerberos/Python driver issue @joshelser mentioned on the Avatica ticket, and and call it out in the release notes. (i.e. add the info to this ticket's release notes)

@richardantal

Copy link
Copy Markdown
Contributor Author

I've ran the full python testsuite via mvn clean verify -Drun.full.python.testsuite=true command and there were test failures , both with this change and on master so we can assume something is still not right with mac kerberos/python driver.
Istvan can you run the test to validate if the failures are only on mac?

@stoty

stoty commented Aug 24, 2022

Copy link
Copy Markdown
Contributor

I also got failures on master, but those errors are not related to kerberos, but look more like test setup issues.
Check phoenix-queryserver-parent/phoenix-queryserver-it/target/python-stderr.log

@richardantal
richardantal merged commit 0b11a1e into apache:master Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants