Added support of autocert (lestencrypt)#2
Conversation
Run on boxes with older libc. Prevent this error: ./lnaddrd: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.34' not found (required by ./lnaddrd) ./lnaddrd: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.32' not found (required by ./lnaddrd)
|
Hi, and thanks very much for the contribution! I like the idea of adding autocert support 👍 but i'm not sure adding an encryption layer is necessary here. If a user wishes to keep their sensitive files secure against exposure to physical theft, they should be mounting those files on a volume with full-disk encryption (e.g. LUKS). If they are concerned about other programs on the same machine having access, then those files should be With that said, if you're open to splitting the encryption changes out (possibly into a separate PR if you'd like to continue discussion), we can move forward with autocert support in this PR |
starius
force-pushed
the
encryption-and-autocert
branch
from
b5310de to
58fc9b0
Compare
|
Hi! I removed the commit adding encryption and tested it. |
starius
changed the title
| lis, err := encrypt.NewListener(tlsConfig, tcpLis) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create listener: %w", err) | ||
| } |
There was a problem hiding this comment.
I think we can remove this encrypt-autocert-cache dependency, as it seems the only thing it's doing now is returning a listener which wraps tls.Server.
Could you elaborate on why tcpConn.SetKeepAlive is needed? Doesn't the vanilla tls.Listen work OK?
| certHandler := manager.HTTPHandler(http.HandlerFunc(redirectToHTTPS)) | ||
| tlsConfig := manager.TLSConfig() | ||
| tlsConfig.MinVersion = tls.VersionTLS12 | ||
| tcpLis, err := net.Listen("tcp", ":443") |
There was a problem hiding this comment.
I think we should still respect the user's cfg.BindAddress.
The user may want to expose the server only on a specific interface (e.g. LAN only), and here you're binding to 0.0.0.0 (all interfaces).
Furthermore, the user may not want to bind lndaddrd to ports 80/443. For example, if the user is exposing the server behind a proxy like nginx, they will want to leave those ports open for the upstream proxy to listen on. The proxy then passes the raw TCP-level traffic downstream to lndaddrd. This kind of use case would be impossible unless we allow the user to specify their own IP and port to listen on.
| HostPolicy: autocert.HostWhitelist(cfg.AutocertDomains...), | ||
| } | ||
|
|
||
| certHandler := manager.HTTPHandler(http.HandlerFunc(redirectToHTTPS)) |
There was a problem hiding this comment.
To avoid the need to expose a plaintext HTTP listener, could we get away with only supporting tls-alpn-01 challenge type? I believe autocert handles those challenges automatically, since we're using autocert.Manager.TLSConfig in the TLS listener.
There was a problem hiding this comment.
Interesting! I didn't know about tls-alpn-01. Thanks! I'll update the PR.
One question regarding port 80. It also serves as redirect from http to https. Is it needed for LN address functionality? Is it worth having it under an option? So nginx is not needed to do this redirect.
There was a problem hiding this comment.
Sorry, it's been a busy week 😅
The LNURL spec says URLs can either be https://, or they can be http:// if the domain name ends with .onion (source), so i think we can get away without HTTP here. Usually HTTP -> HTTPS redirection is set up for compatibility with old web browsers, which still default to plaintext HTTP connections when you type a bare domain into the browser URL bar. Wallets on the other hand should never be making plaintext requests when asking for lightning invoices, because a middle-man could serve fake responses.
If a user needs an HTTP->HTTPS redirection server for some reason, it's very easy to write a second program or script which handles that redirect independently. Most popular webservers like nginx allow you to configure HTTP -> HTTPS redirectors very easily too.
In the interest of avoiding clutter and complexity in the config, we should keep things simple and just expose a single port, which could either be listening with TLS (via autocert), or listening on a plaintext TCP socket (while some upstream program terminates TLS for us).
|
|
||
| certHandler := manager.HTTPHandler(http.HandlerFunc(redirectToHTTPS)) | ||
| tlsConfig := manager.TLSConfig() | ||
| tlsConfig.MinVersion = tls.VersionTLS12 |
There was a problem hiding this comment.
I believe the MinVersion field already defaults to TLSv1.2, at least as of golang v1.22
package tls // import "crypto/tls"
type Config struct {
// MinVersion contains the minimum TLS version that is acceptable.
//
// By default, TLS 1.2 is currently used as the minimum. TLS 1.0 is the minimum
// supported by this package.
//
// The server-side default can be reverted to TLS 1.0 by including the value
// "tls10server=1" in the GODEBUG environment variable.
MinVersion uint16
// ... other fields elided ...
}2 participants
This PR includes some minor fixes and updates as well as one important new feature:
golang.org/x/crypto.I updated instructions in README and tested that new feature works.
Important new dependencies: