-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Auto regenerate TLS cert #3011
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
Auto regenerate TLS cert #3011
Conversation
|
I'm suffering from this issue. the auto generated cert has expired and connections now fail. cannot even unlock the wallet. Organization: lnd autogenerated cert [lncli] rpc error: code = Unavailable desc = all SubConns are in TransientFailure, latest connection error: connection error: desc = "transport: authentication handshake failed: x509: certificate has expired or is not yet valid" |
cfromknecht
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@orbitalturtle thanks for fixing this! completed an initial pass, changes look pretty good to me. mostly minor nits
|
@redstorm1 in the meantime you can try deleting the |
|
@orbitalturtle any progress on this front? should we keep this slated for 0.7? |
|
Hey @cfromknecht - sure that all sounds good, I'll push up my revisions tomorrow. :) |
|
@orbitalturtle excellent, thanks! |
|
@cfromknecht Ok finally revised that! Lmk if I can be of any more assistance. Less busy now, so will be more responsive. |
cfromknecht
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @orbitalturtle! couple small comments, otherwise looking pretty good
|
@orbitalturtle this probably needs a rebase, since the main |
|
@orbitalturtle be sure to run also recommend not using the github "merge w/ master" button, typically better to rebase over master and force push :) |
|
@cfromknecht Ahhhh my bad, fixed that |
halseth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the PR! This is definitely useful, and the change looks good to me.
However, I don't think this warrants another integration test (hey have quite an overhead). Could you instead make the getTLSConfig method more testable and add a unit test? :)
lnd.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: missing period end of sentence.
|
@halseth Sure thing, I can take a stab at that tonight. Just a bit confused about how to approach that if you can elaborate a bit! Like which file do you see the unit test being in? |
|
@orbitalturtle You can put the test in |
|
@halseth should we consider doing that as a follow up given that the PR is complete as is? |
|
@orbitalturtle if you can rebase on latest master and leave the itest, we can get this merged in time for the release candidate. If not we may need to push off to 0.8 |
|
@cfromknecht Sure thing, just rebased as is Almost done with the new test in server_test.go I think, but I suppose that would require more review. So if this is merged as is, I’ll just add another PR updating the tests? |
|
@orbitalturtle awesome thanks! yeah since it doesn't affect the operational behavior, i'm fine with moving it to a unit test in a separate PR |
cfromknecht
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ⛵️
Auto-regenerates TLS files once expired.
Fixes #2758