Re: [9] RFR: 8166530: sun/net/www/protocol/https/HttpsClient/ProxyAuthTest.java fails intermittently

[Artem Smotrakov]

Hello,

Please take a look.

Artem


On 10/07/2016 01:33 PM, Artem Smotrakov wrote:

Hello,

Please review the patch below for 
sun/net/www/protocol/https/HttpsClient/ProxyAuthTest.java test.


The test has been observed to fail intermittently, but the failure is 
not reproducible standalone. The patch updates the test to follow the 
approach from SSLSocketSample.java


http://hg.openjdk.java.net/jdk9/dev/jdk/file/1f044f413e6c/test/javax/net/ssl/templates/SSLSocketSample.java 



I merged OriginServer.java to ProxyAuthTest.java since only this test 
uses that file. I also added a couple of new common methods to 
SSLTest.java. They are not used by ProxyAuthTest.java, but can be 
useful in other tests.


Bug: https://bugs.openjdk.java.net/browse/JDK-8166530
Webrev: http://cr.openjdk.java.net/~asmotrak/8166530/webrev.00/

Artem

[9] RFR: 8166530: sun/net/www/protocol/https/HttpsClient/ProxyAuthTest.java fails intermittently

[Artem Smotrakov]

Hello,

Please review the patch below for 
sun/net/www/protocol/https/HttpsClient/ProxyAuthTest.java test.


The test has been observed to fail intermittently, but the failure is 
not reproducible standalone. The patch updates the test to follow the 
approach from SSLSocketSample.java


http://hg.openjdk.java.net/jdk9/dev/jdk/file/1f044f413e6c/test/javax/net/ssl/templates/SSLSocketSample.java

I merged OriginServer.java to ProxyAuthTest.java since only this test 
uses that file. I also added a couple of new common methods to 
SSLTest.java. They are not used by ProxyAuthTest.java, but can be useful 
in other tests.


Bug: https://bugs.openjdk.java.net/browse/JDK-8166530
Webrev: http://cr.openjdk.java.net/~asmotrak/8166530/webrev.00/

Artem

Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException

[Artem Smotrakov]

Hello,

If you don't mind, I moved some common code from SSLSocketTemplate.java 
to SSLTest.java, so that it can be re-used by other tests. It may help 
to avoid duplicate code.


http://cr.openjdk.java.net/~asmotrak/8164591/webrev.01/

Please take a look.

Artem


On 09/15/2016 11:49 AM, Artem Smotrakov wrote:

Hi Xuelei, Chris,

Thank you for looking into it. Please see inline.


On 09/15/2016 12:53 AM, Chris Hegarty wrote:

On 15 Sep 2016, at 02:55, Xuelei Fan  wrote:

On 9/15/2016 9:45 AM, Artem Smotrakov wrote:
Well, in this particular case it's not clear that it has the same 
issue
with free port (at least to me). The exception occurred on client 
side,
so it's not the case where we don't know where the handshake came 
from.


;-) Yeh, you catch the point.  But there is a free-port issue 
although the exception stack in the bug description may be not that 
case.


Let's look at a scenarios:
1. server open a server socket and listen.
2. other test case connect to the server socket.
3. this test case try to connect to server socket.
4. this test case would fail as the server only accept one connections.

I did not check it very carefully, I think for #4, the exception 
stack can be similar to the one in the bug description.

Looks like a rare, but valid case.


Anyway, as a free port is used, there are free-port issues. Please 
consider to make the enhancement in the fix. Otherwise, you cannot 
avoid the intermittent failure for this test case in the current 
testing environment.

+1.   Please remove any use of the free-port anti-pattern.
Just to be clear, this is not an issue with getting a free port with 
ServerSocket.getLocalPort() and them re-using it to create a new 
ServerSocket. It's more tricky (for example, please see the scenario 
above).


Okay, let me update it to follow the approach which is implemented in 
SSLSocketSample.java


Artem


-Chris.


Xuelei

I can make this enhancement, but like I said I don't think it's 
going to

help, so I would like to keep debug output on.

Artem


On 09/14/2016 06:39 PM, Xuelei Fan wrote:

On 9/15/2016 9:23 AM, Artem Smotrakov wrote:

Hi Xuelei,

For this one, I am not sure that it would help here since the test
failed after it already had started handshaking.

It has the same issue as a free-port is used.  We don't actually know
the handshake is coming from the right client.

Xuelei


I would prefer to have it as a separate enhancement.

Artem


On 09/14/2016 06:19 PM, Xuelei Fan wrote:

As you were already there, I would suggest to consider the
SSLSocketSample.java template as the comment in JDK-8163924 review
thread.

Thanks,
Xuelei

On 9/15/2016 9:13 AM, Artem Smotrakov wrote:
Not urgent, but I would appreciate if someone can get a chance 
to look

at this.

Artem


On 09/07/2016 03:17 PM, Artem Smotrakov wrote:

Sending to net-dev@openjdk.java.net as well.

Artem


On 09/07/2016 12:28 PM, Artem Smotrakov wrote:

Hello,

Please review the following patch for
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java

The test has been observed to fail a couple of times, but 
it's still
not clear why it failed because there is not much info in 
logs. The
patch updates the test to enable additional debug output, so 
that we

have more info if it fails next time.

While looking at the test, I notices a couple of issues, but 
they

don't seem to cause these intermittent failures:
- The test sets system properties for JSSE in a loop, but JSSE
provider reads them only once while initialization. As a result,
only
values which were set in the first iteration are actually used.
- The test doesn't close files and sockets sometimes.

The patch also fixed the issues above, and there are a couple
cosmetic changes.

Bug: https://bugs.openjdk.java.net/browse/JDK-8164591
Webrev: http://cr.openjdk.java.net/~asmotrak/8164591/webrev.00/

Artem

Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException

[Artem Smotrakov]

Hi Xuelei, Chris,

Thank you for looking into it. Please see inline.


On 09/15/2016 12:53 AM, Chris Hegarty wrote:

On 15 Sep 2016, at 02:55, Xuelei Fan  wrote:

On 9/15/2016 9:45 AM, Artem Smotrakov wrote:

Well, in this particular case it's not clear that it has the same issue
with free port (at least to me). The exception occurred on client side,
so it's not the case where we don't know where the handshake came from.


;-) Yeh, you catch the point.  But there is a free-port issue although the 
exception stack in the bug description may be not that case.

Let's look at a scenarios:
1. server open a server socket and listen.
2. other test case connect to the server socket.
3. this test case try to connect to server socket.
4. this test case would fail as the server only accept one connections.

I did not check it very carefully, I think for #4, the exception stack can be 
similar to the one in the bug description.

Looks like a rare, but valid case.


Anyway, as a free port is used, there are free-port issues.  Please consider to 
make the enhancement in the fix.  Otherwise, you cannot avoid the intermittent 
failure for this test case in the current testing environment.

+1.   Please remove any use of the free-port anti-pattern.
Just to be clear, this is not an issue with getting a free port with 
ServerSocket.getLocalPort() and them re-using it to create a new 
ServerSocket. It's more tricky (for example, please see the scenario above).


Okay, let me update it to follow the approach which is implemented in 
SSLSocketSample.java


Artem


-Chris.


Xuelei


I can make this enhancement, but like I said I don't think it's going to
help, so I would like to keep debug output on.

Artem


On 09/14/2016 06:39 PM, Xuelei Fan wrote:

