Support of SSL protocol #170

oleg-jukovec · 2022-04-22T09:45:09Z

The patch adds support for using SSL to encrypt the client-server
communications [1]. It uses a wrapper around the OpenSSL library for
full compatibility with Tarantool Enterprise (GOST cryptographic
algorithms [2] are not supported by Golang's crypto/tls). The feature
can be disabled using a build tag [3] 'go_tarantool_ssl_disable'.

Related issues:

#155

We have forked go-openssl library (I hope that temporarily). It will be great if you review my commints there too:
https://github.com/tarantool/go-openssl/tree/v0.0.7-24-g0fadeb4-tarantool

I am not familiar with GitHub's CI/CD. I might have missed something. Please, check CI/CD more carefully if you can.

P.S. I will update a Tarantool Enterprise version in tests from beta after the release (on Tuesday?).

ssl_test.go

.github/workflows/testing.yml

ssl_certs/localhost.crt

.github/workflows/testing.yml

ssl_test.go

DifferentialOrange

To be honest, I'm not familiar with SSL, but since a similar task in assigned to me in tarantool-python, I decided to review that one. Thank you for providing useful links in the description, it helped me to better understand what's going on.

I have left several comments. There are some parts of code I haven't reviewed in-depth yet, but I think I'll give it a shot later.

The main question is compatibility. I've got a bit confused while reviewing the code. What will happen if we connect with SSL-builded connector to older Tarantool? With SSL opts? What do we need ssl_disable dummy functions for: is it only a question of language imports or there is something more?

.github/workflows/testing.yml

config.lua

CONTRIBUTING.md

example_test.go

ssl_certs/generate.sh

ssl.go

export_test.go

oleg-jukovec · 2022-04-26T13:08:12Z

The main question is compatibility. I've got a bit confused while reviewing the code. What will happen if we connect with SSL-builded connector to older Tarantool? With SSL opts?

I don't see any problem.

If Transport = 'ssl' is set It will be the same as with another incorrect configuration (host, port etc). Just do it in a correct way (don't set Transport = 'ssl', set up right ip, port etc) and all be fine.

What do we need ssl_disable dummy functions for: is it only a question of language imports or there is something more?

An user will be able to make build without OpenSSL dependency (maybe he doesn't need it), as it was before. Linking with the OpenSSL library can complicate build.

.github/workflows/testing.yml

CONTRIBUTING.md

example_test.go

export_test.go

ssl.go

ssl_test.go

test_helpers/main.go

ligurio removed their request for review

testdata/generate.sh

ssl_test.go

ssl_certs/localhost.crt

ssl_test.go

testdata/generate.sh

.github/workflows/testing.yml

Totktonada · 2022-05-26T23:50:40Z

test_helpers/main.go

+	// PingServer changes a host to connect to test startup of a Tarantool
+	// instance. By default, it uses Listen value as the host for the connection.
+	PingServer string
+
+	// PingTransport changes Opts.Transport for a connection that checks startup
+	// of a Tarantool instance.
+	PingTransport string
+
+	// PingSsl changes Opts.Ssl for a connection that checks startup of
+	// a Tarantool instance.
+	PingSsl tarantool.SslOpts


A thing of taste: feel free to incorpore or ignore.

PingServer and PingTransport can be extracted from Listen. If we want to leave ability to pass a client completely different host/port/transport, there is the usual term for that: advertize URI/host/port. At least it is used in etcd in the sense 'URI to use for connecting to the instance' as opposed to 'URI to listen on'.

PingSsl: I would named it just Ssl, because it does not clash with anything. Or, say, ClientSsl, because it is client's options in fact.

Thank you. It's a good idea to replace Ping because it's a implementation detail. So I've renamed PingServer, PingTransport, PingSsl to ClientServer, ClientTransport, ClientSsl.

It useful to set these values directly in tests (to set the wrong ones, for example, we use SSL for a server, but not for a client).

Totktonada

LGTM. I don't see problems in the code and can't add more value here.

Well, there is (a low?) risk of extra dynamic dependency on system's openssl (discussed in Georgy's comment). I'm okay to let Oleg to take the risk.

I would like to know more regarding ability to link openssl statically into resulting executable and leave some instructions for a user. Maybe even a sanity check like 'if static-openssl build flag is enabled, the connector will fail on building at attempt to link it to openssl dynamically'. However I'm okay to work on it later if there will be a demand.

Let's acquire a second approve (I think Georgy can finish its review) and proceed.

Totktonada

<..duplicated message, removed..>

DifferentialOrange · 2022-05-30T11:20:52Z

Well, there is (a low?) risk of extra dynamic dependency on system's openssl (discussed in Georgy's comment). I'm okay to let Oleg to take the risk.

Then maybe we should leave this patch until the next tag, after we tag current stable and compatible things?

DifferentialOrange

Should be LGTM after resolving remaining things.

To be honest, I'm also not a fan of adding an SSL dependencies by default, but if discussions result is like this, well, I will not oppose it.

.github/workflows/testing.yml

DifferentialOrange · 2022-05-31T07:28:35Z

connection.go

@@ -383,11 +416,17 @@ func (conn *Connection) dial() (err error) {
 	} else if addrLen >= 4 && address[0:4] == "tcp:" {
 		address = address[4:]
 	}
-	connection, err = net.DialTimeout(network, address, timeout)
+	if transport == connTransportNone {
+		connection, err = net.DialTimeout(network, address, timeout)


If we're missing libssl, libcrypto and never use transport == connTransportSsl, will application with go-tarantool work is it was built with default flags? I'm not well up in linking.

The resulting executable will have the dynamic dependency on libssl and libcrypto (shown in ldd or otool -L). The dynamic loader will refuse to load a dynamic executable if any library is missing.

It is safe while given executable is used within a distro of particular version (because distros guarantees ABI compatibility -- where we had openssl 1.1, it will not be updated to openssl 3; at least not from base repositories). So if you build an application for each distro or at least have separate builds for openssl 1.1 and openssl 3 distros, everything should be fine.

If you want to build an executable that would work on, say, Fedora 35 and Fedora 36 both (or Ubuntu Impish and Ubuntu Jammy both), you're in the trouble.

So the question is actually about ways to deliver the resulting application. I can't judge about some 'usual way' here.

ssl_test.go

test_helpers/main.go

Totktonada · 2022-06-01T01:29:10Z

<...> maybe we should leave this patch until the next tag, after we tag current stable and compatible things?

Okay. It seems, it is time to make a release. @oleg-jukovec, can you, please, release the connector?

The patch adds support for using SSL to encrypt the client-server communications [1]. It uses a wrapper around the OpenSSL library for full compatibility with Tarantool Enterprise (GOST cryptographic algorithms [2] are not supported by Golang's crypto/tls). The feature can be disabled using a build tag [3] 'go_tarantool_ssl_disable'. 1. https://www.tarantool.io/en/enterprise_doc/security/#enterprise-iproto-encryption 2. https://github.com/gost-engine/engine 3. https://pkg.go.dev/go/build#hdr-Build_Constraints Closes #155

oleg-jukovec · 2022-06-02T12:15:00Z

@oleg-jukovec, can you, please, release the connector?

Sure, it's done.

oleg-jukovec requested review from AnaNek and LeonidVas April 22, 2022 09:48

ligurio requested changes Apr 22, 2022

View reviewed changes

ssl_test.go Show resolved Hide resolved

.github/workflows/testing.yml Show resolved Hide resolved

ssl_certs/localhost.crt Outdated Show resolved Hide resolved

.github/workflows/testing.yml Show resolved Hide resolved

DifferentialOrange self-requested a review April 22, 2022 13:32

DifferentialOrange reviewed Apr 22, 2022

View reviewed changes

.github/workflows/testing.yml Outdated Show resolved Hide resolved

ligurio previously requested changes Apr 22, 2022

View reviewed changes

ssl_test.go Show resolved Hide resolved

oleg-jukovec force-pushed the oleg-jukovec/gh-155-ssl-support branch 7 times, most recently from ae1ac77 to 788e2b6 Compare April 25, 2022 10:35

oleg-jukovec requested review from DifferentialOrange and ligurio April 25, 2022 10:35

oleg-jukovec force-pushed the oleg-jukovec/gh-155-ssl-support branch from 788e2b6 to 32d0148 Compare April 25, 2022 14:40

DifferentialOrange reviewed Apr 26, 2022

View reviewed changes

oleg-jukovec force-pushed the oleg-jukovec/gh-155-ssl-support branch from 32d0148 to 520e1ca Compare April 26, 2022 12:57

oleg-jukovec force-pushed the oleg-jukovec/gh-155-ssl-support branch 2 times, most recently from f41cab7 to 8f66b0b Compare April 26, 2022 13:36

oleg-jukovec requested a review from DifferentialOrange April 26, 2022 14:09

oleg-jukovec force-pushed the oleg-jukovec/gh-155-ssl-support branch 2 times, most recently from 807069b to 2aa8178 Compare April 29, 2022 11:23

DifferentialOrange reviewed May 11, 2022

View reviewed changes

.github/workflows/testing.yml Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

example_test.go Show resolved Hide resolved

export_test.go Outdated Show resolved Hide resolved

ssl.go Show resolved Hide resolved

oleg-jukovec force-pushed the oleg-jukovec/gh-155-ssl-support branch 2 times, most recently from d6bc3f7 to 0d72eae Compare May 11, 2022 17:32

DifferentialOrange reviewed May 12, 2022

View reviewed changes

ssl_test.go Outdated Show resolved Hide resolved

test_helpers/main.go Outdated Show resolved Hide resolved

oleg-jukovec force-pushed the oleg-jukovec/gh-155-ssl-support branch 2 times, most recently from 241bb16 to 595449a Compare May 12, 2022 10:09

oleg-jukovec requested review from DifferentialOrange and Totktonada May 16, 2022 16:27

oleg-jukovec force-pushed the oleg-jukovec/gh-155-ssl-support branch 2 times, most recently from 5a22cd1 to 32b2b6f Compare May 19, 2022 15:09

ligurio reviewed May 20, 2022

View reviewed changes

testdata/generate.sh Outdated Show resolved Hide resolved

testdata/generate.sh Show resolved Hide resolved

ssl_test.go Show resolved Hide resolved

ssl_certs/localhost.crt Outdated Show resolved Hide resolved

ligurio requested changes May 20, 2022

View reviewed changes

ssl_test.go Show resolved Hide resolved

oleg-jukovec force-pushed the oleg-jukovec/gh-155-ssl-support branch 3 times, most recently from f2b5764 to 223b013 Compare May 20, 2022 13:55

Totktonada reviewed May 25, 2022

View reviewed changes

testdata/generate.sh Outdated Show resolved Hide resolved

Totktonada reviewed May 25, 2022

View reviewed changes

.github/workflows/testing.yml Show resolved Hide resolved

oleg-jukovec force-pushed the oleg-jukovec/gh-155-ssl-support branch 2 times, most recently from dabffea to b1475b7 Compare May 26, 2022 06:39

Totktonada reviewed May 26, 2022

View reviewed changes

Totktonada approved these changes May 27, 2022

View reviewed changes

DifferentialOrange reviewed May 31, 2022

View reviewed changes

oleg-jukovec force-pushed the oleg-jukovec/gh-155-ssl-support branch 3 times, most recently from 2bffdba to 1cbf099 Compare May 31, 2022 08:27

oleg-jukovec force-pushed the oleg-jukovec/gh-155-ssl-support branch 2 times, most recently from d595853 to 97ee5a2 Compare June 2, 2022 11:52

oleg-jukovec force-pushed the oleg-jukovec/gh-155-ssl-support branch from 97ee5a2 to 851079c Compare June 2, 2022 12:05

oleg-jukovec merged commit f8e9c70 into master Jun 2, 2022

oleg-jukovec deleted the oleg-jukovec/gh-155-ssl-support branch June 2, 2022 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support of SSL protocol #170

Support of SSL protocol #170

oleg-jukovec commented Apr 22, 2022

DifferentialOrange left a comment

oleg-jukovec commented Apr 26, 2022

Totktonada May 26, 2022

oleg-jukovec May 31, 2022

Totktonada left a comment

Totktonada left a comment •

edited

Loading

DifferentialOrange commented May 30, 2022

DifferentialOrange left a comment

DifferentialOrange May 31, 2022

Totktonada Jun 1, 2022

Totktonada commented Jun 1, 2022

oleg-jukovec commented Jun 2, 2022

Support of SSL protocol #170

Support of SSL protocol #170

Conversation

oleg-jukovec commented Apr 22, 2022

DifferentialOrange left a comment

Choose a reason for hiding this comment

oleg-jukovec commented Apr 26, 2022

Totktonada May 26, 2022

Choose a reason for hiding this comment

oleg-jukovec May 31, 2022

Choose a reason for hiding this comment

Totktonada left a comment

Choose a reason for hiding this comment

Totktonada left a comment • edited Loading

Choose a reason for hiding this comment

DifferentialOrange commented May 30, 2022

DifferentialOrange left a comment

Choose a reason for hiding this comment

DifferentialOrange May 31, 2022

Choose a reason for hiding this comment

Totktonada Jun 1, 2022

Choose a reason for hiding this comment

Totktonada commented Jun 1, 2022

oleg-jukovec commented Jun 2, 2022

Totktonada left a comment •

edited

Loading