feat: add signing of public URL #407

danpoletaev · 2025-02-19T09:15:29Z

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 · 2025-02-19T09:17:52Z

src/apify/apify_storage_client/_key_value_store_client.py

        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 · 2025-02-24T11:37:05Z

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

danpoletaev · 2025-02-25T15:58:23Z

@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 · 2025-02-27T09:00:05Z

src/apify/_crypto.py

    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 · 2025-02-27T09:08:47Z

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 · 2025-02-27T09:19:36Z

src/apify/apify_storage_client/_key_value_store_client.py

        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 · 2025-02-27T09:23:18Z

tests/unit/test_crypto.py

        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 · 2025-02-27T09:24:08Z

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 · 2025-03-04T10:22:55Z

@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 · 2025-03-04T10:57:15Z

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 · 2025-03-04T10:58:59Z

src/apify/apify_storage_client/_key_value_store_client.py

        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 · 2025-03-04T11:02:31Z

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 core services 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

		return input_data


		CHARSET = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ'

		CHARSET = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ'


		def encode_base62(num: int) -> str:

		assert char in 'abcdefghijklmnopqrstuvwxyzABCEDFGHIJKLMNOPQRSTUVWXYZ0123456789'


		# Check if the method is compatible with js version of the same method in:

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