On 9/15/2016 9:23 AM, Artem Smotrakov wrote:

Hi Xuelei,

For this one, I am not sure that it would help here since the test
failed after it already had started handshaking.

It has the same issue as a free-port is used.  We don't actually know
the handshake is coming from the right client.

Xuelei


I would prefer to have it as a separate enhancement.

Artem


On 09/14/2016 06:19 PM, Xuelei Fan wrote:

As you were already there, I would suggest to consider the
SSLSocketSample.java template as the comment in JDK-8163924 review
thread.

Thanks,
Xuelei

On 9/15/2016 9:13 AM, Artem Smotrakov wrote:

Not urgent, but I would appreciate if someone can get a chance to look
at this.

Artem


On 09/07/2016 03:17 PM, Artem Smotrakov wrote:

Sending to net-dev@openjdk.java.net as well.

Artem


On 09/07/2016 12:28 PM, Artem Smotrakov wrote:

Hello,

Please review the following patch for
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java

The test has been observed to fail a couple of times, but it's still
not clear why it failed because there is not much info in logs. The
patch updates the test to enable additional debug output, so that we
have more info if it fails next time.

While looking at the test, I notices a couple of issues, but they
don't seem to cause these intermittent failures:
- The test sets system properties for JSSE in a loop, but JSSE
provider reads them only once while initialization. As a result,
only
values which were set in the first iteration are actually used.
- The test doesn't close files and sockets sometimes.

The patch also fixed the issues above, and there are a couple
cosmetic changes.

Bug: https://bugs.openjdk.java.net/browse/JDK-8164591
Webrev: http://cr.openjdk.java.net/~asmotrak/8164591/webrev.00/

Artem

Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException

[Artem Smotrakov]

Well, in this particular case it's not clear that it has the same issue 
with free port (at least to me). The exception occurred on client side, 
so it's not the case where we don't know where the handshake came from.


I can make this enhancement, but like I said I don't think it's going to 
help, so I would like to keep debug output on.


Artem


On 09/14/2016 06:39 PM, Xuelei Fan wrote:

On 9/15/2016 9:23 AM, Artem Smotrakov wrote:

Hi Xuelei,

For this one, I am not sure that it would help here since the test
failed after it already had started handshaking.
It has the same issue as a free-port is used.  We don't actually know 
the handshake is coming from the right client.


Xuelei


I would prefer to have it as a separate enhancement.

Artem


On 09/14/2016 06:19 PM, Xuelei Fan wrote:

As you were already there, I would suggest to consider the
SSLSocketSample.java template as the comment in JDK-8163924 review
thread.

Thanks,
Xuelei

On 9/15/2016 9:13 AM, Artem Smotrakov wrote:

Not urgent, but I would appreciate if someone can get a chance to look
at this.

Artem


On 09/07/2016 03:17 PM, Artem Smotrakov wrote:

Sending to net-dev@openjdk.java.net as well.

Artem


On 09/07/2016 12:28 PM, Artem Smotrakov wrote:

Hello,

Please review the following patch for
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java

The test has been observed to fail a couple of times, but it's still
not clear why it failed because there is not much info in logs. The
patch updates the test to enable additional debug output, so that we
have more info if it fails next time.

While looking at the test, I notices a couple of issues, but they
don't seem to cause these intermittent failures:
- The test sets system properties for JSSE in a loop, but JSSE
provider reads them only once while initialization. As a result, 
only

values which were set in the first iteration are actually used.
- The test doesn't close files and sockets sometimes.

The patch also fixed the issues above, and there are a couple
cosmetic changes.

Bug: https://bugs.openjdk.java.net/browse/JDK-8164591
Webrev: http://cr.openjdk.java.net/~asmotrak/8164591/webrev.00/

Artem

Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException

[Artem Smotrakov]

Hi Xuelei,

For this one, I am not sure that it would help here since the test 
failed after it already had started handshaking. I would prefer to have 
it as a separate enhancement.


Artem


On 09/14/2016 06:19 PM, Xuelei Fan wrote:
As you were already there, I would suggest to consider the 
SSLSocketSample.java template as the comment in JDK-8163924 review 
thread.


Thanks,
Xuelei

On 9/15/2016 9:13 AM, Artem Smotrakov wrote:

Not urgent, but I would appreciate if someone can get a chance to look
at this.

Artem


On 09/07/2016 03:17 PM, Artem Smotrakov wrote:

Sending to net-dev@openjdk.java.net as well.

Artem


On 09/07/2016 12:28 PM, Artem Smotrakov wrote:

Hello,

Please review the following patch for
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java

The test has been observed to fail a couple of times, but it's still
not clear why it failed because there is not much info in logs. The
patch updates the test to enable additional debug output, so that we
have more info if it fails next time.

While looking at the test, I notices a couple of issues, but they
don't seem to cause these intermittent failures:
- The test sets system properties for JSSE in a loop, but JSSE
provider reads them only once while initialization. As a result, only
values which were set in the first iteration are actually used.
- The test doesn't close files and sockets sometimes.

The patch also fixed the issues above, and there are a couple
cosmetic changes.

Bug: https://bugs.openjdk.java.net/browse/JDK-8164591
Webrev: http://cr.openjdk.java.net/~asmotrak/8164591/webrev.00/

Artem

Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException

[Artem Smotrakov]

Not urgent, but I would appreciate if someone can get a chance to look 
at this.


Artem


On 09/07/2016 03:17 PM, Artem Smotrakov wrote:

Sending to net-dev@openjdk.java.net as well.

Artem


On 09/07/2016 12:28 PM, Artem Smotrakov wrote:

Hello,

Please review the following patch for 
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java


The test has been observed to fail a couple of times, but it's still 
not clear why it failed because there is not much info in logs. The 
patch updates the test to enable additional debug output, so that we 
have more info if it fails next time.


While looking at the test, I notices a couple of issues, but they 
don't seem to cause these intermittent failures:
- The test sets system properties for JSSE in a loop, but JSSE 
provider reads them only once while initialization. As a result, only 
values which were set in the first iteration are actually used.

- The test doesn't close files and sockets sometimes.

The patch also fixed the issues above, and there are a couple 
cosmetic changes.


Bug: https://bugs.openjdk.java.net/browse/JDK-8164591
Webrev: http://cr.openjdk.java.net/~asmotrak/8164591/webrev.00/

Artem

Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException

[Artem Smotrakov]

Sending to net-dev@openjdk.java.net as well.

Artem


On 09/07/2016 12:28 PM, Artem Smotrakov wrote:

Hello,

Please review the following patch for 
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java


The test has been observed to fail a couple of times, but it's still 
not clear why it failed because there is not much info in logs. The 
patch updates the test to enable additional debug output, so that we 
have more info if it fails next time.


While looking at the test, I notices a couple of issues, but they 
don't seem to cause these intermittent failures:
- The test sets system properties for JSSE in a loop, but JSSE 
provider reads them only once while initialization. As a result, only 
values which were set in the first iteration are actually used.

- The test doesn't close files and sockets sometimes.

The patch also fixed the issues above, and there are a couple cosmetic 
changes.


Bug: https://bugs.openjdk.java.net/browse/JDK-8164591
Webrev: http://cr.openjdk.java.net/~asmotrak/8164591/webrev.00/

Artem

[9] RFR: 8164592: java/net/MulticastSocket/NoLoopbackPackets.java tests may leave a daemon thread

[Artem Smotrakov]

Hello,

Please review the following patch for NoLoopbackPackets.java test.

The test starts a daemon thread which has an infinite loop. If jtreg 
uses the same JVM to run multiple tests (agent VM), then this thread 
will be keep running until the agent VM stops. This is not good, it 
would be better to stop the thread when the test is done.


Webrev: http://cr.openjdk.java.net/~asmotrak/8164592/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8164592

Artem

Re: RFR 8022580: sun.net.ftp.impl.FtpClient.nameList(null)

[Artem Smotrakov]

Hi Svetlana,

Thank you for addressing the comments below. The test looks fine to me. 
Just a couple of minor comments.


1. You don't really need fields in lines 77-79.

2. try-catch block in line 86 is not really necessary as well.

It would be better to update bug subject to something more meaningful.

Artem

On 07/07/2016 08:31 AM, Svetlana Nikandrova wrote:

Artem,

thank you for suggested test improvements. Here is updated webrev:
http://cr.openjdk.java.net/~snikandrova/8022580/webrev.02/ 
<http://cr.openjdk.java.net/%7Esnikandrova/8022580/webrev.02/>


Chris,

seem like you have already looked into that issue before. May be you 
can find some time to review this fix?


Thank you,
Svetlana

On 06.07.2016 20:35, Artem Smotrakov wrote:

Hi Svetlana,

Thanks for addressing my comments. Could you please take a look at a 
couple of comments about TestFtpClientNameListWithNull.java below?


1. lines 64-70: should "client" be closed in "finally" block? I also 
think it might be better to re-throw IOExceptions instead of ignoring 
them (it may hide some problems). You can just add "throws" to main() 
method.


2. line 107: you close "out" in a couple of places in handelClient() 
method, it might be better to add a "finally" block, and shutdown 
everything there.


3. lines 176-182: you can close a client socket in handelClient(). 
Since it's a test for JDK 9, you can use try-with-resources like the 
following:


