Make node TLS certificates totally ephemeral #2675

abukosek · 2020-02-14T13:22:40Z

Closes #2098.

TODO:

go/common/grpc/grpc.go

go/common/identity/identity.go

kostko · 2020-02-14T13:26:37Z

go/common/identity/identity.go

 	}
+	nextCert, err := tlsCert.Generate(CommonName)


Also make sure to reduce certificate expiration once this works.

Sentry nodes and IAS proxies require non-rotating certificates, so I've left this as-is for now.

go/runtime/committee/client.go

go/worker/registration/worker.go

go/registry/api/api.go

codecov · 2020-02-20T15:09:59Z

Codecov Report

Merging #2675 into master will increase coverage by 0.02%.
The diff coverage is 50.89%.

@@            Coverage Diff             @@
##           master    #2675      +/-   ##
==========================================
+ Coverage   65.67%   65.69%   +0.02%     
==========================================
  Files         342      342              
  Lines       32579    32866     +287     
==========================================
+ Hits        21395    21591     +196     
- Misses       8483     8581      +98     
+ Partials     2701     2694       -7

Impacted Files	Coverage Δ
go/common/node/node.go	`64.54% <0.00%> (-0.55%)`	⬇️
go/oasis-node/cmd/ias/auth_registry.go	`0.00% <0.00%> (ø)`
go/oasis-node/cmd/identity/identity.go	`37.50% <0.00%> (ø)`
go/oasis-node/cmd/storage/benchmark/benchmark.go	`3.92% <0.00%> (ø)`
go/worker/sentry/grpc/worker.go	`10.56% <0.00%> (-1.25%)`	⬇️
go/worker/sentry/grpc/init.go	`25.24% <6.66%> (-6.01%)`	⬇️
go/registry/api/api.go	`37.57% <30.30%> (-0.56%)`	⬇️
go/oasis-node/cmd/node/node.go	`53.95% <33.33%> (ø)`
go/sentry/sentry.go	`52.38% <33.33%> (-8.34%)`	⬇️
go/sentry/api/grpc.go	`54.16% <40.00%> (-23.62%)`	⬇️
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4aca538...ee4d9fc. Read the comment docs.

Yawning · 2020-03-27T12:27:33Z

go/oasis-node/cmd/identity/identity.go

@@ -50,7 +50,7 @@ func doNodeInit(cmd *cobra.Command, args []string) {
 		)
 		os.Exit(1)
 	}
-	if _, err = identity.LoadOrGenerate(dataDir, nodeSignerFactory); err != nil {
+	if _, err = identity.LoadOrGenerate(dataDir, nodeSignerFactory, true); err != nil {


Does this mean that nodes provisioned this way won't have ephemeral TLS certs unless they obliterate the persisted cert?

Yes. As rotation is off by default for now, I thought this would make most sense.
Once we enable rotation by default, we can also disable persistent TLS certs here or add a flag to control this behavior.

.buildkite/code.pipeline.yml

ptrus · 2020-03-31T10:08:42Z

.changelog/2098.feature.md

@@ -0,0 +1,6 @@
+node: Add automatic TLS certificate rotation support


Probably open an issue in https://github.com/oasislabs/docs once this is merged, so we wont forget adding/updating docs

go/common/identity/identity.go

ptrus · 2020-03-31T10:19:02Z

go/oasis-node/cmd/ias/auth_registry.go

-	ch, sub, err := auth.client.WatchRuntimes(ctx)
+	var redialAttempts uint
+
+Redial:


Could use the cenkalti/backoff package we use in other similar cases?

I think that would be an overkill for this simple use case -- the only time this connection is dropped is if the upstream node rotates its TLS certificates, a constant 2s retry is good enough.

go/worker/sentry/grpc/init.go

go/worker/sentry/worker.go

abukosek · 2020-04-03T10:46:53Z

Thanks to everyone for your previous review comments. This PR is now ready for re-review :)

Yawning · 2020-04-03T12:29:12Z

go/common/identity/identity.go

-	// TLSCertificate is a certificate that can be used for TLS.
-	TLSCertificate *tls.Certificate
+	// tlsSigner is a node TLS certificate signer.
+	tlsSigner signature.Signer


As a matter of style I prefer private struct members to be separated from public ones (and not interleaved like it is here).

ptrus · 2020-04-03T14:37:37Z

go/worker/sentry/grpc/worker.go

+		dialUpstream := func() error {
+			_, err := g.upstreamDialer(g.ctx)
+			if err != nil {
+				if numRetries < 60 {


Could instead wrap the existing constant backoff with WithMaxRetries

ptrus · 2020-04-03T14:58:58Z

go/worker/sentry/grpc/init.go

-		upstreamConn, err := initConnection(identity)
-		if err != nil {
-			return nil, fmt.Errorf("gRPC sentry worker initializing upstream connection failure: %w", err)
+		g.upstreamDialer = func(ctx context.Context) (*grpc.ClientConn, error) {


Could the mutex be removed if instead the dialer just did initConnection and returned it (without storing it in the sentry worker struct)?

It might be also a bit cleaner, since in that case there will always be two connections, one the proxy will use and one the policy watcher? (and each would keep track of it's own connection like both already do, and if needed, initialize more connections). I think the only difference to current impl. would be that currently the connection is ?sometimes? shared (I think currently it depends if proxy or sentry first invoke the dialer, if the connection is shared or not - since upstreamDialer updates the internal connection state of the sentry, but doesn't the one of the proxy?).

On the other hand if we want to keep using one connection i think it could be done cleaner if upstreamDialer is it's own struct (and not part of sentry worker), that maintains a connection (or a pool of connections), and exposes a dial method that returns it (and redials it if necessary).

The idea is to replace the policy watcher with a new sentry command (e.g. UpdatePolicies or something) that is called by the upstream node in a similar way to SetUpstreamTLSCertificates added in this PR. This will allow further simplifications of the sentry code, but that should be done in a separate PR.
I'll make an issue for this after I merge the PR.

ptrus · 2020-04-03T15:00:45Z

.changelog/2098.breaking.md

@@ -0,0 +1,5 @@
+Sentry nodes no longer require TLS certificate file of the upstream node


Nice 👍 🎉

ptrus

left 2 minor comments, otherwise looks good! 👍

abukosek · 2020-04-04T08:06:21Z

Thanks!

kostko reviewed Feb 14, 2020

View reviewed changes

abukosek force-pushed the andrej/feature/ephemeral-node-tls-certs branch 26 times, most recently from 413c698 to 5e69dc3 Compare February 20, 2020 14:55

abukosek force-pushed the andrej/feature/ephemeral-node-tls-certs branch 2 times, most recently from a0e883a to f9a5dbe Compare February 20, 2020 16:09

Yawning reviewed Mar 27, 2020

View reviewed changes

abukosek force-pushed the andrej/feature/ephemeral-node-tls-certs branch 2 times, most recently from 57581bb to 6e591d1 Compare March 31, 2020 06:11

ptrus reviewed Mar 31, 2020

View reviewed changes

abukosek added the c:breaking/cfg Category: breaks configuration label Mar 31, 2020

abukosek force-pushed the andrej/feature/ephemeral-node-tls-certs branch 3 times, most recently from 1216289 to 36e985d Compare April 3, 2020 10:10

Yawning reviewed Apr 3, 2020

View reviewed changes

abukosek force-pushed the andrej/feature/ephemeral-node-tls-certs branch from 36e985d to acdb3c9 Compare April 3, 2020 13:45

Yawning approved these changes Apr 3, 2020

View reviewed changes

ptrus reviewed Apr 3, 2020

View reviewed changes

ptrus approved these changes Apr 3, 2020

View reviewed changes

abukosek added 8 commits April 4, 2020 09:37

node: Add automatic TLS certificate rotation support

4a2d3a3

worker/sentry: Handle upstream node TLS cert rotations

b36c31c

grpc/proxy: Dial upstream on-demand

2493d4e

worker/sentry: Add commands to set/get upstream node TLS certs

6d00bf4

ias: Re-dial registry if connection is dropped

979fcf4

sentry/api: Simplify {Set,Get}UpstreamTLSCertificates commands

d2e4174

grpc/proxy: Redial if connection unexpectedly shuts down

04bd6b3

sentry/client: Use separate TLS certificate

ee4d9fc

abukosek force-pushed the andrej/feature/ephemeral-node-tls-certs branch from acdb3c9 to ee4d9fc Compare April 4, 2020 07:38

This was referenced Apr 4, 2020

sentry: Replace policy watcher with API command #2820

Closed

Update sentry node docs oasisprotocol/old-docs#235

Closed

abukosek merged commit 076f06f into master Apr 4, 2020

abukosek deleted the andrej/feature/ephemeral-node-tls-certs branch April 4, 2020 08:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make node TLS certificates totally ephemeral #2675

Make node TLS certificates totally ephemeral #2675

abukosek commented Feb 14, 2020 •

edited

Loading

kostko Feb 14, 2020

abukosek Feb 21, 2020

codecov bot commented Feb 20, 2020 •

edited

Loading

Yawning Mar 27, 2020

abukosek Mar 27, 2020

ptrus Mar 31, 2020

abukosek Mar 31, 2020

ptrus Mar 31, 2020

abukosek Mar 31, 2020

abukosek commented Apr 3, 2020

Yawning Apr 3, 2020

ptrus Apr 3, 2020 •

edited

Loading

ptrus Apr 3, 2020 •

edited

Loading

abukosek Apr 4, 2020

ptrus Apr 3, 2020 •

edited

Loading

ptrus left a comment

abukosek commented Apr 4, 2020

		@@ -0,0 +1,6 @@
		node: Add automatic TLS certificate rotation support

		@@ -0,0 +1,5 @@
		Sentry nodes no longer require TLS certificate file of the upstream node

Make node TLS certificates totally ephemeral #2675

Make node TLS certificates totally ephemeral #2675

Conversation

abukosek commented Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 20, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abukosek commented Apr 3, 2020

Choose a reason for hiding this comment

ptrus Apr 3, 2020 • edited Loading

Choose a reason for hiding this comment

ptrus Apr 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptrus Apr 3, 2020 • edited Loading

Choose a reason for hiding this comment

ptrus left a comment

Choose a reason for hiding this comment

abukosek commented Apr 4, 2020

abukosek commented Feb 14, 2020 •

edited

Loading

codecov bot commented Feb 20, 2020 •

edited

Loading

ptrus Apr 3, 2020 •

edited

Loading

ptrus Apr 3, 2020 •

edited

Loading

ptrus Apr 3, 2020 •

edited

Loading