Adding SocketChannel toString to connection exception messages

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

Adding SocketChannel toString to connection exception messages

Steven Schlansker
Hi core-libs-dev,

While tracking down a connectivity issue, we identified that two of our hosts
are unable to talk to each other due to a misconfiguration of the network.

This manifested as:

2017-12-21T11:00:34.840Z DEBUG <> [default-pool-34] o.e.j.client.AbstractConnectionPool - Connection 1/32 creation failed
java.net.ConnectException: Connection refused
        at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method)
        at sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:716)
        at org.eclipse.jetty.io.SelectorManager.doFinishConnect(SelectorManager.java:353)
        at org.eclipse.jetty.io.ManagedSelector.processConnect(ManagedSelector.java:157)
...

The message, 'Connection refused', does tell you something -- but it doesn't emit
really any information about what actually happened (i.e. host / port of refused connection)
that would allow you to track down exactly where it failed.

While the application can generally figure this out (it probably knows where it tried to connect)
the SocketChannelImpl could helpfully attach this information to the message, rather than relying
on the application to correctly figure out the information at every connect point.

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.

Would such a contribution be accepted?
Thanks for considering!

Reply | Threaded
Open this post in threaded view
|

Re: Adding SocketChannel toString to connection exception messages

Steven Schlansker

> 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 started hacking around with this, first with PlainSocketImpl only.
I apologize in advance for what is likely to be terrible JNI style, as I don't have much experience
with it and I omitted any error handling for the POC - but I think it proves that this isn't too big of a project, as
I would want to cover SocketChannelImpl and maybe UnixAsynchronousSocketChannelImpl and call it good enough for my uses.

Behold!

[me@myhost]% java pkg.SocketFail
java.net.ConnectException: Connection refused (Connection refused)

[me@myhost]% ~/code/jdk10/build/macosx-x86_64-normal-server-release/jdk/bin/java pkg.SocketFail
java.net.ConnectException: Connection refused (Connection refused: localhost/127.0.0.1:12345)


Am I correct that the best path towards acceptance for this kind of change is to target jdk10 and then beg for a backport to jdk9u?
Thanks again for considering this improvement! WIP patch below inline.


diff --git a/src/java.base/unix/native/libnet/PlainSocketImpl.c b/src/java.base/unix/native/libnet/PlainSocketImpl.c
--- a/src/java.base/unix/native/libnet/PlainSocketImpl.c
+++ b/src/java.base/unix/native/libnet/PlainSocketImpl.c
@@ -217,6 +217,9 @@
     (*env)->SetIntField(env, fdObj, IO_fd_fdID, fd);
 }

+// private helper for socketConnect to describe errors
+static void sockErrDescribe(JNIEnv *env, char* errbuf, char* msg, jobject ia, jint port);
+
 /*
  * inetAddress is the address object passed to the socket connect
  * call.
@@ -230,6 +233,7 @@
                                             jobject iaObj, jint port,
                                             jint timeout)
 {
+    char errmsg[256];
     jint localport = (*env)->GetIntField(env, this, psi_localportID);
     int len = 0;
     /* fdObj is the FileDescriptor field on this */
@@ -249,7 +253,8 @@
     int connect_rv = -1;

     if (IS_NULL(fdObj)) {
-        JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException", "Socket closed");
+        sockErrDescribe(env, errmsg, "Socket closed", iaObj, port);
+        JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException", errmsg);
         return;
     } else {
         fd = (*env)->GetIntField(env, fdObj, IO_fd_fdID);
@@ -329,8 +334,8 @@
             jlong prevNanoTime = JVM_NanoTime(env, 0);

             if (errno != EINPROGRESS) {
-                NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "ConnectException",
-                             "connect failed");
+                sockErrDescribe(env, errmsg, "connect failed", iaObj, port);
+                NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "ConnectException", errmsg);
                 SET_BLOCKING(fd);
                 return;
             }
@@ -372,8 +377,8 @@
             } /* while */

             if (connect_rv == 0) {
-                JNU_ThrowByName(env, JNU_JAVANETPKG "SocketTimeoutException",
-                            "connect timed out");
+                sockErrDescribe(env, errmsg, "connect timed out", iaObj, port);
+                JNU_ThrowByName(env, JNU_JAVANETPKG "SocketTimeoutException", errmsg);

                 /*
                  * Timeout out but connection may still be established.
@@ -419,36 +424,37 @@
          * a more descriptive exception text is used.
          */
         if (connect_rv == -1 && errno == EINVAL) {
-            JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException",
-                "Invalid argument or cannot assign requested address");
+            sockErrDescribe(errmsg, "Invalid argument or cannot assign requested address", iaObj, port)
+            JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException", errmsg);
             return;
         }
 #endif
 #if defined(EPROTO)
         if (errno == EPROTO) {
-            NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "ProtocolException",
-                           "Protocol error");
+            sockErrDescribe(env, errmsg, "Protocol error", iaObj, port);
+            NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "ProtocolException", errmsg);
             return;
         }
 #endif
         if (errno == ECONNREFUSED) {
-            NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "ConnectException",
-                           "Connection refused");
+            sockErrDescribe(env, errmsg, "Connection refused", iaObj, port);
+            NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "ConnectException", errmsg);
         } else if (errno == ETIMEDOUT) {
-            NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "ConnectException",
-                           "Connection timed out");
+            sockErrDescribe(env, errmsg, "Connection timed out", iaObj, port);
+            NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "ConnectException", errmsg);
         } else if (errno == EHOSTUNREACH) {
-            NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "NoRouteToHostException",
-                           "Host unreachable");
+            sockErrDescribe(env, errmsg, "Host unreachable", iaObj, port);
+            NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "NoRouteToHostException", errmsg);
         } else if (errno == EADDRNOTAVAIL) {
-            NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "NoRouteToHostException",
-                             "Address not available");
+            sockErrDescribe(env, errmsg, "Address not available", iaObj, port);
+            NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "NoRouteToHostException", errmsg);
         } else if ((errno == EISCONN) || (errno == EBADF)) {
-            JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException",
-                            "Socket closed");
+            sockErrDescribe(env, errmsg, "Socket closed", iaObj, port);
+            JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException", errmsg);
         } else {
+            sockErrDescribe(env, errmsg, "connect failed", iaObj, port);
             JNU_ThrowByNameWithMessageAndLastError
-                (env, JNU_JAVANETPKG "SocketException", "connect failed");
+                (env, JNU_JAVANETPKG "SocketException", errmsg);
         }
         return;
     }
@@ -479,6 +485,15 @@
     }
 }

+static void sockErrDescribe(JNIEnv *env, char* errbuf, char* msg, jobject ia, jint port) {
+    jclass obj = (*env)->FindClass(env, "java/lang/Object");
+    jmethodID toStr = (*env)->GetMethodID(env, obj, "toString", "()Ljava/lang/String;");
+    jstring iaJstr = (*env)->CallObjectMethod(env, ia, toStr);
+    const char *iaStr = (*env)->GetStringUTFChars(env, iaJstr, 0);
+    jio_snprintf(errbuf, 128, "%s: %s:%d", msg, iaStr, port);
+    (*env)->ReleaseStringUTFChars(env, iaJstr, iaStr);
+}
+
 /*
  * Class:     java_net_PlainSocketImpl
  * Method:    socketBind



Reply | Threaded
Open this post in threaded view
|

Re: Adding SocketChannel toString to connection exception messages

David Holmes
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.

David
-----

> I started hacking around with this, first with PlainSocketImpl only.
> I apologize in advance for what is likely to be terrible JNI style, as I don't have much experience
> with it and I omitted any error handling for the POC - but I think it proves that this isn't too big of a project, as
> I would want to cover SocketChannelImpl and maybe UnixAsynchronousSocketChannelImpl and call it good enough for my uses.
>
> Behold!
>
> [me@myhost]% java pkg.SocketFail
> java.net.ConnectException: Connection refused (Connection refused)
>
> [me@myhost]% ~/code/jdk10/build/macosx-x86_64-normal-server-release/jdk/bin/java pkg.SocketFail
> java.net.ConnectException: Connection refused (Connection refused: localhost/127.0.0.1:12345)
>
>
> Am I correct that the best path towards acceptance for this kind of change is to target jdk10 and then beg for a backport to jdk9u?
> Thanks again for considering this improvement! WIP patch below inline.
>
>
> diff --git a/src/java.base/unix/native/libnet/PlainSocketImpl.c b/src/java.base/unix/native/libnet/PlainSocketImpl.c
> --- a/src/java.base/unix/native/libnet/PlainSocketImpl.c
> +++ b/src/java.base/unix/native/libnet/PlainSocketImpl.c
> @@ -217,6 +217,9 @@
>       (*env)->SetIntField(env, fdObj, IO_fd_fdID, fd);
>   }
>
> +// private helper for socketConnect to describe errors
> +static void sockErrDescribe(JNIEnv *env, char* errbuf, char* msg, jobject ia, jint port);
> +
>   /*
>    * inetAddress is the address object passed to the socket connect
>    * call.
> @@ -230,6 +233,7 @@
>                                               jobject iaObj, jint port,
>                                               jint timeout)
>   {
> +    char errmsg[256];
>       jint localport = (*env)->GetIntField(env, this, psi_localportID);
>       int len = 0;
>       /* fdObj is the FileDescriptor field on this */
> @@ -249,7 +253,8 @@
>       int connect_rv = -1;
>
>       if (IS_NULL(fdObj)) {
> -        JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException", "Socket closed");
> +        sockErrDescribe(env, errmsg, "Socket closed", iaObj, port);
> +        JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException", errmsg);
>           return;
>       } else {
>           fd = (*env)->GetIntField(env, fdObj, IO_fd_fdID);
> @@ -329,8 +334,8 @@
>               jlong prevNanoTime = JVM_NanoTime(env, 0);
>
>               if (errno != EINPROGRESS) {
> -                NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "ConnectException",
> -                             "connect failed");
> +                sockErrDescribe(env, errmsg, "connect failed", iaObj, port);
> +                NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "ConnectException", errmsg);
>                   SET_BLOCKING(fd);
>                   return;
>               }
> @@ -372,8 +377,8 @@
>               } /* while */
>
>               if (connect_rv == 0) {
> -                JNU_ThrowByName(env, JNU_JAVANETPKG "SocketTimeoutException",
> -                            "connect timed out");
> +                sockErrDescribe(env, errmsg, "connect timed out", iaObj, port);
> +                JNU_ThrowByName(env, JNU_JAVANETPKG "SocketTimeoutException", errmsg);
>
>                   /*
>                    * Timeout out but connection may still be established.
> @@ -419,36 +424,37 @@
>            * a more descriptive exception text is used.
>            */
>           if (connect_rv == -1 && errno == EINVAL) {
> -            JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException",
> -                "Invalid argument or cannot assign requested address");
> +            sockErrDescribe(errmsg, "Invalid argument or cannot assign requested address", iaObj, port)
> +            JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException", errmsg);
>               return;
>           }
>   #endif
>   #if defined(EPROTO)
>           if (errno == EPROTO) {
> -            NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "ProtocolException",
> -                           "Protocol error");
> +            sockErrDescribe(env, errmsg, "Protocol error", iaObj, port);
> +            NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "ProtocolException", errmsg);
>               return;
>           }
>   #endif
>           if (errno == ECONNREFUSED) {
> -            NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "ConnectException",
> -                           "Connection refused");
> +            sockErrDescribe(env, errmsg, "Connection refused", iaObj, port);
> +            NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "ConnectException", errmsg);
>           } else if (errno == ETIMEDOUT) {
> -            NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "ConnectException",
> -                           "Connection timed out");
> +            sockErrDescribe(env, errmsg, "Connection timed out", iaObj, port);
> +            NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "ConnectException", errmsg);
>           } else if (errno == EHOSTUNREACH) {
> -            NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "NoRouteToHostException",
> -                           "Host unreachable");
> +            sockErrDescribe(env, errmsg, "Host unreachable", iaObj, port);
> +            NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "NoRouteToHostException", errmsg);
>           } else if (errno == EADDRNOTAVAIL) {
> -            NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "NoRouteToHostException",
> -                             "Address not available");
> +            sockErrDescribe(env, errmsg, "Address not available", iaObj, port);
> +            NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "NoRouteToHostException", errmsg);
>           } else if ((errno == EISCONN) || (errno == EBADF)) {
> -            JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException",
> -                            "Socket closed");
> +            sockErrDescribe(env, errmsg, "Socket closed", iaObj, port);
> +            JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException", errmsg);
>           } else {
> +            sockErrDescribe(env, errmsg, "connect failed", iaObj, port);
>               JNU_ThrowByNameWithMessageAndLastError
> -                (env, JNU_JAVANETPKG "SocketException", "connect failed");
> +                (env, JNU_JAVANETPKG "SocketException", errmsg);
>           }
>           return;
>       }
> @@ -479,6 +485,15 @@
>       }
>   }
>
> +static void sockErrDescribe(JNIEnv *env, char* errbuf, char* msg, jobject ia, jint port) {
> +    jclass obj = (*env)->FindClass(env, "java/lang/Object");
> +    jmethodID toStr = (*env)->GetMethodID(env, obj, "toString", "()Ljava/lang/String;");
> +    jstring iaJstr = (*env)->CallObjectMethod(env, ia, toStr);
> +    const char *iaStr = (*env)->GetStringUTFChars(env, iaJstr, 0);
> +    jio_snprintf(errbuf, 128, "%s: %s:%d", msg, iaStr, port);
> +    (*env)->ReleaseStringUTFChars(env, iaJstr, iaStr);
> +}
> +
>   /*
>    * Class:     java_net_PlainSocketImpl
>    * Method:    socketBind
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: Adding SocketChannel toString to connection exception messages

Steven Schlansker

> 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?

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?)

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 David Holmes
On Thu, Dec 21, 2017 at 6:35 PM, David Holmes <[hidden email]> wrote:
> 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.

I tend to agree here.  However - and this is a big "however" - while
this is a good policy for the JDK, higher-level applications and
libraries often do not have such concerns, since they can often more
accurately determine whether a piece of information is actually
sensitive; the JDK must simply assume that it is.  It should be
*possible* for libraries and applications to *add* this information.
Right now this is a very, very difficult task: there is an entire
hierarchy of exceptions, and there is no easy way to change the
message of an exception.  The only option is to wrap the exception
with another, more descriptive exception, which is not great for users
or for developers.

It would be nice if there were some way to set some supplementary
information on SocketException (or even IOException) after the
exception were constructed, which would be included in getMessage() if
set.  This would allow frameworks and applications to provide some
context without causing a security problem for the JDK.


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

Re: Adding SocketChannel toString to connection exception messages

Remi Forax
Hi David,
you can "abuse" of the suppressed exception feature for that, i.e add a CommentException as suppressed exception to the SocketException with no stacktrace and a descriptive error message.

Rémi

----- Mail original -----
> De: "David Lloyd" <[hidden email]>
> À: "David Holmes" <[hidden email]>
> Cc: "core-libs-dev" <[hidden email]>
> Envoyé: Vendredi 22 Décembre 2017 16:22:41
> Objet: Re: Adding SocketChannel toString to connection exception messages

> On Thu, Dec 21, 2017 at 6:35 PM, David Holmes <[hidden email]> wrote:
>> 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.
>
> I tend to agree here.  However - and this is a big "however" - while
> this is a good policy for the JDK, higher-level applications and
> libraries often do not have such concerns, since they can often more
> accurately determine whether a piece of information is actually
> sensitive; the JDK must simply assume that it is.  It should be
> *possible* for libraries and applications to *add* this information.
> Right now this is a very, very difficult task: there is an entire
> hierarchy of exceptions, and there is no easy way to change the
> message of an exception.  The only option is to wrap the exception
> with another, more descriptive exception, which is not great for users
> or for developers.
>
> It would be nice if there were some way to set some supplementary
> information on SocketException (or even IOException) after the
> exception were constructed, which would be included in getMessage() if
> set.  This would allow frameworks and applications to provide some
> context without causing a security problem for the JDK.
>
>
> --
> - DML
Reply | Threaded
Open this post in threaded view
|

Re: Adding SocketChannel toString to connection exception messages

Steven Schlansker
In reply to this post by Steven Schlansker
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).
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.

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?

> 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

Peter Levart
In reply to this post by David Holmes
Hi,

David Holmes je 22. 12. 2017 ob 01:35 napisal:

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

For debugging purposes it might sometimes be enough to get just a hint
about the actual address / port but not reveal it entirely. The person
doing debugging probably knows more about the environment than an
average person so the hint might give him enough information to discern
the actual address / port. Exposing just the last octet of an IP address
and the last digit of the port might do. For example:

java.net.ConnectException: Connection to X.X.X.205:XXX8 refused.

So Steven, I'm curious whether such hint would help in your case?

An attacker that knows something about the environment could find out
the missing pieces without such hints anyway (simply by scanning IPs /
ports), so such partial information is not that sensitive nowadays.

Another idea: define a one way function that maps the IP:port pair into
a value which is displayed in the exception message. For debugging
purposes this might be enough since the one doing debugging might know
the set of possible IP:port pairs in advance. He could then apply the
function to each of them in turn and find out the matching pair.

Regards,

Peter

Reply | Threaded
Open this post in threaded view
|

Re: Adding SocketChannel toString to connection exception messages

Steven Schlansker
In reply to this post by Steven Schlansker

On Dec 31, 2017, at 7:24 AM, Peter Levart <[hidden email]> wrote:

>
> Hi,
>>
>> 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.
>>
>> David
>
> For debugging purposes it might sometimes be enough to get just a hint about the actual address / port but not reveal it entirely. The person doing debugging probably knows more about the environment than an average person so the hint might give him enough information to discern the actual address / port. Exposing just the last octet of an IP address and the last digit of the port might do. For example:
>
> java.net.ConnectException: Connection to X.X.X.205:XXX8 refused.
>
> So Steven, I'm curious whether such hint would help in your case?

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.

The author of the code in question obviously had not considered a failure mode where one of the "It Is In The Cloud So It Always Works" load balancer instances did not behave like the others -- so it did not emit any diagnostic information other than the hostname, and until we got a network operator in with WireShark it was quite difficult to determine that it was in fact exactly one instance of three failing.  The pain of using WireShark to diagnose what should have been a quick scan through the logs, coupled with the sheer impossibility of annotating every network call in every application across our organization, pushed me to open this discussion to do it in one central place :)

> An attacker that knows something about the environment could find out the missing pieces without such hints anyway (simply by scanning IPs / ports), so such partial information is not that sensitive nowadays.

I've never really understood the justification behind this rule -- we have a similar policy in our organization, all IP addresses must be stripped.  But knowing the layout of the network, I can tell you that if you are on the outside and have one of our 10.x.x.x IP addresses it gives you almost nothing (good luck routing to it) -- and if you are on the inside, it's not exactly difficult to enumerate these.  DNS contains every name / IP you might find and it'll happily divulge all that information to you, and failing that, nmap can map out the majority of the network fairly reliably in seconds...

I respect that the JVM has to be conservative, but there is a real usability problem here, and the security gain is a little questionable to me!

> Another idea: define a one way function that maps the IP:port pair into a value which is displayed in the exception message. For debugging purposes this might be enough since the one doing debugging might know the set of possible IP:port pairs in advance. He could then apply the function to each of them in turn and find out the matching pair.


This is interesting, but only if you can enumerate the IP:port that might match.  We have a Apache Mesos cluster with O(100) nodes, each of which is assigned a pool of 1000 ports to assign to applications.  Needing to enumerate a million possible hash matches could be a royal pain, and my understanding is that 100 nodes is not even near the size of the largest production clusters out there.  It's not quite to the level of a non-starter, but as those numbers scale it could easily become useless.

Some of the exceptions may not have a specific port (hostname resolution), may have port 0, or such a dynamically assigned port, so you'd have to guess these special cases too.

Additionally, with cloud providers, you may not understand the space of IPs that an Abstract Cloud Device like ELB might come from -- it's coming from an AWS pool that you have no visibility into.


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