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

[gdal] Allow storing credential key/value credential options in uris #57801

Closed
wants to merge 3 commits into from

Conversation

nyalldawson
Copy link
Collaborator

Extends decode/encodeUri to handle credential options. This is
modeled off the existing support for storing open options.

When credential options are found in a layer's URI, we use
GDAL's VSISetPathSpecificOption to set the credential option
for that VSI driver and bucket. This allows per-vsi driver & bucket
credentials for layers, whereas other approaches (like environment
variable setting) force a single set of credentials to be used
for an entire QGIS session.

Requires GDAL 3.5+

If this approach is acceptable, I'll implement for the OGR provider also.

Extends decode/encodeUri to handle credential options. This is
modeled off the existing support for storing open options.

When credential options are found in a layer's URI, we use
GDAL's VSISetPathSpecificOption to set the credential option
for that VSI driver and bucket. This allows per-vsi driver & bucket
credentials for layers, whereas other approaches (like environment
variable setting) force a single set of credentials to be used
for an entire QGIS session.

Requires GDAL 3.5+
@github-actions github-actions bot added this to the 3.38.0 milestone Jun 19, 2024
Copy link

github-actions bot commented Jun 19, 2024

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit dbe9d3d)

for ( auto it = credentialOptions.constBegin(); it != credentialOptions.constEnd(); ++it )
{
#if GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(3, 6, 0)
VSISetPathSpecificOption( bucket.toLocal8Bit().constData(), it.key().toLocal8Bit().constData(), it.value().toString().toLocal8Bit().constData() );
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rouault does this approach look valid to you?

Copy link
Contributor

@rouault rouault left a comment

Choose a reason for hiding this comment

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

LGTM. There's a potential caveat if we would import 2 layers with conflicting path specific options, but that's more a GDAL design constraint than something QGIS can do about.
Could be worth mentioning as code comment.

I was also wondering if on provider closing we should undo the setting of the path specific options. That could perhaps avoid some "hysteresis" effects

Hum, another aspect is more about security. If users set keys/secrets, they will be stored in plain text in the project. Did you consider using the auth manager?

Comment on lines +309 to +311
VSISetPathSpecificOption( bucket.toLocal8Bit().constData(), it.key().toLocal8Bit().constData(), it.value().toString().toLocal8Bit().constData() );
#elif GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(3, 5, 0)
VSISetCredential( bucket.toLocal8Bit().constData(), it.key().toLocal8Bit().constData(), it.value().toString().toLocal8Bit().constData() );
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
VSISetPathSpecificOption( bucket.toLocal8Bit().constData(), it.key().toLocal8Bit().constData(), it.value().toString().toLocal8Bit().constData() );
#elif GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(3, 5, 0)
VSISetCredential( bucket.toLocal8Bit().constData(), it.key().toLocal8Bit().constData(), it.value().toString().toLocal8Bit().constData() );
VSISetPathSpecificOption( bucket.toUtf8().constData(), it.key().toUtf8().constData(), it.value().toString().toUtf8().constData() );
#elif GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(3, 5, 0)
VSISetCredential( bucket.toUtf8().constData(), it.key().toUtf8().constData(), it.value().toString().toUtf8().constData() );

}
else
{
credentialOptions.insert( match.captured( 1 ), QString() );
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not completely sure if we need to handle a credential that is a key without value. I can't think of a case where it is needed currently. Unless this is meant to remove a path specific option. But in that case VSISetPathSpecificOption() should be called with a nullptr value and not an empty string

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I'll remove this support. (I wasn't sure if in future there'd be valueless options added in gdal)

@nyalldawson
Copy link
Collaborator Author

@rouault

Thank you!

There's a potential caveat if we would import 2 layers with conflicting path specific options, but that's more a GDAL design constraint than something QGIS can do about.
Could be worth mentioning as code comment.

Do you mean two layers from the same bucket but with different options?

I was also wondering if on provider closing we should undo the setting of the path specific options. That could perhaps avoid some "hysteresis" effects

Sounds sensible, I'll do that

Hum, another aspect is more about security. If users set keys/secrets, they will be stored in plain text in the project. Did you consider using the auth manager?

I did, but auth manager comes with some pretty severe user complexity (eg it's complex to share projects which utilise auth config, and you can't publish QLRs for layers requiring auth config). My particular use case is for publicly available datasets, and making these easily accessible to users.

Ideally we'd have both authcfg + plain text support, just like we do for eg wms, postgres, etc. But that's out of scope for my current project.

@rouault
Copy link
Contributor

rouault commented Jun 19, 2024

Do you mean two layers from the same bucket but with different options?

yes. Clearly, not nominal. (note that potential path-specific options can apply to sub-paths in a same bucket, so you could define something for "/vsis3/my_bucket/dir_a" and something else for "/vsis3/my_bucket/dir_b", although I'm not sure if any cloud provider would allow in practice to configure sub-bucket options)

Ideally we'd have both authcfg + plain text support, just like we do for eg wms, postgres, etc. But that's out of scope for my current project.

Fine to me. Was just for the sake of touring the different options. If there's a GUI for this functionality, could perhaps be worth to have a tooltip alerting users about not putting sensitive information, and/or warnings if they try to set path-specific options like AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, GS_SECRET_ACCESS_KEY, GS_ACCESS_KEY_ID, GS_OAUTH2_PRIVATE_KEY, GS_OAUTH2_CLIENT_SECRET, AZURE_STORAGE_CONNECTION_STRING, AZURE_STORAGE_ACCESS_TOKEN, AZURE_STORAGE_ACCESS_KEY, AZURE_STORAGE_SAS_TOKEN, OSS_SECRET_ACCESS_KEY, OSS_ACCESS_KEY_ID, SWIFT_AUTH_TOKEN, SWIFT_KEY

@nyalldawson
Copy link
Collaborator Author

If there's a GUI for this functionality, could perhaps be worth to have a tooltip alerting users about not putting sensitive information, and/or warnings if they try to set path-specific options like

Sounds good!

I'll close this pr for now and reopen a reworked one including ogr provider support + gui

@nyalldawson
Copy link
Collaborator Author

@rouault

I was also wondering if on provider closing we should undo the setting of the path specific options. That could perhaps avoid some "hysteresis" effects

Actually I don't think we can do this -- otherwise we'd have issues with this scenario:

  1. User adds a layer from a bucket, credential options are set
  2. User adds a second layer from same bucket with same credentials
  3. User removes one of the layers -- the credentials are unset on provider destruction
  4. The other remaining layer is now broken -- the credentials won't be around anymore to use for it

Or are matching PathSpecificOptions always copied and stored in the gdal dataset after opening, and we could then safely remove them immediately after the GDALOpenEx call?

@rouault
Copy link
Contributor

rouault commented Jun 20, 2024

Or are matching PathSpecificOptions always copied and stored in the gdal dataset after opening,

no, they aren't, so yes we cannot unset them

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.

None yet

2 participants