HTTPCLIENT-2358 Implement a mutual authentication capable SPNEGO scheme

[stoty]

[ok2c]

@stoty I cannot contribute here much. My only wish, and I understand it is a big ask, to also create a compatibility test similar to those we have for BASIC / DIGEST with Squid and Apache HTTPD

https://github.com/apache/httpcomponents-client/tree/master/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/compatibility

[stoty]

I was afraid you're going to say that...

There are two ways to go about that:

• Try to find an docker image with a KDC and web server, and model the test on the Squid test
• Initialize a local (non-dockerized) KDC with Kerby, start Jetty, and use that.

I don't know which of the two is more work, but I know where to copy the test setup from for the second one.

Do you have a preference ?

[ok2c]

I was afraid you're going to say that...

@stoty Kek.

Anyways, We already test for compatibility with Jetty in core by running it in a Docker container

https://github.com/apache/httpcomponents-core/blob/master/httpcore5-testing/src/test/java/org/apache/hc/core5/testing/compatibility/JettyCompatIT.java

I t would be my preferred option but I do not know how difficult it is to pack extra Jetty dependencies into a Docker container. I was also hoping Apache HTTPD or Ngnix might have Kerberos support.

If it is too much effort, disregard my request.

[stoty]

It is a reasonable request, and it IS helpful to catch any regressions, etc. I will try to find and existing docker image that is close enough.

[garydgregory]

Apache Kerby implements Kerberos, you can get inspiration (copy) how they do it: https://directory.apache.org/kerby/

[stoty]

Kerby is great for in-JVM tests, but for dockerized tests it's probably easier to use MIT kerberos.

[stoty]

After poking around a bit, it seems that neither Apache Httpd, not Nginx supports SPNEGO out of the box. While both have third party SPNEGO modules, creating a test image with a full working setup looks like a lot of work.

For now, I plan to make a local test with Kerby + Jetty, without Docker.

[michael-o]

A full coverage test contains JGSS (via Tomcat my SpnegoAuthenticator), MIT Kerberos (via mod_auth_gssapi), Microsoft Kerberos (via IIS (SSPI)). Virtually impossible to automate. I will do manual testing anyway.

I have everything in place at work to test.

[stoty]

Thanks. I've made the changes to MutualKerberosConfig.

While I don't use Tomcat, testing Jgss with Jetty is doable, in fact one of the ways I tested this was via the SPNEGO test in HttpCLient->Calcite Avatica->Phoenix PQS. It does bring in a ton of extra test dependencies, and I couldn't really find where to fit the that integration test in the project.

[stoty]

I have added tests for direct connection to httpd + mod_auth_spnego, @michael-o . The tests have immediately found a problem, which I also fixed.

The async client cannot use the Subject set with Subject.callAs to figure out the Kerberos credentials (probably due to the auth being run from a different thread) Is that acceptable, or should we extract and store the Subject ?

Async still works if we create and set KerbersCredentials. (as in the new tests I added)

[stoty]

I have added tests for SPNEGO authentication for Squid.

I feel that this is ready now, @michael-o .

[michael-o]

I have added tests for direct connection to httpd + mod_auth_spnego, @michael-o . The tests have immediately found a problem, which I also fixed.

The async client cannot use the Subject set with Subject.callAs to figure out the Kerberos credentials (probably due to the auth being run from a different thread) Is that acceptable, or should we extract and store the Subject ?

Async still works if we create and set KerbersCredentials. (as in the new tests I added)

Which bug exactly?

[stoty]

I can do that, but that would mean the the three formerly mutual classes only differ in case and package from the old deprecated classes. I can see that causing a lot of gried for users down the line.

• Since in both auth and impl.path packages contains multiple new classes there should be in respective new subpackages gss for tidiness.

Sure.

• Don't use the term "Kerberos" in classnames becausee we perform SPNEGO with GSS-API, correctly it should be called "Gss"

Sure.

• SPNEGO in class names should be written consistently, In PascalCase and not mixed case.

Only the old deprecated classes use SPNego, and fixing those would make it even easier to mix them up with the new ones. (apart from breaking backwards compatibility, if we care about that)

• Drop the prefix "Mutual" in class names altogether because one can disable on request making it a contradiction and the other party could pontentially deny it.