public void handelClient(Socket cl) {
...
try (cl) {
...
}


(please note that it doesn't work for JDK 8)

It would reduce the code and simplify the test (you would avoid some 
"try" blocks).


I am not sure that you need to close "out" if you close the socket.

4. Typo: handelClient -> handleClient

Artem

On 07/06/2016 09:11 AM, Svetlana Nikandrova wrote:

Hi Artem,

thank you for your replay. I believe I addressed all your comments. 
Please see updated diff:
http://cr.openjdk.java.net/~snikandrova/8022580/webrev.01/ 
<http://cr.openjdk.java.net/%7Esnikandrova/8022580/webrev.01/>


Thank you,
Svetlana

On 05.07.2016 20:54, Artem Smotrakov wrote:

Hi Svetlana,

I am not an official reviewer, but I have a couple comments below.

TestFtpClientNameListWithNull.java:

1. Shouldn't "commandHasArgs" be volatile? Looks like it may cause 
intermittent failures. As a sanity check, you may want to run the 
test in a loop to make sure it is stable.


2. You may want to make FtpServer implement AutoCloseable 
(terminate() method just becomes close()). Then, you can use it in 
try-with-resource block which would simplify the code (you can 
avoid a couple of nested try-catch blocks).


By the way, it might be good to make sun.net.ftp.FtpClient 
implement AutoCloseable as well, but probably it should be a 
separate enhancement.


3. line 62-63, 66: should it be in "finally" block?

4. How many connection is FtpServer supposed to handle in this 
test? If only one, it might be better to remove the "while" loop 
and "done" variable to simplify the code.


5. Is it necessary to handle a client connections in a separate 
thread? Avoiding too many threads would simplify the test.


6. The test ignores a couple of IOException, it might be better to 
fail it they occur.


7. Why is it necessary to use daemons?

8. Please use braces for "if" statements, see Java Coding Conventions.

FtpClient.java: please update copyright year.

Artem

On 07/05/2016 07:40 AM, Svetlana Nikandrova wrote:

Hello,

please review this fix for bug
https://bugs.openjdk.java.net/browse/JDK-8022580
Webrev:
http://cr.openjdk.java.net/~msolovie/8022580/webrev.00/ 
<http://cr.openjdk.java.net/%7Emsolovie/8022580/webrev.00/>


Description:
There is no handling for null path parameter in the method 
sun.net.ftp.impl.FtpClient#nameList(), while javadoc says that 
"@param path a String containing the pathname of the directory to 
list or null for the current working directory". Changeset adds 
check that if param is null NLST will be sent without parameter 
(no-parameter default is current directory).


JPRT tested.

Thank you,
Svetlana

Re: RFR 8022580: sun.net.ftp.impl.FtpClient.nameList(null)

[Artem Smotrakov]

Hi Svetlana,

Thanks for addressing my comments. Could you please take a look at a 
couple of comments about TestFtpClientNameListWithNull.java below?


1. lines 64-70: should "client" be closed in "finally" block? I also 
think it might be better to re-throw IOExceptions instead of ignoring 
them (it may hide some problems). You can just add "throws" to main() 
method.


2. line 107: you close "out" in a couple of places in handelClient() 
method, it might be better to add a "finally" block, and shutdown 
everything there.


3. lines 176-182: you can close a client socket in handelClient(). Since 
it's a test for JDK 9, you can use try-with-resources like the following:


public void handelClient(Socket cl) {
...
try (cl) {
...
}


(please note that it doesn't work for JDK 8)

It would reduce the code and simplify the test (you would avoid some 
"try" blocks).


I am not sure that you need to close "out" if you close the socket.

4. Typo: handelClient -> handleClient

Artem

On 07/06/2016 09:11 AM, Svetlana Nikandrova wrote:

Hi Artem,

thank you for your replay. I believe I addressed all your comments. 
Please see updated diff:
http://cr.openjdk.java.net/~snikandrova/8022580/webrev.01/ 
<http://cr.openjdk.java.net/%7Esnikandrova/8022580/webrev.01/>


Thank you,
Svetlana

On 05.07.2016 20:54, Artem Smotrakov wrote:

Hi Svetlana,

I am not an official reviewer, but I have a couple comments below.

TestFtpClientNameListWithNull.java:

1. Shouldn't "commandHasArgs" be volatile? Looks like it may cause 
intermittent failures. As a sanity check, you may want to run the 
test in a loop to make sure it is stable.


2. You may want to make FtpServer implement AutoCloseable 
(terminate() method just becomes close()). Then, you can use it in 
try-with-resource block which would simplify the code (you can avoid 
a couple of nested try-catch blocks).


By the way, it might be good to make sun.net.ftp.FtpClient implement 
AutoCloseable as well, but probably it should be a separate enhancement.


3. line 62-63, 66: should it be in "finally" block?

4. How many connection is FtpServer supposed to handle in this test? 
If only one, it might be better to remove the "while" loop and "done" 
variable to simplify the code.


5. Is it necessary to handle a client connections in a separate 
thread? Avoiding too many threads would simplify the test.


6. The test ignores a couple of IOException, it might be better to 
fail it they occur.


7. Why is it necessary to use daemons?

8. Please use braces for "if" statements, see Java Coding Conventions.

FtpClient.java: please update copyright year.

Artem

On 07/05/2016 07:40 AM, Svetlana Nikandrova wrote:

Hello,

please review this fix for bug
https://bugs.openjdk.java.net/browse/JDK-8022580
Webrev:
http://cr.openjdk.java.net/~msolovie/8022580/webrev.00/ 
<http://cr.openjdk.java.net/%7Emsolovie/8022580/webrev.00/>


Description:
There is no handling for null path parameter in the method 
sun.net.ftp.impl.FtpClient#nameList(), while javadoc says that 
"@param path a String containing the pathname of the directory to 
list or null for the current working directory". Changeset adds 
check that if param is null NLST will be sent without parameter 
(no-parameter default is current directory).


JPRT tested.

Thank you,
Svetlana

Re: RFR 8022580: sun.net.ftp.impl.FtpClient.nameList(null)

[Artem Smotrakov]

Hi Svetlana,

I am not an official reviewer, but I have a couple comments below.

TestFtpClientNameListWithNull.java:

1. Shouldn't "commandHasArgs" be volatile? Looks like it may cause 
intermittent failures. As a sanity check, you may want to run the test 
in a loop to make sure it is stable.


2. You may want to make FtpServer implement AutoCloseable (terminate() 
method just becomes close()). Then, you can use it in try-with-resource 
block which would simplify the code (you can avoid a couple of nested 
try-catch blocks).


By the way, it might be good to make sun.net.ftp.FtpClient implement 
AutoCloseable as well, but probably it should be a separate enhancement.


3. line 62-63, 66: should it be in "finally" block?

4. How many connection is FtpServer supposed to handle in this test? If 
only one, it might be better to remove the "while" loop and "done" 
variable to simplify the code.


5. Is it necessary to handle a client connections in a separate thread? 
Avoiding too many threads would simplify the test.


6. The test ignores a couple of IOException, it might be better to fail 
it they occur.


7. Why is it necessary to use daemons?

8. Please use braces for "if" statements, see Java Coding Conventions.

FtpClient.java: please update copyright year.

Artem

On 07/05/2016 07:40 AM, Svetlana Nikandrova wrote:

Hello,

please review this fix for bug
https://bugs.openjdk.java.net/browse/JDK-8022580
Webrev:
http://cr.openjdk.java.net/~msolovie/8022580/webrev.00/ 



Description:
There is no handling for null path parameter in the method 
sun.net.ftp.impl.FtpClient#nameList(), while javadoc says that "@param 
path a String containing the pathname of the directory to list or null 
for the current working directory". Changeset adds check that if param 
is null NLST will be sent without parameter (no-parameter default is 
current directory).


JPRT tested.

Thank you,
Svetlana

Re: RFR 8136933: Additional tests for Solaris SO_FLOW_SLA socket option in JDK 9

[Artem Smotrakov]

Hi Svetlana,

Please see a couple of comments below. I'll leave the final review to 
official reviewers.


1. You may use test/lib/testlibrary/jdk/testlibrary/OSInfo.java to check 
OS type. I think it also may be good to add getSolarisVersion() to 
OSInfo.java (see getWindowsVersion() method)


You can also use ProcessTools.java in getSolarisVersion()

2. line 46: Did you mean java.lang.String

3. Should it throw an exception after lines 173 and 181? Or, this may 
depend on other constraints? I see you added a comment for 
checkSocketOption() method, but it maybe better to clarify why no 
exception is also fine.


I am also wondering if it should expect no exception in getOption() if 
setOption() succeeded.


Artem

On 05/19/2016 09:13 AM, Svetlana Nikandrova wrote:

Hello,

please review additional test for Solaris SO_FLOW_SLA socket option.
Test checks that SO_FLOW_SLA option is supported on Solaris 11.2+ and 
not supported on other platforms.


http://cr.openjdk.java.net/~snikandrova/8136933/webrev.00/ 



Thank you,
Svetlana

[9] RFR: 8157107: HTTP/2 client may fail with NPE if additional logging enabled

[Artem Smotrakov]

Hello,

Please review this patch for 9.

NPE may occur if additional logging is enabled with 
"java.net.http.HttpClient.log" system property because 
AsyncSSLDelegate.logParams(SSLParameters) doesn't check for null values 
which were returned by SSLParameters instance.


I also noticed that setting "java.net.http.HttpClient.log" system 
property to "all" doesn't enable additional logging for "ssl". Not sure 
if that was done for some purpose.


Changes:
- added a couple of checks to AsyncSSLDelegate.logParams(SSLParameters) 
method to prevent NPEs
- updated java/net/http/Log.java to enable logging for "ssl" if 
"java.net.http.HttpClient.log" system property was set to "all"
- updated java/net/http/Http2TestServer.java to implement AutoCloseable, 
so that it's possible to use it in try-with-resource blocks (which 
simplifies tests a little bit)
- added TLSConnection.java test which run HTTP/2 client with different 
SSL parameters, and checks that they were actually used

- a couple of cosmetic changes (unused imports, etc)

TLSConnection.java test also checks that 
https://bugs.openjdk.java.net/browse/JDK-8150769 was fixed (this bug has 
been fixed while HTTP/2 integration).


Bug: https://bugs.openjdk.java.net/browse/JDK-8157107
Webrev: http://cr.openjdk.java.net/~asmotrak/8157107/webrev.01/

Artem

[9] RFR: 8156710: HttpTimeoutException should be thrown if server doesn't respond

[Artem Smotrakov]

Hello,

Please review this for 9.

Stream.getResponse() method can hang because it calls join() method 
without any timeout. It would be better if it took into account a 
timeout value specified for a connection, and threw HttpTimeoutException 
if timeout was reached.


The patch updates Stream.getResponse() method to use 
CompletableFuture.get() method with timeout if a timeout value was 
specified for HTTPRequest.


Timeout.java test starts a TLS server which just reads incoming data, 
but doesn't reply anything. The client sets a timeout with 
HTTPRequestBuilder.Builder.timeout() method, and then expects 
HttpTimeoutException.


The test initializes JSSE with standard way by system properties. RSA 
keys are located in keystore.p12 which can be re-used by other tests. 
The keys were created by the following command:


keytool -genkey -alias rsa2048keypair -keystore keystore.p12 -storepass 
password -keypass password -validity 3650 -keysize 2048 -dname "CN=Test 
RSA 2048 key"


Bug: https://bugs.openjdk.java.net/browse/JDK-8156710
Webrev: http://cr.openjdk.java.net/~asmotrak/8156710/webrev.01/

Artem

Re: [9] RFR: 8138990: Implementation of HTTP Digest authentication may be more flexible

[Artem Smotrakov]

Hi Michael,

On 01/04/2016 02:28 AM, Michael McMahon wrote:

On 30/12/15 03:22, Artem Smotrakov wrote:

Hi Michael,

Thanks for review, it looks like BNF notation uses only a comma as a 
separator


http://www.w3.org/Notation.html

...
#element

indicating at least l and at most m elements, each separated by one 
or more commas (",").

...



Hi Artem,

The BNF definitions used in RFC 2617 come from section 2.1 of RFC 2616
which allows linear white space between tokens.

#rule
   A construct "#" is defined, similar to "*", for defining lists of
   elements. The full form is "#element" indicating at least
and at most  elements, each separated by one or more commas
   (",") and OPTIONAL linear white space (LWS).
If I read it correctly, it means that a comma is used as a separator, 
but there also may be a linear whitespace after a comma. So for example 
"auth,auth-int" and "auth, auth-int" are equal, and should result to a 
list of two elements. Current version of webrev seems to follow it 
(first, it splits a string by a comma, and then ignores white spaces):


http://cr.openjdk.java.net/~asmotrak/http_auth_digest/webrev.01/

Am I missing something?

Artem



So, I think we should be conservative and check for white-space.

- Michael.
Michael

And here is "qop" definition from https://tools.ietf.org/html/rfc2617

...
  qop-options   = "qop" "=" <"> 1#qop-value <">
  qop-value = "auth" | "auth-int" | token
...

Please take a look at updated webrev:

http://cr.openjdk.java.net/~asmotrak/http_auth_digest/webrev.01/

Artem

On 12/22/2015 05:59 AM, Michael McMahon wrote:

Hi Artem,


On 04/12/15 11:41, Artem Smotrakov wrote:

Hello,

Please review this small fix for DigestAuthentication class.

1. Added a check in DigestAuthentication.setNonce(String) that 
nonce is not null. NPE may happen if a buggy HTTP server returns 
"WWW-Authenticate" header which doesn't contain a "nonce" field. 
According to RFCs 2069 [1] and 2617 [2], this is not expected 
behaviour, but it would be better if an HTTP client threw a checked 
IOException instead of NPE.




That's fine.

2. Updated DigestAuthentication.setQop(String) method to accept 
both a whitespace and a comma as a delimiter. RFC 2617 [2] says 
that "qop" may contain more than one token, but it doesn't specify 
a delimiter for "qop" field in "WWW-Authenticate" header. There is 
an example of "WWW-Authenticate" header in RFC 2617 [2] where a 
comma is used as a delimiter of value in "qop" field.




It looks like the BNF specification mandates a comma and optional 
linear white space.
So, the old code was buggy, but we didn't see the problem because 
there is typically
only at most ever one value used for the qop field. But, to be 
strictly correct, we would
have to check for TABs also. So, I think the correct behavior is to 
delimit using comma

and remove any white space

- Michael.


3. Added a test for Digest authentication.

Bug: https://bugs.openjdk.java.net/browse/JDK-8138990
Webrev: 
http://cr.openjdk.java.net/~asmotrak/http_auth_digest/webrev.00/


[1] https://tools.ietf.org/html/rfc2069
[2] https://tools.ietf.org/html/rfc2617

Artem

Re: [9] RFR: 8138990: Implementation of HTTP Digest authentication may be more flexible

[Artem Smotrakov]

Hi Michael,

Thanks for review, it looks like BNF notation uses only a comma as a 
separator


http://www.w3.org/Notation.html

...
#element

indicating at least l and at most m elements, each separated by one or 
more commas (",").

...

And here is "qop" definition from https://tools.ietf.org/html/rfc2617

...
  qop-options   = "qop" "=" <"> 1#qop-value <">
  qop-value = "auth" | "auth-int" | token
...

Please take a look at updated webrev:

http://cr.openjdk.java.net/~asmotrak/http_auth_digest/webrev.01/

Artem

On 12/22/2015 05:59 AM, Michael McMahon wrote:

Hi Artem,


On 04/12/15 11:41, Artem Smotrakov wrote:

Hello,

Please review this small fix for DigestAuthentication class.

1. Added a check in DigestAuthentication.setNonce(String) that nonce 
is not null. NPE may happen if a buggy HTTP server returns 
"WWW-Authenticate" header which doesn't contain a "nonce" field. 
According to RFCs 2069 [1] and 2617 [2], this is not expected 
behaviour, but it would be better if an HTTP client threw a checked 
IOException instead of NPE.




That's fine.

2. Updated DigestAuthentication.setQop(String) method to accept both 
a whitespace and a comma as a delimiter. RFC 2617 [2] says that "qop" 
may contain more than one token, but it doesn't specify a delimiter 
for "qop" field in "WWW-Authenticate" header. There is an example of 
"WWW-Authenticate" header in RFC 2617 [2] where a comma is used as a 
delimiter of value in "qop" field.




It looks like the BNF specification mandates a comma and optional 
linear white space.
So, the old code was buggy, but we didn't see the problem because 
there is typically
only at most ever one value used for the qop field. But, to be 
strictly correct, we would
have to check for TABs also. So, I think the correct behavior is to 
delimit using comma

and remove any white space

- Michael.


3. Added a test for Digest authentication.

Bug: https://bugs.openjdk.java.net/browse/JDK-8138990
Webrev: http://cr.openjdk.java.net/~asmotrak/http_auth_digest/webrev.00/

[1] https://tools.ietf.org/html/rfc2069
[2] https://tools.ietf.org/html/rfc2617

Artem

[9] RFR: 8138990: Implementation of HTTP Digest authentication may be more flexible

[Artem Smotrakov]

Hello,

Please review this small fix for DigestAuthentication class.

1. Added a check in DigestAuthentication.setNonce(String) that nonce is 
not null. NPE may happen if a buggy HTTP server returns 
"WWW-Authenticate" header which doesn't contain a "nonce" field. 
According to RFCs 2069 [1] and 2617 [2], this is not expected behaviour, 
but it would be better if an HTTP client threw a checked IOException 
instead of NPE.


2. Updated DigestAuthentication.setQop(String) method to accept both a 
whitespace and a comma as a delimiter. RFC 2617 [2] says that "qop" may 
contain more than one token, but it doesn't specify a delimiter for 
"qop" field in "WWW-Authenticate" header. There is an example of 
"WWW-Authenticate" header in RFC 2617 [2] where a comma is used as a 
delimiter of value in "qop" field.


3. Added a test for Digest authentication.

Bug: https://bugs.openjdk.java.net/browse/JDK-8138990
Webrev: http://cr.openjdk.java.net/~asmotrak/http_auth_digest/webrev.00/

[1] https://tools.ietf.org/html/rfc2069
[2] https://tools.ietf.org/html/rfc2617

Artem

Re: RFR [9] 8143554: UnsupportedOperationException is not thrown for unsupported options

[Artem Smotrakov]

Hi Svetlana,

I'll leave the mail review to official reviewers, a couple of minor 
comments about your test.


It seems to work fine, but you may want to use "else if" in 
UnsupportedOptionsTest.getOption() method because "Unsupported socket 
type" error can occure in case of supported socket type.


Not sure that IllegalAccessException can be thrown in main() method.

Artem

On 12/03/2015 04:06 PM, Svetlana Nikandrova wrote:

Hello,

please review a simple fix for:
https://bugs.openjdk.java.net/browse/JDK-8143554

See webrev here:
http://cr.openjdk.java.net/~kshefov/8143554/webrev.00/ 



Fix added explicit check for option support to getOption and setOption 
sockets' methods similar way as it was done prior jdk 9 in Sockets class.
Fix also exposed another problem with sockets' supported options: 
JDK-8143923  that 
cause new test failure test/java/net/SocketOption/OptionsTest.java. I 
added test to the ProblemList.


Thank you,
Svetlana

Re: RFR: JDK-8134577 - Eliminate or standardize a replacement for sun.net.spi.nameservice.NameServiceDescriptor

[Artem Smotrakov]

Hi Mark,

I am not a reviewer, just have a couple of comments about InetAddress.java

1. It may be better to create an instance of Scanner in 
try-with-resource block to be sure that Scanner.close() method is called.


2. Lines 909-923:

There are two similar "if" blocks in the loop. Looks like the first one 
is not necessary (I also see similar code in lookupAllHostAddr() method, 
maybe this code could be moved to a separate method).


3. extractHostAddr() and extractHost() methods:

The methods assume that "hostEntry" contains at least one whitespace, 
and access first and second elements of "mapping " array. It may be 
better to check that length of "mapping" is more than one to avoid a 
possible ArrayIndexOutOfBoundsException.


Looks like those methods may also be static.

Artem

On 10/26/2015 02:32 AM, Mark Sheppard wrote:

Hi,
   please oblige and review the following changes
http://cr.openjdk.java.net/~msheppar/8134577/webrev/

which address the issue raised in
https://bugs.openjdk.java.net/browse/JDK-8134577

the operative word has been "eliminate".
As such, the interface and service descriptor 
sun.net.spi.nameservice.NameService

sun.net.spi.nameservice.NameServiceDescriptor*
*together with its implementation (sun.net.nameservice.dns.DNSNameService)
has been remove from the JDK libraries.

The immediate impact is seen in the JDK testing framework.

To facilitate testing activity, and provide a replacement for the 
customized NameService implementations in the

JDK tests,  the default NameService has been extended to support
the retrieval of host to IP address mappings from a file.
The file path is specified with a system property " jdk.internal.hosts".

Previously a nameservice provider was specified by setting the system 
property

"sun.net.spi.nameservice.provider.", as per the documentation
http://docs.oracle.com/javase/8/docs/technotes/guides/net/properties.html

InetAddress now tests to determine if this property is set and will 
throw a ServiceConfigurationError
indicating that this functionality is no longer supported. The choice 
of ServideConfigurationError may cause
some debate, or disagreement. The rationale was that InternalError,  
is documented to relate to a JVM error,

and javax.naming.NamingException has a context of DirContext.
A possible alternative candidate could be 
javax.naming.ServiceUnavailableException.
As such, the setting of the property 
"sun.net.spi.nameservice.provider." was used, previously, as a 
configuration
parameter for the loading of a NamerService service provider, and as 
this is now (considered) an error, ServiceConfigurationError,

seemed a best fit!

These changes impacted a number of jdk security tests, also. The 
affected tetsts have been amended to adopt the
changes, with the exception of 
test/sun/security/x509/URICertStore/ExtensionsWithLDAP.java

which will require some rewrite.

regards
Mark

Re: [9] RFR 8138953: HttpURLConnection doesn't fallback to another auth scheme if negotiate process failed

[Artem Smotrakov]

Hi Max,

Please see inline.

On 10/16/2015 05:18 AM, Wang Weijun wrote:

Let's go back to the bug description:

But no fallback happens if:

1. an HTTP server supports both Negotiate (via Kerberos) and Basic 
authentication schemes
2. first, a user provides correct Kerberos credentials, and a connection is 
successfully established with Negotiate scheme
3. then, a user provides wrong Kerberos credentials, but correct Basic 
credentials

So, with #2, the HTTPP connection already succeeds. When will #3 happen?
At #3, a user creates a new HttpURLConnection instance (in the same 
JVM), and tries to connect to the same HTTP server again. Please see the 
test I added for this bug:


http://cr.openjdk.java.net/~asmotrak/8138953/webrev.02/

Visiting another page on the same server and see another 401?
Yes, it uses the same page on the same HTTP server. I updated the test 
to visit another page, and it fails on JDK 9 b83, and succeeds with the 
fix (please see the webrev above).

If this is a new connection, does HttpURLConnection still remember #2?
Yes, HttpURLConnection is quite smart, and has a number of caches. For 
example, keep-alive cache, cache for auth data (for the same realms only).


Artem


Sorry for asking these. I have always been afraid of HttpURLConnection and 
although I've made some modifications to the code, I never dare say I fully 
understand it, at least not today.

Thanks
Max

Re: [9] RFR 8138953: HttpURLConnection doesn't fallback to another auth scheme if negotiate process failed

[Artem Smotrakov]

Hi Max,

RFC 2617 [1] requires a user agent to use one of the challenges with the 
strongest auth scheme it understands (please see section 1.2):


...
   The user agent MUST
   choose to use one of the challenges with the strongest auth-scheme it
   understands and request credentials from the user based upon that
   challenge.
...

This RFC doesn't mention fallbacks. But Negotiate/Kerberos 
authentication requires remote KDCs (AD controllers) which may be 
temporary down for some reason. In this case, it may be better to 
fallback to another scheme in something went wrong with Negotiate. Just 
to be able to authenticate users if something is wrong with infra.


Taking into account the note about strongest auth scheme above, it might 
probably be better not to fallback to another schemes (like Digest -> 
Basic).


It is possible to use "http.auth.preference" system property to specify 
preferred auth scheme. But setting this property means that the only 
specified scheme should be used (no fallback should happen at all). 
Maybe we can add a method to HttpURLConnection which can set a scheme in 
case of fallback.


[1] http://www.ietf.org/rfc/rfc2617.txt

Artem

On 10/08/2015 11:41 AM, Wang Weijun wrote:

On Oct 7, 2015, at 11:51 PM, Artem Smotrakov  wrote:

Hi Max,

HttpURLConnection obtains credentials for HTTP authentication from 
Authenticator [1] implementation. Only one authenticator can be set in JVM 
instance. It can have built-in credentials, or do some interactions with user 
to get them. Theoretically, it can provide different credentials depending on a 
user/application/etc. I don't know how it is used in real application, but it 
seems to be a possible situation. When I was looking into this, I found a tech 
note [2] which says the following about fallback

...
Fallback
If the server has provided more than one authentication schemes (including 
Negotiate), according to the processing order mentioned in the last section, 
Java will try to challenge the Negotiate scheme. However, if the protocol 
cannot be established successfully (e.g. The kerberos configuration is not 
correct, or the server's hostname is not recorded in the KDC principal DB, or 
the username and password provided by Authenticator is wrong), then the 2nd 
strongest scheme will be automatically used. Attention : If 
http.auth.preference is set to SPNEGO or Kerberos, then we assume you only want 
to try the Negotiate scheme even if it fails. we won't fallback to any other 
scheme and your program will result in throwing an IOException saying it 
receives a 401 or 407 error from the HTTP response.
...

This was written by me. As I said in the previous mail, this is the only case 
where I think a fallback is worth doing.


As far as I understand, the current version of HttpURLConnection doesn't seem 
to follow this. That's why I think it needs to be fixed. Otherwise, the tech 
note [2] should be updated.

Not exactly. The tech note is mostly about config error and not about wrong 
credentials (see the e.g. line). It is quite rare someone provides wrong 
credentials for one scheme and correct ones for another. If it really could 
happen, we should consider a more generalized fallback and not only from 
Negotiate.


It doesn't look like a serious issue for me (that's why it is P3, or maybe it 
should be P4). Furthermore, it looks like nobody has had such a problem before 
because I didn't fine any bug about that at https://bugs.openjdk.java.net

According to [2], Digest -> Basic fallback should not happen.

Yes, according to the words. But we are talking about what the most correct 
behavior is.

Thanks
Max


HttpURLConnection is quite smart, and if I understand correctly, we have only 
"http.auth.preference" and Authenticator.setDefault() to control HTTP 
authentication process. Maybe we can make it more configurable.

[1] http://docs.oracle.com/javase/8/docs/api/java/net/Authenticator.html
[2] https://docs.oracle.com/javase/8/docs/technotes/guides/net/http-auth.html

Artem

Re: [9] RFR 8138953: HttpURLConnection doesn't fallback to another auth scheme if negotiate process failed

[Artem Smotrakov]

Please see updated webrev

http://cr.openjdk.java.net/~asmotrak/8138953/webrev.01/

Artem

On 10/07/2015 06:51 PM, Artem Smotrakov wrote:

Hi Max,

HttpURLConnection obtains credentials for HTTP authentication from 
Authenticator [1] implementation. Only one authenticator can be set in 
JVM instance. It can have built-in credentials, or do some 
interactions with user to get them. Theoretically, it can provide 
different credentials depending on a user/application/etc. I don't 
know how it is used in real application, but it seems to be a possible 
situation. When I was looking into this, I found a tech note [2] which 
says the following about fallback


