Allow to send headers to proxied documents #42

marcospri · 2022-12-20T15:06:21Z

For:

Proxy D2L files from Via lms#4768

Testing

Follow the instructions over: hypothesis/lms#4792

src/h_vialib/client.py

marcospri · 2022-12-21T13:29:57Z

src/h_vialib/client.py

@@ -56,7 +57,10 @@ def __init__(self, secret, service_url=None, html_service_url=None):
            "via.external_link_mode": "new-tab",
        }

-    def url_for(self, url, content_type=None, options=None, blocked_for=None):
+    # pylint:disable=too-many-arguments


I reckon this might keep growing and have a similar interface to requests.request.

Not a problem IMO.

src/h_vialib/client.py

src/h_vialib/secure/secrets.py

marcospri · 2022-12-21T13:32:51Z

src/h_vialib/secure/secrets.py

+
+        return json.loads(self._decrypt(aes_iv, cipher))
+
+    def _decrypt(self, aes_iv, encrypted) -> bytes:


Making these private. We might need them in the future but as this is a library we want to keep the interface under control.

So the existing SecureToken and SecureURL classes aren't appropriate for this use-case? I guess not because it looks like SecureURL only signs the URL doesn't encrypt it.

I wonder if we should rename some of these classes, to clarify what they do and don't do? For example perhaps SecureURL should be SignedURL if it just signs and doesn't encrypt. I think SecureSecrets encrypts a dict but doesn't sign it, so perhaps EncryptedDict?

I think we should probably stop using the words "secret" and "secure" because it's not clear exactly what they mean, and maybe instead say "signed" and "encrypted".

EncryptedDict is a better name 👍.

I wrote this as EncryptedDict but from the callers perspetive it looked like it was as dict subclass supporting encryption. It didn't feel right.

I renamed the class to a more explicit Encryption and keep the method names.

tests/unit/h_vialib/secure/secrets_test.py

seanh

Posting some initial questions. I want to come back and take a closer look at the crypto code in here when I have more time, just to double-check...

.cookiecutter/includes/setuptools/install_requires

src/h_vialib/client.py

seanh · 2022-12-22T11:07:38Z

src/h_vialib/client.py

+    def url_for(
+        self, url, content_type=None, options=None, blocked_for=None, headers=None
+    ):


headers are only supported by certain Via endpoints, right? The ones that proxy in Python, not the ones that proxy in NGINX. Are there some url's where url_for(url, headers=my_headers) will return a URL containing headers that Via will actually ignore because it's going to proxy that URL through NGINX?

will return a URL containing headers that Via will actually ignore because it's going to proxy that URL through NGINX?

Yes, all HTML ones and pdfs not proxied by python (so all of them except one drive and d2l ones).

Applying the headers to everything will be a big tasks (not sure even if possible without writing nginx lua code).

Maybe we could just explain the limitation on the method's docstring?

Added a comment about this. Not ideal but naming this headers_for_python_proxing is not better.

Maybe we could make Via throw some sort of "not implemented" error if any headers are passed to it and it would have proxied the request through NGINX rather than through Python?

seanh · 2022-12-22T11:19:29Z

src/h_vialib/secure/secrets.py

+
+        return json.loads(self._decrypt(aes_iv, cipher))
+
+    def _decrypt(self, aes_iv, encrypted) -> bytes:


So the existing SecureToken and SecureURL classes aren't appropriate for this use-case? I guess not because it looks like SecureURL only signs the URL doesn't encrypt it.

I wonder if we should rename some of these classes, to clarify what they do and don't do? For example perhaps SecureURL should be SignedURL if it just signs and doesn't encrypt. I think SecureSecrets encrypts a dict but doesn't sign it, so perhaps EncryptedDict?

I think we should probably stop using the words "secret" and "secure" because it's not clear exactly what they mean, and maybe instead say "signed" and "encrypted".

.cookiecutter/includes/setuptools/install_requires

marcospri · 2023-01-09T10:03:24Z

src/h_vialib/secure/encryption.py

+    def __init__(self, secret: bytes):
+        self._secret = secret.ljust(32)[:32]
+
+    def encrypt_dict(self, payload: dict) -> str:


Using JWE simplified this a lot.

src/h_vialib/secure/token.py

tests/unit/h_vialib/secure/token_test.py

seanh

This all looks good to me. Made a comment below about potentially wanting to make Via (not h-vialib) error if headers are sent for an NGINX or Via HTML-proxied URL where headers aren't supported

seanh · 2023-01-10T12:41:50Z

src/h_vialib/client.py

@@ -56,7 +57,10 @@ def __init__(self, secret, service_url=None, html_service_url=None):
            "via.external_link_mode": "new-tab",
        }

-    def url_for(self, url, content_type=None, options=None, blocked_for=None):
+    # pylint:disable=too-many-arguments


seanh · 2023-01-10T12:43:21Z

src/h_vialib/client.py

+    def url_for(
+        self, url, content_type=None, options=None, blocked_for=None, headers=None
+    ):


Maybe we could make Via throw some sort of "not implemented" error if any headers are passed to it and it would have proxied the request through NGINX rather than through Python?

Allows to encrypt dictionaries using AES and encode both the cipher text and IV in a base64 encoded string.

Take an optional `headers` parameters which wild be sent AES encoded over the wire to Via.

Instead of our own mix of base64,AES,iv... just use JWE for the same functionality.

marcospri marked this pull request as ready for review December 21, 2022 11:51

marcospri force-pushed the secret-headers branch from 1ae4ec7 to 2671b1a Compare December 21, 2022 13:25

marcospri mentioned this pull request Dec 21, 2022

Proxy D2L files using python hypothesis/via#845

Merged

marcospri changed the title ~~[WIP]Secret headers~~ Allow to send headers to proxied PDFs Dec 21, 2022

marcospri changed the title ~~Allow to send headers to proxied PDFs~~ Allow to send headers to proxied documents Dec 21, 2022

marcospri commented Dec 21, 2022

View reviewed changes

marcospri mentioned this pull request Dec 21, 2022

Suppport for D2L files proxing using Via hypothesis/lms#4792

Merged

marcospri requested a review from seanh December 21, 2022 14:02

seanh reviewed Dec 22, 2022

View reviewed changes

marcospri force-pushed the secret-headers branch 2 times, most recently from edb9587 to 2aa15de Compare January 9, 2023 10:08

marcospri commented Jan 9, 2023

View reviewed changes

marcospri force-pushed the secret-headers branch from 2aa15de to 4c4d797 Compare January 9, 2023 10:18

marcospri changed the base branch from main to replace-jwt-library January 9, 2023 10:18

marcospri requested a review from seanh January 9, 2023 10:35

seanh approved these changes Jan 10, 2023

View reviewed changes

Base automatically changed from replace-jwt-library to main January 11, 2023 08:49

marcospri mentioned this pull request Jan 11, 2023

Make Via throw some "not implemented" error if any headers are passed to it and it would have proxied the request through NGINX hypothesis/via#856

Open

marcospri added 4 commits January 11, 2023 09:52

Secret AES encryption helper

49f2617

Allows to encrypt dictionaries using AES and encode both the cipher text and IV in a base64 encoded string.

Take headers in url_for

c4e99fb

Take an optional `headers` parameters which wild be sent AES encoded over the wire to Via.

Rename SecureSecrets to Encryption

a7c7b0e

Switch over JWEs

744dbf0

Instead of our own mix of base64,AES,iv... just use JWE for the same functionality.

marcospri force-pushed the secret-headers branch from 4c4d797 to 744dbf0 Compare January 11, 2023 08:52

marcospri merged commit fab19b4 into main Jan 11, 2023

marcospri deleted the secret-headers branch January 11, 2023 08:54

marcospri mentioned this pull request Feb 27, 2024

Accept a query parameters for encrypted query values #66

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to send headers to proxied documents #42

Allow to send headers to proxied documents #42

marcospri commented Dec 20, 2022 •

edited

Loading

marcospri Dec 21, 2022

seanh Jan 10, 2023

marcospri Dec 21, 2022

seanh Dec 22, 2022

marcospri Dec 22, 2022

marcospri Jan 9, 2023

seanh left a comment

seanh Dec 22, 2022

marcospri Dec 22, 2022

marcospri Jan 9, 2023

seanh Jan 10, 2023

seanh Dec 22, 2022

marcospri Jan 9, 2023

seanh left a comment

seanh Jan 10, 2023

seanh Jan 10, 2023


		return json.loads(self._decrypt(aes_iv, cipher))

		def _decrypt(self, aes_iv, encrypted) -> bytes:

Allow to send headers to proxied documents #42

Allow to send headers to proxied documents #42

Conversation

marcospri commented Dec 20, 2022 • edited Loading

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcospri commented Dec 20, 2022 •

edited

Loading