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.

Sign up for GitHub

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

Jump to bottom

Improved handling of self-signed certs when uploading with known URL #80

Merged
merged 1 commit into from
Jun 5, 2023
Merged

Improved handling of self-signed certs when uploading with known URL #80

merged 1 commit into from
Jun 5, 2023

Conversation

jasper-vdhoven
Copy link
Contributor

Problem

When following the example in the README to upload data to a known files path, an exception will be raised if the server uses a self-signed certificate. Passing the option verify_tls_cert=False as input to the Uploader constructor does not apply this setting to all the functions involved, thus triggering an exception.

Solution

The culprit for this exception is two-fold. First is the positioning of self.verify_tls_cert = verify_tls_cert. In the current position this configuration will not be reached before an exception is raised; moving this line to the top resolves this.

        self.file_path = file_path
        self.file_stream = file_stream
        self.stop_at = self.get_file_size()
        self.client = client
        self.metadata = metadata or {}
        self.metadata_encoding = metadata_encoding
        self.store_url = store_url
        self.url_storage = url_storage
        self.fingerprinter = fingerprinter or fingerprint.Fingerprint()
        self.offset = 0
        self.url = None
        self.__init_url_and_offset(url)
        self.chunk_size = chunk_size
        self.verify_tls_cert = verify_tls_cert
        self.retries = retries
        self.request = None
        self._retried = 0
        self.retry_delay = retry_delay
        self.upload_checksum = upload_checksum
        self.__checksum_algorithm_name, self.__checksum_algorithm = \
            self.CHECKSUM_ALGORITHM_PAIR

https://github.com/tus/tus-py-client/blob/d231499b63a08783fedfca0eccda428b22724b48/tusclient/uploader/baseuploader.py#LL117C1-L137C41

The second issue is in the HEAD request made by the baseuploader.py script. The made HEAD request lacks the addition of the verify_tls_cert option defined in the Constructor. Adding this configuration to the HEAD request resolves this and makes it function similar to PATCH requests made in similar fashion.

        resp = requests.head(self.url, headers=self.get_headers())

https://github.com/tus/tus-py-client/blob/d231499b63a08783fedfca0eccda428b22724b48/tusclient/uploader/baseuploader.py#LL175C8-L175C67

Tests

I have tried running the present tests via python setup.py test and all the tests have passed for this change.

Expand for tests 
(.venv311) ❯ python setup.py test    
running test
WARNING: Testing via this command is deprecated and will be removed in a future version. Users looking for a generic test entry point independent of test runner are encouraged to use tox.
/home/adev/Dev/tus-py-client/.venv311/lib64/python3.11/site-packages/setuptools/installer.py:27: SetuptoolsDeprecationWarning: setuptools.installer is deprecated. Requirements should be satisfied by a PEP 517 installer.
  warnings.warn(
running egg_info
writing tuspy.egg-info/PKG-INFO
writing dependency_links to tuspy.egg-info/dependency_links.txt
writing requirements to tuspy.egg-info/requires.txt
writing top-level names to tuspy.egg-info/top_level.txt
reading manifest file 'tuspy.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
adding license file 'LICENSE'
adding license file 'AUTHORS'
writing manifest file 'tuspy.egg-info/SOURCES.txt'
running build_ext
test_upload (tests.test_async_uploader.AsyncUploaderTest.test_upload) ... /home/adev/Dev/tus-py-client/tusclient/uploader/baseuploader.py:120: ResourceWarning: unclosed file <_io.BufferedReader name='./LICENSE'>
  self.stop_at = self.get_file_size()
ResourceWarning: Enable tracemalloc to get the object allocation traceback
/home/adev/Dev/tus-py-client/tusclient/uploader/uploader.py:108: ResourceWarning: unclosed file <_io.BufferedReader name='./LICENSE'>
  self.stop_at = stop_at or self.get_file_size()
ResourceWarning: Enable tracemalloc to get the object allocation traceback
/home/adev/Dev/tus-py-client/tests/test_async_uploader.py:55: ResourceWarning: unclosed file <_io.BufferedReader name='./LICENSE'>
  self.async_uploader.get_file_size())
ResourceWarning: Enable tracemalloc to get the object allocation traceback
ok
test_upload_chunk (tests.test_async_uploader.AsyncUploaderTest.test_upload_chunk) ... /usr/lib64/python3.11/ipaddress.py:1315: ResourceWarning: unclosed file <_io.BufferedReader name='./LICENSE'>
  self._ip = self._ip_int_from_string(addr_str)
ResourceWarning: Enable tracemalloc to get the object allocation traceback
ok
test_upload_retry (tests.test_async_uploader.AsyncUploaderTest.test_upload_retry) ... ok
test_upload_verify_tls_cert (tests.test_async_uploader.AsyncUploaderTest.test_upload_verify_tls_cert) ... ok
test_async_uploader (tests.test_client.TusClientTest.test_async_uploader) ... ok
test_instance_attributes (tests.test_client.TusClientTest.test_instance_attributes) ... ok
test_set_headers (tests.test_client.TusClientTest.test_set_headers) ... ok
test_uploader (tests.test_client.TusClientTest.test_uploader) ... ok
test_set_get_remove_item (tests.test_filestorage.FileStorageTest.test_set_get_remove_item) ... ok
test_get_fingerpint (tests.test_fingerprint.FileStorageTest.test_get_fingerpint) ... ok
test_unique_fingerprint (tests.test_fingerprint.FileStorageTest.test_unique_fingerprint) ... ok
test_perform (tests.test_request.TusRequestTest.test_perform) ... ok
/usr/lib64/python3.11/unittest/suite.py:107: ResourceWarning: unclosed file <_io.BufferedReader name='./LICENSE'>
  for index, test in enumerate(self):
