Re: Adding SocketChannel toString to connection exception messages

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: Adding SocketChannel toString to connection exception messages

David Holmes
cc'ing net-dev as that might be the more appropriate list.

On 22/12/2017 10:59 AM, Steven Schlansker wrote:

>
>> On Dec 21, 2017, at 4:35 PM, David Holmes <[hidden email]> wrote:
>>
>> On 22/12/2017 10:29 AM, Steven Schlansker wrote:
>>>> On Dec 21, 2017, at 11:11 AM, Steven Schlansker <[hidden email]> wrote:
>>>>
>>>> What if ConnectException included the attempted hostname / IP / port SocketAddress?
>>>> java.net.ConnectException: Connection to 'foo.mycorp.com[10.x.x.x]:12345' refused
>>>> Much more useful!  This could also be extended to various other socket exceptions.
>>
>> I believe there are concerns with too much information that can be considered "sensitive" (like host names and IP addresses) appearing in error messages due to them ending up in log files and bug reports.
>
> Unfortunately that's exactly the information that is crucial to someone trying to diagnose issues...
> Could it be an opt-in policy?  Perhaps by a system property?

The expectation is that such information should be added by layers
higher in the call chain, rather than the JDK libraries assuming it is
okay to do.

> Currently the alternative I'm faced with is going through every piece of user code and library that *might*
> throw this exception and wrapping it to add this critical diagnostic information.  For an application that uses
> java.net heavily, you can imagine how that is a tall task and possibly even not realistically achievable...
>
> (Is there a written policy regarding this somewhere, or is it up to the personal feelings of the contributors?)

This is covered by the secure coding guidelines, under 'Confidential
information':

http://www.oracle.com/technetwork/java/seccodeguide-139067.html#2

"Guideline 2-1 / CONFIDENTIAL-1: Purge sensitive information from
exceptions"

I know for a fact that we'd have to scrub this information from test
failures when putting the info into a bug report.

If net-dev folk can't expand further on this then I suggest filing a CSR
request so that the CSR group can consider it. But I think this will be
a no-go (I'm a member of the CSR group).

Cheers,
David
Reply | Threaded
Open this post in threaded view
|

Re: Adding SocketChannel toString to connection exception messages

Chris Hegarty
On 22/12/17 01:27, David Holmes wrote:

> cc'ing net-dev as that might be the more appropriate list.
>
> On 22/12/2017 10:59 AM, Steven Schlansker wrote:
>>
>>> On Dec 21, 2017, at 4:35 PM, David Holmes <[hidden email]>
>>> wrote:
>>>
>>> On 22/12/2017 10:29 AM, Steven Schlansker wrote:
>>>>> On Dec 21, 2017, at 11:11 AM, Steven Schlansker
>>>>> <[hidden email]> wrote:
>>>>>
>>>>> What if ConnectException included the attempted hostname / IP /
>>>>> port SocketAddress?
>>>>> java.net.ConnectException: Connection to
>>>>> 'foo.mycorp.com[10.x.x.x]:12345' refused
>>>>> Much more useful!  This could also be extended to various other
>>>>> socket exceptions.
>>>
>>> I believe there are concerns with too much information that can be
>>> considered "sensitive" (like host names and IP addresses) appearing
>>> in error messages due to them ending up in log files and bug reports.
>>
>> Unfortunately that's exactly the information that is crucial to
>> someone trying to diagnose issues...

And to someone trying to discern private information about a network.

>> Could it be an opt-in policy?  Perhaps by a system property?

Well, you don't know where a log file or exception will end up.
Most likely on stackoverflow.

> The expectation is that such information should be added by layers
> higher in the call chain, rather than the JDK libraries assuming it is
> okay to do.

Agreed. It may be some work, but if the issue is significant
enough to the user then it may be worth their effort, as well
as affording the opportunity to opt-out at any particular
point in the code.

>> Currently the alternative I'm faced with is going through every piece
>> of user code and library that *might*
>> throw this exception and wrapping it to add this critical diagnostic
>> information.  For an application that uses
>> java.net heavily, you can imagine how that is a tall task and possibly
>> even not realistically achievable...
>>
>> (Is there a written policy regarding this somewhere, or is it up to
>> the personal feelings of the contributors?)
>
> This is covered by the secure coding guidelines, under 'Confidential
> information':
>
> http://www.oracle.com/technetwork/java/seccodeguide-139067.html#2
>
> "Guideline 2-1 / CONFIDENTIAL-1: Purge sensitive information from
> exceptions"

Exactly.

> I know for a fact that we'd have to scrub this information from test
> failures when putting the info into a bug report.
>
> If net-dev folk can't expand further on this then I suggest filing a CSR
> request so that the CSR group can consider it. But I think this will be
> a no-go (I'm a member of the CSR group).

I have to agree with David here. I don't think that such a request
could be supported.

-Chris.
Reply | Threaded
Open this post in threaded view
|

Re: Adding SocketChannel toString to connection exception messages

Seán Coffey
As someone who works with alot of log files, I'd like to chime in and
support Steven's end goal. Looking at a "Connection refused" error in
the middle of a log file that possibly extends to millions of lines is
near useless. In the era of cloud compute, diagnosing network issues is
sure to become a more common task.

While we may not be able to put the sensitive information in an
exception message, I think we could put it behind a (new?) system
property which might be able to log this information. Logs contain all
sorts of sensitive data. Take javax.net.debug=ssl output for example.

Regards,
Sean.

On 22/12/17 09:57, Chris Hegarty wrote:

> On 22/12/17 01:27, David Holmes wrote:
>> cc'ing net-dev as that might be the more appropriate list.
>>
>> On 22/12/2017 10:59 AM, Steven Schlansker wrote:
>>>
>>>> On Dec 21, 2017, at 4:35 PM, David Holmes <[hidden email]>
>>>> wrote:
>>>>
>>>> On 22/12/2017 10:29 AM, Steven Schlansker wrote:
>>>>>> On Dec 21, 2017, at 11:11 AM, Steven Schlansker
>>>>>> <[hidden email]> wrote:
>>>>>>
>>>>>> What if ConnectException included the attempted hostname / IP /
>>>>>> port SocketAddress?
>>>>>> java.net.ConnectException: Connection to
>>>>>> 'foo.mycorp.com[10.x.x.x]:12345' refused
>>>>>> Much more useful!  This could also be extended to various other
>>>>>> socket exceptions.
>>>>
>>>> I believe there are concerns with too much information that can be
>>>> considered "sensitive" (like host names and IP addresses) appearing
>>>> in error messages due to them ending up in log files and bug reports.
>>>
>>> Unfortunately that's exactly the information that is crucial to
>>> someone trying to diagnose issues...
>
> And to someone trying to discern private information about a network.
>
>>> Could it be an opt-in policy?  Perhaps by a system property?
>
> Well, you don't know where a log file or exception will end up.
> Most likely on stackoverflow.
>
>> The expectation is that such information should be added by layers
>> higher in the call chain, rather than the JDK libraries assuming it
>> is okay to do.
>
> Agreed. It may be some work, but if the issue is significant
> enough to the user then it may be worth their effort, as well
> as affording the opportunity to opt-out at any particular
> point in the code.
>
>>> Currently the alternative I'm faced with is going through every
>>> piece of user code and library that *might*
>>> throw this exception and wrapping it to add this critical diagnostic
>>> information.  For an application that uses
>>> java.net heavily, you can imagine how that is a tall task and
>>> possibly even not realistically achievable...
>>>
>>> (Is there a written policy regarding this somewhere, or is it up to
>>> the personal feelings of the contributors?)
>>
>> This is covered by the secure coding guidelines, under 'Confidential
>> information':
>>
>> http://www.oracle.com/technetwork/java/seccodeguide-139067.html#2
>>
>> "Guideline 2-1 / CONFIDENTIAL-1: Purge sensitive information from
>> exceptions"
>
> Exactly.
>
>> I know for a fact that we'd have to scrub this information from test
>> failures when putting the info into a bug report.
>>
>> If net-dev folk can't expand further on this then I suggest filing a
>> CSR request so that the CSR group can consider it. But I think this
>> will be a no-go (I'm a member of the CSR group).
>
> I have to agree with David here. I don't think that such a request
> could be supported.
>
> -Chris.

Reply | Threaded
Open this post in threaded view
|

Re: Adding SocketChannel toString to connection exception messages

Chris Hegarty

On 22/12/17 14:59, Seán Coffey wrote:

> As someone who works with alot of log files, I'd like to chime in and
> support Steven's end goal. Looking at a "Connection refused" error in
> the middle of a log file that possibly extends to millions of lines is
> near useless. In the era of cloud compute, diagnosing network issues is
> sure to become a more common task.
>
> While we may not be able to put the sensitive information in an
> exception message, I think we could put it behind a (new?) system
> property which might be able to log this information. Logs contain all
> sorts of sensitive data. Take javax.net.debug=ssl output for example.

I have some sympathy for (capital-L)ogging such detail messages
( given the reasonable restriction on access to log files ), but
a system property that effectively amends exception detail
messages, or prints to stdout/stderr is not a runner in my
opinion.

Maybe we should be looking at instrumentation with JFR events, or
similar. My point being, if someone has time and energy enough
to spend on this, then we can do better than javax.net.debug=ssl.
Also, someone should check that divulging such sensitive information,
even in log files, is acceptable from a security perspective. I'm
personally still dubious.

-Chris.
Reply | Threaded
Open this post in threaded view
|

Re: Adding SocketChannel toString to connection exception messages

Bernd Eckenfels-4
Hello,

I also dearly miss Socket addresses in connection exceptions, however it looks like it is not going to make it. However if we add a getter for the Peer address (not included in toString) then logging frameworks could detect instances of ConnectException and process them accordingly.

Gruss
Bernd
--
http://bernd.eckenfels.net

From: net-dev <[hidden email]> on behalf of Chris Hegarty <[hidden email]>
Sent: Friday, December 22, 2017 4:17:31 PM
To: Seán Coffey; David Holmes; Steven Schlansker
Cc: core-libs-dev; [hidden email]
Subject: Re: Adding SocketChannel toString to connection exception messages
 

On 22/12/17 14:59, Seán Coffey wrote:
> As someone who works with alot of log files, I'd like to chime in and
> support Steven's end goal. Looking at a "Connection refused" error in
> the middle of a log file that possibly extends to millions of lines is
> near useless. In the era of cloud compute, diagnosing network issues is
> sure to become a more common task.
>
> While we may not be able to put the sensitive information in an
> exception message, I think we could put it behind a (new?) system
> property which might be able to log this information. Logs contain all
> sorts of sensitive data. Take javax.net.debug=ssl output for example.

I have some sympathy for (capital-L)ogging such detail messages
( given the reasonable restriction on access to log files ), but
a system property that effectively amends exception detail
messages, or prints to stdout/stderr is not a runner in my
opinion.

Maybe we should be looking at instrumentation with JFR events, or
similar. My point being, if someone has time and energy enough
to spend on this, then we can do better than javax.net.debug=ssl.
Also, someone should check that divulging such sensitive information,
even in log files, is acceptable from a security perspective. I'm
personally still dubious.

-Chris.
Reply | Threaded
Open this post in threaded view
|

Re: Adding SocketChannel toString to connection exception messages

Chris Hegarty

> On 29 Dec 2017, at 00:33, Steven Schlansker <[hidden email]> wrote:
>
> Thanks for the discussion!
>
> So, it sounds like amending the message by default is going to be a non-starter -- but at least adding the information otherwise might be possible.
>
> One possibility would be to add new fields to SocketException, for example an InetSocketAddress representing both the local and remote (if available).

You would need to careful to not disclose resolved addresses to untrusted code. SocketException, since a subtype of IOException, can wind up in many places.

Would you be proposing to add these new fields to the serial-form of SocketException? What behaviour do you envisage for deserializing instances from previous releases? This will have an impact of any potential API.

> This would not be rendered by default, but could be retrieved by a cooperating application or library.  Or maybe a second step could be a '-Djavax.net.debug=exceptions' that then appends this information by default, but that sounds more controversial.

Logging seems like a better alternative than a system property, to me.

> Then at least libraries and applications have the ability to get these diagnostics, even if they choose not to.
> Is this an acceptable approach?  Would it be accepted as a patch?

I suspect that a webrev/patch would help drive the discussion, rather than a commitment to accepting it into the JDK.

-Chris.



>> On Dec 22, 2017, at 8:42 AM, Bernd Eckenfels <[hidden email]> wrote:
>>
>> Hello,
>>
>> I also dearly miss Socket addresses in connection exceptions, however it looks like it is not going to make it. However if we add a getter for the Peer address (not included in toString) then logging frameworks could detect instances of ConnectException and process them accordingly.
>>
>> Gruss
>> Bernd
>> --
>> http://bernd.eckenfels.net
>> ________________________________
>> From: net-dev <[hidden email]> on behalf of Chris Hegarty <[hidden email]>
>> Sent: Friday, December 22, 2017 4:17:31 PM
>> To: Seán Coffey; David Holmes; Steven Schlansker
>> Cc: core-libs-dev; [hidden email]
>> Subject: Re: Adding SocketChannel toString to connection exception messages
>>
>>
>>> On 22/12/17 14:59, Seán Coffey wrote:
>>> As someone who works with alot of log files, I'd like to chime in and
>>> support Steven's end goal. Looking at a "Connection refused" error in
>>> the middle of a log file that possibly extends to millions of lines is
>>> near useless. In the era of cloud compute, diagnosing network issues is
>>> sure to become a more common task.
>>>
>>> While we may not be able to put the sensitive information in an
>>> exception message, I think we could put it behind a (new?) system
>>> property which might be able to log this information. Logs contain all
>>> sorts of sensitive data. Take javax.net.debug=ssl output for example.
>>
>> I have some sympathy for (capital-L)ogging such detail messages
>> ( given the reasonable restriction on access to log files ), but
>> a system property that effectively amends exception detail
>> messages, or prints to stdout/stderr is not a runner in my
>> opinion.
>>
>> Maybe we should be looking at instrumentation with JFR events, or
>> similar. My point being, if someone has time and energy enough
>> to spend on this, then we can do better than javax.net.debug=ssl.
>> Also, someone should check that divulging such sensitive information,
>> even in log files, is acceptable from a security perspective. I'm
>> personally still dubious.
>>
>> -Chris.
>

Reply | Threaded
Open this post in threaded view
|

Re: Adding SocketChannel toString to connection exception messages

Alan Bateman
In reply to this post by Bernd Eckenfels-4
On 29/12/2017 00:33, Steven Schlansker wrote:
> Thanks for the discussion!
>
> So, it sounds like amending the message by default is going to be a non-starter -- but at least adding the information otherwise might be possible.
>
There are examples in other area where exceptions include detail
information (for diagnostics purposes) when not running with a security
manager. This may be something to look at here (and easy to try out too
as it wouldn't require touching any native code either).

-Alan



Reply | Threaded
Open this post in threaded view
|

Re: Adding SocketChannel toString to connection exception messages

Peter Levart
Hi Steven,

Steven Schlansker je 02. 01. 2018 ob 22:25 napisal:
On Jan 2, 2018, at 8:35 AM, Alan Bateman [hidden email] wrote:

On 29/12/2017 00:33, Steven Schlansker wrote:
Thanks for the discussion!

So, it sounds like amending the message by default is going to be a non-starter -- but at least adding the information otherwise might be possible.

There are examples in other area where exceptions include detail information (for diagnostics purposes) when not running with a security manager. This may be something to look at here (and easy to try out too as it wouldn't require touching any native code either).
I like that!

For now, I will proceed with improving my prototype to follow this suggestion -- unless there is a security manager, the exceptions are annotated.

An alternate way to activate it, by configuring the platform logger e.g. "java.net.SocketException=TRACE", might be useful in case we want to allow end users to configure the feature explicitly and independent of security manager.

Unfortunately I don't see how I can avoid changing the native code -- the exception message is initialized in native code, and by the time we call back to Java code, the necessary information is not passed in to the SocketException subclass constructor.  So I may be misinterpreting your guidance here, but I'm not seeing it.

I think I will hold off on adding Java level fields to the exception types.  I expect if I do that, I will then have to get a spec change to add appropriate public API to make the data actually usable.  Since the goal here is for log diagnostics as opposed to more structured data at the Java level, I'll avoid it.  It also avoids any complications with regards to changing the serial format for such a common type.  But I do think it means the work has to be done in the native code.


I haven't studied the actual code, but one thing to consider might be to create a java.net package-private exception type (unrelated to java.net.SocketException) with a constructor that takes necessary information (host, IP, port, but no message) and then throw this exception from native code instead of java.net.SocketException. On the Java side, you then wrap calls to native methods with try/catch and throw java.net.SocketException with appropriately formatted message instead (and with no cause). Moving formatting logic to Java might simplify native code and it's always good to keep native code simple (not having to deal with SecurityManager, system properties, logger(s) etc.).

What do you think?

Regards, Peter

Reply | Threaded
Open this post in threaded view
|

Re: Adding SocketChannel toString to connection exception messages

Alan Bateman
In reply to this post by Alan Bateman
On 02/01/2018 21:25, Steven Schlansker wrote:

> :
> This would definitely be better than nothing!  But it's still difficult, for example a common allocation pattern for us would be to assign networks to availability zones:
>
> 10.0.1.0/24 10.0.2.0/24 10.0.3.0/24
>
> then if you pick the same last number for a well known service, you might end up with instances at 10.0.1.2, 10.0.2.2, 10.0.3.2 -- so depending on which octets are obscured you may end up with no useful information.
>
> The triggering incident for us was that one of our Amazon ELBs started responding incorrectly (blackholing data) --
> so when you resolve "my-elb-1.amazonaws.amazon.com" you'd get three different IP addresses, and depending on which one
> is picked for the connect operation, you'll get all data blackholed.
Socket/SocketChannel connect take a single address, not a hostname that
is potentially mapped to multiple addresses. Maybe you mean the Socket
constructors that takes a hostname and do the lookup before attempt to
establish a connection?

In any case, if the exception message includes the address/port details
then it shouldn't matter which constructors are used. Also note that
when you look at the non-blocking cases then you'll see the exception is
thrown by finishConnect, not connect. This is nothing reason to handle
this completely in java and avoid changes to the native code.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: Adding SocketChannel toString to connection exception messages

Bernd Eckenfels-4
In reply to this post by David Holmes
Hello,

Just to add to the discussion, if proxies are used, the error should make clear if the problem is connecting the proxy or the endpoint. And for that case (especially with multiple proxies) the actual failed address is also interesting for trouble shooting (in that case higher level code won’t have the information)

Gruss
Bernd
--
http://bernd.eckenfels.net
Reply | Threaded
Open this post in threaded view
|

Re: Adding SocketChannel toString to connection exception messages

Chris Hegarty
Hi Bernd,

Are you specifically talking about a SOCKS proxy, or something else?

-Chris.

On 3 Jan 2018, at 12:10, Bernd Eckenfels <[hidden email]> wrote:

Hello,

Just to add to the discussion, if proxies are used, the error should make clear if the problem is connecting the proxy or the endpoint. And for that case (especially with multiple proxies) the actual failed address is also interesting for trouble shooting (in that case higher level code won’t have the information)


Reply | Threaded
Open this post in threaded view
|

Re: Adding SocketChannel toString to connection exception messages

Bernd Eckenfels-4
Hello,

Yes. i think OpenJDK has only a Socks Implementation, but it is a general problem that there is no API framework to communicate network problems with intermediates.

Gruss
Bernd
--
http://bernd.eckenfels.net

From: Chris Hegarty <[hidden email]>
Sent: Wednesday, January 3, 2018 1:39:46 PM
To: Bernd Eckenfels
Cc: [hidden email]
Subject: Re: Adding SocketChannel toString to connection exception messages
 
Hi Bernd,

Are you specifically talking about a SOCKS proxy, or something else?

-Chris.

On 3 Jan 2018, at 12:10, Bernd Eckenfels <[hidden email]> wrote:

Hello,

Just to add to the discussion, if proxies are used, the error should make clear if the problem is connecting the proxy or the endpoint. And for that case (especially with multiple proxies) the actual failed address is also interesting for trouble shooting (in that case higher level code won’t have the information)


Reply | Threaded
Open this post in threaded view
|

Re: Adding SocketChannel toString to connection exception messages

David Lloyd-2
In reply to this post by Chris Hegarty
On Fri, Dec 29, 2017 at 11:15 AM, Chris Hegarty
<[hidden email]> wrote:

> On 29 Dec 2017, at 00:33, Steven Schlansker <[hidden email]> wrote:
>> Thanks for the discussion!
>>
>> So, it sounds like amending the message by default is going to be a non-starter -- but at least adding the information otherwise might be possible.
>>
>> One possibility would be to add new fields to SocketException, for example an InetSocketAddress representing both the local and remote (if available).
>
> You would need to careful to not disclose resolved addresses to untrusted code. SocketException, since a subtype of IOException, can wind up in many places.
>
> Would you be proposing to add these new fields to the serial-form of SocketException? What behaviour do you envisage for deserializing instances from previous releases? This will have an impact of any potential API.

This is an advantage to a setter-only message supplement method: if
the supplementary field is set, the writeReplace method can swap it
for a new instance with the combined message.

--
- DML