HTTPCLIENT-1625 Completely overhaul GSS-API-based authentication backend #577

stoty · 2024-09-06T06:14:25Z

No description provided.

michael-o

First, incomplete review.

httpclient5/src/main/java/org/apache/hc/client5/http/auth/AuthScheme.java

httpclient5/src/main/java/org/apache/hc/client5/http/auth/AuthSchemeV2.java

michael-o · 2024-09-06T11:57:00Z

httpclient5/src/main/java/org/apache/hc/client5/http/auth/KerberosConfig.java

-    private final Option stripPort;
-    private final Option useCanonicalHostname;
-    private final Option requestDelegCreds;
+    private final Option stripPort; //Effective default is ENABLE


I would invert this. For the past 15 years, I haven't seen a single deployment using the port with HTTP service class, especially because that major clients do not even support this.

That's certainly possible, but that would introduce an incompatibility with the 5.2 behaviour.

michael-o · 2024-09-06T11:57:30Z

httpclient5/src/main/java/org/apache/hc/client5/http/auth/KerberosConfig.java

+    private final Option stripPort; //Effective default is ENABLE
+    private final Option useCanonicalHostname; //Effective default is ENABLE
+    private final Option requestDelegCreds; //Effective default is DISABLE
+    private final Option requestMutualAuth; //Effective default is DISABLE


I would like to see a usecase why this should not be on by default without an option at all.

All I can came up with is:

GSS does not default to mutual auth

Backwards compatibility

I would at least leave an option to disable this, so that there is an out for very broken cases.
Of course there is also a case for forcing users to clean up their potentially broken setup.

I have looked at this, and the current API is really built around using the gss library default settings.

Since we are making incompatible changes anyway, if we want to set non-default gss options by default, (and keep them configurable), then we should remove the default options, and use simple booleans with explicit true/false defaults.

httpclient5/src/main/java/org/apache/hc/client5/http/auth/StandardAuthScheme.java

michael-o · 2024-09-06T12:04:39Z

httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/GGSSchemeBase.java

+                throw new AuthenticationException(gsse.getMessage(), gsse);
+            }
+            // other error
+            throw new AuthenticationException(gsse.getMessage(), gsse);


Won't those duplicate the message?

Not really.
But the Auth. execption would have the same message as the cause.
We can write some new message for the Auth. exception, I just really hate when a library swallows the original exception instead of wrapping it.

httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/GGSSchemeBase.java

michael-o · 2024-09-06T12:06:05Z

httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/KerberosScheme.java

 */
-@Deprecated


michael-o · 2024-09-06T12:07:01Z

httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/SPNegoScheme.java

 */
