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.

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

Introduce [AWS/GS/AZURE]_NO_CREDENTIALS as a more user-friendly name … #11472

Closed
wants to merge 1 commit into from

Conversation

rouault
Copy link
Member

@rouault rouault commented Dec 10, 2024

…instead of [AWS/GS/AZURE]_NO_SIGN_REQUEST

@rouault rouault added this to the 3.11.0 milestone Dec 10, 2024
@hobu
Copy link
Contributor

hobu commented Dec 10, 2024

This idea came from me. Error reports with AWS_NO_SIGN_REQUEST in them are not particularly descriptive and doe not provide an implicit remedy, whereas AWS_NO_CREDENTIALS=YES gives the user a clue (despite the double negative).

@tbonfort
Copy link
Member

tbonfort commented Dec 10, 2024

nitpick: removing the double negative with the same stone would be nice. suggestions: AWS_AUTHENTICATED=NO , AWS_USE_CREDENTIALS=NO, AWS_ANONYMOUS=YES. My favorite would be AWS_UNAUTHENTICATED=YES but I'm too weary of handing out a stick to be beaten with given my initial take against double negatives

@hobu
Copy link
Contributor

hobu commented Dec 10, 2024

removing the double negative with the same stone would be nice

I agree, but then it is effectively a new option, not a synonym.

@sgillies
Copy link
Contributor

sgillies commented Dec 10, 2024

Instead of creating a new synonym that also has all the bad qualities of a double negative, can we not make unsigned requests be implicit if no credentials are given? Am I missing something? I've forgotten why we needed "no sign request" in the first place.

@rouault
Copy link
Member Author

rouault commented Dec 10, 2024

I've forgotten why we needed "no sign request" in the first place.

presumably because that's the behavior of aws CLI

$ mv ~/.aws ~/.aws.old
$ aws s3 cp s3://prd-tnm/StagedProducts/Elevation/OPR/Projects/CA_Klamath_Bathymetry_2018/TIFF/USGS_OPR_CA_Klamath_Bathymetry_2018_tb_c_10TFM015735.tif .
fatal error: Unable to locate credentials
$ aws s3 cp s3://prd-tnm/StagedProducts/Elevation/OPR/Projects/CA_Klamath_Bathymetry_2018/TIFF/USGS_OPR_CA_Klamath_Bathymetry_2018_tb_c_10TFM015735.tif . --no-sign-request
download: s3://prd-tnm/StagedProducts/Elevation/OPR/Projects/CA_Klamath_Bathymetry_2018/TIFF/USGS_OPR_CA_Klamath_Bathymetry_2018_tb_c_10TFM015735.tif to ./USGS_OPR_CA_Klamath_Bathymetry_2018_tb_c_10TFM015735.tif

The risk of defaulting to unauthenticated requests is that users might have a harder time figuring out why their requests that require authentication don't work.
A potential compromise could be to default to unauthenticated requests with a CPLDebug() message (was wondering if CE_Warning might not be preferable ?)

@dbaston
Copy link
Member

dbaston commented Dec 10, 2024

A potential compromise could be to default to unauthenticated requests

It is feasible to default to unauthenticated requests, and then append a hint like "Do you need to set authentication?" to error messages? Or would this involve touching the code in too many places?

@tbonfort
Copy link
Member

I agree it is customary for s3/gs requests to be signed by default, and unauthenticated buckets are usually advertised through their plain https endpoints. I would prefer an explicit gdal error before emitting the request when no credentials are found, with a helpful message indicating that AWS_NO_SIGN_REQUEST should be explicitly set if accessing a public bucket. yes I know this is a double negative, but it's the pendant to aws cli's flag. gsutil will emit an anonymous request when no credentials are found, but when using the official APIs anonymous request must be explicitly enabled.

@rouault
Copy link
Member Author

rouault commented Dec 10, 2024

It is feasible to default to unauthenticated requests, and then append a hint like "Do you need to set authentication?" to error messages? Or would this involve touching the code in too many places?

that's not so terrible: attempt at https://github.com/rouault/gdal/pull/new/aws_default_to_unauthenticated

$ gdalinfo /vsis3/my_bucket/byte.tif
ERROR 15: HTTP response code on https://my_bucket.s3.eu-central-1.amazonaws.com/byte.tif: 403. No AWS credentials detected. You may need to set AWS_SECRET_ACCESS_KEY and other related configuration options, or set /home/even/.aws/credentials. Consult https://gdal.org/en/stable/user/virtual_file_systems.html#vsis3-aws-s3-files for more details.

However we do not seem to reach a consensus on what to do here. Probably do nothing and keep existing behavior, with tiny improvements in the error message, is the most reasonable outcome for now.

@rouault rouault marked this pull request as draft December 10, 2024 18:23
@rouault
Copy link
Member Author

rouault commented Dec 10, 2024

Probably do nothing and keep existing behavior, with tiny improvements in the error message

==> cf #11474

@rouault rouault closed this Dec 10, 2024
@coveralls
Copy link
Collaborator

Coverage Status

coverage: 69.879% (-0.02%) from 69.897%
when pulling 5ed1d7b on rouault:AWS_NO_CREDENTIALS
into 1ee024f on OSGeo:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants