feat: add signing of public URL #407

danpoletaev · Feb 19, 2025

This PR is part of Issue #19363, which updates KeyValueStore.getPublicUrl(recordKey) to generate signed links using HMAC.

This PR:

creates signature and appends it to the public URL of the record in KV store

P.S. Before merging, we need to wait for the release of Crawlee, so we can update version of Crawlee.

P.P.S It's hard to test the changes, since master branch of SDK and master branch of Crawlee are out of sync. Crawlee has some breaking changes in version 6.0, which are not yet addressed in master branch of SDK.

Previous PR that adds storageObject to Crawlee Python is here

Same PR in SDK JS is here

danpoletaev · Feb 19, 2025

src/apify/apify_storage_client/_key_value_store_client.py

@@ -90,5 +92,10 @@ async def get_public_url(self, key: str) -> str:
            key: The key for which the URL should be generated.
        """
        public_api_url = self._api_public_base_url
+        public_url = f'{public_api_url}/v2/key-value-stores/{self._client.resource_id}/records/{key}'
+
+        url_signing_secret_key = getattr(self.storage_object, 'url_signing_secret_key', None)  # type: ignore[attr-defined]


I'm a bit worried that url_signing_secret_key isn't being properly transformed from camelCase? In the API response, it shows up as urlSigningSecretKey

vdusek · Feb 24, 2025

@danpoletaev sorry, we've been asked to review, but the CI is broken, could you please fix it first?

danpoletaev · Feb 25, 2025

@danpoletaev sorry, we've been asked to review, but the CI is broken, could you please fix it first?

I'm afraid that I won't be able to fix CI before the release of Crawlee 0.6 version and updating SDK to deal with breaking changes, but I'll not merge it before that

Pijukatel · Feb 27, 2025

src/apify/_crypto.py

@@ -153,3 +155,37 @@ def decrypt_input_secrets(private_key: rsa.RSAPrivateKey, input_data: Any) -> An
                )

    return input_data
+
+
+CHARSET = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ'


Preferably use predefined standard string constants: string.digits+string.ascii_letters
https://docs.python.org/3/library/string.html#string-constants

Pijukatel · Feb 27, 2025

src/apify/_crypto.py

+CHARSET = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ'
+
+
+def encode_base62(num: int) -> str:


This method alone is perfect for writing unit test for. Could you please add one parametrized test with couple of input values.

https://docs.pytest.org/en/stable/how-to/parametrize.html#pytest-mark-parametrize

src/apify/_crypto.py

Pijukatel · Feb 27, 2025

src/apify/apify_storage_client/_key_value_store_client.py

@@ -90,5 +92,10 @@ async def get_public_url(self, key: str) -> str:
            key: The key for which the URL should be generated.
        """
        public_api_url = self._api_public_base_url
+        public_url = f'{public_api_url}/v2/key-value-stores/{self._client.resource_id}/records/{key}'
+
+        url_signing_secret_key = getattr(self.storage_object, 'url_signing_secret_key', None)  # type: ignore[attr-defined]


I am confused about the ignore. I guess it is complaining about self.storage_object in such case we should not ignore it as it could raise an exception in cases where self does not have storage_object

I guess you can remove it and wait for your other change in crawlee to be merged and that should silence the warning.

Pijukatel · Feb 27, 2025

tests/unit/test_crypto.py

@@ -105,3 +112,20 @@ def test_crypto_random_object_id_length_and_charset() -> None:
    long_random_object_id = crypto_random_object_id(1000)
    for char in long_random_object_id:
        assert char in 'abcdefghijklmnopqrstuvwxyzABCEDFGHIJKLMNOPQRSTUVWXYZ0123456789'
+
+
+# Check if the method is compatible with js version of the same method in:


Not sure what is the comment telling the reader. Could you rephrase it please. Currently it looks like a remainder to the author of the change to do something

Pijukatel · Feb 27, 2025

tests/unit/test_crypto.py

+    # This test uses the same secret key and message as in JS tests.
+    secret_key = 'hmac-same-secret-key'
+    message = 'hmac-same-message-to-be-authenticated'
+    for _ in range(5):


Why stress testing it? Is there some non-determinism involved?

There is no non-determinism involved, but I'd rather be sure, that for same input we get same output.
Refactored test to avoid "stress testing"

B4nan · Mar 4, 2025

@vdusek please hold the SDK release till this one is merged, it was blocked by crawlee release (which we will first need to do once apify/crawlee-python#993 is merged)

janbuchar · Mar 4, 2025

src/apify/apify_storage_client/_key_value_store_client.py

+        public_url = f'{public_api_url}/v2/key-value-stores/{self._client.resource_id}/records/{key}'
+
+        url_signing_secret_key = getattr(self.storage_object, 'url_signing_secret_key', None)
+        if url_signing_secret_key:
+            public_url += f'?signature={create_hmac_signature(url_signing_secret_key, key)}'


Could you please use yarl.URL to build the URL? It is already a dependency of Crawlee.

janbuchar · Mar 4, 2025

src/apify/apify_storage_client/_key_value_store_client.py

@@ -90,5 +92,10 @@ async def get_public_url(self, key: str) -> str:
            key: The key for which the URL should be generated.
        """
        public_api_url = self._api_public_base_url
+        public_url = f'{public_api_url}/v2/key-value-stores/{self._client.resource_id}/records/{key}'
+
+        url_signing_secret_key = getattr(self.storage_object, 'url_signing_secret_key', None)


Strictly speaking, there is no guarantee if there is an attribute called url_signing_secret_key on the storage object that it will actually be what you expect. Could you use an isinstance check instead to make sure that the storage_object is of the correct type?

janbuchar · Mar 4, 2025

tests/integration/test_actor_key_value_store.py

-
-            assert record_url == f'{public_api_url}/v2/key-value-stores/{default_store_id}/records/dummy'
+            record_key = 'dummy'
+            record_url = await cast(KeyValueStoreClient, store._resource_client).get_public_url(record_key)


Huh, I don't think we need to access KeyValueStoreClient._resource_client anymore - just KeyValueStore.get_public_url should be enough.

vdusek

LGTM

src/apify/_crypto.py

danpoletaev added 3 commits February 19, 2025 09:03

feat: added create_hmac_signature function

1e2eb11

feat: update get_public_url to sign urls

fb622d2

chore: updated test_generate_public_url_for_kvs_record test

208a253

danpoletaev requested review from janbuchar and vdusek February 19, 2025 09:15

danpoletaev self-assigned this Feb 19, 2025

github-actions bot added this to the 109th sprint - Platform team milestone Feb 19, 2025

github-actions bot added t-core-services Issues with this label are in the ownership of the platform team. tested Temporary label used only programatically for some analytics. labels Feb 19, 2025

danpoletaev commented Feb 19, 2025

View reviewed changes

danpoletaev added 2 commits February 25, 2025 15:49

Merge branch 'master' into feat/sign-public-url

9ce1ae3

fix: import

7f5d373

vdusek requested review from Pijukatel and removed request for vdusek February 25, 2025 15:20

Pijukatel reviewed Feb 27, 2025

View reviewed changes

refactor: clean up

2451a25

danpoletaev requested a review from Pijukatel February 27, 2025 15:48

janbuchar requested changes Mar 4, 2025

View reviewed changes

danpoletaev added 2 commits March 5, 2025 14:14

Merge branch 'master' into feat/sign-public-url

101fe41

refactor: clean up

b058ca3

danpoletaev requested a review from janbuchar March 7, 2025 14:37

janbuchar approved these changes Mar 7, 2025

View reviewed changes

vdusek approved these changes Mar 7, 2025

View reviewed changes

src/apify/_crypto.py Outdated Show resolved Hide resolved

Update src/apify/_crypto.py

de1d874

vdusek changed the title ~~feat: sign public url~~ feat: add signing of public URL Mar 7, 2025

vdusek merged commit a865461 into master Mar 7, 2025
28 checks passed

vdusek deleted the feat/sign-public-url branch March 7, 2025 15:58

fnesveda added the validated Issues that are resolved and their solutions fulfill the acceptance criteria. label Mar 10, 2025

		CHARSET = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ'


		def encode_base62(num: int) -> str:

Search code, repositories, users, issues, pull requests...

feat: add signing of public URL #407

feat: add signing of public URL #407

Uh oh!

Conversation

danpoletaev commented Feb 19, 2025

Uh oh!

danpoletaev Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdusek commented Feb 24, 2025

Uh oh!

danpoletaev commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

B4nan commented Mar 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdusek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danpoletaev Feb 19, 2025 •

edited

Loading

danpoletaev commented Feb 25, 2025 •

edited

Loading