...
Fallback
If the server has provided more than one authentication schemes 
(including Negotiate), according to the processing order mentioned in 
the last section, Java will try to challenge the Negotiate scheme. 
However, if the protocol cannot be established successfully (e.g. The 
kerberos configuration is not correct, or the server's hostname is not 
recorded in the KDC principal DB, or the username and password 
provided by Authenticator is wrong), then the 2nd strongest scheme 
will be automatically used. Attention : If http.auth.preference is set 
to SPNEGO or Kerberos, then we assume you only want to try the 
Negotiate scheme even if it fails. we won't fallback to any other 
scheme and your program will result in throwing an IOException saying 
it receives a 401 or 407 error from the HTTP response.

...

As far as I understand, the current version of HttpURLConnection 
doesn't seem to follow this. That's why I think it needs to be fixed. 
Otherwise, the tech note [2] should be updated.


It doesn't look like a serious issue for me (that's why it is P3, or 
maybe it should be P4). Furthermore, it looks like nobody has had such 
a problem before because I didn't fine any bug about that at 
https://bugs.openjdk.java.net


According to [2], Digest -> Basic fallback should not happen. 
HttpURLConnection is quite smart, and if I understand correctly, we 
have only "http.auth.preference" and Authenticator.setDefault() to 
control HTTP authentication process. Maybe we can make it more 
configurable.


