Uploaded image for project: 'Log4j 2'
  1. Log4j 2
  2. LOG4J2-2013

SslSocketManager does not apply SSLContext on TCP reconnect

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.8.2
    • Fix Version/s: 2.9.0
    • Component/s: Core
    • Labels:
      None

      Description

      The SslSocketManger is not applying the SSLContext correctly when the TCP connection is restarted. Currently the SslSocketManager inherits a Reconnector class from TcpSocketManger, which only restarts the TCP connection and does not apply SSL correctly.

        Issue Links

          Activity

          Hide
          taylorp36 Taylor Patton added a comment -
          Show
          taylorp36 Taylor Patton added a comment - I have a PR here: https://github.com/apache/logging-log4j2/pull/108
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user taylorp36 opened a pull request:

          https://github.com/apache/logging-log4j2/pull/108

          LOG4J2-2013 Handle reconnect in SSLSocketManager without losing SSLCo…

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/taylorp36/logging-log4j2 master

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/logging-log4j2/pull/108.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #108


          commit f53c9b0de944f171c4777ea342e3944aeeecaf8b
          Author: Taylor Patton <taypatto@cisco.com>
          Date: 2017-08-14T20:57:13Z

          LOG4J2-2013 Handle reconnect in SSLSocketManager without losing SSLContext


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user taylorp36 opened a pull request: https://github.com/apache/logging-log4j2/pull/108 LOG4J2-2013 Handle reconnect in SSLSocketManager without losing SSLCo… You can merge this pull request into a Git repository by running: $ git pull https://github.com/taylorp36/logging-log4j2 master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/logging-log4j2/pull/108.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #108 commit f53c9b0de944f171c4777ea342e3944aeeecaf8b Author: Taylor Patton <taypatto@cisco.com> Date: 2017-08-14T20:57:13Z LOG4J2-2013 Handle reconnect in SSLSocketManager without losing SSLContext
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user garydgregory commented on the issue:

          https://github.com/apache/logging-log4j2/pull/108

          I'm not crazy about some of the duplication and change in hierarchy. I'll try to take a look. Also, no unit tests. I think we need those as well.

          Show
          githubbot ASF GitHub Bot added a comment - Github user garydgregory commented on the issue: https://github.com/apache/logging-log4j2/pull/108 I'm not crazy about some of the duplication and change in hierarchy. I'll try to take a look. Also, no unit tests. I think we need those as well.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit e9561c00347db7c024b0fb5617927b5d76e8ba05 in logging-log4j2's branch refs/heads/master from Gary Gregory
          [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=e9561c0 ]

          Add test related to LOG4J2-2013
          SslSocketManager does not apply SSLContext on TCP reconnect.

          Show
          jira-bot ASF subversion and git services added a comment - Commit e9561c00347db7c024b0fb5617927b5d76e8ba05 in logging-log4j2's branch refs/heads/master from Gary Gregory [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=e9561c0 ] Add test related to LOG4J2-2013 SslSocketManager does not apply SSLContext on TCP reconnect.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 70830215a3341fae375b51650bae1244b2ca95e4 in logging-log4j2's branch refs/heads/master from Gary Gregory
          [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=7083021 ]

          Add test related to LOG4J2-2013
          SslSocketManager does not apply SSLContext on TCP reconnect.

          Show
          jira-bot ASF subversion and git services added a comment - Commit 70830215a3341fae375b51650bae1244b2ca95e4 in logging-log4j2's branch refs/heads/master from Gary Gregory [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=7083021 ] Add test related to LOG4J2-2013 SslSocketManager does not apply SSLContext on TCP reconnect.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit f2ddaf93aeee3101cd95ba61d13c79bcec112b48 in logging-log4j2's branch refs/heads/master from Gary Gregory
          [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=f2ddaf9 ]

          LOG4J2-2013 SslSocketManager does not apply SSLContext on TCP
          reconnect. Add disabled test that shows the issue.

          Show
          jira-bot ASF subversion and git services added a comment - Commit f2ddaf93aeee3101cd95ba61d13c79bcec112b48 in logging-log4j2's branch refs/heads/master from Gary Gregory [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=f2ddaf9 ] LOG4J2-2013 SslSocketManager does not apply SSLContext on TCP reconnect. Add disabled test that shows the issue.
          Hide
          garydgregory Gary Gregory added a comment -

          I added the test org.apache.logging.log4j.core.appender.net.SecureSocketAppenderConnectReConnectIT.

          Show
          garydgregory Gary Gregory added a comment - I added the test org.apache.logging.log4j.core.appender.net.SecureSocketAppenderConnectReConnectIT .
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user taylorp36 commented on the issue:

          https://github.com/apache/logging-log4j2/pull/108

          I added a test in the SocketAppenderTest, are there any more tests we need for this?

          Show
          githubbot ASF GitHub Bot added a comment - Github user taylorp36 commented on the issue: https://github.com/apache/logging-log4j2/pull/108 I added a test in the SocketAppenderTest, are there any more tests we need for this?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user garydgregory commented on the issue:

          https://github.com/apache/logging-log4j2/pull/108

          Please see if your new test covers a different code path from the tests I added in the *log4j-core-its* module in the package *org.apache.logging.log4j.core.appender.net*.

          Thank you,
          Gary

          Show
          githubbot ASF GitHub Bot added a comment - Github user garydgregory commented on the issue: https://github.com/apache/logging-log4j2/pull/108 Please see if your new test covers a different code path from the tests I added in the * log4j-core-its * module in the package * org.apache.logging.log4j.core.appender.net *. Thank you, Gary
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user taylorp36 commented on the issue:

          https://github.com/apache/logging-log4j2/pull/108

          The code path is very similar; the tests in *org.apache.logging.log4j.core.appender.net* call the appender directly, and the test in *org.apache.logging.log4j.core.appender* applies the appender to the Logger, and uses the logger to send the event (via the appender). I don't think there is much benefit with both if we just want to test that the secure appender can send events. Would you like me to remove the test I added in SocketAppenderTest? Any suggestions about the initial design of the fix?

          Show
          githubbot ASF GitHub Bot added a comment - Github user taylorp36 commented on the issue: https://github.com/apache/logging-log4j2/pull/108 The code path is very similar; the tests in * org.apache.logging.log4j.core.appender.net * call the appender directly, and the test in * org.apache.logging.log4j.core.appender * applies the appender to the Logger, and uses the logger to send the event (via the appender). I don't think there is much benefit with both if we just want to test that the secure appender can send events. Would you like me to remove the test I added in SocketAppenderTest? Any suggestions about the initial design of the fix?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user garydgregory commented on the issue:

          https://github.com/apache/logging-log4j2/pull/108

          Swamped with work ATM. I will take a look later this week, but my initial comment is the same: the duplication does not seen right.

          Show
          githubbot ASF GitHub Bot added a comment - Github user garydgregory commented on the issue: https://github.com/apache/logging-log4j2/pull/108 Swamped with work ATM. I will take a look later this week, but my initial comment is the same: the duplication does not seen right.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user taylorp36 commented on the issue:

          https://github.com/apache/logging-log4j2/pull/108

          Yeap, the trouble I had extending TcpSocketManager was that Reconnector and Socket were both being mutated in the write() and reconnect(). Without having setters on those fields in TcpSocketManager, duplicating those 2 methods seemed necessary. I could have still extended TcpSocketManger, but it added unnecessary complexity just to re-use a few other small methods. The complexity being that I would have been passing arguments to the TcpSocketManger constructor that would never be used. Open to any suggestions, maybe there is a solution here I'm not seeing.

          Show
          githubbot ASF GitHub Bot added a comment - Github user taylorp36 commented on the issue: https://github.com/apache/logging-log4j2/pull/108 Yeap, the trouble I had extending TcpSocketManager was that Reconnector and Socket were both being mutated in the write() and reconnect(). Without having setters on those fields in TcpSocketManager, duplicating those 2 methods seemed necessary. I could have still extended TcpSocketManger, but it added unnecessary complexity just to re-use a few other small methods. The complexity being that I would have been passing arguments to the TcpSocketManger constructor that would never be used. Open to any suggestions, maybe there is a solution here I'm not seeing.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user remkop commented on the issue:

          https://github.com/apache/logging-log4j2/pull/108

          Maybe there should be a TcpSocketManagerBuilder? It could be initialized from scratch by calling its setter methods, or initialized from a TcpSocketManager instance so you'd only need to call the setters of the attributes you want to modify. Then call `build` to instantiate the TcpSocketManager.

          Show
          githubbot ASF GitHub Bot added a comment - Github user remkop commented on the issue: https://github.com/apache/logging-log4j2/pull/108 Maybe there should be a TcpSocketManagerBuilder? It could be initialized from scratch by calling its setter methods, or initialized from a TcpSocketManager instance so you'd only need to call the setters of the attributes you want to modify. Then call `build` to instantiate the TcpSocketManager.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit eb285c876f6743479d9f46267af1041369c113f0 in logging-log4j2's branch refs/heads/master from Gary Gregory
          [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=eb285c8 ]

          LOG4J2-2013
          SslSocketManager does not apply SSLContext on TCP reconnect. As a first
          step, refactor common code between Tcp's FatctoryData and
          SslfactoryData.

          Show
          jira-bot ASF subversion and git services added a comment - Commit eb285c876f6743479d9f46267af1041369c113f0 in logging-log4j2's branch refs/heads/master from Gary Gregory [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=eb285c8 ] LOG4J2-2013 SslSocketManager does not apply SSLContext on TCP reconnect. As a first step, refactor common code between Tcp's FatctoryData and SslfactoryData.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user garydgregory commented on the issue:

          https://github.com/apache/logging-log4j2/pull/108

          Let me see if my current fix works, it needs more testing...

          Show
          githubbot ASF GitHub Bot added a comment - Github user garydgregory commented on the issue: https://github.com/apache/logging-log4j2/pull/108 Let me see if my current fix works, it needs more testing...
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user taylorp36 commented on the issue:

          https://github.com/apache/logging-log4j2/pull/108

          Sure, I like the builder idea. I was hesitant to modify TcpSocketManager initially. Sounds like Gary might already have a fix though.

          Show
          githubbot ASF GitHub Bot added a comment - Github user taylorp36 commented on the issue: https://github.com/apache/logging-log4j2/pull/108 Sure, I like the builder idea. I was hesitant to modify TcpSocketManager initially. Sounds like Gary might already have a fix though.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user garydgregory commented on the issue:

          https://github.com/apache/logging-log4j2/pull/108

          I had to get some work done here, but yes, I have an almost fix that I can get back to tonight. Please be patient

          Show
          githubbot ASF GitHub Bot added a comment - Github user garydgregory commented on the issue: https://github.com/apache/logging-log4j2/pull/108 I had to get some work done here, but yes, I have an almost fix that I can get back to tonight. Please be patient
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit fb6f73d48b85c8248ad7576468f77bbbb7acf670 in logging-log4j2's branch refs/heads/master from Gary Gregory
          [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=fb6f73d ]

          LOG4J2-2013 SslSocketManager does not apply SSLContext on TCP
          reconnect.

          Show
          jira-bot ASF subversion and git services added a comment - Commit fb6f73d48b85c8248ad7576468f77bbbb7acf670 in logging-log4j2's branch refs/heads/master from Gary Gregory [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=fb6f73d ] LOG4J2-2013 SslSocketManager does not apply SSLContext on TCP reconnect.
          Hide
          garydgregory Gary Gregory added a comment -

          In Git master. Thank you all for your help. Please verify and close.

          Show
          garydgregory Gary Gregory added a comment - In Git master. Thank you all for your help. Please verify and close.
          Hide
          garydgregory Gary Gregory added a comment -

          Taylor Patton,

          May you verify please?

          Gary

          Show
          garydgregory Gary Gregory added a comment - Taylor Patton , May you verify please? Gary
          Hide
          taylorp36 Taylor Patton added a comment -

          Checking

          Show
          taylorp36 Taylor Patton added a comment - Checking
          Hide
          taylorp36 Taylor Patton added a comment -

          I had some issues where the app I was testing with isn't compatible with 2.9+ changes. I will try again today to test this, maybe by cherrypicking your change to 2.8.2

          Show
          taylorp36 Taylor Patton added a comment - I had some issues where the app I was testing with isn't compatible with 2.9+ changes. I will try again today to test this, maybe by cherrypicking your change to 2.8.2
          Hide
          ralph.goers@dslextreme.com Ralph Goers added a comment -

          Can you elaborate on the compatibility problem? We would definitely like to try to take care of issues like that before 2.9 is released.

          Show
          ralph.goers@dslextreme.com Ralph Goers added a comment - Can you elaborate on the compatibility problem? We would definitely like to try to take care of issues like that before 2.9 is released.
          Hide
          taylorp36 Taylor Patton added a comment -

          Should have both answers by end of today!

          Show
          taylorp36 Taylor Patton added a comment - Should have both answers by end of today!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user taylorp36 closed the pull request at:

          https://github.com/apache/logging-log4j2/pull/108

          Show
          githubbot ASF GitHub Bot added a comment - Github user taylorp36 closed the pull request at: https://github.com/apache/logging-log4j2/pull/108
          Hide
          taylorp36 Taylor Patton added a comment -

          Confirmed SSLContext is re-initiated on TCP restart. Thanks all!

          Show
          taylorp36 Taylor Patton added a comment - Confirmed SSLContext is re-initiated on TCP restart. Thanks all!
          Hide
          garydgregory Gary Gregory added a comment -

          YW. Note that the release vote for 2.9.0 is under way. If all goes well, we should have a release out by the end of the week.

          Show
          garydgregory Gary Gregory added a comment - YW. Note that the release vote for 2.9.0 is under way. If all goes well, we should have a release out by the end of the week.
          Hide
          taylorp36 Taylor Patton added a comment -

          Great news Gary Gregory. For the compatibility issue, there was actually a problem with some unit test we had but I figured that out and the current release candidate 2.9.0 seems to work fine.

          Show
          taylorp36 Taylor Patton added a comment - Great news Gary Gregory . For the compatibility issue, there was actually a problem with some unit test we had but I figured that out and the current release candidate 2.9.0 seems to work fine.
          Hide
          garydgregory Gary Gregory added a comment -

          Big Happy then.

          Show
          garydgregory Gary Gregory added a comment - Big Happy then.

            People

            • Assignee:
              garydgregory Gary Gregory
              Reporter:
              taylorp36 Taylor Patton
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development