R4R: Implement HTTPS for the LCD REST server #2364

alessio · 2018-09-20T04:31:59Z

In order to guarantee a secure connection between apps and the LCD the communication must be encrypted - even if clients and server run on the same local machine, credentials must never be transmitted in clear text.

Upon start up, if no certificate/key file pair is supplied by the user, the server generates a self-signed certificate and a key; both are stored as temporary files and removal is guaranteed on exit.

This new behaviour is now enabled by default, though users are provided
with a --insecure flag to switch it off.

See #595

Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
rereviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

codecov · 2018-09-20T11:00:58Z

Codecov Report

Merging #2364 into develop will decrease coverage by 0.38%.
The diff coverage is 39.87%.

@@             Coverage Diff             @@
##           develop    #2364      +/-   ##
===========================================
- Coverage    64.77%   64.39%   -0.39%     
===========================================
  Files          137      138       +1     
  Lines         8469     8617     +148     
===========================================
+ Hits          5486     5549      +63     
- Misses        2620     2693      +73     
- Partials       363      375      +12

zmanian · 2018-09-20T21:01:21Z

One of the very important attacks that this change should mitigate is the ability to a website to use javascript to search for an instance of LCD and interact with it b/c the browser will not trust this certificate and javascript can't override this.

zmanian · 2018-09-20T21:06:07Z

client/lcd/certificates.go

+)
+
+// default: 10 years
+const defaultValidFor = 365 * 24 * time.Hour


Can anyone think a reason why 10 year validity is dangerous for a ephemeral cert?

In most cases where you would have a unusually long lived instance of the LCD best practice would be to supply your own Cert.

@zmanian I've set it to 1 year already. Do you reckon that is reasonable?

Maybe 30 days? @jessysaurusrex What do you think?

Certainly not a year either I think. 30 days sounds reasonable. Some other system event might even kill the LCD before that anyway.

client/lcd/certificates.go

zmanian · 2018-09-20T21:24:32Z

client/lcd/certificates.go

+// default: 10 years
+const defaultValidFor = 365 * 24 * time.Hour
+
+func generateSelfSignedCert(host, ecdsaCurve string, rsaBits int) (certBytes []byte, priv interface{}, err error) {


Maybe it should only be localhost?

Dropped a test case for IPv6 addresses too. IMvHO that should be user's call, though "localhost" sounds like a sane default to me.

client/lcd/root.go

zmanian

Remove the configurability of the self signed cert option and create an option to pass in an externally generated cert.

zmanian

utACK

In order to guarantee a secure connection between apps and the LCD the communication must be encrypted - even if clients and server run on the same local machine, credentials must never be transmitted in clear text. Upon start up, the server generates a self-signed certificate and a key. Both are stored as temporary files; removal is guaranteed on exit. This new behaviour is now enabled by default, though users are provided with a --insecure flag to switch it off. See #595

alexanderbez

Few minor comments, otherwise great work @alessio 👍

alexanderbez · 2018-09-21T13:23:45Z

client/lcd/certificates.go

+)
+
+// default: 10 years
+const defaultValidFor = 365 * 24 * time.Hour


Certainly not a year either I think. 30 days sounds reasonable. Some other system event might even kill the LCD before that anyway.

alexanderbez · 2018-09-21T13:24:16Z

client/lcd/certificates.go

