CA-208537: vdi-copy between local SRs proposes unwanted ciphers #26
Conversation
|
DONT merge, just for review currently. /cc @thomassa |
|
Basically verified, @thomassa , are you happy to start review it now 😃 |
|
Review from @thomassa specifically requested. He's off sick today so assigning to him and adding the blocked label so he can pick them up on his return. |
|
The CI for this was blocked because the OPAM file in the repo was not passing lint. I've fixed this here #27. By fixed, I mean I've disabled lint in the CI job. This is the best we can do because we are not upstream for this repo. |
|
I've restarted the travis job. You should be able to expect one of the subjobs to pass (the OCaml 4.01 one: https://travis-ci.org/xapi-project/vhd-tool/jobs/132751108) -- scrap that, the CI for this repo is more broken that I thought :) |
|
@thomassa has returned so I'll remove the blocked label. |
src/channels.ml
Outdated
|
|
||
| let of_ssl_fd fd ssl_legacy good_ciphersuites legacy_ciphersuites = | ||
| let good_ciphersuites = match good_ciphersuites with None -> failwith "good_ciphersuites not specified" | Some x -> x in | ||
| let legacy_ciphersuites = match legacy_ciphersuites with None -> failwith "legacy_ciphersuites not specified" | Some x -> x in |
There was a problem hiding this comment.
Specifying legacy_ciphersuites should be optional. So this line should be
let legacy_ciphersuites = match legacy_ciphersuites with None -> "" | Some x -> x in
|
The commit message needs a brief description of the new functionality. |
|
Thanks for @thomassa comments. I reworked the commit. For docstring in https://github.com/xapi-project/vhd-tool/pull/26/files#diff-37fdba691cdb40a308c807f7605ba30bR72 I leave a leading space for them, to make |
src/main.ml
Outdated
| Arg.(value & flag & info ["ssl-legacy"] ~doc) | ||
|
|
||
| let good_ciphersuites = | ||
| let doc = "SSL good cipher suites for TLS protocols" in |
There was a problem hiding this comment.
Description for good_ciphersuites:
let doc = "The list of ciphersuites to allow for TLS" in
|
Thank you: that looks good now except for the two remaining doc strings in main.ml. |
To enable TLSv1.2 capability for vhd-tool/sparse_dd, This changes add 3 new options to CLI parameters, ssl_legacy: boolean, allow all protocol versions instead of default TLSv1.2 good_ciphersuites: string, SSL good cipher suites for TLS protocols legacy_ciphersuites: optional string, SSL legacy cipher suites for TLS protocols Signed-off-by: Phus Lu <phus.lu@citrix.com>
|
Thanks @thomassa . |
|
@phusl All good! The CI test-build is still failing (because of badly-defined dependencies in opam, and bad packaging-instructions). So I'm considering merging this anyway... |
2 participants
To enable TLSv1.2 capability for vhd-tool/sparse_dd,
This changes add 3 new options to CLI parameters,
ssl_legacy: boolean, allow all protocol versions instead of default TLSv1.2
good_ciphersuites: string, SSL good cipher suites for TLS protocols
legacy_ciphersuites: optional string, SSL legacy cipher suites for TLS protocols
Signed-off-by: Phus Lu phus.lu@citrix.com