CORE-5589 Add config to select minimum TLS version #21372

michael-redpanda · 2024-07-12T20:32:05Z

Backports Required

Release Notes

Features

Adds new cluster config tls_min_version that allows users of Redpanda to specify the minimum version of TLS Redpanda will support. By default, the value is TLS v1.2.

michael-redpanda · 2024-07-12T20:32:33Z

/dt

vbotbuildovich · 2024-07-12T23:06:16Z

skipped ducktape retry in https://buildkite.com/redpanda/redpanda/builds/51444#0190a8ff-f282-4962-9130-cdae90c56395:
pandatriage cache was not found

skipped ducktape retry in https://buildkite.com/redpanda/redpanda/builds/51444#0190a8ff-f283-4374-b05f-c8f3ace0da97:
pandatriage cache was not found

skipped ducktape retry in https://buildkite.com/redpanda/redpanda/builds/51444#0190a900-3c9c-4c3d-bb30-f7515b63be79:
pandatriage cache was not found

skipped ducktape retry in https://buildkite.com/redpanda/redpanda/builds/51444#0190a900-3c9a-47f4-a945-b7234c7c4231:
pandatriage cache was not found

skipped ducktape retry in https://buildkite.com/redpanda/redpanda/builds/51444#0190a900-3c9e-4f7b-99bc-0f992927e9c8:
pandatriage cache was not found

michael-redpanda · 2024-07-15T20:16:00Z

/dt

vbotbuildovich · 2024-07-15T23:10:12Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/51544#0190b861-ef5a-44a6-9745-c5d733a65b9d

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/51585#0190bc60-8720-4496-92aa-093c7a1664fb

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/51585#0190bc61-afa7-4c95-ab0e-76c7e1a9aceb

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/51585#0190bc61-afa5-4650-8c17-53a7948dc346

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/51722#0190c614-6cde-4282-8ace-4a0b0c2ac07b

michael-redpanda · 2024-07-16T15:22:42Z

Force push 9620dba:

Switched from minimum_tls_version to tls_min_version

Deflaimun · 2024-07-16T17:51:08Z

What specific string do we want our users using in this property?
"v.1.3" or "1.3" ?

kbatuigas · 2024-07-16T19:15:15Z

src/v/config/configuration.cc

+  , tls_min_version(
+      *this,
+      "tls_min_version",
+      "The minimum version of TLS that Redpanda will support.",


Suggested change

"The minimum version of TLS that Redpanda will support.",

"The minimum TLS version that Redpanda supports.",

Nit, for economy / alignment with docs style guide

michael-redpanda · 2024-07-16T19:51:04Z

What specific string do we want our users using in this property? "v.1.3" or "1.3" ?

"v1.3"

oleiman

looks good. i need to go over the dt tests in a bit more detail, but I wanted to flag the legacy_default thing before bed.

oleiman · 2024-07-17T00:21:18Z

src/v/config/types.h

+          .match(to_string_view(tls_version::v1_0), tls_version::v1_0)
+          .match(to_string_view(tls_version::v1_1), tls_version::v1_1)
+          .match(to_string_view(tls_version::v1_2), tls_version::v1_2)
+          .match(to_string_view(tls_version::v1_3), tls_version::v1_3);


suggestion: a default match to avoid throwing?

suggestion: a default match to avoid throwing?

Yeah, and I think this can be reused in the next commit.

tests/rptest/tests/tls_version_test.py

oleiman · 2024-07-17T05:36:47Z

src/v/config/configuration.cc

+       tls_version::v1_1,
+       tls_version::v1_2,
+       tls_version::v1_3},
+      legacy_default<tls_version>(tls_version::v1_0, legacy_version{12})) {}


So this is the issue from late last year that @dotnwat was referring to. I think we're subject to this weird behavior here as well. You might try:

upgrade from <= logical 12 to HEAD

set the min tls version to 1.2

restart the broker

see whether the min tls version is set back to 1.0

In this case I think the escape hatch would be to set something other than the current version non-legacy default (e.g. 1.1 or 1.3), but it's worth being aware of, esp. if this is a compliance concern.

Thanks for flagging. I'll write up a DT test for this specifically

Yeah thanks for flagging that is an issue. Setting to v1.2 and then restarting resets it back to v1.0....

BenPope · 2024-07-17T10:11:33Z

src/v/config/types.h

+          .match(to_string_view(tls_version::v1_0), tls_version::v1_0)
+          .match(to_string_view(tls_version::v1_1), tls_version::v1_1)
+          .match(to_string_view(tls_version::v1_2), tls_version::v1_2)
+          .match(to_string_view(tls_version::v1_3), tls_version::v1_3);


suggestion: a default match to avoid throwing?

Yeah, and I think this can be reused in the next commit.

src/v/config/convert.h

BenPope · 2024-07-17T10:41:58Z

src/v/config/convert.h

+        if (
+          std::find(acceptable_values.begin(), acceptable_values.end(), value)
+          == acceptable_values.end()) {
+            return false;
+        }
+
+        rhs = string_switch<type>(std::string_view{value})
+                .match(to_string_view(type::v1_0), type::v1_0)
+                .match(to_string_view(type::v1_1), type::v1_1)
+                .match(to_string_view(type::v1_2), type::v1_2)
+                .match(to_string_view(type::v1_3), type::v1_3);
+
+        return true;


is acceptable_values used anywhere else?

perhaps:

Suggested change

if (

std::find(acceptable_values.begin(), acceptable_values.end(), value)

== acceptable_values.end()) {

return false;

}

rhs = string_switch<type>(std::string_view{value})

.match(to_string_view(type::v1_0), type::v1_0)

.match(to_string_view(type::v1_1), type::v1_1)

.match(to_string_view(type::v1_2), type::v1_2)

.match(to_string_view(type::v1_3), type::v1_3);

return true;

auto out = string_switch<std::optional<type>>(std::string_view{value})

.match(to_string_view(type::v1_0), type::v1_0)

.match(to_string_view(type::v1_1), type::v1_1)

.match(to_string_view(type::v1_2), type::v1_2)

.match(to_string_view(type::v1_3), type::v1_3)

.default_match(std::nullopt);

if(out.has_value()) {

rhs = out.value();

}

return out.has_value();

aanthony-rp

couple nits, not blocking. Looks clear to me what's going on. nice work, good tests.

src/v/config/convert.h

tests/rptest/tests/cluster_config_test.py

Signed-off-by: Michael Boquard <michael@redpanda.com>

michael-redpanda · 2024-07-17T20:38:45Z

Force push 1cb3b6d:

Lint zee python

dotnwat

lgtm.

curious why legacy_default was removed? it's not that i think it should or short not be used, but rather, if it was thought to have been needed before, it's surprising that the need disappeared given it solves such a specific problem.

BenPope · 2024-07-18T10:11:24Z

src/v/cloud_storage_clients/configuration.cc

