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.

Sign up for GitHub

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

Jump to bottom

SSL support #28

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

SSL support #28

wants to merge 4 commits into from

Conversation

DifferentialOrange
Copy link

Fix running tests for Tarantool Enterprise

Stream tests use tarantool -V output to check version. For Tarantool
Enterprise, tarantool -V output is "Tarantool Enterprise ",
thus it is incorrect to extract the version tag by splitting by spaces.

Part of #22

Rename internal _transport variable

"transport" is a keyword that is used to define SSL connection in
Tarantool Enterprise 2.10 and newer. It would be convenient to use
self._transport to store user input transport variable, but this
name is already used to store connection transport. This patch renames
the instance variable.

Part of #22

Support SSL encrypted connection to Tarantool EE

This patch adds support for using SSL to encrypt the client-server
communications [1]. The patch is based on a similar patch in
tarantool/tarantool-python connector [2].

To use SSL encrypted connection, use Connection parameters:

conn = asynctnt.Connection(host='127.0.0.1',
                           port=3301,
                           transport=asynctnt.Transport.SSL,
                           ssl_key_file='./ssl/host.key',
                           ssl_cert_file='./ssl/host.crt',
                           ssl_ca_file='./ssl/ca.crt',
                           ssl_ciphers='ECDHE-RSA-AES256-GCM-SHA384')

If Tarantool server uses "ssl" transport, client connection also need to
use asynctnt.Transport.SSL transport. If server side had ssl_ca_file
set, ssl_key_file and ssl_cert_file are mandatory from the client side,
otherwise optional. CA file and ciphers are optional. See available
ciphers in Tarantool EE documentation [3].

  1. https://www.tarantool.io/en/enterprise_doc/security/#enterprise-iproto-encryption
  2. SSL support tarantool/tarantool-python#220
  3. https://www.tarantool.io/en/enterprise_doc/security/#supported-ciphers

Closes #22

Support starting Tarantool server with SSL

SSL encrypted server could be started with Tarantool Enterprise 2.10 or
newer. To configure encryption, additional listen params must be passed.
ssl_key_file and ssl_cert_file are mandatory if transport is
asynctnt.Transport.SSL .

Follows up #22

Add SSL tests

To run SSL tests, use Tarantool Enterprise 2.10 or newer and set
TEST_TT_SSL=TRUE flag. The patch is based on a similar patch in
tarantool/tarantool-python connector [1].

  1. SSL support tarantool/tarantool-python#220

Follows up #22

@DifferentialOrange
Copy link
Author

DifferentialOrange commented Aug 11, 2022 •
edited

To run SSL tests with CI, it is required to have a token to download Tarantool Enterprise Edition. Unfortunately, it seems that it is impossible for now due to legal reasons. We're discussing this with Product team to make it possible or provide some alternative solutions, but for now there are no results.

@oleg-jukovec
Copy link

oleg-jukovec commented Aug 11, 2022 •
edited

To run SSL tests with CI, it is required to have a token to download Tarantool Enterprise Edition. Unfortunately, it seems that it is impossible for now due to legal reasons. We're discussing this with Product team to make it possible or provide some alternative solutions, but for now there are no results.

Is there a way to emulate SSL support from Tarantool Enterprise for tests?

nginx proxy + Tarantool or openssl server proxy + Tarantool

It will not be possible to test full compatibility with Tarantool Enterprise, but basic TLSv1.2 support seems to me is possible to test:
https://www.tarantool.io/ru/enterprise_doc/security/#traffic-encryption

@DifferentialOrange
Copy link
Author

To run SSL tests with CI, it is required to have a token to download Tarantool Enterprise Edition. Unfortunately, it seems that it is impossible for now due to legal reasons. We're discussing this with Product team to make it possible or provide some alternative solutions, but for now there are no results.

Is there a way to emulate SSL support from Tarantool Enterprise for tests?

nginx proxy + Tarantool or openssl server proxy + Tarantool

It will not be possible to test full compatibility with Tarantool Enterprise, but basic TLSv1.2 support seems to me is possible to test: https://www.tarantool.io/ru/enterprise_doc/security/#traffic-encryption

@Mons said in Russian Tarantool Community chat discussion that it won't work: https://t.me/tarantoolru/188258

@oleg-jukovec
Copy link

oleg-jukovec commented Aug 11, 2022 •
edited

@Mons said in Russian Tarantool Community chat discussion that it won't work: https://t.me/tarantoolru/188258

To be honest, I don't understand why. If we look at the code in the pull request, the binary protocol is simply wrapped by TLS. Could you try to test it (or ask @Mons for the exact reason) to make it clear?

@DifferentialOrange
Copy link
Author

@Mons said in Russian Tarantool Community chat discussion that it won't work: https://t.me/tarantoolru/188258

To be honest, I don't understand why. If we look at the code in the pull request, the binary protocol is simply wrapped by TLS. Could you try to test it (or ask @Mons for the exact reason) to make it clear?

Yeah, I will try to

igorcoding
igorcoding requested changes Aug 16, 2022
View reviewed changes
Copy link
Owner

@igorcoding igorcoding left a comment

Choose a reason for hiding this comment

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