[1] http://docs.oracle.com/javase/8/docs/api/java/net/Authenticator.html
[2] 
https://docs.oracle.com/javase/8/docs/technotes/guides/net/http-auth.html


Artem

On 10/07/2015 05:39 PM, Wang Weijun wrote:

I will look into this. Busy on something else at the moment.

Do you think this would happen in reality? There weren't a lot of 
fallback in HTTP auth, IMO, is that because in most cases username 
and password are the same for all schemes, and if one fails, we 
believe the pair is wrong and there is no need to try another. 
Negotiate was picked as a special case because configuration could go 
wrong even if the username and password are correct, and we provide a 
fallback.


For example, what about fallback from Digest to Basic? Could the 
digest credentials be provided correctly at first and wrong later? 
What would happen?


I haven't read the HttpURLConnection class for a long time and I 
could be wrong.


Thanks
Max

On Oct 7, 2015, at 7:19 PM, Artem Smotrakov 
 wrote:


Hello,

Please review this for 9.

According to [1], an HTTP client should try to use another HTTP 
authentication scheme if negotiate process failed for some reason, 
and a user didn't specify SPNEGO or Kerberos in 
"http.auth.preference" system property. But no fallback happens if, 
for example:
- an HTTP server supports both Negotiate (via Kerberos) and Basic 
authentication schemes
- first, a user provides correct Kerberos credentials, and a 
connection is successfully established with Negotiate scheme
- then, a user provides wrong Kerberos credentials, but correct 
Basic credentials


This fix updates HttpURLConnection to try another authentication 
scheme negotiate process failed, and SPNEGO and Kerberos schemes are 
not preferred. The fix may be shorter, for example:


if ( serverAuthentication != null || inNegotiate && 
!"negotiate".equals(AuthenticationHeader.authPref)) {


, but I thought that some logging might be helpful.

Also added a test which checks this and a couple of other scenarios 
work fine.


Bug: https://bugs.openjdk.java.net/browse/JDK-8138953
Webrev: http://cr.openjdk.java.net/~asmotrak/8138953/webrev.00/

[1] 
https://docs.oracle.com/javase/8/docs/technotes/guides/net/http-auth.html


Artem

Re: [9] RFR 8138953: HttpURLConnection doesn't fallback to another auth scheme if negotiate process failed

[Artem Smotrakov]

Hi Max,

HttpURLConnection obtains credentials for HTTP authentication from 
Authenticator [1] implementation. Only one authenticator can be set in 
JVM instance. It can have built-in credentials, or do some interactions 
with user to get them. Theoretically, it can provide different 
credentials depending on a user/application/etc. I don't know how it is 
used in real application, but it seems to be a possible situation. When 
I was looking into this, I found a tech note [2] which says the 
following about fallback


...
Fallback
If the server has provided more than one authentication schemes 
(including Negotiate), according to the processing order mentioned in 
the last section, Java will try to challenge the Negotiate scheme. 
However, if the protocol cannot be established successfully (e.g. The 
kerberos configuration is not correct, or the server's hostname is not 
recorded in the KDC principal DB, or the username and password provided 
by Authenticator is wrong), then the 2nd strongest scheme will be 
automatically used. Attention : If http.auth.preference is set to SPNEGO 
or Kerberos, then we assume you only want to try the Negotiate scheme 
even if it fails. we won't fallback to any other scheme and your program 
will result in throwing an IOException saying it receives a 401 or 407 
error from the HTTP response.

...

As far as I understand, the current version of HttpURLConnection doesn't 
seem to follow this. That's why I think it needs to be fixed. Otherwise, 
the tech note [2] should be updated.


It doesn't look like a serious issue for me (that's why it is P3, or 
maybe it should be P4). Furthermore, it looks like nobody has had such a 
problem before because I didn't fine any bug about that at 
https://bugs.openjdk.java.net


According to [2], Digest -> Basic fallback should not happen. 
HttpURLConnection is quite smart, and if I understand correctly, we have 
only "http.auth.preference" and Authenticator.setDefault() to control 
HTTP authentication process. Maybe we can make it more configurable.


[1] http://docs.oracle.com/javase/8/docs/api/java/net/Authenticator.html
[2] 
https://docs.oracle.com/javase/8/docs/technotes/guides/net/http-auth.html


Artem

On 10/07/2015 05:39 PM, Wang Weijun wrote:

I will look into this. Busy on something else at the moment.

Do you think this would happen in reality? There weren't a lot of fallback in 
HTTP auth, IMO, is that because in most cases username and password are the 
same for all schemes, and if one fails, we believe the pair is wrong and there 
is no need to try another. Negotiate was picked as a special case because 
configuration could go wrong even if the username and password are correct, and 
we provide a fallback.

For example, what about fallback from Digest to Basic? Could the digest 
credentials be provided correctly at first and wrong later? What would happen?

I haven't read the HttpURLConnection class for a long time and I could be wrong.

Thanks
Max


On Oct 7, 2015, at 7:19 PM, Artem Smotrakov  wrote:

Hello,

Please review this for 9.

According to [1], an HTTP client should try to use another HTTP authentication scheme if 
negotiate process failed for some reason, and a user didn't specify SPNEGO or Kerberos in 
"http.auth.preference" system property. But no fallback happens if, for example:
- an HTTP server supports both Negotiate (via Kerberos) and Basic 
authentication schemes
- first, a user provides correct Kerberos credentials, and a connection is 
successfully established with Negotiate scheme
- then, a user provides wrong Kerberos credentials, but correct Basic 
credentials