ResourceWarning: Enable tracemalloc to get the object allocation traceback
test_perform_checksum (tests.test_request.TusRequestTest.test_perform_checksum) ... /usr/lib64/python3.11/unittest/case.py:579: ResourceWarning: unclosed file <_io.BufferedReader name='./LICENSE'>
  if method() is not None:
ResourceWarning: Enable tracemalloc to get the object allocation traceback
ok
test_verify_tls_cert (tests.test_request.TusRequestTest.test_verify_tls_cert) ... ok
/usr/lib64/python3.11/unittest/suite.py:84: ResourceWarning: unclosed file <_io.BufferedReader name='./LICENSE'>
  return self.run(*args, **kwds)
ResourceWarning: Enable tracemalloc to get the object allocation traceback
test_create_url_absolute (tests.test_uploader.UploaderTest.test_create_url_absolute) ... /home/adev/Dev/tus-py-client/tusclient/uploader/baseuploader.py:150: ResourceWarning: unclosed file <_io.BufferedReader name='./LICENSE'>
  headers['upload-length'] = str(self.get_file_size())
ResourceWarning: Enable tracemalloc to get the object allocation traceback
ok
test_create_url_relative (tests.test_uploader.UploaderTest.test_create_url_relative) ... ok
test_encode_metadata (tests.test_uploader.UploaderTest.test_encode_metadata) ... ok
test_encode_metadata_utf8 (tests.test_uploader.UploaderTest.test_encode_metadata_utf8) ... ok
test_file_size (tests.test_uploader.UploaderTest.test_file_size) ... /home/adev/Dev/tus-py-client/tests/test_uploader.py:103: ResourceWarning: unclosed file <_io.BufferedReader name='./LICENSE'>
  self.assertEqual(self.uploader.get_file_size(), os.path.getsize(self.uploader.file_path))
ResourceWarning: Enable tracemalloc to get the object allocation traceback
ok
test_get_file_stream (tests.test_uploader.UploaderTest.test_get_file_stream) ... ok
test_get_offset (tests.test_uploader.UploaderTest.test_get_offset) ... ok
test_headers (tests.test_uploader.UploaderTest.test_headers) ... ok
test_instance_attributes (tests.test_uploader.UploaderTest.test_instance_attributes) ... ok
test_request_length (tests.test_uploader.UploaderTest.test_request_length) ... /home/adev/Dev/tus-py-client/tests/test_uploader.py:87: ResourceWarning: unclosed file <_io.BufferedReader name='./LICENSE'>
  self.uploader.chunk_size = self.uploader.get_file_size() + 3000
ResourceWarning: Enable tracemalloc to get the object allocation traceback
/home/adev/Dev/tus-py-client/tests/test_uploader.py:88: ResourceWarning: unclosed file <_io.BufferedReader name='./LICENSE'>
  self.assertEqual(self.uploader.get_request_length(), self.uploader.get_file_size())
ResourceWarning: Enable tracemalloc to get the object allocation traceback
ok
test_upload (tests.test_uploader.UploaderTest.test_upload) ... /home/adev/Dev/tus-py-client/tusclient/uploader/uploader.py:36: ResourceWarning: unclosed file <_io.BufferedReader name='./LICENSE'>
  self.stop_at = stop_at or self.get_file_size()
ResourceWarning: Enable tracemalloc to get the object allocation traceback
/home/adev/Dev/tus-py-client/tests/test_uploader.py:124: ResourceWarning: unclosed file <_io.BufferedReader name='./LICENSE'>
  self.assertEqual(self.uploader.offset, self.uploader.get_file_size())
ResourceWarning: Enable tracemalloc to get the object allocation traceback
ok
test_upload_checksum (tests.test_uploader.UploaderTest.test_upload_checksum) ... /home/adev/Dev/tus-py-client/tests/test_uploader.py:145: ResourceWarning: unclosed file <_io.BufferedReader name='./LICENSE'>
  self.assertEqual(self.uploader.offset, self.uploader.get_file_size())
ResourceWarning: Enable tracemalloc to get the object allocation traceback
ok
test_upload_chunk (tests.test_uploader.UploaderTest.test_upload_chunk) ... ok
test_upload_retry (tests.test_uploader.UploaderTest.test_upload_retry) ... ok
test_url (tests.test_uploader.UploaderTest.test_url) ... /home/adev/Dev/tus-py-client/.venv311/lib64/python3.11/site-packages/responses/__init__.py:218: ResourceWarning: unclosed file <_io.TextIOWrapper name='/home/adev/Dev/tus-py-client/tests/storage_file' mode='r+' encoding='UTF-8'>
  return func(*args, **kwargs)
ResourceWarning: Enable tracemalloc to get the object allocation traceback
ok

----------------------------------------------------------------------
Ran 29 tests in 19.421s

OK

@jasper-vdhoven
moved self.verif_tls_cert configuration to the top of the list. In th…
168e3e4 
…e current position an exception will be raised before the configuration can be applied to avoid raising said exception. The aforementioned self.verify_tls_cert has also been added to the HEAD request made with the underlying requests library.
Acconut
Acconut approved these changes Jun 5, 2023
View reviewed changes
Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

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

Thank you very for this PR and your explanation!

@Acconut Acconut merged commit 4eb426c into tus:main Jun 5, 2023
@jasper-vdhoven jasper-vdhoven deleted the self-signed-cert-handling branch June 7, 2023 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Acconut Acconut Acconut approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jasper-vdhoven @Acconut