Skip to content
Open
Show file tree
Hide file tree
Changes from 10 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
13 changes: 7 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,14 @@ def open(self):
six.reraise(*last_error)

def _create_socket(self, sock):
self.socket = ssl.wrap_socket(
purpose = ssl.Purpose.SERVER_AUTH
context = ssl.create_default_context(purpose=purpose, capath=self.ca_certs)
Copy link

Choose a reason for hiding this comment

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

This should be cafile=... instead of capath=..., because the underlying configuration parameter points to a file.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

context.check_hostname = False
Copy link

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
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

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
19 changes: 11 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,20 @@ def interrupt_handler(trigger, frame):
for cipher in auth_suite_ciphers:
self._logger.debug(cipher)

self._socket = ssl.wrap_socket(
purpose = ssl.Purpose.CLIENT_AUTH
capath = self.config.settings.get('ca_path')
context = ssl.create_default_context(purpose=purpose, capath=capath)
Copy link

Choose a reason for hiding this comment

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

This should be cafile=capath instead of capath=capath, because the underlying configuration parameter points to a file. (The naming is unfortunately inconsistent between the Python ssl module and pykmip.)

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.

You must set

context.verify_mode=ssl.CERT_REQUIRED

after creating context. This matches what the previous code did. Without this, the server won't ask the client to send a certificate, and without a client certificate this will later fail at higher levels (such as

certificate = auth.get_certificate_from_connection(
).

Copy link
Author

Choose a reason for hiding this comment

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

Done

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)

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
4 changes: 2 additions & 2 deletions kmip/tests/unit/services/server/test_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ 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.wrap_socket') as ssl_mock:
socket_mock.return_value = a_mock
ssl_mock.return_value = b_mock

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