Skip to content
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

feat(core): added support of the TLS certificates along with x-token authorisation token for the gRPC connection #3954

Merged
merged 15 commits into from
Dec 11, 2024

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented Nov 20, 2024

The PR introduces changes to the Core config. It extends the config with 2 additional fields that allow for configuring a secure grpc connection:

  • TLSEnabled - specifies whether the connection is secure or not;
  • XTokenPath - the path to the directory with JSON file containing the X-Token for gRPC authentication.

Three additional flags have been added to configure these fields:

  • core.tls - allows to configure a secure connection. By default: false
  • core.xtoken.path - allows to set an authorisation token. Should be passed along with the core.tls flag

@vgonkivs vgonkivs requested a review from jcstein November 20, 2024 14:22
@vgonkivs vgonkivs self-assigned this Nov 20, 2024
@vgonkivs vgonkivs added kind:break! Attached to breaking PRs area:config CLI and config area:core_and_app Relationship with Core node and Celestia-App labels Nov 20, 2024
@codecov-commenter
Copy link

codecov-commenter commented Nov 20, 2024

Codecov Report

Attention: Patch coverage is 31.48148% with 74 lines in your changes missing coverage. Please review.

Please upload report for BASE (feature_branch_grpc@941dead). Learn more about missing BASE report.

Files with missing lines Patch % Lines
state/core_access.go 34.61% 30 Missing and 4 partials ⚠️
nodebuilder/core/tls.go 0.00% 24 Missing ⚠️
nodebuilder/core/flags.go 60.86% 7 Missing and 2 partials ⚠️
nodebuilder/state/core.go 22.22% 6 Missing and 1 partial ⚠️
Additional details and impacted files
@@                  Coverage Diff                   @@
##             feature_branch_grpc    #3954   +/-   ##
======================================================
  Coverage                       ?   45.20%           
======================================================
  Files                          ?      309           
  Lines                          ?    22179           
  Branches                       ?        0           
======================================================
  Hits                           ?    10025           
  Misses                         ?    11054           
  Partials                       ?     1100           

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

nodebuilder/core/tls.go Outdated Show resolved Hide resolved
nodebuilder/state/core.go Show resolved Hide resolved
state/core_access.go Outdated Show resolved Hide resolved
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

@vgonkivs this isn't a breaking change bc those fields are purely additive - nothing about validation logic for config has changed so no worries to base it on api-breaks. You can re-base it on main actually.

nodebuilder/core/config.go Show resolved Hide resolved
nodebuilder/core/flags.go Outdated Show resolved Hide resolved
@vgonkivs
Copy link
Member Author

vgonkivs commented Nov 21, 2024

@vgonkivs this isn't a breaking change bc those fields are purely additive - nothing about validation logic for config has changed so no worries to base it on api-breaks. You can re-base it on main actually.

I'd propose merging it inside api breaks as we have one more PR related to the grpc connection. So, imo it's better to keep everything in one place. + I'm going to open one more PR based on the @rach-id changes to unify grpc clients. Merging current PR to main will block clients unification

@vgonkivs vgonkivs changed the title feat!(core): added support of the TLS certificates along with x-token authorisation token for the gRPC connection feat(core): added support of the TLS certificates along with x-token authorisation token for the gRPC connection Nov 21, 2024
@walldiss
Copy link
Member

I was reviewing the PR and noticed that we're configuring client certificates, which seems to be for setting up mutual TLS (mTLS). My understanding is that we only need to verify the server's certificate since we'll be sending an x-token authorization header for client authentication. Do we really need client certificates on the client side in this case?

Also, I didn't see any setup for root CA certificates, which are necessary when dealing with self-signed or custom certificates—a pretty common use case. Without specifying the root CA, the client won't be able to validate the server's certificate.

Could we adjust the implementation to focus on server-side certificates and include support for custom root CAs? What are your thoughts?

@vgonkivs
Copy link
Member Author

We agreed with @walldiss in DM that we don't want to keep any certificates for now(as they were out of the scope of the initial request)

@vgonkivs vgonkivs changed the base branch from feature/api-breaks to main November 25, 2024 14:56
@vgonkivs vgonkivs changed the base branch from main to feature_branch_grpc December 6, 2024 11:30
@vgonkivs vgonkivs added kind:feat Attached to feature PRs and removed kind:break! Attached to breaking PRs labels Dec 6, 2024
nodebuilder/core/tls.go Outdated Show resolved Hide resolved
nodebuilder/core/tls.go Show resolved Hide resolved
nodebuilder/core/tls.go Outdated Show resolved Hide resolved
state/core_access.go Outdated Show resolved Hide resolved
state/core_access.go Outdated Show resolved Hide resolved
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

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

happy with it, just some nits.

nodebuilder/core/flags.go Outdated Show resolved Hide resolved
nodebuilder/core/tls.go Outdated Show resolved Hide resolved
nodebuilder/core/tls.go Outdated Show resolved Hide resolved
nodebuilder/state/core.go Show resolved Hide resolved
nodebuilder/state/core.go Show resolved Hide resolved
@walldiss
Copy link
Member

Looks good and simple. Additionaly it might be worth to add grpc layer unit tests for:

  • tls enforced mod
  • x-token can be retrieved by server

@vgonkivs vgonkivs merged commit b5fc555 into celestiaorg:feature_branch_grpc Dec 11, 2024
28 checks passed
vgonkivs added a commit that referenced this pull request Jan 15, 2025
vgonkivs added a commit that referenced this pull request Jan 20, 2025
cmwaters pushed a commit that referenced this pull request Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:config CLI and config area:core_and_app Relationship with Core node and Celestia-App kind:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants