Skip to content

Conversation

DarkSpir
Copy link
Contributor

@DarkSpir DarkSpir commented Dec 5, 2024

What is the purpose of this change? What does it change?

With tls activated, the rest-server will provide insecure tls versions and insecure or broken tls cipher suits. This change configures the default settings in tls mode, sets a secure set of cipher suits (according to CIS NGINX Benchmark v2.1.0) and sets up TLSv1.2 as min version. It also adds a command line parameter to select a different version as min version.

The current version of the crypto.tls library also sets a small, secure set of cipher suits as default and limits the available TLS versions to 1.2 and 1.3. Earlier versions are also available but have to be explicitly enabled during compilation.

Was the change discussed in an issue or in the forum before?

Closes #251

Checklist

  • I have enabled maintainer edits for this PR
  • I have added tests for all changes in this PR
  • I have added documentation for the changes (in the manual)
  • There's a new file in changelog/unreleased/ that describes the changes for our users (template here)
  • I have run gofmt on the code in all commits
  • All commit messages are formatted in the same style as the other commits in the repo
  • I'm done, this Pull Request is ready for review

@DarkSpir
Copy link
Contributor Author

DarkSpir commented Dec 5, 2024

I actually have no idea how the tests work. From what I can guess out of main_test.go I assume it tests the program response to certain conditions to ensure it will throw errors when parameters are missing or misleading. If that's the case I think it is not necessary to add tests for what I did, because if --tls-min-ver is missing or provides a wrong value, it defaults to TLSv1.2. Even when a user sets --tls-min-ver 1.0 and the build option is not set, the library will internally default to 1.2.

Copy link
Member

@MichaelEischer MichaelEischer left a comment

Choose a reason for hiding this comment

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

#322 will bump Go to 1.22 which disables TLS before 1.2 by default. TLS 1.2 is older than Go 1.0. So I don't think anyone will ever need an older TLS version (if they do then they should fix their setup instead).

That would sort of make the --tls-min-version somewhat obsolete. WDYT?

Removing the 3DES ciphers is fine. The cipher suite subset is already added by https://datatracker.ietf.org/doc/html/rfc5289 which is from 2008. That also implicitly means that clients that only support TLS < 1.2 won't be able to use any of those ciphers, which is one reason more to not even consider offering TLS 1.0 and 1.1.

@DarkSpir
Copy link
Contributor Author

DarkSpir commented Feb 8, 2025

That would sort of make the --tls-min-version somewhat obsolete. WDYT?

Technically yes but it could still be used to enforce a TLS 1.3 only-setup which could be preferable to calm down a few paranoid CISOs out there. Should we remove it entirely or keep and document it?

@MichaelEischer
Copy link
Member

Should we remove it entirely or keep and document it?

We can keep the flag. But we should document the allowed TLS versions.

@leapfog
Copy link

leapfog commented Mar 27, 2025

Is there a chance of a new release, with sane SSL defaults, in the near future?

@DarkSpir
Copy link
Contributor Author

Sorry, I somehow forgot about this. I changed everything as requested and I hope we're good to go now.

Copy link
Member

@MichaelEischer MichaelEischer left a comment

Choose a reason for hiding this comment

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

LGTM. I've rebased the PR to remove the merge commit and also updated the readme a bit further.

@MichaelEischer MichaelEischer enabled auto-merge (squash) April 14, 2025 18:59
DarkSpir and others added 2 commits April 14, 2025 21:02
rest-server/main.go: Added parameter handling for TLS min version

rest-server/main.go: Added crypto.tls, implemented and configured tlsConfig object
Copy link
Member

@MichaelEischer MichaelEischer left a comment

Choose a reason for hiding this comment

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

Rebased again due to another conflict and fixed the linter error.

@MichaelEischer MichaelEischer enabled auto-merge (squash) April 14, 2025 19:09
@MichaelEischer MichaelEischer merged commit 2b6f0b3 into restic:master Apr 14, 2025
4 checks passed
@DarkSpir DarkSpir deleted the tlsoptions branch April 14, 2025 20:24
@Framsfex
Copy link

https://github.com/restic/rest-server/releases/ offers still rest-server 0.13.0 (2024-07-26)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Secure default or configurable TLS settings
4 participants