-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Add shared access signature authentication support #42117
Conversation
Pinging @elastic/es-distributed |
Jenkins run elasticsearch-ci/bwc |
I see this labelled as v7.2.0 and v8.0.0 - can we please examine 6.x and 7.1.x for this too? |
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 think this looks fine in general :) But two things:
- I think we can make this a lot simpler code wise as suggested in-line by not having the credentials interface.
- We need to add a third party test for this imo. If you need help with this, let me know in Slack and I'm happy to give you some pointers or code it up myself real quick (we'll also need infra's help here after we've coded it up).
|
||
package org.elasticsearch.repositories.azure; | ||
|
||
public interface AzureCredentials { |
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.
@slider It seems to me we're building the connection string from final fields only in AzureStorageSettings
. Can't we simply build that string in the constructor of AzureStorageSettings
and put it in a final String
field instead? It seems to me that would save a lot of code?
We could just build the string in https://github.com/elastic/elasticsearch/pull/42117/files#diff-fa5785f3bb417953978dce2a4d63bbebR202 where we currently set up the instances of this interface and it would be simpler all around wouldn't it?
Side note: I think a back port to 6.x+ should be safe here (IMO). If we simplify the code the way I suggested above this is a really short and isolated change. |
* Added setting for SAS token * Added support for the token in tests * Relates elastic#42117
* Added setting for SAS token * Added support for the token in tests * Relates #42117
) * Added setting for SAS token * Added support for the token in tests * Relates elastic#42117
) (elastic#43618) * Added setting for SAS token * Added support for the token in tests * Relates elastic#42117
The current default authentication scheme for the azure repository plugin are the account name/key credentials of a storage account. This is basically "root" access for all the containers/blobs within a storage account and makes it hard to safely seperate multiple tenants within a storage account.
This change adds support for shared access signatures, which allow to control access on container or blob level instead of only having service level authentication.
To achieve that I introduced a new config setting (
sas_token
) and had to move the connection String methods around a bit.Since I last used Java about a decade ago I'm happy about any feedback regarding the code style, I tried my best ;)