-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
tlsmanager: fix autocert autogeneration #7739
Conversation
Edit: tACK Setup:
Question: Is the following letsencrypt flow as it is supposed to be? On a fresh
This call fails with the following message...
... while the letsencrypt generation succeeds.
|
@hieblmi I think generally using let's encrypt with lncli is wonky. The way we can accept a cert using a grpc request is by leaving the tls config empty (e.g. AFAIK lncli doesn't support that. Also Line 84 in cf1f44a
|
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.
I see, I wasn't aware of the difficulty testing this with lncli
. Thanks for the explanation.
Sorry for intervening! Can you please fix this issue a little bit faster? I had to update my LND to resolve another problem, and then I lost my Let's Encrypt. Thank you. |
@sputn1ck, remember to re-request review from reviewers when ready |
Hi guys. Just trying to remind you that there's a broken piece of your "beta" software that people relied on, and it's been a long time for it to get fixed. |
@sputn1ck needs a rebase. |
@khashmeshab can you confirm this patch is working for you? The whole Let's Encrypt setup is quite difficult to test in non-production environments. |
As the getConfig() function would previously overwrite the GetCertificateFunction of the tls config, the autocert manager would never be used.
@guggero I'm on it. |
@guggero patched, compiled, and tested both with my old letsencrypt dir and then after I deleted it, and it seems to work flawlessly. You may check: https://maxod.ir:8081 |
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 🎉
Thank you for testing! |
As the getConfig() function would previously overwrite the GetCertificate function of the tls config, the autocert manager would never be used.
This PR moves the
setUpLetsEncrypt
func to where it would correctly override theGetCertificate
function.Closes #7734