This fix updates HttpURLConnection to try another authentication scheme 
negotiate process failed, and SPNEGO and Kerberos schemes are not preferred. 
The fix may be shorter, for example:

if ( serverAuthentication != null || inNegotiate && 
!"negotiate".equals(AuthenticationHeader.authPref)) {

, but I thought that some logging might be helpful.

Also added a test which checks this and a couple of other scenarios work fine.

Bug: https://bugs.openjdk.java.net/browse/JDK-8138953
Webrev: http://cr.openjdk.java.net/~asmotrak/8138953/webrev.00/

[1] https://docs.oracle.com/javase/8/docs/technotes/guides/net/http-auth.html

Artem

Re: [9] RFR 8138953: HttpURLConnection doesn't fallback to another auth scheme if negotiate process failed

[Artem Smotrakov]

Hi Sean,

Sure, it may be useful to print it out. I will update the webrev.

Artem

On 10/07/2015 05:28 PM, Seán Coffey wrote:
Thanks for handling Artem. I'll leave the main review to someone more 
knowledgeable with http authentication schemes but can I suggest that 
your print the AuthenticationHeader.authPref string out with the 
"Negotiate process failed, fallback" logger message. It's a useful 
variable to capture.

Regards,
Sean.
On 07/10/2015 12:19, Artem Smotrakov wrote:

Hello,

Please review this for 9.

According to [1], an HTTP client should try to use another HTTP 
authentication scheme if negotiate process failed for some reason, 
and a user didn't specify SPNEGO or Kerberos in 
"http.auth.preference" system property. But no fallback happens if, 
for example:
- an HTTP server supports both Negotiate (via Kerberos) and Basic 
authentication schemes
- first, a user provides correct Kerberos credentials, and a 
connection is successfully established with Negotiate scheme
- then, a user provides wrong Kerberos credentials, but correct Basic 
credentials


This fix updates HttpURLConnection to try another authentication 
scheme negotiate process failed, and SPNEGO and Kerberos schemes are 
not preferred. The fix may be shorter, for example:


if ( serverAuthentication != null || inNegotiate && 
!"negotiate".equals(AuthenticationHeader.authPref)) {


, but I thought that some logging might be helpful.

Also added a test which checks this and a couple of other scenarios 
work fine.


Bug: https://bugs.openjdk.java.net/browse/JDK-8138953
Webrev: http://cr.openjdk.java.net/~asmotrak/8138953/webrev.00/

[1] 
https://docs.oracle.com/javase/8/docs/technotes/guides/net/http-auth.html


Artem

[9] RFR 8138953: HttpURLConnection doesn't fallback to another auth scheme if negotiate process failed

[Artem Smotrakov]

Hello,

Please review this for 9.

According to [1], an HTTP client should try to use another HTTP 
authentication scheme if negotiate process failed for some reason, and a 
user didn't specify SPNEGO or Kerberos in "http.auth.preference" system 
property. But no fallback happens if, for example:
- an HTTP server supports both Negotiate (via Kerberos) and Basic 
authentication schemes
- first, a user provides correct Kerberos credentials, and a connection 
is successfully established with Negotiate scheme
- then, a user provides wrong Kerberos credentials, but correct Basic 
credentials


This fix updates HttpURLConnection to try another authentication scheme 
negotiate process failed, and SPNEGO and Kerberos schemes are not 
preferred. The fix may be shorter, for example:


if ( serverAuthentication != null || inNegotiate && 
!"negotiate".equals(AuthenticationHeader.authPref)) {


, but I thought that some logging might be helpful.

Also added a test which checks this and a couple of other scenarios work 
fine.


Bug: https://bugs.openjdk.java.net/browse/JDK-8138953
Webrev: http://cr.openjdk.java.net/~asmotrak/8138953/webrev.00/

[1] 
https://docs.oracle.com/javase/8/docs/technotes/guides/net/http-auth.html


Artem

[9] RFR 8137174: NTLM impl should use doPrivileged when it reads system properties

[Artem Smotrakov]

Hello,

Please review this small fix which updates NTLM implementation to use 
doPrivileged() methods when it reads system properties. Currently it 
requires property permissions to read "ntlm.debug" and "ntlm.version" 
system properties. Also added a test which runs NTLM auth if security 
manager is set.


Webrev: http://cr.openjdk.java.net/~asmotrak/8137174/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8137174

Artem

[9] RFR: 8129444: "socksProxyVersion" system property is ignored if Socket(Proxy) constructor is used

[Artem Smotrakov]

Hello,

Please review this fix for 9.

If a socket was created with Socket(Proxy) constructor [1], then it 
doesn't take into account "socksProxyVersion" system property. As a 
result, it is not possible to use SOCKS v4 (v5 is used by default [2]). 
Currently the property is checked only in SocksSocketImpl.connect() method.


This fix updates SocksSocketImpl class to check "socksProxyVersion" 
system property when an instance is created, and in 
SocksSocketImpl.socksBind() method.


Bug: https://bugs.openjdk.java.net/browse/JDK-8129444
Webrev: http://cr.openjdk.java.net/~asmotrak/socks4/webrev.01/

[1] 
http://docs.oracle.com/javase/8/docs/api/java/net/Socket.html#Socket-java.net.Proxy- 

[2] 
http://docs.oracle.com/javase/8/docs/api/java/net/doc-files/net-properties.html


Artem

Re: Code Review request: 8047186: jdk.net.Sockets throws InvocationTargetException instead of original runtime exceptions

[Artem Smotrakov]

Hi Michael,

Yes, I think it is better to throw the original runtime exception.

Artem

On 06/18/2014 06:28 PM, Michael McMahon wrote:

On 18/06/14 13:53, Artem Smotrakov wrote:

Hello,

Please review this fix for 8u:

https://bugs.openjdk.java.net/browse/JDK-8047186
http://cr.openjdk.java.net/~asmotrak/so_flow_sla/sockets_exceptions/webrev.01/ 



getOption() and setOption() methods of jdk.net.Sockets class throw 
InvocationTargetException instead of actual runtime exceptions like 
NullPointerException and AccessControlException. I think it is better 
to throw an original runtime exception to reduce and simplify stack 
traces.


jdk/net/Sockets/Test.java should have caught the issue, but currently 
it fails due to https://bugs.openjdk.java.net/browse/JDK-8047187. 
That's why I didn't added separate test for this bug.


Artem


I think what you're saying is that we are currently wrapping all 
RuntimeExceptions in a new non-specific

RuntimeException, and it's just better to throw the original exception.

Seems reasonable to me.

Michael.

Code Review request: 8047186: jdk.net.Sockets throws InvocationTargetException instead of original runtime exceptions

[Artem Smotrakov]

Hello,

Please review this fix for 8u:

https://bugs.openjdk.java.net/browse/JDK-8047186
http://cr.openjdk.java.net/~asmotrak/so_flow_sla/sockets_exceptions/webrev.01/

getOption() and setOption() methods of jdk.net.Sockets class throw 
InvocationTargetException instead of actual runtime exceptions like 
NullPointerException and AccessControlException. I think it is better to 
throw an original runtime exception to reduce and simplify stack traces.


jdk/net/Sockets/Test.java should have caught the issue, but currently it 
fails due to https://bugs.openjdk.java.net/browse/JDK-8047187. That's 
why I didn't added separate test for this bug.


Artem