Skip to content
Open
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/tox.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ jobs:
- {version: '3.9', env: py39}
- {version: '3.10', env: py310}
- {version: '3.11', env: py311}
- {version: '3.12', env: py312}
test_mode: [0, 1]
runs-on: ${{ matrix.os }}
env:
Expand All @@ -35,7 +36,7 @@ jobs:
strategy:
matrix:
os: [ubuntu-latest]
python: ['3.8', '3.9', '3.10', '3.11']
python: ['3.8', '3.9', '3.10', '3.11', '3.12']
test: [pep8, bandit, docs]
runs-on: ${{ matrix.os }}
env:
Expand Down
15 changes: 9 additions & 6 deletions kmip/services/kmip_client.py
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This no longer pays attention to the configuration settings self.cert_reqs and self.ssl_versions. That's probably okay in practice, since the defaults are good, but these settings are documented, so they should work.

One option might be to not use ssl.create_default_context() and instead use ssl.SSLContext directly and apply these settings. Like

        context = ssl.SSLContext(self.ssl_version)
        context.verify_mode = self.cert_reqs
        if self.ca_certs:
            context.load_verify_locations(self.ca_certs)

This is just about as much code as before but it reproduces the previous behavior more precisely.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done

Original file line number Diff line number Diff line change
Expand Up @@ -285,13 +285,16 @@ def open(self):
six.reraise(*last_error)

def _create_socket(self, sock):
self.socket = ssl.wrap_socket(
context = ssl.SSLContext(self.ssl_version)
context.verify_mode = self.cert_reqs
if self.ca_certs:
context.load_verify_locations(self.ca_certs)
context.check_hostname = False
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This line doesn't appear to be needed. Probably left over from a previous version that used create_default_context().

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Removed

if self.certfile:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm testing combinations of "forgetting" to specify certfile or keyfile in the configuration file. With this code, if you specify certfile but not keyfile, you'll get an exception from load_cert_chain. If you specify keyfile but not certfile, this code is skipped and you'll get whatever the SSL stack ends up deciding. The old module-level wrap_socket had an additional check

    if keyfile and not certfile:
        raise ValueError("certfile must be specified")

to prevent this. Maybe this would be good to have here as well.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done

context.load_cert_chain(self.certfile, self.keyfile)
self.socket = context.wrap_socket(

Check failure

Code scanning / CodeQL

Use of insecure SSL/TLS version

Insecure SSL/TLS protocol version TLSv1 allowed by [call to ssl.create_default_context](1). Insecure SSL/TLS protocol version TLSv1_1 allowed by [call to ssl.create_default_context](1).
sock,
keyfile=self.keyfile,
certfile=self.certfile,
cert_reqs=self.cert_reqs,
ssl_version=self.ssl_version,
ca_certs=self.ca_certs,
server_side=False,
do_handshake_on_connect=self.do_handshake_on_connect,
suppress_ragged_eofs=self.suppress_ragged_eofs)
self.socket.settimeout(self.timeout)
Expand Down
23 changes: 15 additions & 8 deletions kmip/services/server/server.py
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Similar to the client, we should make sure that this change does not lose any previously working configuration settings. I see for example that ssl_version=self.auth_suite.protocol and ciphers=self.auth_suite.ciphers don't get used anymore.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Updated

Original file line number Diff line number Diff line change
Expand Up @@ -287,17 +287,24 @@ def interrupt_handler(trigger, frame):
for cipher in auth_suite_ciphers:
self._logger.debug(cipher)

self._socket = ssl.wrap_socket(
cafile = self.config.settings.get('ca_path')
context = ssl.SSLContext(self.ssl_version)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This should be

context = ssl.SSLContext(self.auth_suite.protocol)

(wrongly copy-pasted from client code?)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed

context.verify_mode = ssl.CERT_REQUIRED
if auth_suite_ciphers:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This should be self.auth_suite.ciphers. I'm not sure exactly what the relationship between that and auth_suite_ciphers is, but this one crashes.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed

context.set_ciphers(auth_suite_ciphers)
if cafile:
context.load_verify_locations(cafile)
certfile = self.config.settings.get('certificate_path')

if certfile:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

As with the client, it's worth checking here about how different combinations of not specifying certificate or key behave. For example, the old module-level wrap_socket() had a check

    if server_side and not certfile:
        raise ValueError("certfile must be specified for server-side "
                         "operations")

so omitting the certificate would have been a hard error.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think what I had in mind was like

         if certfile:
             keyfile = self.config.settings.get('key_path')
             context.load_cert_chain(certfile, keyfile=keyfile)
+        else:
+            raise ValueError("certfile must be specified for server-side operations")

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I instead let the context raise an appropriate error. This has the benefit of letting us mock the context behavior in the tests.

keyfile = self.config.settings.get('key_path')
context.load_cert_chain(certfile, keyfile=keyfile)

self._socket = context.wrap_socket(

Check failure

Code scanning / CodeQL

Use of insecure SSL/TLS version

Insecure SSL/TLS protocol version TLSv1 allowed by [call to ssl.create_default_context](1). Insecure SSL/TLS protocol version TLSv1_1 allowed by [call to ssl.create_default_context](1).
self._socket,
keyfile=self.config.settings.get('key_path'),
certfile=self.config.settings.get('certificate_path'),
server_side=True,
cert_reqs=ssl.CERT_REQUIRED,
ssl_version=self.auth_suite.protocol,
ca_certs=self.config.settings.get('ca_path'),
do_handshake_on_connect=False,
suppress_ragged_eofs=True,
ciphers=self.auth_suite.ciphers
suppress_ragged_eofs=True
)

try:
Expand Down
6 changes: 3 additions & 3 deletions kmip/tests/unit/services/server/test_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,9 +210,9 @@ def test_start(self,
# Test that in ideal cases no errors are generated and the right
# log messages are.
with mock.patch('socket.socket') as socket_mock:
with mock.patch('ssl.wrap_socket') as ssl_mock:
with mock.patch('ssl.SSLContext') as ssl_mock:
socket_mock.return_value = a_mock
ssl_mock.return_value = b_mock
ssl_mock.return_value.wrap_socket.return_value = b_mock

manager_mock.assert_not_called()
monitor_mock.assert_not_called()
Expand Down Expand Up @@ -271,7 +271,7 @@ def test_start(self,

# Test that a NetworkingError is generated if the socket bind fails.
with mock.patch('socket.socket') as socket_mock:
with mock.patch('ssl.wrap_socket') as ssl_mock:
with mock.patch('ssl.SSLContext.wrap_socket') as ssl_mock:
socket_mock.return_value = a_mock
ssl_mock.return_value = b_mock

Expand Down