Fix TLS #253
Fix TLS #253
Conversation
There was a problem hiding this comment.
@aakselrod excellent work on this PR, I have left some minor suggestions and nit picks :) It would be great to have a couple more comments in genCertPair, just to help document the intermediary steps of constructing the certificate, but otherwise LGTM ⚡️
We may later want to consider zeroing out private keys and sensitive data before genCertPair returns, since multiple references would otherwise remain in memory unnecessarily until being garbage collected (or maybe not at all!). I think this is beyond the scope of this PR, but wouldn't mind submitting that as a follow up :)
lnd.go
Outdated
| @@ -29,7 +36,8 @@ import ( | |||
| ) | |||
|
|
|||
| const ( | |||
| autogenCertValidity = 10 * 365 * 24 * time.Hour | |||
| // 1 year | |||
| autogenCertValidity = 1 /*year*/ * 365 /*days*/ * 24 * time.Hour | |||
There was a problem hiding this comment.
I've thought about this a little more—should we consider setting this to something slightly over one year to allow a buffer for those attempting to update their cert on a yearly basis? Maybe around 365 + 2*30 = 425 days? Should allow people to pick a calendar date and have some slack :)
There was a problem hiding this comment.
I've modified this to be 14 * 30 = 14 (average) months = 420 days. 5 days shorter but more readable.
| if err != nil { | ||
| return err | ||
| } | ||
| tlsConf := &tls.Config{ |
There was a problem hiding this comment.
With the weirdness observed when interfacing with gRPC clients in other languages, it would be nice to have a comment here explaining why these cipher suites and their ordering were chosen :)
There was a problem hiding this comment.
I've added a comment about the selection criteria for the cipher suites and reordered them from strongest to weakest primitives, but client preference order has a stronger effect on negotiation (and I'm not sure ordering matters at all on the server).
lnd.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // end of ASN.1 time | ||
| endOfTime := time.Date(2049, 12, 31, 23, 59, 59, 0, time.UTC) |
There was a problem hiding this comment.
Could we break endOfTime out into a constant? Similarly for serialNumberLimit?
There was a problem hiding this comment.
These are initialized with function calls, so they have to be vars, not consts but I broke them out into the var section at the top of the file.
lnd.go
Outdated
| return err | ||
| } | ||
|
|
||
| ipAddresses := []net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("::1")} |
There was a problem hiding this comment.
Little bit of a nit pick: this line should be moved to just before the addIP closure, since ipAddresses isn't referenced until then.
There was a problem hiding this comment.
I've rearranged this whole section so it's more readable and added comments.
lnd.go
Outdated
| now := time.Now() | ||
| validUntil := now.Add(autogenCertValidity) | ||
|
|
||
| priv, err := rsa.GenerateKey(rand.Reader, 4096) |
There was a problem hiding this comment.
This appears to be the most expensive statement in the method, and the result isn't used until CreateCertificate is called. I would suggest delaying the key generation until all of the less expensive validation have succeeded, i.e. dates, address parsing, serial number generation. This will allow us to fail faster if the other validations are unsuccessful.
There was a problem hiding this comment.
I've moved this to just before template creation.
There was a problem hiding this comment.
Excellent! Now users won't have to generate their own certificates or do other awkward hacks around golang's default set of strict ciphers.
The changes read well to me, and also I've tested it locally with a Python script (using gRPC) and found that this solves the issues we were having.
Once Conner's comments are addressed, I'll get this merged 👍.
LGTM 🎉
This PR does the following:
btcutiland modifies it to generate RSA certificates compatible out-of-the-box with most clients (in particular, both Python and Node gRPC clients).