+
+func generateSelfSignedCert(host string) (certBytes []byte, priv *ecdsa.PrivateKey, err error) {
+	priv, err = ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	notBefore := time.Now()


Should we be using UTC for consistency?

alexanderbez · 2018-09-21T13:24:56Z

client/lcd/certificates.go

+	template := x509.Certificate{
+		SerialNumber: serialNumber,
+		Subject: pkix.Name{
+			Organization: []string{"Acme Co"},


Do we want some other org here? I guess it doesn't really matter for self-signed certs.

Naah, it does not really matter. I could let the user customise it though.

Do users see this (maybe if they inspect the certificate) - what about "Gaia Lite" as the organization?

cwgoes

Thanks - mostly looks good, a few minor comments.

client/lcd/certificates.go

cwgoes · 2018-09-21T15:47:42Z

client/lcd/certificates.go

+	template := x509.Certificate{
+		SerialNumber: serialNumber,
+		Subject: pkix.Name{
+			Organization: []string{"Acme Co"},


Do users see this (maybe if they inspect the certificate) - what about "Gaia Lite" as the organization?

client/lcd/certificates.go

client/lcd/root.go

cwgoes · 2018-09-21T16:15:28Z

~~Ah @alessio looks like this breaks the integration tests, can we change those to test the HTTPS server?~~

Never mind, just an intermittent failure.

alessio added the wip label Sep 20, 2018

alessio requested review from cwgoes, ebuchman and rigelrozanski as code owners September 20, 2018 04:31

alessio force-pushed the alessio/595-lcd-over-https branch 2 times, most recently from e474e98 to 44b929a Compare September 20, 2018 13:47

alessio requested a review from zramsay as a code owner September 20, 2018 13:57

alessio changed the title ~~WIP: Implement HTTPS for the LCD REST server~~ R4R: Implement HTTPS for the LCD REST server Sep 20, 2018

alessio added ready-for-review and removed wip labels Sep 20, 2018

alessio requested review from fedekunze, faboweb, jaekwon, alexanderbez, zmanian and ValarDragon September 20, 2018 13:59

alessio added the prelaunch label Sep 20, 2018

zmanian reviewed Sep 20, 2018

View reviewed changes

client/lcd/certificates.go Outdated Show resolved Hide resolved

zmanian reviewed Sep 20, 2018

View reviewed changes

client/lcd/root.go Outdated Show resolved Hide resolved

zmanian requested changes Sep 20, 2018

View reviewed changes

alessio force-pushed the alessio/595-lcd-over-https branch from 5cbf95b to 4f00d6b Compare September 21, 2018 00:37

zmanian approved these changes Sep 21, 2018

View reviewed changes

alessio force-pushed the alessio/595-lcd-over-https branch from 01f350c to 0e1d5d9 Compare September 21, 2018 10:26

alessio force-pushed the alessio/595-lcd-over-https branch from 0e1d5d9 to f5ea1e6 Compare September 21, 2018 10:27

alexanderbez approved these changes Sep 21, 2018

View reviewed changes

Alessio Treglia added 3 commits September 21, 2018 14:34

Flags validation

44faf88

One year is perhaps too long as well

c9e7b0b

Left comment

3b0b660

cwgoes suggested changes Sep 21, 2018

View reviewed changes

address @cwgoes comments

7a8ceb8

cwgoes approved these changes Sep 21, 2018

View reviewed changes

cwgoes merged commit e2da4ca into develop Sep 21, 2018

cwgoes deleted the alessio/595-lcd-over-https branch September 21, 2018 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Implement HTTPS for the LCD REST server #2364

R4R: Implement HTTPS for the LCD REST server #2364

alessio commented Sep 20, 2018 •

edited

Loading

codecov bot commented Sep 20, 2018 •

edited

Loading

zmanian commented Sep 20, 2018

zmanian Sep 20, 2018

alessio Sep 20, 2018

zmanian Sep 20, 2018

alexanderbez Sep 21, 2018

zmanian Sep 20, 2018

alessio Sep 20, 2018

zmanian left a comment

zmanian left a comment

alexanderbez left a comment

alexanderbez Sep 21, 2018

alexanderbez Sep 21, 2018

alexanderbez Sep 21, 2018

alessio Sep 21, 2018

cwgoes Sep 21, 2018

cwgoes left a comment

cwgoes Sep 21, 2018

cwgoes commented Sep 21, 2018 •

edited

Loading

R4R: Implement HTTPS for the LCD REST server #2364

R4R: Implement HTTPS for the LCD REST server #2364

Conversation

alessio commented Sep 20, 2018 • edited Loading

codecov bot commented Sep 20, 2018 • edited Loading

Codecov Report

zmanian commented Sep 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmanian left a comment

Choose a reason for hiding this comment

zmanian left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes commented Sep 21, 2018 • edited Loading

alessio commented Sep 20, 2018 •

edited

Loading

codecov bot commented Sep 20, 2018 •

edited

Loading

cwgoes commented Sep 21, 2018 •

edited

Loading