grizzly
  1. grizzly
  2. GRIZZLY-1397

Failed renegotiation results in SSLEngine being left in invalid state

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.16
    • Fix Version/s: 2.3.15, 3.0
    • Component/s: None
    • Labels:
      None

      Description

      When renegotiation fails (null client cert chain as an example), any attempt to write is met with:

      java.lang.IllegalStateException: Handshake is not completed!
      at org.glassfish.grizzly.ssl.SSLFilter.accurateWrite(SSLFilter.java:569)
      at org.glassfish.grizzly.ssl.SSLFilter.handleWrite(SSLFilter.java:216)
      at org.glassfish.grizzly.filterchain.ExecutorResolver$8.execute(ExecutorResolver.java:111)
      at org.glassfish.grizzly.filterchain.DefaultFilterChain.executeFilter(DefaultFilterChain.java:265)
      at org.glassfish.grizzly.filterchain.DefaultFilterChain.executeChainPart(DefaultFilterChain.java:200)
      at org.glassfish.grizzly.filterchain.DefaultFilterChain.execute(DefaultFilterChain.java:134)
      at org.glassfish.grizzly.filterchain.DefaultFilterChain.process(DefaultFilterChain.java:112)
      at org.glassfish.grizzly.ProcessorExecutor.execute(ProcessorExecutor.java:78)

      This error should be raised and the connection closed.

        Activity

        Hide
        Pier Fumagalli added a comment -

        I believe the bug here is in RequestUtils.populateCertificateAttribute(Request):

        At line 74, the CertificateEvent is constructed as new SSLBaseFilter.CertificateEvent(true), basically forcing needClientAuth to be true and always forcing renegotiation (regardless of what is specified in SSLEngineConfiguration.

        Show
        Pier Fumagalli added a comment - I believe the bug here is in RequestUtils.populateCertificateAttribute(Request) : At line 74, the CertificateEvent is constructed as new SSLBaseFilter.CertificateEvent(true) , basically forcing needClientAuth to be true and always forcing renegotiation (regardless of what is specified in SSLEngineConfiguration .
        Hide
        Pier Fumagalli added a comment -

        This patch seems to correct the behaviour, according to my (quick) tests. The first one is for org.glassfish.grizzly.ssl.SSLBaseFilter

        diff -w -r -U3 grizzly-framework.orig/org/glassfish/grizzly/ssl/SSLBaseFilter.java grizzly-framework/org/glassfish/grizzly/ssl/SSLBaseFilter.java
        --- grizzly-framework.orig/org/glassfish/grizzly/ssl/SSLBaseFilter.java	2014-04-30 20:49:08.000000000 +0900
        +++ grizzly-framework/org/glassfish/grizzly/ssl/SSLBaseFilter.java	2014-07-04 14:57:39.000000000 +0900
        @@ -242,7 +242,7 @@
                     final CertificateEvent ce = (CertificateEvent) event;
                     ce.certs = getPeerCertificateChain(obtainSslConnectionContext(ctx.getConnection()),
                                                        ctx,
        -                                               ce.needClientAuth);
        +                                               serverSSLEngineConfigurator.needClientAuth);
                     return ctx.getStopAction();
                 }
                 return ctx.getInvokeAction();
        @@ -789,7 +789,7 @@
                     return certs;
                 }
         
        -        if (needClientAuth) {
        +        if (needClientAuth && renegotiateOnClientAuthWant) {
                     renegotiate(sslCtx, context);
                 }
         
        @@ -916,14 +916,11 @@
         
                 private Object[] certs;
         
        -        private final boolean needClientAuth;
        -
        -
                 // -------------------------------------------------------- Constructors
         
         
        -        public CertificateEvent(final boolean needClientAuth) {
        -            this.needClientAuth = needClientAuth;
        +        public CertificateEvent() {
        +            // Nothing to do...
                 }
         

        And the second one is for org.glassfish.grizzly.http.server.util.RequestUtils

        diff -w -r -U3 grizzly-http-server.orig/org/glassfish/grizzly/http/server/util/RequestUtils.java grizzly-http-server/org/glassfish/grizzly/http/server/util/RequestUtils.java
        --- grizzly-http-server.orig/org/glassfish/grizzly/http/server/util/RequestUtils.java	2014-04-30 20:49:08.000000000 +0900
        +++ grizzly-http-server/org/glassfish/grizzly/http/server/util/RequestUtils.java	2014-07-04 14:59:56.000000000 +0900
        @@ -71,7 +71,7 @@
                         }
                     }
                     
        -            SSLBaseFilter.CertificateEvent event = new SSLBaseFilter.CertificateEvent(true);
        +            SSLBaseFilter.CertificateEvent event = new SSLBaseFilter.CertificateEvent();
                     request.getContext().notifyDownstream(event);
                     certificates = event.getCertificates();
                     request.setAttribute(SSLSupport.CERTIFICATE_KEY, certificates);
        

        They are pretty self-explanatory, but basically, the gist is that we don't want to force needClientAuth at request time (by embedding it in the CertificateEvent) but we want to evaluate whether we need the certificates depending on the configurations specified at construction (what's in SSLEngineConfiguration).

        This does the trick, in my quick set of tests (tried with OpenSSL's own s_client mode, with and without certificates).

        Show
        Pier Fumagalli added a comment - This patch seems to correct the behaviour, according to my (quick) tests. The first one is for org.glassfish.grizzly.ssl.SSLBaseFilter diff -w -r -U3 grizzly-framework.orig/org/glassfish/grizzly/ssl/SSLBaseFilter.java grizzly-framework/org/glassfish/grizzly/ssl/SSLBaseFilter.java --- grizzly-framework.orig/org/glassfish/grizzly/ssl/SSLBaseFilter.java 2014-04-30 20:49:08.000000000 +0900 +++ grizzly-framework/org/glassfish/grizzly/ssl/SSLBaseFilter.java 2014-07-04 14:57:39.000000000 +0900 @@ -242,7 +242,7 @@ final CertificateEvent ce = (CertificateEvent) event; ce.certs = getPeerCertificateChain(obtainSslConnectionContext(ctx.getConnection()), ctx, - ce.needClientAuth); + serverSSLEngineConfigurator.needClientAuth); return ctx.getStopAction(); } return ctx.getInvokeAction(); @@ -789,7 +789,7 @@ return certs; } - if (needClientAuth) { + if (needClientAuth && renegotiateOnClientAuthWant) { renegotiate(sslCtx, context); } @@ -916,14 +916,11 @@ private Object [] certs; - private final boolean needClientAuth; - - // -------------------------------------------------------- Constructors - public CertificateEvent( final boolean needClientAuth) { - this .needClientAuth = needClientAuth; + public CertificateEvent() { + // Nothing to do ... } And the second one is for org.glassfish.grizzly.http.server.util.RequestUtils diff -w -r -U3 grizzly-http-server.orig/org/glassfish/grizzly/http/server/util/RequestUtils.java grizzly-http-server/org/glassfish/grizzly/http/server/util/RequestUtils.java --- grizzly-http-server.orig/org/glassfish/grizzly/http/server/util/RequestUtils.java 2014-04-30 20:49:08.000000000 +0900 +++ grizzly-http-server/org/glassfish/grizzly/http/server/util/RequestUtils.java 2014-07-04 14:59:56.000000000 +0900 @@ -71,7 +71,7 @@ } } - SSLBaseFilter.CertificateEvent event = new SSLBaseFilter.CertificateEvent( true ); + SSLBaseFilter.CertificateEvent event = new SSLBaseFilter.CertificateEvent(); request.getContext().notifyDownstream(event); certificates = event.getCertificates(); request.setAttribute(SSLSupport.CERTIFICATE_KEY, certificates); They are pretty self-explanatory, but basically, the gist is that we don't want to force needClientAuth at request time (by embedding it in the CertificateEvent ) but we want to evaluate whether we need the certificates depending on the configurations specified at construction (what's in SSLEngineConfiguration ). This does the trick, in my quick set of tests (tried with OpenSSL's own s_client mode, with and without certificates).
        Hide
        oleksiys added a comment -

        Hi Pier,

        looks like it's not a bug, it's kind of feature or legacy behavior we preserve since Glassfish 2.x.
        What we can do is introduce system property of HttpServer/NetworkListener flag, which will switch "force" mode on/off.

        What do you think?

        Show
        oleksiys added a comment - Hi Pier, looks like it's not a bug, it's kind of feature or legacy behavior we preserve since Glassfish 2.x. What we can do is introduce system property of HttpServer/NetworkListener flag, which will switch "force" mode on/off. What do you think?
        Hide
        Pier Fumagalli added a comment -

        Anything works, really...

        As long as "getUserPrincipal()" doesn't shut down the whole HTTPS socket and returns null (or throws an exception, whatever) is good for me...

        Right now the problem is that if I don't intercept the call (which basically means adding pre-matching filters in Jersey on all my apps) the connections shut down and nothing works

        Show
        Pier Fumagalli added a comment - Anything works, really... As long as "getUserPrincipal()" doesn't shut down the whole HTTPS socket and returns null (or throws an exception, whatever) is good for me... Right now the problem is that if I don't intercept the call (which basically means adding pre-matching filters in Jersey on all my apps) the connections shut down and nothing works
        Hide
        oleksiys added a comment -

        fixed.
        We changed the default behavior to not force SSL re-handshake in case SSL certificate wasn't sent by a client.
        In order to restore the previous behavior pls. use system property:

        -Dorg.glassfish.grizzly.http.server.Request..force-client-auth-on-get-user-principal=true
        

        [2.3.x]
        Revision: b87cb55608f0178b556eed1fb1543b363c6e9df6
        Date: 2014-07-07 22:02:56 UTC

        [master]
        Revision: 23fe41c053310754d5c67d60dd637ae39d407139
        Date: 2014-07-07 22:02:56 UTC

        Log Message:
        ------------
        + fix issue #1397
        https://java.net/jira/browse/GRIZZLY-1397
        "Failed renegotiation results in SSLEngine being left in invalid state"

        Show
        oleksiys added a comment - fixed. We changed the default behavior to not force SSL re-handshake in case SSL certificate wasn't sent by a client. In order to restore the previous behavior pls. use system property: -Dorg.glassfish.grizzly.http.server.Request..force-client-auth-on-get-user-principal= true [2.3.x] Revision: b87cb55608f0178b556eed1fb1543b363c6e9df6 Date: 2014-07-07 22:02:56 UTC [master] Revision: 23fe41c053310754d5c67d60dd637ae39d407139 Date: 2014-07-07 22:02:56 UTC Log Message: ------------ + fix issue #1397 https://java.net/jira/browse/GRIZZLY-1397 "Failed renegotiation results in SSLEngine being left in invalid state"

          People

          • Assignee:
            Unassigned
            Reporter:
            Ryan Lubke
          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: