|
[
Permlink
| « Hide
]
Bernt M. Johnsen added a comment - 20/Feb/07 01:21 PM
Uploaded patch for this improvement. Comments are welcome
Just a few quick comments!
1) Typo in Property.java [drda.NaiveTrustManager] 2) Strange sentence in JavaDoc for getSocketFactory() 3) No JavaDoc for the other methods. Are these documented elsewhere? For instance, what form/value is authType supposed to have? 4) Could getAcceptedIssuers() return an empty array instead of null? [NetworkServerControlImpl] 5) I guess SSL_OFF and default are the same for now. It would be nice with a comment to document the fall-through. 6) Long line/missing newline in getSSLModeValue [net.NaiveTrustManager] 7) See items 2-4. [NetConnection] 8) Several long lines. Was long before, but maybe it is possible to break some of them? [OpenSocketAction] 9) Document fall-through in switch? There also seems to be a mix of tabs and spaces in the changed code. Even though the code already contains both, it would be nice to stick to one alternative when adding/changing lines. I'll give the functionality a try tomorrow! Thanks, A few comments after my first take at trying out the (v1) patch:
(I have only tried ssl=basic so far...) 1) No server commands (e.g. shutdown, ping, runtimeinfo) worked after the server was started with SSL on (basic) . The message I'm getting is: Invalid reply header from network server: Invalid string . 2) Using -Dderby.drda.sslMode=basic (and ssl=basic in the client URL) seemed to work fine, although I did not actually inspect the network traffic to verify encryption. 3) Using ssl=basic as an option to the NetworkServerControl start command did not work: Command line: java <properties> -jar derbyrun.jar server start ssl=basic Result: Invalid number of arguments for command start. Command line: java <properties> -jar derbyrun.jar server start -ssl=basic Result: Argument -ssl=basic is unknown. I tried both with and without the -unsecure option/plain-text authentication. 4) The funcSpec says: SSL at the server side is activated with the property derby.drda.sslMode (default off) or the -ssl option for the server command. By "the server command", do you mean the start command of the server? This should perhaps be clarified in the funcSpec? 5) The funcSpec also says: The property may have three values: "off", "basic" and "peerAuthentication" However, the example in section 2.3 is using ssl=authenticate. Also, comments in the patch seem to indicate that "false", "true" and "auth" are also valid property values. What is (or should be) the correct set of valid values? 6) I verified that connection attempts against a server started with SSL off, but with ssl=basic in the client URL, resulted in an informative error message on the client side. Thanks John!
1) You need -ssl basic on these too, since they are clients. My mistake, it's not mentioned in the func spec, but it is implemented. This is also another argument for DERBY-2362. 3) This is in line with the other options, e.g. java -jar derbyrun.jar server start p=1234 Invalid number of arguments for command start. or java -jar derbyrun.jar server start -p=1234 Argument -p=1234 is unknown. 4) I will clarify this 5) Typo. I will fix them (some are already fixed in my sandbox) Yes, using the -ssl basic option for the other NetworkServerControl commands as well worked just great, makes sense. So 4) would essentially be clear enough by just appending an "s" to "the server command" :)
Regarding 3), I only followed the examples in the funcSpec, thinking this was somehow implemented differently. The regular variant <command> -ssl <sslMode> [otherOptions] works just fine. Uploaded new funcspec and V2 of the patch. Thanks to John and Kristian for their comments.
Uploaded v3. Small change from v2: Avoid creating a new TrustManager for each connection.
Committed revision 511785.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||