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

pkg/transport: reload TLS certificates for every client requests #7829

Merged
merged 2 commits into from
Apr 27, 2017

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Apr 27, 2017

No description provided.

@@ -66,6 +70,89 @@ func TestDialTLSExpired(t *testing.T) {
}
}

// TestDialTLSExpiredReload ensures server reloads expired certs,
// rejecting client requests, and vice versa.
func TestDialTLSExpiredReload(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

integration/ instead of clientv3/integration since it's testing server behavior that's client independent?

@@ -173,3 +260,52 @@ func TestDialForeignEndpoint(t *testing.T) {
t.Fatal(err)
}
}

// copyTLSFiles clones certs files to temp directory.
func copyTLSFiles(ti transport.TLSInfo) (transport.TLSInfo, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

func copyTLSFiles(ti transport.TLSInfo, dst string) (transport.TLSInfo, error) instead of bothering with tempdirs here; can be passed in as an argument

if err = w.Sync(); err != nil {
return err
}
if _, err = w.Seek(0, io.SeekStart); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

?

defer clus.Terminate(t)

// replace certs directory with expired ones
if err = os.Rename(certsDir, tmpDir); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Two separate tests: atomic directory rename and copy-over. The copy-over case would cause connection failures during cert update (or might take down the server in some way).


// now server expects 'tls: bad certificate'
// on incoming client requests
tls, err := ts.ClientConfig()
Copy link
Contributor

Choose a reason for hiding this comment

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

Some concurrency would be good here. Boot a cluster, start a goroutine that spams it with connections, manipulate the certs while connections concurrently hit the server. The connection goroutine would complete when it transitions from good connections to tls rejects.

@gyuho gyuho added the WIP label Apr 27, 2017
@gyuho gyuho force-pushed the certs branch 2 times, most recently from c2f7574 to 8020d42 Compare April 27, 2017 18:13
This changes the baseConfig used when creating tls Configs to utilize
the GetCertificate and GetClientCertificate functions to always reload
the certificates from disk whenever they are needed.

Always reloading the certificates allows changing the certificates via
an external process without interrupting etcd.

Fixes etcd-io#7576

Cherry-picked by Gyu-Ho Lee <gyuhox@gmail.com>
Original commit can be found at etcd-io#7784
@gyuho
Copy link
Contributor Author

gyuho commented Apr 27, 2017

@heyitsanthony PTAL. Thanks.

}
}()

// overwrite valid certs with expired ones
Copy link
Contributor

Choose a reason for hiding this comment

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

copyTLSFiles to replace the copyFiles?

t.Fatal("expected dial timeout in 3 seconds, but never got it")
}

// now, replace expired certs back with valid ones
Copy link
Contributor

Choose a reason for hiding this comment

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

copyTLSFiles to replace the copyFiles?

defer clus.Terminate(t)

// concurrent client dialing while certs transition from valid to expired
errc := make(chan error)
Copy link
Contributor

Choose a reason for hiding this comment

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

make(chan error, 1)

}
cli, cerr := clientv3.New(clientv3.Config{
Endpoints: []string{clus.Members[0].GRPCAddr()},
DialTimeout: 3 * time.Second,
Copy link
Contributor

Choose a reason for hiding this comment

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

1s?

}
cli, cerr := clientv3.New(clientv3.Config{
Endpoints: []string{clus.Members[0].GRPCAddr()},
DialTimeout: 3 * time.Second,
Copy link
Contributor

Choose a reason for hiding this comment

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

1s?

@@ -929,3 +938,42 @@ type grpcAPI struct {
// Auth is the authentication API for the client's connection.
Auth pb.AuthClient
}

// copyTLSFiles clones certs files to dst directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

util_test.go or some other test file for these copy functions since they're not used by the integration package outside of tests?

@gyuho gyuho force-pushed the certs branch 3 times, most recently from 059044f to 4aa95cd Compare April 27, 2017 20:13
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
@heyitsanthony
Copy link
Contributor

lgtm

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

Successfully merging this pull request may close these issues.

2 participants