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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TLS errors when using acme/autocert for local connections #5820

Merged
merged 2 commits into from
Jan 24, 2019

Conversation

joohoi
Copy link
Contributor

@joohoi joohoi commented Jan 23, 2019

The upstream library golang.org/x/crypto/acme/autocert, a package that handles the Let's Encrypt certificate automation terminates incoming HTTP requests if the SNI (ServerName) field isn't in it's whitelist. For internal connection this domain is localhost which cannot be whitelisted for autocert.Manager to automatically try to handle certificates for.

This PR simply adds a ServerName setting to TlsClientConfig for connections made towards localhost, thus mitigating this issue, as the certificate requested from autocert.Manager indeed matches the domain in its whitelist.

This isn't an issue for non - Let's Encrypt setups either, and quite the opposite - in many cases should also be able to eliminate the need for InsecureSkipVerify.

Fixes: #5800

@lafriks lafriks added this to the 1.8.0 milestone Jan 23, 2019
@bkcsoft bkcsoft added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jan 23, 2019
@codecov-io
Copy link

codecov-io commented Jan 24, 2019

Codecov Report

Merging #5820 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5820      +/-   ##
==========================================
- Coverage   37.88%   37.87%   -0.01%     
==========================================
  Files         328      328              
  Lines       48255    48255              
==========================================
- Hits        18280    18278       -2     
- Misses      27345    27348       +3     
+ Partials     2630     2629       -1
Impacted Files Coverage Δ
models/unit.go 0% <0%> (-14.29%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 331c912...015e4ae. Read the comment docs.

@bkcsoft bkcsoft added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 24, 2019
@techknowlogick techknowlogick merged commit 3b36402 into go-gitea:master Jan 24, 2019
@lafriks
Copy link
Member

lafriks commented Jan 24, 2019

@joohoi please send backport to release/v1.7 branch

@lafriks lafriks added the backport/done All backports for this PR have been created label Jan 24, 2019
bmackinney pushed a commit to bmackinney/gitea that referenced this pull request Jan 27, 2019
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GetRepositoryByOwnerAndName fails when running natively on HTTPS
5 participants