@@ -33,6 +33,8 @@ build_tls_credentials(
    cred_builder.set_ciphersuites(
      {config::tlsv1_3_ciphersuites.data(),
       config::tlsv1_3_ciphersuites.size()});
+    cred_builder.set_minimum_tls_version(


I'm intrigued, why is it set here, but not, for example, for the oidc client or the cloud_roles client?

Which also begs the question, should it be possible to configure this per client, or listener?

I'm intrigued, why is it set here, but not, for example, for the oidc client or the cloud_roles client?

Thanks for pointing that out, definitely a miss. I'll make the changes, but it's probably not as big as a deal as ensuring our services can be selective of what TLS version they support.

Which also begs the question, should it be possible to configure this per client, or listener?

Maybe in a future update. For now, to get something done to help our customers was to provide a single cluster config.

michael-redpanda · 2024-07-18T11:54:31Z

lgtm.

curious why legacy_default was removed? it's not that i think it should or short not be used, but rather, if it was thought to have been needed before, it's surprising that the need disappeared given it solves such a specific problem.

Because it was broken for this use case. If I started with a v24.1 cluster and upgraded it, yes the value of the config went to v1.0, but if I tried to set it to v1.2 (the default value) and restarted, the legacy default code would reset it to v1.0.
@deniscoady did some analysis using telemetry data and saw that the TLSv1.2 and TLSv1.3 were the only protocols in use
Other products by default only enable TLSv1.2 and TLSv1.3 out of the box

michael-redpanda · 2024-07-18T12:03:24Z

Force push c3a61b3:

Added set_minimum_tls_version to remaining users of TLS

Signed-off-by: Michael Boquard <michael@redpanda.com>

michael-redpanda · 2024-07-18T12:07:35Z

Force push 8bf2e75:

Being consistent in how tls_min_version is accessed

dotnwat · 2024-07-18T15:28:38Z

Because it was broken for this use case. If I started with a v24.1 cluster and upgraded it, yes the value of the config went to v1.0, but if I tried to set it to v1.2 (the default value) and restarted, the legacy default code would reset it to v1.0.

The feature being broken doesn't eliminate a problem that exists, right?

@deniscoady did some analysis using telemetry data and saw that the TLSv1.2 and TLSv1.3 were the only protocols in use
Other products by default only enable TLSv1.2 and TLSv1.3 out of the box

I guess these are the reasons why altered upgrade behavior is no longer needed?

michael-redpanda · 2024-07-18T17:06:05Z

Because it was broken for this use case. If I started with a v24.1 cluster and upgraded it, yes the value of the config went to v1.0, but if I tried to set it to v1.2 (the default value) and restarted, the legacy default code would reset it to v1.0.

The feature being broken doesn't eliminate a problem that exists, right?

Correct but....

@deniscoady did some analysis using telemetry data and saw that the TLSv1.2 and TLSv1.3 were the only protocols in use
Other products by default only enable TLSv1.2 and TLSv1.3 out of the box

I guess these are the reasons why altered upgrade behavior is no longer needed?

yes this eliminated the need.

michael-redpanda · 2024-07-18T17:08:09Z

CI Failure:

CORE-5514

More details redpanda-data/redpanda#21372

michael-redpanda self-assigned this Jul 12, 2024

github-actions bot added the area/redpanda label Jul 12, 2024

michael-redpanda force-pushed the CORE-5589-Minimum-TLS branch from ab89c76 to 015bf39 Compare July 15, 2024 20:15

michael-redpanda marked this pull request as ready for review July 16, 2024 14:54

michael-redpanda requested a review from a team as a code owner July 16, 2024 14:54

michael-redpanda requested review from a team, pgellert and oleiman and removed request for a team July 16, 2024 14:54

michael-redpanda force-pushed the CORE-5589-Minimum-TLS branch from 040d7e4 to 9620dba Compare July 16, 2024 15:22

michael-redpanda requested review from aanthony-rp, BenPope and deniscoady and removed request for pgellert July 16, 2024 15:27

kbatuigas reviewed Jul 16, 2024

View reviewed changes

oleiman reviewed Jul 17, 2024

View reviewed changes

BenPope reviewed Jul 17, 2024

View reviewed changes

aanthony-rp previously approved these changes Jul 17, 2024

View reviewed changes

src/v/config/convert.h Show resolved Hide resolved

tests/rptest/tests/cluster_config_test.py Show resolved Hide resolved

michael-redpanda added 4 commits July 17, 2024 16:33

config: Introduced config::tls_version

73496c5

Signed-off-by: Michael Boquard <michael@redpanda.com>

config: Add support for parsing config::tls_version

db030c3

Signed-off-by: Michael Boquard <michael@redpanda.com>

config: Introduce tls_min_version

bfa8914

Signed-off-by: Michael Boquard <michael@redpanda.com>

config: Added conversion of config to Seastar

c00ae2d

Signed-off-by: Michael Boquard <michael@redpanda.com>

michael-redpanda dismissed aanthony-rp’s stale review via eb8fcb9 July 17, 2024 20:33

michael-redpanda dismissed oleiman’s stale review via 1cb3b6d July 17, 2024 20:38

michael-redpanda force-pushed the CORE-5589-Minimum-TLS branch from eb8fcb9 to 1cb3b6d Compare July 17, 2024 20:38

michael-redpanda requested review from oleiman, aanthony-rp, kbatuigas and BenPope July 17, 2024 20:38

oleiman previously approved these changes Jul 17, 2024

View reviewed changes

dotnwat reviewed Jul 18, 2024

View reviewed changes

BenPope previously approved these changes Jul 18, 2024

View reviewed changes

michael-redpanda dismissed stale reviews from BenPope and oleiman via c3a61b3 July 18, 2024 12:02

michael-redpanda force-pushed the CORE-5589-Minimum-TLS branch from 1cb3b6d to c3a61b3 Compare July 18, 2024 12:02

michael-redpanda requested review from oleiman, BenPope and dotnwat July 18, 2024 12:03

michael-redpanda added 2 commits July 18, 2024 08:06

net: Using tls_min_version

c651419

Signed-off-by: Michael Boquard <michael@redpanda.com>

dt: Added tests for tls_min_version

8bf2e75

Signed-off-by: Michael Boquard <michael@redpanda.com>

michael-redpanda force-pushed the CORE-5589-Minimum-TLS branch from c3a61b3 to 8bf2e75 Compare July 18, 2024 12:06

BenPope approved these changes Jul 18, 2024

View reviewed changes

oleiman approved these changes Jul 18, 2024

View reviewed changes

michael-redpanda merged commit 9e44166 into redpanda-data:dev Jul 18, 2024
17 of 20 checks passed

vuldin added a commit to redpanda-data/docs that referenced this pull request Jul 29, 2024

Add tls_min_version to cluster properties

365d4df

More details redpanda-data/redpanda#21372

vuldin mentioned this pull request Jul 29, 2024

Add tls_min_version to cluster properties redpanda-data/docs#640

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CORE-5589 Add config to select minimum TLS version #21372

CORE-5589 Add config to select minimum TLS version #21372

michael-redpanda commented Jul 12, 2024 •

edited

Loading

michael-redpanda commented Jul 12, 2024

vbotbuildovich commented Jul 12, 2024 •

edited

Loading

michael-redpanda commented Jul 15, 2024

vbotbuildovich commented Jul 15, 2024 •

edited

Loading

michael-redpanda commented Jul 16, 2024

Deflaimun commented Jul 16, 2024

kbatuigas Jul 16, 2024 •

edited

Loading

michael-redpanda commented Jul 16, 2024

oleiman left a comment

oleiman Jul 17, 2024

BenPope Jul 17, 2024

oleiman Jul 17, 2024

michael-redpanda Jul 17, 2024

michael-redpanda Jul 17, 2024

BenPope Jul 17, 2024

BenPope Jul 17, 2024

aanthony-rp left a comment

michael-redpanda commented Jul 17, 2024

dotnwat left a comment

BenPope Jul 18, 2024

michael-redpanda Jul 18, 2024

michael-redpanda commented Jul 18, 2024 •

edited

Loading

michael-redpanda commented Jul 18, 2024

michael-redpanda commented Jul 18, 2024

dotnwat commented Jul 18, 2024

michael-redpanda commented Jul 18, 2024

michael-redpanda commented Jul 18, 2024 •

edited by jira bot

Loading

	"The minimum version of TLS that Redpanda will support.",
	"The minimum TLS version that Redpanda supports.",

CORE-5589 Add config to select minimum TLS version #21372

CORE-5589 Add config to select minimum TLS version #21372

Conversation

michael-redpanda commented Jul 12, 2024 • edited Loading

Backports Required

Release Notes

Features

michael-redpanda commented Jul 12, 2024

vbotbuildovich commented Jul 12, 2024 • edited Loading

michael-redpanda commented Jul 15, 2024

vbotbuildovich commented Jul 15, 2024 • edited Loading

michael-redpanda commented Jul 16, 2024

Deflaimun commented Jul 16, 2024

kbatuigas Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

michael-redpanda commented Jul 16, 2024

oleiman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aanthony-rp left a comment

Choose a reason for hiding this comment

michael-redpanda commented Jul 17, 2024

dotnwat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-redpanda commented Jul 18, 2024 • edited Loading

michael-redpanda commented Jul 18, 2024

michael-redpanda commented Jul 18, 2024

dotnwat commented Jul 18, 2024

michael-redpanda commented Jul 18, 2024

michael-redpanda commented Jul 18, 2024 • edited by jira bot Loading

michael-redpanda commented Jul 12, 2024 •

edited

Loading

vbotbuildovich commented Jul 12, 2024 •

edited

Loading

vbotbuildovich commented Jul 15, 2024 •

edited

Loading

kbatuigas Jul 16, 2024 •

edited

Loading

michael-redpanda commented Jul 18, 2024 •

edited

Loading

michael-redpanda commented Jul 18, 2024 •

edited by jira bot

Loading