-
Notifications
You must be signed in to change notification settings - Fork 233
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CALCITE-6280] The Jetty's version number leak occurred while using he avatica http server #239
Conversation
…he avatica http server Change-Id: I1e0f1092a91e6a23a0f1c7eea92874553285ef0b
server/src/test/java/org/apache/calcite/avatica/server/HttpServerVersionNotReturnedTest.java
Outdated
Show resolved
Hide resolved
… class. Change-Id: I1e0f1092a91e6a23a0f1c7eea92874553285ef0b
7a7062b
to
ee21a7e
Compare
@@ -186,6 +187,14 @@ private static void setupUsers(File keytabDir) throws KrbException { | |||
assertEquals(401, conn.getResponseCode()); | |||
} | |||
|
|||
@Test public void testServerVersionNotReturnedForUnauthorisedAccess() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While technically any ot the Authentication tests are fine, this is not related to Kerberos in any way.
I think we should either
- add this to every authenitcation test
- add this to the BASIC test
Even though we know that setting is independent of the authentication used (it's not even AAA specific)
TDD principles would dictate the we add this to every authentication test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stoty
It will be overhead if we add to every authentication test or even base test. Every time the logic will be executed.
So, I am wondering if a separate class (which I had proposed earlier) can solve this issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My gut feeling is that the overhead a single HTTP call is minial.
Setting up and starting the Avatica server jetty etc is the expensive operation.
We'd have to add the new method to a huge number of test classes to make starting a new environment worthwile.
You may want to log the start and stop milliseconds in the test to see if the runtime of the new time is signifcant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the test runtime is insignificant (119ms). The Jetty server mentioned in the HTTP Response header is not associated with any authentication mechanism or SSL configurations. I opted for unauthorized access scenarios during validation because they are easy to reproduce. However, I am unclear about the rationale behind invoking it from every class.
Could you please elaborate on which specific tests require the inclusion of this new method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rationale is that the tests are supposed to be behavioural based, and not build on implementation details.
We know that there is a single switch in Jetty, and it applies to every message that Jetty sends, so testing for one case is fine.
However, in theory it is possible that the Jetty implementation changes in the future, and the behaviour wil not be the same for the different authentication methods, and the swich will work for SPENGO or BASIC authenticaton, but not the other one.
If we want to test for this - very unlikely - possibility, then we need to add the test to every variant, i.e. every child of the HttpAuthBase class.
In reality, TDD has its limits, and we cannot write a test case for everything, neither do we have the resources to run them, so there is a lot of room for interpretation. The chances of Jetty deliberately changing the behaviour of the flag is very slim, but there is also a very slight chance of a future Jetty bug that would change the behaviour, which this test might catch.
Having the test only in the most basic (no pun intended) BasicAuthHttpServerTest is also fine by me, choose whichever one you like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @stoty for the explanation.
I have added the new check to few more Test classes. Not it covers basic, spnego, digest etc. authentication. So we have almost covered most of the cases.
…ew more Test classes. Change-Id: I87ffd4decf520674f5791c2770533cf6d2dcf55f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 LGTM
merged manually. |
[CALCITE-6280] The Jetty's version number leak occurred while using he avatica http server
Change-Id: I1e0f1092a91e6a23a0f1c7eea92874553285ef0b