You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "bschuchardt (GitHub)" <gi...@apache.org> on 2018/12/21 22:46:45 UTC

[GitHub] [geode] bschuchardt opened pull request #3036: GEODE-2113 Implement SSL over NIO

This removes old-I/O use in TCPConduit peer-to-peer communications.

This was used for SSL/TLS secure commuications but Java has had an

SSLEngine implementation that allows you to implement secure

communications on new-I/O SocketChannels or any other transport
mechanism.

A new NioSSLEngine class wraps the JDK's SSLEngine and provides the
SSL
handshake as well as encryption/decryption of messages. SocketCreator
performs the SSL handshake and returns a NioSslEngine that TCPConduit

then uses for messaging.

The SSL handshake needs to be done in Connection.java now because the

ByteBuffer used to do the handshake is also used for reading messages
in
Receivers. Because of this the Handshake pool in TCPConduit became

obsolete and I deleted it.

I've also done a lot of cleanup of compilation warnings in
Connection.java
and removed references to "NIO". The primary SSL/TLS
changes in that class
are in writeFully (renamed from nioWriteFully)
and processBuffer (renamed
from processNIOBuffer).

While testing I noticed some places where we're creating non-daemon
threads that were keeping DUnit ChildVM processes from exiting.  I've
changed these places to use daemon threads.  Very few threads in Geode
should be non-daemon.

Porting client/server to use NioSSLEngine will be done under a separate

ticket and a different version of NioEngine may be created to secure
UDP
messaging.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

- [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?

- [x] Is your initial contribution a single, squashed commit?

- [x] Does `gradlew build` run cleanly?

- [x] Have you written or updated unit tests to verify your changes?

- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.


[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] galen-pivotal commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.
Will `NioSslEngine.close()` block if the socket is hung somehow due to network weirdness? Maybe that's fine and I don't want to block the PR for this, but I wanted to solicit your opinion.

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "kirklund (GitHub)" <gi...@apache.org>.
Reuse of `br` in the do-loop looks kind of surprising. I think the auto-closing of `soMaxConnIsr` closes `br` on line 377 (`}`) so when you loop back around the `br.close()` on line 369 is a no-op... but I'm not 100% positive. I'd use a new var that only exists within the inner-try.

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] galen-pivotal commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.
This method can be private.

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "kirklund (GitHub)" <gi...@apache.org>.
If most of your implementations aren't going to implement close() you might want to change it to:
```
default void close() {
  // nothing by default
}
```

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] bschuchardt commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "bschuchardt (GitHub)" <gi...@apache.org>.
We have a lot of these lingering around.  I prefer String.format() because it doesn't allow you to accidentally have the wrong number of arguments.  I had to fix a lot of those when I got rid of LocalizedStrings.  I'll change it to use varargs though.

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] bschuchardt commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "bschuchardt (GitHub)" <gi...@apache.org>.
thanks - I've added this and will push when I finish addressing all of your comments

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] galen-pivotal commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.
Two things here:
* These methods reduce code size, but I think they make the code less readable as I don't know what they do from the method name / signature.
* `CacheFactory.getAnyInstance()` is something we'd like to deprecate, so I prefer the pattern of storing the value in a static (or using `ClusterStartupRule` to handle lifecycle) as it manages the reference and lifecycle in a way that doesn't necessitate globals.

Neither of these is a forcing issue for me; I leave it to your conscience.



[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "kirklund (GitHub)" <gi...@apache.org>.
You should be able to get rid of the `Integer.valueOf` statements and just give logger the raw primitives.

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "kirklund (GitHub)" <gi...@apache.org>.
You could make this a real link for easier navigation:
```
* See <a href="https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/JSSERefGuide.html#SSLENG">JSSERefGuide</a>
```
It'll even work if you make it `@see` instead of `See`.

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] bschuchardt commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "bschuchardt (GitHub)" <gi...@apache.org>.
I removed use of CacheFactory.getAnyInstance().

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] galen-pivotal commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.
I think we should close the socket here and in the catch blocks further down in this class, just to be safe and avoid leaking a socket.

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] galen-pivotal commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.
Why does `doneReading` set `position` to `limit` and `limit` to `capacity` rather than just clearing the buffer?

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "kirklund (GitHub)" <gi...@apache.org>.
The Log4J syntax is nicer than using String.format and you can get rid of the boxing. Try:
```
logger.fatal("Problem forming SSL connection {}[{}].", socket.getInetAddress(), socket.getPort(), ex);
```

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] bschuchardt commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "bschuchardt (GitHub)" <gi...@apache.org>.
I'm leaving it but adding a comment.  It took me a while to find this debug setting & I don't want to have to do that again.

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "kirklund (GitHub)" <gi...@apache.org>.
Reuse of `br` in the do-loop looks kind of surprising. I think the auto-closing of `soMaxConnIsr` closes `br` on line 377 (`}`) so when you look pack around the `br.close()` on line 369 is a no-op... but I'm not 100% positive. I'd use a new var that only exists within the inner-try.

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] galen-pivotal commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.
Please remove dead code here and elsewhere

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] galen-pivotal commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.
You could make this a try-with-resources to make the code a little more concise.

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] bschuchardt commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "bschuchardt (GitHub)" <gi...@apache.org>.
I thought I got rid of all of those Integer.valueOf() calls in that file.

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] galen-pivotal commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.
sounds good.

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] bschuchardt commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "bschuchardt (GitHub)" <gi...@apache.org>.
Thanks - that's much better

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] galen-pivotal commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.
`cache.close();` is more idiomatic than `cache.getDistributedSystem().disconnect();` and closes the distributed system as well.

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] bschuchardt commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "bschuchardt (GitHub)" <gi...@apache.org>.
fixed - old habits from the days of using JUnit4CacheTestCase.  That base class configures the cache to not disconnect the DistributedSystem.

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] bschuchardt commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "bschuchardt (GitHub)" <gi...@apache.org>.
Yes - even socket.close() can block when there are network problems.  That's why we usually spin off AsyncCloser threads to close sockets.

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] galen-pivotal commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.
The timeout variables should be removed where they are declared above, since they're no longer used.

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] bschuchardt commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "bschuchardt (GitHub)" <gi...@apache.org>.
That's what the code in Connection was doing & I preserved it.  I think the deal is that in some situations we don't consume anything from the buffer & its position hasn't advanced.  In that case we need to read more data into the buffer.

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] bschuchardt commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "bschuchardt (GitHub)" <gi...@apache.org>.
I'm glad you mentioned this because I was used to the JUnit4CacheTestCase behavior of disabling shutdown of the DistributedSystem when the cache is closed.  The new DistributedRule doesn't appear to do this.

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] galen-pivotal commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.
`s/unsecure/insecure/`

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] bschuchardt closed pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "bschuchardt (GitHub)" <gi...@apache.org>.
[ pull request closed by bschuchardt ]

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] galen-pivotal commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.
This can be private and some of the visibility of other methods on this class can be further restricted.

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] bschuchardt commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "bschuchardt (GitHub)" <gi...@apache.org>.
I tried this but Locator isn't AutoClosable & the compiler objected to the change.

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] bschuchardt commented on pull request #3036: GEODE-2113 Implement SSL over NIO

Posted by "bschuchardt (GitHub)" <gi...@apache.org>.
okay - thanks

[ Full content available at: https://github.com/apache/geode/pull/3036 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org