Try anon=True if no credentials are supplied or found #823

BENR0 · 2023-11-14T09:03:34Z

This PR tries to use anon=True if no credentials are supplied or found in config files

The print statement should probably be changed to a warning also help with the tests would be appreciated.

Closes ENH: Eliminate the need for anon=True when you might be opening a public bucket & object. #675
Tests added
Fully documented

martindurant

Actually, it looks simpler that I has feared.

You should be able to make a test by monkeypatch on ProfileProviderBuilder.load_credentials() to make sure anon gets selected.

s3fs/core.py

martindurant · 2023-11-14T14:56:47Z

s3fs/core.py

@@ -137,6 +138,12 @@ async def _error_wrapper(func, *, args=(), kwargs=None, retries):
        except Exception as e:
            err = e
    err = translate_boto_error(err)
+
+    s3 = func.__self__
+    if isinstance(err, PermissionError) and s3._client_config.signature_version == botocore.UNSIGNED:


Other exceptions might be possible, such as credentials that exist but have expired. I'll test this later (since our corp account has expiring tokens).
Is s3._client_config.signature_version == botocore.UNSIGNED guaranteed to be the same as self.anon ?

Were you able to test this?

I am not sure if anon is guaranteed to be the same as botocore.UNSIGNED but as far as I could see it seems like it. It would be nice though if somebody with more experience with botocore can confirm this.

I am suggesting

Suggested change

if isinstance(err, PermissionError) and s3._client_config.signature_version == botocore.UNSIGNED:

if isinstance(err, PermissionError) and self.anon:

s3fs/core.py

martindurant · 2023-11-14T14:59:40Z

s3fs/core.py

+        if credentials is None and self.key is None and self.secret is None and self.token is None and not self.anon:
+            self.anon = True
+        else:
+            self.key = credentials.access_key


Is there a reason to set these values? They will never get used again.

I added this because the values are used a couple of lines below in the init_kwargs dict. I think it should also work without because somewhere down the line it should be tried to load the credentials from configs like I did above with the credential_resolver but my reasoning was that credentials get loaded from configs here they can directly be supplied and don't need to be checked again later with the credentials resolver.

Oh wait, but what if the user supplied key/secret/token as kwargs, didn't they get lost here? The kwargs passed to Session above don't include these.

Actually, is anon=True and there really are no creds, we get to this path too, and

AttributeError: 'NoneType' object has no attribute 'access_key'

martindurant

I'm going to look into this...

martindurant · 2023-11-20T14:31:52Z

s3fs/core.py

+        if credentials is None and self.key is None and self.secret is None and self.token is None and not self.anon:
+            self.anon = True
+        else:
+            self.key = credentials.access_key


Oh wait, but what if the user supplied key/secret/token as kwargs, didn't they get lost here? The kwargs passed to Session above don't include these.

martindurant · 2023-11-20T18:00:17Z

s3fs/core.py

+        if credentials is None and self.key is None and self.secret is None and self.token is None and not self.anon:
+            self.anon = True
+        else:
+            self.key = credentials.access_key


Actually, is anon=True and there really are no creds, we get to this path too, and

AttributeError: 'NoneType' object has no attribute 'access_key'

martindurant · 2023-11-21T15:07:40Z

I ended up only doing the lookup when we are certain the user has no creds of their own. How does this look?

BENR0 · 2023-11-23T12:56:15Z

Yes I think that should work. Nice refactor of the PermissionError :-).

So only unittests missing now right?

…#823)" This reverts commit 8a87309.

* Revert "Don't assume attributes after connect (#832)" This reverts commit cc6663f. * Revert "Try anon=True if no credentials are supplied or found (#823)" This reverts commit 8a87309.

bsipocz · 2024-06-08T01:42:09Z

I was so happy to see this feature, but then saw that it got reverted. So just leave this message, that we run into this same problem, too. (But I expect most of the anon=True cases can be hidden away in the client library rather than telling our users to directly work with the parquet files)

martindurant · 2024-06-08T16:51:52Z

I'm sorry: unfortunately, it led to far more problems than it solved. I'd be happy to see a more solid implementation, if some wants to try.

BENR0 · 2024-06-12T07:28:08Z

@bsipocz I still really would like this also. I am willing to work on this again but I think it seems like I might not have enough knowledge to foresee possible problems as was apparent. I someone is willing to step in and point me in the right direction it would be appreciated. I think the solution itself is not really bad but needs some more refinement.

feat: try anon=True if no credentials are supplied or found.

81cfcde

BENR0 mentioned this pull request Nov 14, 2023

ENH: Eliminate the need for anon=True when you might be opening a public bucket & object. #675

Closed

martindurant reviewed Nov 14, 2023

View reviewed changes

refactor: add logging statement and comment for credentials resolver.

2b09181

martindurant reviewed Nov 20, 2023

View reviewed changes

Make creds lookup conditional

1e8db83

fix

6804fde

martindurant added 2 commits December 2, 2023 20:51

Add simple test

396ffee

fix

2988be8

martindurant merged commit 8a87309 into fsspec:main Dec 3, 2023
21 checks passed

BENR0 deleted the feat_anon_true_default branch December 4, 2023 09:33

nicornk mentioned this pull request Dec 5, 2023

Change introduced in "Try anon=True if no credentials are supplied or found" broke connection to private S3 compatible storage with custom AioSession and AioDeferredRefreshableCredentials. #834

Closed

martindurant added a commit to martindurant/s3fs that referenced this pull request Dec 5, 2023

Revert "Try anon=True if no credentials are supplied or found (fsspec…

58e34c2

…#823)" This reverts commit 8a87309.

martindurant mentioned this pull request Dec 5, 2023

Revert anon fallback #835

Merged

pgarrison mentioned this pull request Jul 24, 2024

Read from public s3:// paths without authentication, without requiring any code change from users bioio-devs/bioio#61

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try anon=True if no credentials are supplied or found #823

Try anon=True if no credentials are supplied or found #823

BENR0 commented Nov 14, 2023

martindurant left a comment

martindurant Nov 14, 2023

BENR0 Nov 20, 2023

martindurant Nov 20, 2023

martindurant Nov 14, 2023

BENR0 Nov 20, 2023

martindurant Nov 20, 2023

martindurant Nov 20, 2023

martindurant left a comment

martindurant Nov 20, 2023

martindurant Nov 20, 2023

martindurant commented Nov 21, 2023

BENR0 commented Nov 23, 2023

bsipocz commented Jun 8, 2024

martindurant commented Jun 8, 2024

BENR0 commented Jun 12, 2024

	if isinstance(err, PermissionError) and s3._client_config.signature_version == botocore.UNSIGNED:
	if isinstance(err, PermissionError) and self.anon:

Try anon=True if no credentials are supplied or found #823

Try anon=True if no credentials are supplied or found #823

Conversation

BENR0 commented Nov 14, 2023

martindurant 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

Choose a reason for hiding this comment

martindurant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martindurant commented Nov 21, 2023

BENR0 commented Nov 23, 2023

bsipocz commented Jun 8, 2024

martindurant commented Jun 8, 2024

BENR0 commented Jun 12, 2024