Skip to content

fix(nodebuilder): TLS key/cert name variables #4203

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

koenmtb1
Copy link
Contributor

@koenmtb1 koenmtb1 commented Apr 8, 2025

Depending on how one generates the TLS certificates it might use different names for the key files.
These are currently hard-coded to cert.pem and key.pem, but for example letsencrypt by default uses keychain.pem which does load correctly into Go TLS.

To prevent having to manually create a symlink using an env variable will resolve that.

One outstanding question that I still have is that if TLS certs are renewed, does the node pick it up automatically? As far as I can tell it currently doesn't, happy to work on that too but a pointer of would be helpful.

@github-actions github-actions bot added the external Issues created by non node team members label Apr 8, 2025
@cristaloleg cristaloleg added the kind:feat Attached to feature PRs label Apr 8, 2025
@cristaloleg cristaloleg changed the title refactor(nodebuilder): TLS key/cert name variables fix(nodebuilder): TLS key/cert name variables Apr 8, 2025
@cristaloleg cristaloleg added kind:fix Attached to bug-fixing PRs and removed kind:feat Attached to feature PRs labels Apr 8, 2025
@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2025

Codecov Report

Attention: Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.

Project coverage is 44.75%. Comparing base (2469e7a) to head (b32f09a).
Report is 480 commits behind head on main.

Files with missing lines Patch % Lines
nodebuilder/p2p/tls.go 0.00% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4203      +/-   ##
==========================================
- Coverage   44.83%   44.75%   -0.09%     
==========================================
  Files         265      312      +47     
  Lines       14620    22959    +8339     
==========================================
+ Hits         6555    10275    +3720     
- Misses       7313    11563    +4250     
- Partials      752     1121     +369     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cristaloleg
Copy link
Contributor

Please run make fmt

smuu
smuu previously approved these changes Apr 10, 2025
cristaloleg
cristaloleg previously approved these changes Apr 10, 2025
Copy link
Contributor

@cristaloleg cristaloleg left a comment

Choose a reason for hiding this comment

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

SGTM

Copy link
Member

@vgonkivs vgonkivs left a comment

Choose a reason for hiding this comment

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

Generally LGMT. But it makes me think that we don't need TLS_PATH anymore and we can have TLS_KEY_PATH and TLS_CERT_PATH instead. It also covers a case when key and certs can be set in different folders.

UPD: we can do much more with the tls stuff in future, so maybe we can have a separate structure in the node's config for it?
Thoughts @renaynay @Wondertan @walldiss @cristaloleg

@koenmtb1
Copy link
Contributor Author

The thought behind keeping TLS_KEY_PATH is to not make this a breaking change and retain backwards compatibility, happy to update if deemed helpful.
Happy to update node config for this too, if you think thats a better location.

I've also noticed there's no docs for this (or any requirement for having TLS at all). Is that something worth exploring?

@koenmtb1 koenmtb1 dismissed stale reviews from cristaloleg and smuu via b32f09a April 10, 2025 10:08
@koenmtb1 koenmtb1 requested a review from vgonkivs April 10, 2025 10:09
@vgonkivs
Copy link
Member

I've also noticed there's no docs for this (or any requirement for having TLS at all). Is that something worth exploring?

do you mean these docs https://docs.celestia.org/how-to-guides/light-node#optional-start-light-node-with-core-endpoint-with-authentication ?

@Wondertan
Copy link
Member

do you mean these docs https://docs.celestia.org/how-to-guides/light-node#optional-start-light-node-with-core-endpoint-with-authentication ?

this TLS config is for libp2p's websocket, not for core

@Wondertan
Copy link
Member

Hey @koenmtb1, aside from this PR, you may wanna take a look at https://blog.libp2p.io/autotls/. We haven't integrated it yet, but it's targeting this particular struggle of managing TLS certs. This is gonna be super helpful for bootstrapper infra by automating cert issuing. We would appreciate if you can take a look into it and help us and other bootstrappers with integration of this tool.

@koenmtb1
Copy link
Contributor Author

Hey @koenmtb1, aside from this PR, you may wanna take a look at https://blog.libp2p.io/autotls/. We haven't integrated it yet, but it's targeting this particular struggle of managing TLS certs. This is gonna be super helpful for bootstrapper infra by automating cert issuing. We would appreciate if you can take a look into it and help us and other bootstrappers with integration of this tool.

This is interesting, would be open to adding support for this. Probably a separate PR and keep support for self serving certificates?
Any thoughts on using their default libp2p.direct or setting up a custom service?

@Wondertan
Copy link
Member

Probably a separate PR and keep support for self serving certificates?

Yes

Any thoughts on using their default libp2p.direct or setting up a custom service?

We can start with the default one for simplicity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external Issues created by non node team members kind:fix Attached to bug-fixing PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants