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

golink: listen on HTTPS and redirect HTTP traffic #99

Merged
merged 10 commits into from
Dec 18, 2023
Merged

Conversation

patrickod
Copy link
Contributor

@patrickod patrickod commented Dec 8, 2023

Updates #9
Fixes #29

On tailnets with TLS enabled serve HTTP traffic with a separate redirectHandler which sends requests to our HTTPS listener destination.

Add -L to documented examples of using curl to follow these redirects if present.

Updates #9

On tailnets with TLS enabled serve HTTP traffic with a separate
redirectHandler which sends requests to our HTTPS listener destination.

Add `-L` to documented examples of using `curl` to follow these
redirects if present.

Signed-off-by: Patrick O'Doherty <[email protected]>
@noncombatant
Copy link
Contributor

I think this is Updates #9 and Fixes #29.


l80, err := srv.Listen("tcp", ":80")
// create tsNet server and wait for it to be ready & connected.
localClient, _ = srv.LocalClient()
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're here, maybe now is a good time to

localClient, err = srv.LocalClient()
if err != nil {
  return err
}

Copy link
Member

Choose a reason for hiding this comment

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

As long as the server is started, LocalClient promises not to report an error in this context.

golink.go Outdated Show resolved Hide resolved
golink.go Outdated Show resolved Hide resolved
golink.go Outdated Show resolved Hide resolved
@willnorris
Copy link
Member

tsnet has the ListenTLS func which does a tiny bit of this for you already. Would it be worth fleshing that out or adding additional helpers that golink could call into? Particularly if we expect this to be a common pattern.

@willnorris
Copy link
Member

Though I guess it's also worth keeping in mind that @maisem is working on removing the hard dependence on tsnet (#95), so we can't necessarily assume it will always be tsnet. Maybe the helpers belong in tsweb then? Or maybe this really does just need to be handled in the individual applications, though that would be kind of unfortunate.

golink.go Outdated Show resolved Hide resolved
golink.go Outdated Show resolved Hide resolved
golink.go Outdated Show resolved Hide resolved
golink.go Outdated Show resolved Hide resolved
golink.go Outdated
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
path := r.URL.Path
newUrl := fmt.Sprintf("https://%s%s", hostname, path)
http.Redirect(w, r, newUrl, http.StatusMovedPermanently)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely sure this works, I vaguely recall something about go/foo not working once you start doing redirects.

@maisem
Copy link
Contributor

maisem commented Dec 10, 2023

tsnet has the ListenTLS func which does a tiny bit of this for you already

Please use ListenTLS. We currently only send down one CertDomain, that may change in the future but programs today should only account for the exact one. There are no promises made on what that would mean in the future or how that would change. ListenTLS aims to abstract that away from the caller.

* use `srv.ListenTLS` API instead of DIY'ing it.
* DRY up http & https listener code.
* use type safe URL generation for redirect handler
* use status API to determine HTTPS capabilities directly.
* handle http only case gracefully.

Signed-off-by: Patrick O'Doherty <[email protected]>
@patrickod
Copy link
Contributor Author

Please use ListenTLS. We currently only send down one CertDomain, that may change in the future but programs today should only account for the exact one. There are no promises made on what that would mean in the future or how that would change. ListenTLS aims to abstract that away from the caller.

@maisem thank you - I am not surprised to discover that I was "holding it wrong" so to speak. The latest impl with ListenTLS is much more concise.

golink.go Show resolved Hide resolved
golink.go Outdated Show resolved Hide resolved
golink.go Outdated Show resolved Hide resolved
golink.go Outdated Show resolved Hide resolved
golink.go Outdated Show resolved Hide resolved
golink.go Outdated Show resolved Hide resolved
* create discrete ctx variables for localClient & tsnet server
  interactions.
* DRY up the http(s) handler code even further.
* call log.Fatal for https serving errors that were previously
  swallowed.

Signed-off-by: Patrick O'Doherty <[email protected]>
golink.go Outdated Show resolved Hide resolved
If users belong to multiple tailnets with golinks deployed (as is common) then
permanent redirects for one will conflict with the others. To prevent this we
will use `http.StatusFound` to prompt browsers to continue to visit the `go/`
URL in the future.

Signed-off-by: Patrick O'Doherty <[email protected]>
@patrickod patrickod marked this pull request as ready for review December 15, 2023 21:57
Inspect the `Host` header to ensure that we do not return HSTS headers
for short domains which can lead to some clients pinning short domains to
endpoints with invalid certificates.

Signed-off-by: Patrick O'Doherty <[email protected]>
golink.go Outdated Show resolved Hide resolved
Append section about HTTPS behavior to the README. Include a note about the use
of `-L` with all `curl` scripts in such deployments to prevent silent early
termination with empty responses.

Signed-off-by: Patrick O'Doherty <[email protected]>
README.md Outdated Show resolved Hide resolved
@patrickod patrickod merged commit 3f822b6 into main Dec 18, 2023
4 checks passed
@patrickod patrickod deleted the patrickod/https branch December 18, 2023 22:14
@patrickod patrickod mentioned this pull request Dec 18, 2023
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.

Add SSL support
5 participants