-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Conversation
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+
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() ); |
There was a problem hiding this comment.
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?
There was a problem hiding this 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?
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() ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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() ); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
Thank you!
Do you mean two layers from the same bucket but with different options?
Sounds sensible, I'll do that
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. |
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)
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 |
Sounds good! I'll close this pr for now and reopen a reworked one including ogr provider support + gui |
Actually I don't think we can do this -- otherwise we'd have issues with this scenario:
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? |
no, they aren't, so yes we cannot unset them |
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.