-@Deprecated
 @Experimental
 public class SPNegoScheme extends GGSSchemeBase {


Should also go in favor of SpnegoScheme

stoty · 2024-09-06T12:28:50Z

Thanks for the initial review @michael-o .

Regarding the renames and changing the defaults, I have specifically tried to touch as little as possible because much of the GSS/Kerberos code was not removed from codebase when disabling SPNEGO, which I assumed was for brackwards compatibility reasons.

I'm fine with your proposed changes, but those may theoretically cause issues with pre-5.3 apps, and will cause the api compare plugin to go wild.

michael-o · 2024-09-08T07:06:32Z

Thanks for the initial review @michael-o .

Regarding the renames and changing the defaults, I have specifically tried to touch as little as possible because much of the GSS/Kerberos code was not removed from codebase when disabling SPNEGO, which I assumed was for brackwards compatibility reasons.

I'm fine with your proposed changes, but those may theoretically cause issues with pre-5.3 apps, and will cause the api compare plugin to go wild.

Will engage next week again.

stoty · 2024-09-10T07:35:22Z

I believe I have addressed your comments which do not change the API.
I will work on your comments that introduce incompatible changes now.

stoty · 2024-09-17T07:41:36Z

Can you take another look at the current patch @michael-o ?
I have implemented most of your suggestions, but I had a few more questions comments regarding KerberosConfig.

michael-o · 2024-09-17T07:43:57Z

Can you take another look at the current patch @michael-o ? I have implemented most of your suggestions, but I had a few more questions comments regarding KerberosConfig.

Will pick end of this week or next week.

stoty · 2024-09-30T06:19:58Z

Rebased to current master

michael-o · 2024-09-30T07:47:00Z

@ok2c @stoty I gave this another conceptual thought: We said that we are going to remove the current code for good, but this PR reuses it and makes it partially undeprecated. For me, this is not straight forward. My gut feeling says that we should remove completely first and the @stoty can readd the code he thinks is required to satisfy this PR. WDYT?

ok2c · 2024-09-30T08:04:41Z

@michael-o We cannot remove old implementation unless we are willing to go to the major release cycle and release these changes as 6.0. I suggest that the old classes are left deprecated as there might still be users that rely on them and new implementation is added in parallel (with different names or in a different package).

michael-o · 2024-09-30T08:17:59Z

@michael-o We cannot remove old implementation unless we are willing to go to the major release cycle and release these changes as 6.0. I suggest that the old classes are left deprecated as there might still be users that rely on them and new implementation is added in parallel (with different names or in a different package).

I see, than we need new packages names. Agreed.

…names

stoty · 2024-09-30T09:02:11Z

I have reverted the removals.

I don't see what alternative package name we could use instead of "org.apache.hc.client5.http.auth"
Using another package might also break package level visibility.

Maybe I could prefix the new classes with "Mutual" ?
i.e
MutualGssSchemeBase
MutualSpengoScheme

The other classes are strictly backwards compatible, like KerberosCredentials, and KerberosConfig.
Should I duplicate those as well ?

michael-o · 2024-09-30T09:06:02Z

I have reverted the removals.

I don't see what alternative package name we could use instead of "org.apache.hc.client5.http.auth" Using another package might also break package level visibility.

Maybe I could prefix the new classes with "Mutual" ? i.e MutualGssSchemeBase MutualSpengoScheme

The other classes are strictly backwards compatible, like KerberosCredentials, and KerberosConfig. Should I duplicate those as well ?

I'll get back to this question this week, have some serious trouble at $work I need to solve first.

stoty · 2024-09-30T09:06:05Z

I don't think that removing deprecation is problem, especially as these classes were deprecated without providing a replacement, not because they were replaced by something.

ok2c · 2024-09-30T09:48:11Z

I don't think that removing deprecation is problem, especially as these classes were deprecated without providing a replacement, not because they were replaced by something.

@stoty Un-deprecate those classes that can be kept fully backward compatible and create new classes for those that cannot

stoty force-pushed the HTTPCLIENT-1625 branch 2 times, most recently from 01354e2 to cf04092 Compare September 6, 2024 06:18

michael-o marked this pull request as draft September 6, 2024 11:53

michael-o requested changes Sep 6, 2024

View reviewed changes

stoty force-pushed the HTTPCLIENT-1625 branch from 99bb75c to de8dd8d Compare September 30, 2024 06:19

stoty added 6 commits September 30, 2024 10:22

HTTPCLIENT-1625 Completely overhaul GSS-API-based authentication backend

2a3805a

rename AuthSchemeV2 to AuthScheme2, and some internal GSS variable re…

aafc327

…names

rename gss serverName to PeerName (was spn in previous commit)

fe55b44

don't add SPNEGO to DefaultAuthenticationStrategy

1bf4679

checkstyle fixes

eeb241b

fix some deprecation warnings

5b5564b

stoty force-pushed the HTTPCLIENT-1625 branch from de8dd8d to 5b5564b Compare September 30, 2024 08:51

Re-deprecate KerberosScheme

f2a6195

stoty added 2 commits September 30, 2024 13:54

duplicate and rename mutual SPNEGO related classes

d73101b

re-deprecate StandardAuthScheme.KERBEROS

247e6cf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTPCLIENT-1625 Completely overhaul GSS-API-based authentication backend #577

HTTPCLIENT-1625 Completely overhaul GSS-API-based authentication backend #577

stoty commented Sep 6, 2024

michael-o left a comment

michael-o Sep 6, 2024

stoty Sep 6, 2024

michael-o Sep 6, 2024

stoty Sep 6, 2024

stoty Sep 10, 2024

michael-o Sep 6, 2024

stoty Sep 6, 2024

michael-o Sep 6, 2024

michael-o Sep 6, 2024

stoty Sep 6, 2024

stoty commented Sep 6, 2024

michael-o commented Sep 8, 2024

stoty commented Sep 10, 2024

stoty commented Sep 17, 2024

michael-o commented Sep 17, 2024

stoty commented Sep 30, 2024

michael-o commented Sep 30, 2024

ok2c commented Sep 30, 2024

michael-o commented Sep 30, 2024

stoty commented Sep 30, 2024

michael-o commented Sep 30, 2024

stoty commented Sep 30, 2024

ok2c commented Sep 30, 2024

		*/
		@Deprecated

HTTPCLIENT-1625 Completely overhaul GSS-API-based authentication backend #577

Are you sure you want to change the base?

HTTPCLIENT-1625 Completely overhaul GSS-API-based authentication backend #577

Conversation

stoty commented Sep 6, 2024

michael-o left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoty commented Sep 6, 2024

michael-o commented Sep 8, 2024

stoty commented Sep 10, 2024

stoty commented Sep 17, 2024

michael-o commented Sep 17, 2024

stoty commented Sep 30, 2024

michael-o commented Sep 30, 2024

ok2c commented Sep 30, 2024

michael-o commented Sep 30, 2024

stoty commented Sep 30, 2024

michael-o commented Sep 30, 2024

stoty commented Sep 30, 2024

ok2c commented Sep 30, 2024