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

5 participants
@joohoi
Copy link
Contributor

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 the kind/bug label Jan 23, 2019

@lafriks lafriks added this to the 1.8.0 milestone Jan 23, 2019

@bkcsoft bkcsoft added the lgtm/need 1 label Jan 23, 2019

@codecov-io

This comment has been minimized.

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 and removed lgtm/need 1 labels Jan 24, 2019

@techknowlogick techknowlogick merged commit 3b36402 into go-gitea:master Jan 24, 2019

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details
@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Jan 24, 2019

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

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