Add TLS min/max version. #641

tsaarni · 2023-01-25T14:24:40Z

Documentation for new optional flags for setting TLS versions, which will be introduced in PR etcd-io/etcd#15156

Signed-off-by: Tero Saarni tero.saarni@est.tech

tsaarni · 2023-01-25T19:05:37Z

Note to reviewers: I created the subheading "Common options" and included also ciphers intentionally under that. Currently it might be bit too easy to confuse that to belong to "Peer (server-to-server / cluster) communication" options, although it impacts both.

ahrtr · 2023-01-25T23:16:20Z

@ptabor @spzala Do you know usually how do you update the configuration guide?

I think probably it would better if we could automatically sync the content in help.go.

spzala · 2023-01-26T02:11:55Z

@ahrtr afaik, it's a manual update. Considering it's easy to miss manual updating, one quick solution would be to add a sentence in the guide to point to help.go for the latest. For example, add "For the most latest flags, refer to the Security flags." after etcd takes several certificate related configuration options, either through command-line flags or environment variables in the config guide. If we can automate, that's great but for now, I think this workaround should work for now. We can use the similar approach at other places in the doc as well where we mention flags. What do you think? Thanks!

spzala

Thanks @tsaarni Few more small comments besides the one I added in the PR discussion.

spzala · 2023-01-26T02:15:07Z

content/en/docs/v3.6/op-guide/configuration.md

@@ -164,6 +164,10 @@ Flags are presented below using the format `--flag-name DEFAULT_VALUE`.
  Comma-separated whitelist of origins for CORS, or cross-origin resource sharing, (empty or * means allow all).
 --host-whitelist '*'
  Acceptable hostnames from HTTP client requests, if server is not secure (empty or * means allow all).
+--tls-min-version 'TLS12'
+  Minimum TLS version supported by etcd. Possible values: TLS12, TLS13.


I would suggest to remove possible values so that we don't need to maintain updates in the future.

I will fix it, but I wonder how users will find the accepted values?

spzala · 2023-01-26T02:16:26Z

content/en/docs/v3.6/op-guide/configuration.md

+--tls-min-version 'TLS12'
+  Minimum TLS version supported by etcd. Possible values: TLS12, TLS13.
+--tls-max-version ''
+  Maximum TLS version supported by etcd. Possible values: TLS12, TLS13 (empty will be auto-populated by Go).


Same here, we should remove possible values so that we don't need to maintain updates in the future. Leaving, Empty will be auto-populated by Go. is fine.

spzala · 2023-01-26T02:17:29Z

content/en/docs/v3.6/op-guide/security.md

@@ -42,8 +42,16 @@ The peer options work the same way as the client-to-server options:

 If either a client-to-server or peer certificate is supplied the key must also be set. All of these configuration options are also available through the environment variables, `ETCD_CA_FILE`, `ETCD_PEER_CA_FILE` and so on.

+**Common options:**
+
 `--cipher-suites`: Comma-separated list of supported TLS cipher suites between server/client and peers (empty will be auto-populated by Go). Available from v3.2.22+, v3.3.7+, and v3.4+.


We don't support 3.2 and 3.3, so I think Available from v3.2.22+, v3.3.7+, and v3.4+. is not needed.

spzala · 2023-01-26T02:18:12Z

content/en/docs/v3.6/op-guide/security.md

 `--cipher-suites`: Comma-separated list of supported TLS cipher suites between server/client and peers (empty will be auto-populated by Go). Available from v3.2.22+, v3.3.7+, and v3.4+.

+`--tls-min-version=<version>` Sets the minimum TLS version supported by etcd. The default value is `TLS12`.


Same, The default value is TLS12. is not needed.

ahrtr · 2023-01-26T05:55:14Z

@ahrtr afaik, it's a manual update. Considering it's easy to miss manual updating, one quick solution would be to add a sentence in the guide to point to help.go for the latest. For example, add "For the most latest flags, refer to the Security flags." after etcd takes several certificate related configuration options, either through command-line flags or environment variables in the config guide. If we can automate, that's great but for now, I think this workaround should work for now. We can use the similar approach at other places in the doc as well where we mention flags. What do you think? Thanks!

