Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

supported_protocols showed as list #51144

Closed
wants to merge 1 commit into from
Closed

supported_protocols showed as list #51144

wants to merge 1 commit into from

Conversation

eedugon
Copy link
Contributor

@eedugon eedugon commented Jan 17, 2020

There's no mention that the supported_protocols setting is actually a list, which creates confusion at configuration time.

I have added the word "list" in every description of the parameter and also included an example with proper syntax.

there's no mention that the supported_protocols setting is actually a list, which creates confusion at configuration time
@eedugon eedugon added the >docs General docs changes label Jan 17, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-docs (>docs)

@lcawl lcawl added :Security/TLS SSL/TLS, Certificates v8.0.0 labels Jan 17, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security (:Security/Network)

@@ -516,7 +516,7 @@ and `full`. Defaults to `full`.
See <<ssl-tls-settings,`ssl.verification_mode`>> for an explanation of these values.

`ssl.supported_protocols`::
Supported protocols for TLS/SSL (with versions). Defaults to `TLSv1.3,TLSv1.2,TLSv1.1`.
List of supported protocols for TLS/SSL (with versions). Defaults to `TLSv1.3,TLSv1.2,TLSv1.1` (i.e `[ "TLSv1.3", "TLSv1.2", "TLSv1.1" ]`).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could reduce the duplication like this:

Suggested change
List of supported protocols for TLS/SSL (with versions). Defaults to `TLSv1.3,TLSv1.2,TLSv1.1` (i.e `[ "TLSv1.3", "TLSv1.2", "TLSv1.1" ]`).
List of supported protocols for TLS/SSL (with versions). Defaults to `[ "TLSv1.3", "TLSv1.2", "TLSv1.1" ]`.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lcawl , that's totally fine too.
I just didn't want to change the convention in the rest of the document where no double quotes are used, and provide an example of real syntax for a yaml file.

I didn't like the duplication either :)

@lcawl
Copy link
Contributor

lcawl commented Jan 17, 2020

I am working on a PR to synchronize all the common TLS setting definitions: #51017 If this PR is approved by the es-security folks, you don't need to backport it. I'll integrate it into my other PR.

@lcawl lcawl requested a review from tvernum January 17, 2020 16:51
@tvernum
Copy link
Contributor

tvernum commented Jan 17, 2020

I don't have any particular objections to this PR , but I'd like to understand where the confusion comes from.

The existing example, as written should work without change. We split a comma separated string into a list, so there is no need to use YAML list syntax.

@eedugon
Copy link
Contributor Author

eedugon commented Jan 18, 2020 via email

@tvernum
Copy link
Contributor

tvernum commented Jan 20, 2020

a lot of users use double quotes in the text values, so if they try "a,b,c" I believe it won’t work.

That shouldn't make any difference. If people are running into problems with configuring this value then I think it's worth understanding exactly what issue they hit, because I'm not very confident that this change will make much difference to them (which isn't to say that the change is bad, I just don't really follow why the addition of the word "list" would shift the doc from confusing to completely clear).

@eedugon
Copy link
Contributor Author

eedugon commented Feb 5, 2020

@tvernum : I totally agree, I thought the syntax was critical but as you said it's working even with "a,b,c" (as soon as it finds at least one valid protocol). I'm closing this PR, definitely the issue I was handling some time ago that lead me to this conclusion was something else.

Thanks and sorry for the noise!

@eedugon eedugon closed this Feb 5, 2020
@eedugon eedugon deleted the eedugon-patch-3 branch February 5, 2020 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>docs General docs changes :Security/TLS SSL/TLS, Certificates v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants