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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Secure Defaults for HTTPS/TLS and for Password-Based Key Derivation #10602

Open
wants to merge 3 commits into
base: master
from

Conversation

@kaepora
Copy link

kaepora commented Mar 4, 2020

Hello,

I represent Symbolic Software, an applied cryptography consultancy that has been a happy Gitea user for quite some time 馃槃I am submitting this pull request which provides two important changes to the default configurations of TLS and of password hashing in Gitea.

All changes made in this pull request have been fully tested.

Changes to TLS

Currently, Gitea allows TLS 1.0 and TLS 1.1 for HTTPS connections. These versions of TLS have long been deprecated due to security vulnerabilities, and are also no longer necessary for wide browser compatibility. The change I propose in this pull request sets TLS 1.2 as the minimum TLS version, with additional support for TLS 1.3.

On SSLLabs, we can see the difference. Before my changes:

Screen Shot 2020-03-04 at 9 46 22 AM

After my changes:

Screen Shot 2020-03-04 at 10 40 38 AM

Changes to Password-Based Key Derivation

Currently, Gitea uses pbkdf2 as the default password hashing function. In this pull request, I replace that with argon2 as the default, for the following reasons:

Therefore, given the above and especially given that argon2 is a well-specified, formally studied design that has won the Password Hashing Competition, I think using it as the default instead of pbkdf2 makes complete sense.

Thank you very much for your work on Gitea, we are huge Gitea fans and look forward to perhaps contributing more in the future.

kaepora added 2 commits Mar 4, 2020
@@ -128,6 +128,8 @@ func (srv *Server) ListenAndServeTLS(certFile, keyFile string, serve ServeFuncti
func (srv *Server) ListenAndServeTLSConfig(tlsConfig *tls.Config, serve ServeFunction) error {
go srv.awaitShutdown()

tlsConfig.MinVersion = tls.VersionTLS12

This comment has been minimized.

Copy link
@lafriks

lafriks Mar 4, 2020

Member

This should be set before when config is creating around here:

return srv.ListenAndServeTLSConfig(config, serve)

Also it would be better if this could be configured in app.ini so that it could be disable in case when needed to use with older browsers or integrations that do not support TLS 1.2

This comment has been minimized.

Copy link
@kaepora

kaepora Mar 4, 2020

Author

I intentionally set the restriction here to make sure it's enforced across all TLS connections ever made by Gitea. Given the security issues present in TLS 1.0 and 1.1, it doesn't make much sense to run TLS with vulnerable versions instead of not running it.

This comment has been minimized.

Copy link
@lafriks

lafriks Mar 4, 2020

Member

Even vulnerable security is better than no security and I agree that default should be secure by default but it would be still better to allow for users to change it in case they need it for backward compatibility

This comment has been minimized.

Copy link
@kaepora

kaepora Mar 4, 2020

Author

OK, what is the best way to expose this via app.ini?

This comment has been minimized.

Copy link
@lafriks

lafriks Mar 4, 2020

Member

I think same way as password hashing algorithm

This comment has been minimized.

Copy link
@kaepora

kaepora Mar 4, 2020

Author

I'll work on it and follow up with an update, thank you.

@GiteaBot GiteaBot added the lgtm/need 2 label Mar 4, 2020
@lafriks lafriks added this to the 1.12.0 milestone Mar 4, 2020
@silverwind

This comment has been minimized.

Copy link
Member

silverwind commented Mar 4, 2020

Can we just keep the TLS defaults to golang's defaults (but overrideable)? I don't think we want to manually configure that stuff ideally and golang is certainly faster doing those changes than we are. So with all default, one should get TLS 1.2,1.3 in golang 1.14.

Copy link
Member

techknowlogick left a comment

Thanks for PR :)

Could you split this up into two PRs, one for hash algo change, and another for TLS changes? This allows for reviewing and merging of each specific component and faster merging of a PR without blocking on non-related reviews.

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Mar 4, 2020

I've fixed the user.yml test case

@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Mar 4, 2020

So actually #10467 might fix tls version issue

@kaepora

This comment has been minimized.

Copy link
Author

kaepora commented Mar 5, 2020

Guys, it's worth noting that Chrome, Firefox and Safari will all completely drop support for TLS 1.0 and 1.1 this month. In light of this news, is it still worth supporting these protocols in Gitea? I strongly but humbly recommend against it.

https://www.zdnet.com/article/browsers-to-block-access-to-https-sites-using-tls-1-0-and-1-1-starting-this-month/

@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Mar 5, 2020

@kaepora in PR #10467 upgrade to golang 1.14 will fix that as it does that by default already so it is not needed to set it in code

@kaepora

This comment has been minimized.

Copy link
Author

kaepora commented Mar 5, 2020

@kaepora in PR #10467 upgrade to golang 1.14 will fix that as it does that by default already so it is not needed to set it in code

@lafriks I could not find a mention of this feature in the Go 1.14 release notes. What gives you the impression that Go 1.14 will default to a minimum of TLS 1.2?

@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Mar 5, 2020

It actually might not, you are right, still it should be moved out to separate PR and set in places where tls.Config is initialized not in code later on

@silverwind

This comment has been minimized.

Copy link
Member

silverwind commented Mar 5, 2020

is it still worth supporting these protocols in Gitea

According to https://caniuse.com/#feat=tls1-1 there are still 2.44% users worldwide whose browser does not support TLS 1.1 or higher, so it should at least be an option, but probably not be the default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can鈥檛 perform that action at this time.