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

Use same lnd's cert package to create TLS config to fix TLS cipher suites #35

Merged
merged 6 commits into from
May 6, 2020

Conversation

guggero
Copy link
Member

@guggero guggero commented Apr 28, 2020

We need to use safe TLS1.2 cipher suites that are also accepted by the golang http2 library. Turns out, that is a subset of what we currently use in lnd.

aperture.go Outdated Show resolved Hide resolved
aperture.go Outdated Show resolved Hide resolved
aperture.go Outdated Show resolved Hide resolved
aperture.go Outdated Show resolved Hide resolved
@guggero guggero force-pushed the tls-cipher-suites branch 6 times, most recently from 67237f4 to 136ebc8 Compare April 29, 2020 08:07
Copy link
Contributor

@lispmeister lispmeister left a comment

Choose a reason for hiding this comment

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

We tested the new setup with insecure: true in the aperture.yaml when booting the full nautilus stack behind a AWS NLB and it works.
The new setup looks like this now:
loop-client ---http2-over-tls--->NLB----http2---->aperture----http2-over-tls--->nautilus

The external TLS connection is terminated on the NLB which also presents the official TLS certificate registered to nautilus.lightningcluster.com.

@guggero
Copy link
Member Author

guggero commented May 1, 2020

We ended up disabling TLS to get it to work. There seems to be a general problem with TLS and load balancers.
I decided to leave all the TLS changes in this PR anyway as this still might come in handy later on when we want to run aperture as a Kubernetes Ingress Controller.

@guggero guggero requested a review from joostjager May 1, 2020 12:10
aperture.go Outdated Show resolved Hide resolved
aperture.go Outdated
@@ -138,6 +152,17 @@ func start() error {
}
log.Infof("Done generating TLS certificates")
}

// Load the certs now so we can create a complete TLS config.
certData, _, err := cert.LoadCert(tlsCertFile, tlsKeyFile)

Choose a reason for hiding this comment

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

Cert loading needs to move to the next commit?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really, we can't create a valid TLSConfig without the certificate.

aperture.go Outdated Show resolved Hide resolved
aperture.go Show resolved Hide resolved
proxy/proxy.go Outdated
@@ -70,8 +71,15 @@ func (p *Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {
}
defer logRequest()

// PRI requests are an internal check of HTTP/2 which we just have to
// say OK to.
if r.Method == "PRI" {

Choose a reason for hiding this comment

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

Some more info on this would be nice. What problem do we have if we don't answer?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the whole commit. We did try out some stuff with this but I think in the end (with TLS turned off, this is not needed anymore). Same with the following 2 commits.

proxy/proxy.go Outdated Show resolved Hide resolved
proxy/proxy.go Outdated Show resolved Hide resolved
aperture.go Show resolved Hide resolved
@guggero
Copy link
Member Author

guggero commented May 5, 2020

We tested this again on the AWS cluster and confirmed it still works.

@guggero guggero requested a review from joostjager May 5, 2020 18:13
aperture.go Show resolved Hide resolved
@guggero guggero merged commit 57a5605 into lightninglabs:master May 6, 2020
@guggero guggero deleted the tls-cipher-suites branch May 6, 2020 08:45
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.

None yet

3 participants