Skip to content
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

[WIP] Add SSL/TLS support to SCP #175

Closed
wants to merge 7 commits into from
Closed

Conversation

cancan101
Copy link
Contributor

@cancan101 cancan101 commented Jun 5, 2018

Reference issue

Add SSL/TLS support to the SCP functionality of ApplicationEntity.
Alternative to: #71
I want to get feedback on the API before fixing tests.

Tasks

  • Unit tests added that reproduce issue or prove feature is working
  • Fix or feature added
  • Documentation updated (if relevant)
  • Unit tests passing after adding fix/feature
  • Apps updated and tested (if relevant)

/CC @fedjo @scaramallion

@@ -250,14 +279,30 @@ def _setup_argparser():
LOGGER.debug('echoscu.py v%s %s', VERSION, '2017-02-04')
LOGGER.debug('')

# Check SSL/TLS options
sslargs = dict()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrap with enable_tls for backward compat

open('keyfile', 'a').close()
certfile_path = 'certfile'
keyfile_path = 'keyfile'
ae.add_ssl(certfile=certfile_path, keyfile=keyfile_path)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix usage of add_ssl in tests.

# pylint: disable=too-many-instance-attributes,too-many-public-methods
def __init__(self, ae_title='PYNETDICOM', port=0, scu_sop_class=None,
scp_sop_class=None, transfer_syntax=None):
scp_sop_class=None, transfer_syntax=None, tls_args=None):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should tls_args be on __init__ or associate?

Copy link
Contributor Author

@cancan101 cancan101 Jun 5, 2018

Choose a reason for hiding this comment

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

One oddity with API is that server port in on init (and is set to 0 for clients) where as client port (ie connect to port) is set on associate. We could duplicate TLS params likewise, though that seems silly.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer it be a keyword arg for AE.associate() and AE.start().

I've been thinking about changing the AE init to remove port and having it as an associate keyword with a default of 0 and a start arg.

@@ -338,6 +344,18 @@ def _monitor_socket(self):
# If theres a connection
if read_list:
client_socket, _ = self.local_socket.accept()
if self.tls_args is not None:
try:
client_socket = ssl.wrap_socket(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should this be wrapped on the local_socket before selecting / accepting?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants