Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
4 changes: 3 additions & 1 deletion .github/workflows/tox.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ jobs:
- {version: '3.9', env: py39}
- {version: '3.10', env: py310}
- {version: '3.11', env: py311}
- {version: '3.12', env: py312}
- {version: '3.13', env: py313}
test_mode: [0, 1]
runs-on: ${{ matrix.os }}
env:
Expand All @@ -35,7 +37,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', '3.13']
test: [pep8, bandit, docs]
runs-on: ${{ matrix.os }}
env:
Expand Down
5 changes: 1 addition & 4 deletions docs/source/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@
#
#html_theme = 'alabaster'
html_theme = "sphinx_rtd_theme"
html_theme_path = [sphinx_rtd_theme.get_html_theme_path()]
# html_theme_path = []

# Theme options are theme-specific and customize the look and feel of a theme
# further. For a list of options available for each theme, see the
Expand Down Expand Up @@ -169,6 +169,3 @@
author, 'PyKMIP', 'One line description of project.',
'Miscellaneous'),
]



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
21 changes: 13 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,22 @@ 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')
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
10 changes: 6 additions & 4 deletions kmip/tests/unit/services/server/test_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,9 +210,10 @@ 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
ssl_mock.return_value.load_cert_chain.return_value = None

manager_mock.assert_not_called()
monitor_mock.assert_not_called()
Expand Down Expand Up @@ -271,9 +272,10 @@ 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') 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
ssl_mock.return_value.load_cert_chain.return_value = None

test_exception = Exception()
b_mock.bind.side_effect = test_exception
Expand Down
2 changes: 2 additions & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,5 +72,7 @@
"Programming Language :: Python :: 3.9",
"Programming Language :: Python :: 3.10",
"Programming Language :: Python :: 3.11",
"Programming Language :: Python :: 3.12",
"Programming Language :: Python :: 3.13",
],
)