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

[stoty]

[stoty]

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]

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]

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]

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]

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]

Rebased to current master

[michael-o]

@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]

@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]

@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.

[stoty]

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]

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]

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]

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]

rebased

[stoty]

I have made the discussed changes.

[ok2c]

@stoty @michael-o Folks, what is the reason this PR is stuck? @stoty Please rebase the latest off master to pick up Docker based compatibility tests. Feel free to disable japicmp but all integration must pass with your changes.

[michael-o]

@stoty @michael-o Folks, what is the reason this PR is stuck? @stoty Please rebase the latest off master to pick up Docker based compatibility tests. Feel free to disable japicmp but all integration must pass with your changes.

I am busy, can only peek once in a while. Hopefully the upcoming weeks will be better.

[ok2c]

@stoty If you want to expedite the review process we could do the following. Once you are more or less happy with the state of things, submit changes to HttpAutheticator and AuthScheme in a separate PR and will happily review them. Kerberos specific code will have to wait until @michael-o has time to review it.

[stoty]

Thank you @ok2c.

I have rebased the patch and temporarily disabled japcmp. I have also added and @Internal annotation to HttpAuthenticator, I cannot imagine realistic user code calling that directly. (though it does not seem to apply retroactively)

I will try to make the time to split the PR.

[ok2c]

I have also added and @internal annotation to HttpAuthenticator, I cannot imagine realistic user code calling that directly. (though it does not seem to apply retroactively)

@stoty There is still enough code in HC that goes back to version 3 and 2. I am fine with deprecating the present HttpAuthenticator and creating a new one marked as internal. However I still want to be able to understand what changes had to be made to the original code.

[stoty]

I don't think that duplicating HttpAuthenticator is a good idea.

For the authentication shemes, a case could be made for duplicating them, as those are pluggable components.

However, HttpAuthenticator is a purely internal class, and no sane external code is expected to call it.

The only way I could think that HttpAuthenticator would be called by external code, is if someone fully re-implemented a new (Closable...)Client class (and the corresponding ...Exec class ), which is not something that I think should be covered by backwards compatibility.

The end result would be replacing HttpAuthenticator with HttpAuthenticator2, and having a dangling HttpAuthenticator, which doesn't work with the other classes anyway (and it would possibly have to be modified anyway just to compile).

[ok2c]

@stoty Please disable japicmp for now and make changes to HttpAuthenticator you deem necessary. I will take it from there.

[stoty]

Thank you. I have already done that.

[stoty]

@michael-o, @ok2c

Now that the festive sesason is over, are you able to find the time for reviews ?

On my side, I still need to implement the requested new config interface for the new auth methods, but that should not affect the rest of the changes.

[ok2c]

@michael-o, @ok2c

Now that the festive sesason is over, are you able to find the time for reviews ?

On my side, I still need to implement the requested new config interface for the new auth methods, but that should not affect the rest of the changes.

@stoty What shall I say? I will repeat the same thing I have said you before. If you split this change-set into two: one with auth API changes only and another one with Kerberos auth, I could help you get the former incorporated into HttpClient. Otherwise you are fully at @michael-o's mercy.

[stoty]

So the first patch would be most of this patch, without the actual MutualSPnego implementation, and corresponding new test, while the second one would add the new MutualSpnego implementation class hierarchy, and the new test class (plus some new comments that refer to them) ?

[ok2c]

So the first patch would be most of this patch, without the actual MutualSPnego implementation, and corresponding new test, while the second one would add the new MutualSpnego implementation class hierarchy, and the new test class (plus some new comments that refer to them) ?

@stoty Correct. This way, in the worst case scenario one would be able to plug-in a custom MutualSpnego implementation even if the auth scheme is not supported by the project.

[stoty]

Opened #612 for the first part.

[michael-o]

@stoty Can you rebase please?

[stoty]

#615 is the rebased PR top of current master @michael-o .

I am closing this now.