-
Notifications
You must be signed in to change notification settings - Fork 925
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
initial POC for grpc tls #3917
base: main
Are you sure you want to change the base?
initial POC for grpc tls #3917
Conversation
I was able to sync a light node with:
using this build. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3917 +/- ##
==========================================
+ Coverage 44.83% 45.42% +0.58%
==========================================
Files 265 307 +42
Lines 14620 21849 +7229
==========================================
+ Hits 6555 9924 +3369
- Misses 7313 10855 +3542
- Partials 752 1070 +318 ☔ View full report in Codecov by Sentry. |
@@ -124,9 +119,15 @@ func (ca *CoreAccessor) Start(ctx context.Context) error { | |||
|
|||
// dial given celestia-core endpoint | |||
endpoint := fmt.Sprintf("%s:%s", ca.coreIP, ca.grpcPort) | |||
grpcOpts := make([]grpc.DialOption, 0) | |||
if ca.enableTLS { | |||
grpcOpts = append(grpcOpts, grpc.WithTransportCredentials(credentials.NewTLS(&tls.Config{}))) |
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.
Without specifying valid certificates in the tls.Config object will not secure gRPC connection. By default, an empty tls.Config{} means that no certificate or private key is provided, which leads to an insecure setup.
Node already has TLS certificates handling logic here for web socket, so you can reuse the certificate for the grpc connection.
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.
This was a proof of concept to get the Light node working against a Quicknode celestia endpoint. QN doesn't support client certs. gRPC auth for a QN endpoint occurs over the x-token
header
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.
One more point here is the solution is not generic. You may not support the certificates but if someone does, the connection won't be established. Also, it is recommended to manually specify the MinTLSVersion(linter confirmed it)
IP string | ||
RPCPort string | ||
GRPCPort string | ||
EnableTLS bool |
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.
let's avoid growing the node's config. We have an env variable that signals that TLS should be enabled.
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 just wanted to create a POC. My recommended approach would be to have a RPC url and gRPC url.
Initial concept for gRPC tls support