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

Add SAS Token Authentication Support to Azure Repo Plugin #42982

Merged

Conversation

@original-brownbear
Copy link
Member

commented Jun 7, 2019

  • Added setting for SAS token
  • Added support for the token in tests
    • I have manually verified that the setting works with real Azure. I'm not 100% happy with the way I'm passing the SAS token setting to tests here, but I figured this might be the easiest approach for infra. The implemented approach allows to only test either a client with a key set or a client with set SAS token in a single Gradle run. This seemed less cumbersome than allowing to provide two separate sets of credentials and then adding code to Gradle to run the third party test goals twice. Also, for infra this is a reasonable approach I think because they can just copy the existing Jenkins job, remove the key setting and add an SAS token setting in its place in the copy => no effort required there outside of copying the Gradle target.
  • Relates #42117
* Added setting for SAS token
* Added support for the token in tests
* Relates #42117
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Jun 7, 2019

Copy link
Contributor

left a comment

I've left two smaller asks. In general, I think we should move away from the low-level connection string creation and use the objects provided by the SDK for that. We can do that as a follow-up, however, as that will possibly complicate backporting

@@ -53,6 +54,10 @@
public static final AffixSetting<SecureString> KEY_SETTING = Setting.affixKeySetting(AZURE_CLIENT_PREFIX_KEY, "key",
key -> SecureSetting.secureString(key, null));

/** Azure SAS token */
public static final AffixSetting<SecureString> SAS_TOKEN_SETTING = Setting.affixKeySetting(AZURE_CLIENT_PREFIX_KEY, "sas_token",

This comment has been minimized.

Copy link
@ywelsch

ywelsch Jun 11, 2019

Contributor

can you add docs for the new setting as well?

This comment has been minimized.

Copy link
@original-brownbear

original-brownbear Jun 11, 2019

Author Member

Done in b7045d0 :)

@@ -166,7 +184,6 @@ public LocationMode getLocationMode() {
public String toString() {
final StringBuilder sb = new StringBuilder("AzureStorageSettings{");
sb.append("account='").append(account).append('\'');
sb.append(", key='").append(key).append('\'');

This comment has been minimized.

Copy link
@ywelsch

ywelsch Jun 11, 2019

Contributor

add connectString instead here?

This comment has been minimized.

Copy link
@original-brownbear

original-brownbear Jun 11, 2019

Author Member

@ywelsch this was actually an on purpose removal, do we really want to add secure credentials in toString methods?

This comment has been minimized.

Copy link
@tlrx

tlrx Jun 11, 2019

Member

I think it's better to remove the key and maybe add a "use sas token=true|false"

This comment has been minimized.

Copy link
@original-brownbear

original-brownbear Jun 11, 2019

Author Member

Unfortunately, for that we'd have to keep some field that holds this information. Maybe not worth it?

This comment has been minimized.

Copy link
@tlrx
@original-brownbear original-brownbear requested a review from ywelsch Jun 11, 2019
@original-brownbear

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

@ywelsch all addressed :)

Copy link
Contributor

left a comment

LGTM. I would like a second pair of eyes on this (@tlrx or @andrershov) given the backport to 6.8.

Copy link
Member

left a comment

It looks great! I left some minor comments that need to be addressed before merging. I tested it both manually and using the 3rd party tests and it worked.

There's an interesting point about adding multiple CI jobs for 3rd party tests vs. adding more gradle test tasks. I think it make sense here to add another CI job (and this is maybe what we'd like to do for S3 too).

@@ -29,14 +30,14 @@ Note that you can also define more than one account:
bin/elasticsearch-keystore add azure.client.default.account
bin/elasticsearch-keystore add azure.client.default.key
bin/elasticsearch-keystore add azure.client.secondary.account
bin/elasticsearch-keystore add azure.client.secondary.key
bin/elasticsearch-keystore add azure.client.default.sas_token

This comment has been minimized.

Copy link
@tlrx

tlrx Jun 11, 2019

Member

Maybe we could have the default account settings with the secret key and the secondary account configured with SAS (and not interleaving both)?

This comment has been minimized.

Copy link
@original-brownbear

original-brownbear Jun 11, 2019

Author Member

Ah my bad that was just a mistake in copy and pasting, thanks for spotting :)

@@ -85,10 +86,11 @@ task thirdPartyTest(type: Test) {
include '**/AzureStorageCleanupThirdPartyTests.class'
systemProperty 'test.azure.account', azureAccount ? azureAccount : ""
systemProperty 'test.azure.key', azureKey ? azureKey : ""
systemProperty 'test.azure.sas', azureSasToken ? azureSasToken : ""

This comment has been minimized.

Copy link
@tlrx

tlrx Jun 11, 2019

Member

Nit: can we also use sas_token for the system property?

This comment has been minimized.

Copy link
@original-brownbear

original-brownbear Jun 11, 2019

Author Member

Sure :)