I can do that, but then then the only difference between the old and new classes will be the packages and the case. Machines don't care, but I see that causing grief for users.

[stoty]

Thanks for the review. I have made the suggested changes, but I am not happy with how close the old new class names are.

I have also left KerberosCredentials alone, because it is used by both the new and old code. Should I make an identical child class with GssCredentials name ?

[michael-o]

I can do that, but that would mean the the three formerly mutual classes only differ in case and package from the old deprecated classes. I can see that causing a lot of gried for users down the line.

• Since in both auth and impl.path packages contains multiple new classes there should be in respective new subpackages gss for tidiness.

Sure.

• Don't use the term "Kerberos" in classnames becausee we perform SPNEGO with GSS-API, correctly it should be called "Gss"

Sure.

• SPNEGO in class names should be written consistently, In PascalCase and not mixed case.

Only the old deprecated classes use SPNego, and fixing those would make it even easier to mix them up with the new ones. (apart from breaking backwards compatibility, if we care about that)

• Drop the prefix "Mutual" in class names altogether because one can disable on request making it a contradiction and the other party could pontentially deny it.

I can do that, but then then the only difference between the old and new classes will be the packages and the case. Machines don't care, but I see that causing grief for users.

Don't rename old ones, only new ones. If old ones are reused maye it would be better to copy them?

[michael-o]

Thanks for the review. I have made the suggested changes, but I am not happy with how close the old new class names are.

I have also left KerberosCredentials alone, because it is used by both the new and old code. Should I make an identical child class with GssCredentials name ?

Yes, please. Don't break old deprecated, but add new classes for the new impl.

[stoty]

I can do that, but then then the only difference between the old and new classes will be the packages and the case. Machines don't care, but I see that causing grief for users.

Don't rename old ones, only new ones. If old ones are reused maye it would be better to copy them?

I wasn't clear. The classes are different, but their names are almost identical.

As the Scheme is not enabled by default, users have to add some boilerplate code to enable SPNEGO at all. It is now too easy to use the old SPNegoScheme instead of the new SpnegoScheme in that boilerplate IMO.

i have renamed KerberCredentials, and added a backwards compatibility subclass.

[stoty]

I have fixed the small issues. It's getting late, I'll try to do the loop condition tomorrow.

[stoty]

I have added a new config option for non-standard servers, and rewritten the established checks.

Please take another look @michael-o .

[stoty]

I have split the mutual auth config option to request and require. Having both takes no effort on our part, but allows for set and pray mutual authentication (though I doubt that value of that).

I can drop the request one, and reqest unconditionally if you prefer.

[stoty]

I opened https://bugs.squid-cache.org/show_bug.cgi?id=5485 for the Squid issues.

[michael-o]

I opened https://bugs.squid-cache.org/show_bug.cgi?id=5485 for the Squid issues.

Thanks, very good. mod_auth_gssapi is the gold standard for me written by very knowledgeable people.

[stoty]

I realize that I somehow didn't upload the changes for GSSSchemeBase in my last pach, that's why a lot of your earlier comments were not addressed.

[stoty]

I hope I got all the fixes we discussed, @michael-o .

Based on my digging, I have set isConnectionBased() to false.

I have also fixed the logic in the needAuthentication() to handle proxy + target authentication correctly. (Should that be split to a separate JIRA ?)

[stoty]

Were you able to check my latest patch @michael-o ?

[stoty]

Ping @michael-o ?

[stoty]

Hi, @michael-o , I hope you had a good summer,

Now that everyone is back at the grinder, I request you you to kindly help get this feature over the finish line.

I have rebased the patch to the latest master (there were no major issues, some dependencies were removed and I had to add an exclusion to animal-sniffer for the reflection classes in SubjectUtil).

I have also found two unresolved comments regarding code comments from you, I have resolved those.

[ok2c]

@michael-o Any chance this feature could make it into 5.6?

[michael-o]

@michael-o Any chance this feature could make it into 5.6?

Do you have a timeframe for 5.6?

[ok2c]

@michael-o Any chance this feature could make it into 5.6?

Do you have a timeframe for 5.6?

@michael-o I would like to release 5.6-alpha1 next week. However, a GA is unlikely sooner than Q2, so there is still some time, given this feature has no major API changes and can be added quite late in the release cycle.