Thanks @spzala for the feedback. The security doc is relatively simple, so I am not much concerning about it. Instead, the configuration doc is supposed to contain all available flags. But obvious it isn't. For example the following flags are missing,

So it makes more sense to add a link in the configuration doc.

tsaarni · 2023-01-26T08:18:12Z

Thanks @tsaarni Few more small comments besides the one I added in the PR discussion.

Thank you for review @spzala!

I have now removed the specifics: protocol versions and cipher-suite etcd versions (just for clarity: etcd-io/etcd#15156 did not touch ciphers). This is to avoid further updates on the web site when the values change in future. The values remain in --help.

spzala · 2023-01-26T14:38:51Z

@ahrtr afaik, it's a manual update. Considering it's easy to miss manual updating, one quick solution would be to add a sentence in the guide to point to help.go for the latest. For example, add "For the most latest flags, refer to the Security flags." after etcd takes several certificate related configuration options, either through command-line flags or environment variables in the config guide. If we can automate, that's great but for now, I think this workaround should work for now. We can use the similar approach at other places in the doc as well where we mention flags. What do you think? Thanks!

Thanks @spzala for the feedback. The security doc is relatively simple, so I am not much concerning about it. Instead, the configuration doc is supposed to contain all available flags. But obvious it isn't. For example the following flags are missing,
* [--experimental-snapshot-catchup-entries](https://github.com/etcd-io/etcd/blob/3b612ce345753f97156833e20e93bf78b56d5267/server/etcdmain/config.go#L282)

* [--warning-unary-request-duration](https://github.com/etcd-io/etcd/blob/3b612ce345753f97156833e20e93bf78b56d5267/server/etcdmain/config.go#L275)
So it makes more sense to add a link in the configuration doc.

@ahrtr yrw, and yes! I was thinking to suggest to make those changes in this PR but with some more thoughts, I think I can clean up config file to address maintenance issue. The help.go is a go file but it's a structured almost as a doc. Let me create a separate PR for this for an easier review of changes. Thanks!

spzala · 2023-01-26T14:44:29Z

Thanks @tsaarni Few more small comments besides the one I added in the PR discussion.

Thank you for review @spzala!

I have now removed the specifics: protocol versions and cipher-suite etcd versions (just for clarity: etcd-io/etcd#15156 did not touch ciphers). This is to avoid further updates on the web site when the values change in future. The values remain in --help.

Thanks @tsaarni - we need to wait for the etcd repo PR to merge.

tsaarni · 2023-01-31T11:21:58Z

The implementation etcd-io/etcd#15156 is now merged!

spzala · 2023-01-31T20:44:22Z

The implementation etcd-io/etcd#15156 is now merged!

@tsaarni thanks! I am good with this PR but one more small request. Can you please add this sentence (or something similar if you have a different opinion) at the end of first section (right before Basic setup section) The list of flags provided in this doc may not be up-to-date due to ongoing development changes. For the latest available flags, run etcd --help or refer to the [etcd help][]. ` Where
[etcd help] is linked to https://github.com/etcd-io/etcd/blob/main/server/etcdmain/help.go
Something similar to this PR - #642

tsaarni · 2023-02-01T12:42:05Z

@tsaarni thanks! I am good with this PR but one more small request. Can you please add this sentence (or something similar if you have a different opinion) at the end of first section (right before Basic setup section)

@spzala Sure, no problem! I've added the text. I added it only to the latest release version i.e. the only version this PR has modified so far.

spzala · 2023-02-01T15:45:07Z

@tsaarni thanks! I am good with this PR but one more small request. Can you please add this sentence (or something similar if you have a different opinion) at the end of first section (right before Basic setup section)

@spzala Sure, no problem! I've added the text. I added it only to the latest release version i.e. the only version this PR has modified so far.

That's good. Thanks @tsaarni

spzala · 2023-02-01T16:52:51Z

cc @ahrtr

ahrtr · 2023-02-01T18:38:55Z

content/en/docs/v3.6/op-guide/configuration.md

@@ -166,6 +166,10 @@ The list of flags provided below may not be up-to-date due to ongoing developmen
  Comma-separated whitelist of origins for CORS, or cross-origin resource sharing, (empty or * means allow all).
 --host-whitelist '*'
  Acceptable hostnames from HTTP client requests, if server is not secure (empty or * means allow all).
+--tls-min-version 'TLS12'


It's "TLS1.2" in etcd-io/etcd#15156, so I suggest to keep it consistent.

Sorry for missing that! I've pushed a fix.

Documentation for new optional flags for setting TLS versions. Signed-off-by: Tero Saarni <tero.saarni@est.tech>

ahrtr

LGTM

Thanks @tsaarni

tsaarni mentioned this pull request Jan 25, 2023

Add TLSv1.3 support. etcd-io/etcd#15156

Merged

spzala reviewed Jan 26, 2023

View reviewed changes

tsaarni force-pushed the add-minmax-allow-tls13 branch from 357eb17 to d5b401c Compare January 26, 2023 08:11

tsaarni force-pushed the add-minmax-allow-tls13 branch from d5b401c to 0b6aebe Compare February 1, 2023 07:37

spzala approved these changes Feb 1, 2023

View reviewed changes

ahrtr reviewed Feb 1, 2023

View reviewed changes

Add TLS min/max version.

566ac46

Documentation for new optional flags for setting TLS versions. Signed-off-by: Tero Saarni <tero.saarni@est.tech>

tsaarni force-pushed the add-minmax-allow-tls13 branch from 0b6aebe to 566ac46 Compare February 1, 2023 18:40

ahrtr approved these changes Feb 1, 2023

View reviewed changes

spzala merged commit d331ef1 into etcd-io:main Feb 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TLS min/max version. #641

Add TLS min/max version. #641

tsaarni commented Jan 25, 2023

tsaarni commented Jan 25, 2023

ahrtr commented Jan 25, 2023

spzala commented Jan 26, 2023

spzala left a comment

spzala Jan 26, 2023

tsaarni Jan 26, 2023

spzala Jan 26, 2023

spzala Jan 26, 2023

spzala Jan 26, 2023

ahrtr commented Jan 26, 2023

tsaarni commented Jan 26, 2023

spzala commented Jan 26, 2023

spzala commented Jan 26, 2023

tsaarni commented Jan 31, 2023

spzala commented Jan 31, 2023

tsaarni commented Feb 1, 2023

spzala commented Feb 1, 2023

spzala commented Feb 1, 2023

ahrtr Feb 1, 2023

tsaarni Feb 1, 2023

ahrtr left a comment

		`--cipher-suites`: Comma-separated list of supported TLS cipher suites between server/client and peers (empty will be auto-populated by Go). Available from v3.2.22+, v3.3.7+, and v3.4+.

		`--tls-min-version=<version>` Sets the minimum TLS version supported by etcd. The default value is `TLS12`.

Add TLS min/max version. #641

Add TLS min/max version. #641

Conversation

tsaarni commented Jan 25, 2023

tsaarni commented Jan 25, 2023

ahrtr commented Jan 25, 2023

spzala commented Jan 26, 2023

spzala left a comment

Choose a reason for hiding this comment

spzala Jan 26, 2023

Choose a reason for hiding this comment

tsaarni Jan 26, 2023

Choose a reason for hiding this comment

spzala Jan 26, 2023

Choose a reason for hiding this comment

spzala Jan 26, 2023

Choose a reason for hiding this comment

spzala Jan 26, 2023

Choose a reason for hiding this comment

ahrtr commented Jan 26, 2023

tsaarni commented Jan 26, 2023

spzala commented Jan 26, 2023

spzala commented Jan 26, 2023

tsaarni commented Jan 31, 2023

spzala commented Jan 31, 2023

tsaarni commented Feb 1, 2023

spzala commented Feb 1, 2023

spzala commented Feb 1, 2023

ahrtr Feb 1, 2023

Choose a reason for hiding this comment

tsaarni Feb 1, 2023

Choose a reason for hiding this comment

ahrtr left a comment

Choose a reason for hiding this comment