-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add SSL support for connection #8098
Conversation
Codecov Report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor edits for the docs
@@ -46,4 +46,11 @@ files: | |||
type: integer | |||
default: 10 | |||
- template: instances/db | |||
- name: use_tls | |||
description: | | |||
This instructs the SAP HANA integration to connect using TLS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This instructs the SAP HANA integration to connect using TLS. | |
Instructs the SAP HANA integration to connect using TLS. |
@@ -100,6 +100,55 @@ instances: | |||
# tags: | |||
# - test:<INTEGRATION> | |||
|
|||
## @param use_tls - boolean - optional - default: false | |||
## This instructs the SAP HANA integration to connect using TLS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## This instructs the SAP HANA integration to connect using TLS. | |
## Instructs the SAP HANA integration to connect using TLS. |
# use_tls: false | ||
|
||
## @param tls_verify - boolean - optional - default: true | ||
## Instructs the check to validate the TLS certificate of services. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## Instructs the check to validate the TLS certificate of services. | |
## Instructs the check to validate the TLS certificates of services. |
I think? multiple services will have multiple certificates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, will have to be in another PR though as this option is used in many integrations. It has to be changed for all integrations at once in our "specs"
3a7b730
to
4b489f6
Compare
self._socket = self.tls_context.wrap_socket(self._socket, server_hostname=self.host) | ||
|
||
# Initialization Handshake | ||
self._socket.sendall(INITIALIZATION_BYTES) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you close the socket somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't, PyHDB does it. As mentioned this function is just an extension from the original implementation that does the same thing (i.e open the socket and initialize the connection): https://github.com/SAP/PyHDB/blob/v0.3.4/pyhdb/connection.py#L65
|
||
response = self._socket.recv(8) | ||
if len(response) != 8: | ||
raise Exception("Connection failed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be helpful to include the response for debugging?
The library we use (PyHDB) does not support SSL options when creating the connection to the database. See SAP-archive/PyHDB#107
This PR updates the Connection class to add SSL support when opening the socket to the db.
The official SAP HANA python driver has all the necessary configuration to connect with SSL), but unfortunately it's using a license that doesn't permit redistribution so we aren't using it.