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

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

Merged
merged 6 commits into from
Dec 3, 2023
Merged

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

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
81cfcde
feat: try anon=True if no credentials are supplied or found.
Nov 14, 2023
2b09181
refactor: add logging statement and comment for credentials resolver.
Nov 20, 2023
1e8db83
Make creds lookup conditional
martindurant Nov 21, 2023
6804fde
fix
martindurant Nov 21, 2023
396ffee
Add simple test
martindurant Dec 3, 2023
2988be8
fix
martindurant Dec 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions s3fs/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
from aiobotocore.config import AioConfig
from botocore.exceptions import ClientError, HTTPClientError, ParamValidationError
from botocore.parsers import ResponseParserError
from botocore.credentials import create_credential_resolver

from s3fs.errors import translate_boto_error
from s3fs.utils import S3BucketRegionCache, ParamKwargsHelper, _get_brange, FileExpired
Expand Down Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I am suggesting

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

print("""No credentials were given and thus anonymous access was tried and failed. Please provide
credentials.""")

raise err


Expand Down Expand Up @@ -464,6 +471,19 @@ async def set_session(self, refresh=False, kwargs={}):
return self._s3
logger.debug("Setting up s3fs instance")

if self.session is None:
self.session = aiobotocore.session.AioSession(**self.kwargs)

cred_resolver = create_credential_resolver(self.session, region_name=self.session._last_client_region_used)
credentials = cred_resolver.load_credentials()
BENR0 marked this conversation as resolved.
Show resolved Hide resolved

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
BENR0 marked this conversation as resolved.
Show resolved Hide resolved
else:
self.key = credentials.access_key
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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'

self.secret = credentials.secret_key
self.token = credentials.token

client_kwargs = self.client_kwargs.copy()
init_kwargs = dict(
aws_access_key_id=self.key,
Expand All @@ -479,6 +499,7 @@ async def set_session(self, refresh=False, kwargs={}):
if "use_ssl" not in client_kwargs.keys():
init_kwargs["use_ssl"] = self.use_ssl
config_kwargs = self._prepare_config_kwargs()

if self.anon:
from botocore import UNSIGNED

Expand All @@ -498,8 +519,6 @@ async def set_session(self, refresh=False, kwargs={}):
config_kwargs["signature_version"] = UNSIGNED

conf = AioConfig(**config_kwargs)
if self.session is None:
self.session = aiobotocore.session.AioSession(**self.kwargs)

for parameters in (config_kwargs, self.kwargs, init_kwargs, client_kwargs):
for option in ("region_name", "endpoint_url"):
Expand Down
Loading