DISPATCH-884 - Added schema property protocols to allow configurable … #224
Conversation
…TLS protocol versions
| @@ -45,6 +45,7 @@ struct qd_config_ssl_profile_t { | |||
| char *ssl_certificate_file; | |||
| char *ssl_private_key_file; | |||
| char *ciphers; | |||
| @@ -45,6 +45,7 @@ struct qd_config_ssl_profile_t { | |||
| char *ssl_certificate_file; | |||
| char *ssl_private_key_file; | |||
| char *ciphers; | |||
| char *protocols; | |||
| @@ -305,6 +305,12 @@ typedef struct qd_server_config_t { | |||
| */ | |||
| char *ciphers; | |||
|
|
|||
| /** | |||
| * This list is a space separated string of the allowed TLS protocols. The current possibilities are TLSv1 TLSv1.1 TLSv1.2. | |||
| * For example, if you want to permit only TLS V.1.1 and TLSv1.2, your value for the protocols would be TLSv1.1 TLSv1.2. If this attribute is not set, then all the TLS protocols are allowed. | |||
| }, | ||
| "protocols": { | ||
| "type": "string", | ||
| "description": "This list is a space separated string of the allowed TLS protocols. The current possibilities are TLSv1 TLSv1.1 TLSv1.2. For example, if you want to permit only TLS V.1.1 and TLSv1.2, your value for the protocols would be TLSv1.1 TLSv1.2. If this attribute is not set, then all the TLS protocols are allowed.", |
There was a problem hiding this comment.
"then all the protocols are allowed" is not quite right. If unset, it defers to the system-wide configuration.
There was a problem hiding this comment.
A couple notes about the description:
I think it's most user-friendly to first define the attribute and then describe the syntax for using it. Something like this:
The TLS protocols that this sslProfile can use. You can specify a list of one or more of TLSv1, TLSv1.1, or TLSv1.2. To specify multiple protocols, separate the protocols with a space. For example, to permit the sslProfile to use TLS v1.1 and TLS v1.2 only, you would set the value to TLSv1.1 TLSv1.2. If you do not specify a value, the sslProfile uses the TLS protocol specified by the system-wide configuration.
Instead of "TLSv1", it would be better to make the value "TLSv1.0". That would make it clear that it's referring to the 1.0 version, and not a superset of 1.x.
Also, we should define "system-wide configuration" - where would this be defined?
There was a problem hiding this comment.
Since the value is really a list of values, would it work to define the possible values in "type"? Then we wouldn't have to describe the permitted values in the description, making it simpler. Sort of like this:
"type": ["TLSv1", "TLSv1.1", "TLSv1.2"]
There was a problem hiding this comment.
setting type to "type": ["TLSv1", "TLSv1.1", "TLSv1.2"] will mean that only one in the list is allowed. That is how router schema syntax works.
For example look at "stripAnnotations". It type is set to "type": ["in", "out", "both", "no"] which means stripAnnotations can have only one of the 4 values "in" or "out" or "both" or "no"
There was a problem hiding this comment.
The proton API requires that I use TLSv1 and not TLSV1.0
Please take a look at the doc of this function
https://github.com/apache/qpid-proton/blob/master/proton-c/include/proton/ssl.h#L242
I will gladly change it to TLSv1.0 if proton does it. This is a question for @ssorj
There was a problem hiding this comment.
Your description for protocols - "The TLS protocols that this sslProfile can use. You can specify a list of one or more of TLSv1, TLSv1.1, or TLSv1.2. To specify multiple protocols, separate the protocols with a space. For example, to permit the sslProfile to use TLS v1.1 and TLS v1.2 only, you would set the value to TLSv1.1 TLSv1.2. If you do not specify a value, the sslProfile uses the TLS protocol specified by the system-wide configuration."
looks good to me. I will update.
| @@ -305,6 +305,12 @@ typedef struct qd_server_config_t { | |||
| */ | |||
| char *ciphers; | |||
|
|
|||
| /** | |||
| * This list is a space separated string of the allowed TLS protocols. The current possibilities are TLSv1 TLSv1.1 TLSv1.2. | |||
There was a problem hiding this comment.
A possible enhancement is to sub spaces for commas and document it as a comma-separated list.
…stead of failing immediately
…pdate test as needed (apache#670) Fixes some issues in clogger by removing the error check for missing acks. clogger is a mis-behaving client and is always SIGTERMed by the tests so no need to track acks.
…TLS protocol versions