Thank you for this awesome contribution! Just a few moments I think need to be addressed. And the main concern of mine are automatic tests. It is super important to be able to test automatically in the Github Actions environment this new SSL addition. It is pretty dangerous to be releasing and (which is more important) supporting in the future a functionality that cannot be tested in any way. Please suggest a viable solution to this. Look forward to hearing from you.

asynctnt/connection.py Outdated Show resolved Hide resolved
asynctnt/connection.py Show resolved Hide resolved
asynctnt/const.py Outdated Show resolved Hide resolved
asynctnt/instance.py Outdated Show resolved Hide resolved
tests/test_ssl.py Show resolved Hide resolved
asynctnt/connection.py Outdated
unix_path
)
unix_path,
ssl=ssl_context)
Copy link
Owner

Choose a reason for hiding this comment

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

maybe it will be good to allow specifying ssl_handshake_timeout as well (forward from Connection __init__ params, with the default to None). In both create_unix_connection and create_connection calls.

Copy link
Author

Choose a reason for hiding this comment

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

Added

Copy link
Author

Choose a reason for hiding this comment

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

Timeout is supported only since 3.7. I've added a description note, but should I prove something more noticeable?

@DifferentialOrange
Copy link
Author

Thank you for this awesome contribution! Just a few moments I think need to be addressed. And the main concern of mine are automatic tests. It is super important to be able to test automatically in the Github Actions environment this new SSL addition. It is pretty dangerous to be releasing and (which is more important) supporting in the future a functionality that cannot be tested in any way. Please suggest a viable solution to this. Look forward to hearing from you.

Yes, I agree with you. Due to a holiday season we haven't been able to discuss this matter yet, but we definitely will as soon as possible.

@Mons
Copy link

Mons commented Aug 17, 2022

@Mons said in Russian Tarantool Community chat discussion that it won't work: https://t.me/tarantoolru/188258

It seems I was wrong

igorcoding
igorcoding requested changes Aug 17, 2022
View reviewed changes
asynctnt/connection.py Outdated
@@ -330,6 +425,8 @@ async def full_connect():
logger.debug("connect is cancelled")
self._reconnect_task = None
raise
except ssl.SSLError as e:
raise SSLError(e)
Copy link
Owner

Choose a reason for hiding this comment

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

Actually I just noticed this thing. This is a debatable solution. Why did you decide to specifically catch SSLError and stop the reconnection process? I see a potential problem here. For example, let's say everything works fine, then Tarantool server is reconfigured but incorrectly which leads to ssl errors while connecting and connection tries to reestablish in the background. And this except block raises exception without a possibility to wait for the reconnect process giving a chance to reconfigure Tarantool once more.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, the idea was just to wrap all SSL-related errors to our SSLError class. I reworked the code so ssl errors are processed as usual exceptions.

@DifferentialOrange
Fix running tests for Tarantool Enterprise
108c6f9 
Stream tests use `tarantool -V` output to check version. For Tarantool
Enterprise, `tarantool -V` output is "Tarantool Enterprise <version>",
thus it is incorrect to extract the version tag by splitting by spaces.

Part of igorcoding#22
@DifferentialOrange
Support SSL encrypted connection to Tarantool EE
8b72969 
This patch adds support for using SSL to encrypt the client-server
communications [1]. The patch is based on a similar patch in
tarantool/tarantool-python connector [2].

To use SSL encrypted connection, use Connection parameters:

  conn = asynctnt.Connection(host='127.0.0.1',
                             port=3301,
                             transport=asynctnt.Transport.SSL,
                             ssl_key_file='./ssl/host.key',
                             ssl_cert_file='./ssl/host.crt',
                             ssl_ca_file='./ssl/ca.crt',
                             ssl_ciphers='ECDHE-RSA-AES256-GCM-SHA384')

If Tarantool server uses "ssl" transport, client connection also need to
use asynctnt.Transport.SSL transport. If server side had ssl_ca_file
set, ssl_key_file and ssl_cert_file are mandatory from the client side,
otherwise optional. CA file and ciphers are optional. See available
ciphers in Tarantool EE documentation [3].

1. https://www.tarantool.io/en/enterprise_doc/security/#enterprise-iproto-encryption
2. tarantool/tarantool-python#220
3. https://www.tarantool.io/en/enterprise_doc/security/#supported-ciphers

Closes igorcoding#22
@DifferentialOrange
Support starting Tarantool server with SSL
6f934fc 
SSL encrypted server could be started with Tarantool Enterprise 2.10 or
newer. To configure encryption, additional listen params must be passed.
ssl_key_file and ssl_cert_file are mandatory if transport is
asynctnt.Transport.SSL .

Follows up igorcoding#22
@DifferentialOrange
Add SSL tests
369d932 
To run SSL tests, use Tarantool Enterprise 2.10 or newer and set
TEST_TT_SSL=TRUE flag. The patch is based on a similar patch in
tarantool/tarantool-python connector [1].

1. tarantool/tarantool-python#220

Follows up igorcoding#22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleg-jukovec oleg-jukovec oleg-jukovec left review comments

@igorcoding igorcoding igorcoding requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support encrypted binary protocol (Tarantool Enterprise)
4 participants
@DifferentialOrange @oleg-jukovec @Mons @igorcoding