Skip to content
Open
Show file tree
Hide file tree
Changes from 20 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
16 changes: 10 additions & 6 deletions kmip/services/kmip_client.py
Copy link

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
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,17 @@ 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)
if self.keyfile and not self.certfile:
raise ValueError("certfile must be specified")
if self.certfile:
Copy link

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
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
25 changes: 17 additions & 8 deletions kmip/services/server/server.py
Copy link

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
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,26 @@ 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.auth_suite.protocol)
context.verify_mode = ssl.CERT_REQUIRED
if self.auth_suite.ciphers:
context.set_ciphers(self.auth_suite.ciphers)
if cafile:
context.load_verify_locations(cafile)
certfile = self.config.settings.get('certificate_path')

if certfile:
Copy link

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
Author

Choose a reason for hiding this comment

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

Fixed

Copy link

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
Author

Choose a reason for hiding this comment

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

Fixed

Copy link
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)
else:
raise ValueError("certfile must be specified for server-side operations")
Copy link

Choose a reason for hiding this comment

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

What we don't have here is the equivalent of the client-side check

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

but it also turns out that the higher levels already check that both certificate and key are specified:

kmip.core.exceptions.ConfigurationError: Setting 'key_path' is missing from the configuration file.
kmip.core.exceptions.ConfigurationError: Setting 'certificate_path' is missing from the configuration file.

So it's probably not necessary.

Copy link

Choose a reason for hiding this comment

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

Also, it looks like this causes the test suite to fail in TestKmipServer.test_start:

ValueError: certfile must be specified for server-side operations

So either this code or the test setup would need adjustments.

Copy link
Author

Choose a reason for hiding this comment

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

Removed from the server


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