Introducing configurable setting for min tls version by pebanb · Pull Request #199 · basecamp/kamal-proxy

pebanb · 2026-03-19T13:56:08Z

Introducing TLS 1.3 as a config flag for more secure server.
An improvement vs #190
Adding also test coverage for the new feature.

Copilot

Pull request overview

This PR adds an (intended) configuration option to require TLS 1.3 as the minimum TLS version for the HTTPS server, while preserving existing TLS/ALPN configuration.

Changes:

Add OnlyTLS1_3 to internal/server.Config to control whether HTTPS enforces MinVersion = tls.VersionTLS13.
Refactor HTTPS server initialization to reuse a tls.Config instance and conditionally set MinVersion.

Tip

If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
internal/server/server.go	Creates a reusable TLS config for the HTTPS server and conditionally enforces TLS 1.3 minimum.
internal/server/config.go	Adds a new config field meant to control the TLS 1.3 minimum-version behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/server/config.go

internal/server/server.go

AnyCPU · 2026-03-20T12:24:49Z

@pebanb my mr just makes implicit explicit, no change in behavior, the same was done for http3.

in your case I would add a --min-tls option that defaults to 1.2 to preserve standard existing behavior.

pebanb · 2026-03-20T12:51:16Z

@pebanb my mr just makes implicit explicit, no change in behavior, the same was done for http3.

in your case I would add a --min-tls option that defaults to 1.2 to preserve standard existing behavior.

hi @AnyCPU the standard existing behavior is kept, it's just possible to force tls 1.3 by a configuration if required for some application, which require to enforce only new ciphers.

AnyCPU · 2026-03-20T15:59:41Z

@pebanb I know I saw the source code, in more generic case --min-tls and default "1.2" it is possible to resolve both mrs:

more explicit value in specific source code (kamal-proxy) than implicit underground of Go;
and in uniform way you are able to force "1.3" version when needed;
as a bonus it will be extremely easy to add upcoming "1.4" when needed.

from dev/ux it is easier to remember a --min-tls name than a --min-tls13 bool flag.

Copilot

Pull request overview

Adds a configurable minimum TLS version for the HTTPS (h1/h2) server, wiring it through CLI/env config and extending tests to cover the new option.

Tip

If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

Changes:

Introduce Config.MinTLS and a --min-tls / MIN_TLS configuration path.
Apply the configured TLS minimum version to the HTTPS server’s tls.Config.
Extend HTTPS deployment tests with new subtests for various minimum TLS values.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
internal/server/testing.go	Extends the test server helper to pass through MinTLS config.
internal/server/server_test.go	Adds subtests intended to validate different configured minimum TLS versions.
internal/server/server.go	Builds an HTTPS `tls.Config` with MinVersion based on config (and errors on invalid values).
internal/server/config.go	Adds `MinTLS` to server configuration struct.
internal/cmd/util.go	Adds `getEnvString` helper for string-valued env configuration.
internal/cmd/run.go	Adds `--min-tls` flag (and `MIN_TLS` env support) to populate `Config.MinTLS`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-26T12:31:27Z

internal/server/server_test.go

+			require.NoError(t, err)
+			assert.Equal(t, http.StatusOK, resp.StatusCode)
+			assert.Equal(t, "HTTP/1.1", resp.Proto)
+			assert.GreaterOrEqual(t, resp.TLS.Version, uint16(tls.VersionTLS10))
+		})


The new min-TLS subtests don't actually verify that the server enforces the configured minimum: asserting resp.TLS.Version >= TLS1.0/1.1/1.2 will pass even if the server ignores MinTLS (because the negotiated version is commonly higher anyway). Consider asserting the server's configured MinVersion directly (tests are in the same package, so you can read server.httpsServer.TLSConfig.MinVersion), and/or add a negative handshake test by forcing the client MaxVersion below the configured minimum and asserting the request fails.

Copilot · 2026-03-26T12:31:27Z

internal/server/server.go

+	case "tls1_3":
+		tlsConfig.MinVersion = tls.VersionTLS13
+	case "":
+		// No minimum TLS version set


When MinTLS is empty, the HTTPS server TLS config leaves MinVersion unset. That means the effective minimum is Go's runtime default, which can be changed by environment/runtime settings and may not meet the PR's stated goal of hardening the server. If the intent is to be secure by default, consider setting an explicit default minimum (e.g., TLS 1.2) when MinTLS is not provided, and reserve an explicit opt-out value if you need to preserve legacy behavior.

Suggested change

// No minimum TLS version set

// Default to a secure minimum TLS version when not explicitly configured.

tlsConfig.MinVersion = tls.VersionTLS12

default and empty case should use tls 1.2 because Go does it implicitly at the moment.

Copilot · 2026-03-26T12:31:27Z

internal/server/server.go

+	default:
+		return fmt.Errorf("unsupported minimum TLS version: %q (use tls1_0, tls1_1, tls1_2, or tls1_3)", s.config.MinTLS)
+	}


The new error branch for unsupported MinTLS values is user-facing behavior but isn't covered by tests. Adding a small test that sets Config.MinTLS to an invalid value and asserts Server.Start() fails with this error would help prevent regressions (especially since the flag/env input is free-form).

pebanb · 2026-03-26T12:34:11Z

@pebanb I know I saw the source code, in more generic case --min-tls and default "1.2" it is possible to resolve both mrs:

more explicit value in specific source code (kamal-proxy) than implicit underground of Go;

and in uniform way you are able to force "1.3" version when needed;

as a bonus it will be extremely easy to add upcoming "1.4" when needed.

from dev/ux it is easier to remember a --min-tls name than a --min-tls13 bool flag.

hi, introduced configurable min tls version, now you can set each version you need :)

Copilot AI review requested due to automatic review settings March 19, 2026 13:56

Copilot AI reviewed Mar 19, 2026

View reviewed changes

internal/server/config.go Show resolved Hide resolved

internal/server/config.go Outdated Show resolved Hide resolved

internal/server/server.go Show resolved Hide resolved

pebanb force-pushed the feature/tls1_3_per_config branch from 898c406 to 627aded Compare March 20, 2026 12:05

Introducing configurable setting for tls 1.3. as min version

6d6be08

pebanb force-pushed the feature/tls1_3_per_config branch from 627aded to 6d6be08 Compare March 20, 2026 12:15

adding a configuration option of min tls version

46e80ae

Copilot AI review requested due to automatic review settings March 26, 2026 12:27

Copilot started reviewing on behalf of pebanb March 26, 2026 12:28 View session

Copilot AI reviewed Mar 26, 2026

View reviewed changes

pebanb changed the title ~~Introducing configurable setting for tls 1.3. as min version~~ Introducing configurable setting for min tls version Mar 26, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing configurable setting for min tls version#199

Introducing configurable setting for min tls version#199
pebanb wants to merge 2 commits intobasecamp:mainfrom
pebanb:feature/tls1_3_per_config

pebanb commented Mar 19, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AnyCPU commented Mar 20, 2026

Uh oh!

pebanb commented Mar 20, 2026 •

edited

Loading

Uh oh!

AnyCPU commented Mar 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 26, 2026

Uh oh!

Copilot AI Mar 26, 2026

Uh oh!

AnyCPU Mar 26, 2026

Uh oh!

Copilot AI Mar 26, 2026

Uh oh!

pebanb commented Mar 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	// No minimum TLS version set
	// Default to a secure minimum TLS version when not explicitly configured.
	tlsConfig.MinVersion = tls.VersionTLS12

Conversation

pebanb commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AnyCPU commented Mar 20, 2026

Uh oh!

pebanb commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AnyCPU commented Mar 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

AnyCPU Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

pebanb commented Mar 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pebanb commented Mar 19, 2026 •

edited

Loading

pebanb commented Mar 20, 2026 •

edited

Loading