[
https://issues.apache.org/jira/browse/THRIFT-106?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12922114#action_12922114
]
Nirmal Ranganathan commented on THRIFT-106:
-------------------------------------------
bq. The spacing is off. We use 2-space indentation.
I'll clean that up.
bq. The addition of getServerSocket to TServerSocket seems unnecessary, since
it's not used anywhere.
I added it for 2 reasons, one is similarity to TSocket.getSocket and the other
if anyone want's to override TServerSocket, it would be nice to have the
underlying socket. Case in point we would require something like that for
Cassandra.
bq. Why override TSocket.open() at all in TSSLSocket? TSocket throws an
exception if it's already open, but TSSLSocket will silently continue. Is this
a necessary/intended semantic change? If we don't need that change, do we need
a TSSLSocket at all?
bq. TSSLServerSocket also seems unnecessary.
That was just an idiomatic addition. Since everyone is used to
Transport.open(). The override ignores the call if it's already open. I can
remove it, if you feel that's unnecessary.
bq. Would it be possible to get a separate unit test for TSSLSocketFactory?
Maybe just set up a simple server-client pair and push some bytes around? The
tutorial stuff is great, but it's not going to be part of our standard test
suite, so we could break it by accident.
Do we require something more than the current TestTSSLSocketFactory that I
added? It does use the underlying ServerTestBase for the tests, but uses a
TSSLSocket and TSSLServerSocket from the TSSLSocketFactory as a simple
client-server pair. Let me know if I misunderstood something there.
bq. Hate to sounds pedantic, but do you mind using ifs with curly braces, even
though the then clause is single-line? Just for readability/consistency.
Sure, I'll update it
> TSSLServerSocket
> ----------------
>
> Key: THRIFT-106
> URL: https://issues.apache.org/jira/browse/THRIFT-106
> Project: Thrift
> Issue Type: New Feature
> Components: Java - Library
> Environment: n/a
> Reporter: rico sec
> Priority: Trivial
> Attachments: java-ssl.patch,
> Sample-keystore-for-tests-and-tutorial.patch, ssl.patch,
> TSSLServerSocket-TSSLSocket-and-factory.patch,
> Updated-tutorial-with-ssl-sample.patch
>
> Original Estimate: 6h
> Remaining Estimate: 6h
>
> SSL Connection w/ autogenerated self signed x509 certs seems to be the state
> of the art for rpc layers.
> if thrift had one ...that would be very good.
> http://java.sun.com/javase/6/docs/technotes/guides/security/jsse/JSSERefGuide.html
> if someone does this pls ping/email me, I will do some testing and write a
> simple key mgmt utility.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