@@ -166,7 +184,6 @@ public LocationMode getLocationMode() {
public String toString() {
final StringBuilder sb = new StringBuilder("AzureStorageSettings{");
sb.append("account='").append(account).append('\'');
sb.append(", key='").append(key).append('\'');

This comment has been minimized.

Copy link
@tlrx

tlrx Jun 11, 2019

Member

I think it's better to remove the key and maybe add a "use sas token=true|false"

@@ -29,12 +29,14 @@ String azureAccount = System.getenv("azure_storage_account")
String azureKey = System.getenv("azure_storage_key")
String azureContainer = System.getenv("azure_storage_container")
String azureBasePath = System.getenv("azure_storage_base_path")
String azureSasToken = System.getenv("azure_storage_sas_token")

if (!azureAccount && !azureKey && !azureContainer && !azureBasePath) {

This comment has been minimized.

Copy link
@tlrx

tlrx Jun 11, 2019

Member

I think we need to change this too, in order to accommodate with the 3 situations we now have: no sys props, sys prop with key, sys prop with sas token

println "Using access key in external service tests."
keystore 'azure.client.integration_test.key', azureKey
}
if (azureSasToken.isEmpty() == false) {

This comment has been minimized.

Copy link
@tlrx

tlrx Jun 11, 2019

Member

See previous comment, CI might break here as azureSasToken will be just null?

This comment has been minimized.

Copy link
@original-brownbear

original-brownbear Jun 11, 2019

Author Member

Right ... obviously in my local testing I just set the env vars to "" when switching between auth methods :D and didn't notice.

@@ -19,7 +19,8 @@ bin/elasticsearch-keystore add azure.client.default.account
bin/elasticsearch-keystore add azure.client.default.key
----------------------------------------------------------------

Where `account` is the azure account name and `key` the azure secret key.
Where `account` is the azure account name and `key` the azure secret key. Instead of an azure secret key under `key`, you can alternatively
define a shared access signatures (SAS) token under `sas_token` to use for authentication instead.

This comment has been minimized.

Copy link
@tlrx

tlrx Jun 11, 2019

Member

I think we should say a word about the type of SAS token and the required permissions

This comment has been minimized.

Copy link
@original-brownbear

original-brownbear Jun 11, 2019

Author Member

I think the type doesn't matter just the permissions (service as well as accounts SAS should work)? I added a comment on the permissions now :)

This comment has been minimized.

Copy link
@tlrx

tlrx Jun 11, 2019

Member

Talked to Armin using another channel. I'd expect to see some create or list permissions too here

This comment has been minimized.

Copy link
@original-brownbear

original-brownbear Jun 11, 2019

Author Member

Alright, updated this with all the details now. create we don't need since it's implied by write for the operations we execute https://docs.microsoft.com/en-us/rest/api/storageservices/Constructing-an-Account-SAS?redirectedfrom=MSDN#blob-service (as a matter of fact it is for all operations on the blob service). But we needed list.
Also, I added some detail on the service and resource type the token has to apply to.

@original-brownbear original-brownbear requested a review from tlrx Jun 11, 2019
@original-brownbear

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

Thanks @tlrx all points addressed I think/hope :)

@tlrx
tlrx approved these changes Jun 11, 2019
Copy link
Member

left a comment

LGTM, thanks!

@original-brownbear

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

thanks @tlrx !

@original-brownbear original-brownbear merged commit faffaa9 into elastic:master Jun 11, 2019
9 checks passed
9 checks passed
CLA All commits in pull request signed
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/docs-check Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details
@original-brownbear original-brownbear deleted the original-brownbear:azure-service-key-auth branch Jun 11, 2019
@original-brownbear

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

Just FYI, I raised an infra issue for getting the new CI job going.

@jakelandis jakelandis added v6.8.2 and removed v7.2.0 v6.8.1 labels Jun 17, 2019
@ywelsch ywelsch removed the v7.1.2 label Jun 17, 2019
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Jun 26, 2019
)

* Added setting for SAS token
* Added support for the token in tests
* Relates elastic#42117
original-brownbear added a commit that referenced this pull request Jun 26, 2019
…43618)

* Added setting for SAS token
* Added support for the token in tests
* Relates #42117
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Jul 1, 2019
) (elastic#43618)

* Added setting for SAS token
* Added support for the token in tests
* Relates elastic#42117
original-brownbear added a commit that referenced this pull request Jul 2, 2019
* Add SAS Token Authentication Support to Azure Repo Plugin (#42982) (#43618)

* Added setting for SAS token
* Added support for the token in tests
* Relates #42117
@Mpdreamz Mpdreamz referenced this pull request Aug 7, 2019
16 of 16 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.