Skip to content

Enable secure communication with TLS#10

Open
bsenel wants to merge 3 commits into
cachecashproject:masterfrom
bsenel:master
Open

Enable secure communication with TLS#10
bsenel wants to merge 3 commits into
cachecashproject:masterfrom
bsenel:master

Conversation

@bsenel
Copy link
Copy Markdown
Contributor

@bsenel bsenel commented Dec 16, 2020

In this pull request, you find three main changes affecting the gRPC helpers, the sections using these helpers, and the Prometheus registering.

How does it work?

First, it requires to generate the CA’s certificate and the CA’s private key. Following that, the server’s certificate and the server’s private key for each daemon, except for caches and logpiped for now, by using the CA’s certificate and the CA’s private key. If the secure mode is enabled using the configuration, the server-side TLS implementation loads the server’s certificate and private key from the /tls directory.

The client-side TLS implementation works in the same way. If CacheCash runs in the secure mode, it loads the CA certificate that signed the server’s certificate. In this way, a client checks if it communicates with the right server by verifying the server’s certificate.

gRPC helpers:

These are the functions that generate gRPC clients and servers for all the relevant daemons. Passing the TLS credentials into the options was missing before. I implement a part that allows passing a secure bool in these helpers. And the helpers load the relevant certificate and private key if the security mode is on. The responsible person for the deployment of CacheCash needs to generate the certificates and put them in the /tls directory.

Note: The file names to store the certificate and private keys at, it should be configurable.

There are comments in the relevant parts to show how to enable handling time histogram for gRPC services.

I propose to use a merge pull request if this passes the review.

@bsenel bsenel added the enhancement New feature or request label Dec 16, 2020
@bsenel bsenel requested a review from JustinCappos December 16, 2020 16:32
Copy link
Copy Markdown
Contributor

@JustinCappos JustinCappos left a comment

Choose a reason for hiding this comment

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

Do we also need add a ticket to back out these changes later? It seems that X.509 verification is an important part of what we would actually eventually want.

Comment thread cache/server.go
ccmsg.RegisterClientCacheServer(grpcServer, &grpcClientCacheServer{cache: c})
ccmsg.RegisterPublisherCacheServer(grpcServer, &grpcPublisherCacheServer{cache: c})
// Enable to observe gRPC latency
// grpc_prometheus.EnableHandlingTimeHistogram()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are we leaving in commented code?

Copy link
Copy Markdown
Contributor Author

@bsenel bsenel Dec 17, 2020

Choose a reason for hiding this comment

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

It is only required for performance analysis, so benchmarking. Therefore, I put this comment until adding a configuration parameter for relevant daemons to enable/disable this feature. A person who is not an expert on Prometheus may spend so much time to find out this feature.

Comment thread bootstrap/server.go
grpcServer := common.NewDBGRPCServer(conf.Insecure, db)
ccmsg.RegisterNodeBootstrapdServer(grpcServer, &grpcBootstrapServer{bootstrap: b})
// Enable to observe gRPC latency
// grpc_prometheus.EnableHandlingTimeHistogram()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are we leaving this in?

Copy link
Copy Markdown
Contributor Author

@bsenel bsenel Dec 17, 2020

Choose a reason for hiding this comment

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

To have fast access to that until having a configuration parameter for this in relevant daemons.

Comment thread log/server/logpipe.go

lp.server = common.NewDBGRPCServer(db)
// LogPipe configuration for secure communication missing
lp.server = common.NewDBGRPCServer(true, db)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What are the ramifications of setting this up without security?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The first thing that comes to mind is that eavesdropping can occur if the logpipe daemon is outside the network or even on a local network. Data tampering may also be possible if I'm not mistaken, so a malicious actor could manipulate the log data.

@bsenel
Copy link
Copy Markdown
Contributor Author

bsenel commented Dec 17, 2020

There is a configuration parameter as CACHECASH_INSECURE. But, putting TRUE/FALSE in that parameter wasn't functioning since the gRPC helpers weren't secured with TLS. These helpers were only working in insecure mode. So, I think we don't need a ticket because X.509 verification is required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Development

Successfully merging this pull request may close these issues.

2 participants