From 92c974c3aea9584fdb103a8e37aa03bca3bd86f9 Mon Sep 17 00:00:00 2001 From: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> Date: Thu, 5 Sep 2024 12:04:02 +0100 Subject: [PATCH 1/8] Fix: Azure Fusion missing authentication if accountKey is provided Changes logic of Azure Environment set up: 1. Is there an account name? (no: error) 2. Is there an accountKey or an accountSas or Managed identity? (no: error) 3. If there is a managed identity yes: return no: Create or add a SAS -> return Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> --- .../cloud/azure/fusion/AzFusionEnv.groovy | 6 ++--- .../cloud/azure/fusion/AzFusionEnvTest.groovy | 22 +++++++++++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy b/plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy index 9e3d90212a..806da2441c 100644 --- a/plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy +++ b/plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy @@ -48,13 +48,13 @@ class AzFusionEnv implements FusionEnv { // the account name is always required result.AZURE_STORAGE_ACCOUNT = cfg.storage().accountName - // If a Managed Identity or Service Principal is configured, Fusion only needs to know the account name - if (cfg.managedIdentity().isConfigured() || cfg.activeDirectory().isConfigured()) { + // If a Managed Identity is configured, Fusion only needs to know the account name + if (cfg.managedIdentity().isConfigured()) { return result } // If a SAS token is configured, instead, Fusion also requires the token value - if (cfg.storage().sasToken) { + if (cfg.storage().sasToken || cfg.storage().accountKey) { result.AZURE_STORAGE_SAS_TOKEN = cfg.storage().getOrCreateSasToken() } diff --git a/plugins/nf-azure/src/test/nextflow/cloud/azure/fusion/AzFusionEnvTest.groovy b/plugins/nf-azure/src/test/nextflow/cloud/azure/fusion/AzFusionEnvTest.groovy index f1942abb88..cc60195b8d 100644 --- a/plugins/nf-azure/src/test/nextflow/cloud/azure/fusion/AzFusionEnvTest.groovy +++ b/plugins/nf-azure/src/test/nextflow/cloud/azure/fusion/AzFusionEnvTest.groovy @@ -61,6 +61,27 @@ class AzFusionEnvTest extends Specification { } + def 'should return env environment with SAS token config when accountKey is provided'() { + given: + Global.session = Mock(Session) { + getConfig() >> [azure: [storage: [accountName: 'myaccount', accountKey: 'myaccountkey']]] + } + def mockStorageObject = Mock(Object) { + getOrCreateSasToken() >> 'generatedSasToken' + } + def config = Mock(FusionConfig) { + storage() >> mockStorageObject + } + + when: + def env = new AzFusionEnv().getEnvironment('az', config) + + then: + env.AZURE_STORAGE_ACCOUNT == 'myaccount' + env.AZURE_STORAGE_SAS_TOKEN + env.size() == 2 + } + def 'should return env environment with SAS token config'() { given: Global.session = Mock(Session) { @@ -99,4 +120,5 @@ class AzFusionEnvTest extends Specification { then: thrown(IllegalArgumentException) } + } From 89710df9a20ce4814471582b482726bcb468b962 Mon Sep 17 00:00:00 2001 From: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> Date: Thu, 5 Sep 2024 12:09:33 +0100 Subject: [PATCH 2/8] Catch when service principal exists but no keys or SAS Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> --- .../src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy b/plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy index 806da2441c..a5aa4fda7d 100644 --- a/plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy +++ b/plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy @@ -54,7 +54,7 @@ class AzFusionEnv implements FusionEnv { } // If a SAS token is configured, instead, Fusion also requires the token value - if (cfg.storage().sasToken || cfg.storage().accountKey) { + if (cfg.storage().sasToken || cfg.storage().accountKey || cfg.activeDirectory().isConfigured()) { result.AZURE_STORAGE_SAS_TOKEN = cfg.storage().getOrCreateSasToken() } From a6006f0485adc4f53d3b5759a2981fb5d560b0c4 Mon Sep 17 00:00:00 2001 From: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> Date: Thu, 5 Sep 2024 12:25:09 +0100 Subject: [PATCH 3/8] Mock AzStorageOpts Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> --- .../test/nextflow/cloud/azure/fusion/AzFusionEnvTest.groovy | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/plugins/nf-azure/src/test/nextflow/cloud/azure/fusion/AzFusionEnvTest.groovy b/plugins/nf-azure/src/test/nextflow/cloud/azure/fusion/AzFusionEnvTest.groovy index cc60195b8d..0ea5a9187c 100644 --- a/plugins/nf-azure/src/test/nextflow/cloud/azure/fusion/AzFusionEnvTest.groovy +++ b/plugins/nf-azure/src/test/nextflow/cloud/azure/fusion/AzFusionEnvTest.groovy @@ -21,6 +21,8 @@ import nextflow.Global import nextflow.Session import nextflow.SysEnv import nextflow.fusion.FusionConfig +import nextflow.cloud.azure.config.AzStorageOpts + import spock.lang.Specification /** @@ -66,7 +68,7 @@ class AzFusionEnvTest extends Specification { Global.session = Mock(Session) { getConfig() >> [azure: [storage: [accountName: 'myaccount', accountKey: 'myaccountkey']]] } - def mockStorageObject = Mock(Object) { + def mockStorageObject = Mock(AzStorageOpts) { getOrCreateSasToken() >> 'generatedSasToken' } def config = Mock(FusionConfig) { From 0171fa3952c33e5122f7c73e9bc7d068576644c0 Mon Sep 17 00:00:00 2001 From: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> Date: Thu, 5 Sep 2024 12:27:16 +0100 Subject: [PATCH 4/8] Improve comment for Fusion env with SAS Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> --- .../src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy b/plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy index a5aa4fda7d..642a9a80e6 100644 --- a/plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy +++ b/plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy @@ -53,7 +53,7 @@ class AzFusionEnv implements FusionEnv { return result } - // If a SAS token is configured, instead, Fusion also requires the token value + // If Fusion does not use a managed identity, get or create a SAS token for Fusion to use if (cfg.storage().sasToken || cfg.storage().accountKey || cfg.activeDirectory().isConfigured()) { result.AZURE_STORAGE_SAS_TOKEN = cfg.storage().getOrCreateSasToken() } From 8b92f8f47da7a62b00bb5d2b57cc309888f7cfab Mon Sep 17 00:00:00 2001 From: Adam Talbot <12817534+adamrtalbot@users.noreply.github.com> Date: Thu, 5 Sep 2024 13:55:36 +0100 Subject: [PATCH 5/8] Update plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy Signed-off-by: Adam Talbot <12817534+adamrtalbot@users.noreply.github.com> --- .../src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy b/plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy index 642a9a80e6..85c34e52a9 100644 --- a/plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy +++ b/plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy @@ -53,7 +53,9 @@ class AzFusionEnv implements FusionEnv { return result } - // If Fusion does not use a managed identity, get or create a SAS token for Fusion to use + // When the Azure machine has a managed identity, it can use that to authenticate to Azure Storage without any keys + // If not using a managed identity, use the existing SAS token or create one using the Nextflow authentication (key, service principal) + // This SAS token confers time limited access to Azure Storage. It is added to the Fusion environment if (cfg.storage().sasToken || cfg.storage().accountKey || cfg.activeDirectory().isConfigured()) { result.AZURE_STORAGE_SAS_TOKEN = cfg.storage().getOrCreateSasToken() } From f11d62ddeccb0aa304c826a65a486dbe9fd07cac Mon Sep 17 00:00:00 2001 From: Alberto Miranda Date: Fri, 6 Sep 2024 17:16:42 +0200 Subject: [PATCH 6/8] Ensure proper SAS tokens are generated for each Azure authentication method Signed-off-by: Alberto Miranda --- .../cloud/azure/config/AzStorageOpts.groovy | 6 - .../cloud/azure/fusion/AzFusionEnv.groovy | 51 +++++--- .../cloud/azure/fusion/AzFusionEnvTest.groovy | 109 +++++++++++++++--- 3 files changed, 128 insertions(+), 38 deletions(-) diff --git a/plugins/nf-azure/src/main/nextflow/cloud/azure/config/AzStorageOpts.groovy b/plugins/nf-azure/src/main/nextflow/cloud/azure/config/AzStorageOpts.groovy index 101c22bf4e..d3e6bb3259 100644 --- a/plugins/nf-azure/src/main/nextflow/cloud/azure/config/AzStorageOpts.groovy +++ b/plugins/nf-azure/src/main/nextflow/cloud/azure/config/AzStorageOpts.groovy @@ -62,10 +62,4 @@ class AzStorageOpts { } return result } - - synchronized String getOrCreateSasToken() { - if( !sasToken ) - sasToken = AzHelper.generateAccountSas(accountName, accountKey, tokenDuration) - return sasToken - } } diff --git a/plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy b/plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy index 85c34e52a9..4888636e4c 100644 --- a/plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy +++ b/plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy @@ -17,49 +17,72 @@ package nextflow.cloud.azure.fusion +import groovy.util.logging.Slf4j +import nextflow.Global +import nextflow.cloud.azure.batch.AzHelper import groovy.transform.CompileStatic import nextflow.cloud.azure.config.AzConfig import nextflow.fusion.FusionConfig import nextflow.fusion.FusionEnv import org.pf4j.Extension + /** * Implement environment provider for Azure specific variables - * + * * @author Paolo Di Tommaso */ @Extension @CompileStatic +@Slf4j class AzFusionEnv implements FusionEnv { @Override Map getEnvironment(String scheme, FusionConfig config) { - if (scheme != 'az') + if (scheme != 'az') { return Collections. emptyMap() + } final cfg = AzConfig.config final result = new LinkedHashMap(10) - if (!cfg.storage().accountName) + if (!cfg.storage().accountName) { throw new IllegalArgumentException("Missing Azure Storage account name") + } - if (cfg.storage().accountKey && cfg.storage().sasToken) + if (cfg.storage().accountKey && cfg.storage().sasToken) { throw new IllegalArgumentException("Azure Storage Access key and SAS token detected. Only one is allowed") + } - // the account name is always required result.AZURE_STORAGE_ACCOUNT = cfg.storage().accountName + // TODO: In theory, generating an impromptu SAS token for authentication methods other than + // `azure.storage.sasToken` should not be necessary, because those methods should already allow sufficient + // access for normal operation. Nevertheless, #5287 heavily implies that failing to do so causes the Azure + // Storage plugin or Fusion to fail. In any case, it may be possible to remove this in the future. + result.AZURE_STORAGE_SAS_TOKEN = getOrCreateSasToken() + + return result + } + + /** + * Return the SAS token if it is defined in the configuration, otherwise generate one based on the requested + * authentication method. + */ + synchronized String getOrCreateSasToken() { - // If a Managed Identity is configured, Fusion only needs to know the account name - if (cfg.managedIdentity().isConfigured()) { - return result + final cfg = AzConfig.config + + // If a SAS token is already defined in the configuration, just return it + if (cfg.storage().sasToken) { + return cfg.storage().sasToken } - // When the Azure machine has a managed identity, it can use that to authenticate to Azure Storage without any keys - // If not using a managed identity, use the existing SAS token or create one using the Nextflow authentication (key, service principal) - // This SAS token confers time limited access to Azure Storage. It is added to the Fusion environment - if (cfg.storage().sasToken || cfg.storage().accountKey || cfg.activeDirectory().isConfigured()) { - result.AZURE_STORAGE_SAS_TOKEN = cfg.storage().getOrCreateSasToken() + // For Active Directory and Managed Identity, we cannot generate an *account* SAS token, but we can generate + // a *container* SAS token for the work directory. + if (cfg.activeDirectory().isConfigured() || cfg.managedIdentity().isConfigured()) { + return AzHelper.generateContainerSasWithActiveDirectory(Global.session.workDir, cfg.storage().tokenDuration) } - return result + // Shared Key authentication can use an account SAS token + return AzHelper.generateAccountSasWithAccountKey(Global.session.workDir, cfg.storage().tokenDuration) } } diff --git a/plugins/nf-azure/src/test/nextflow/cloud/azure/fusion/AzFusionEnvTest.groovy b/plugins/nf-azure/src/test/nextflow/cloud/azure/fusion/AzFusionEnvTest.groovy index 0ea5a9187c..79774e9a9a 100644 --- a/plugins/nf-azure/src/test/nextflow/cloud/azure/fusion/AzFusionEnvTest.groovy +++ b/plugins/nf-azure/src/test/nextflow/cloud/azure/fusion/AzFusionEnvTest.groovy @@ -21,7 +21,6 @@ import nextflow.Global import nextflow.Session import nextflow.SysEnv import nextflow.fusion.FusionConfig -import nextflow.cloud.azure.config.AzStorageOpts import spock.lang.Specification @@ -48,43 +47,117 @@ class AzFusionEnvTest extends Specification { env == Collections.emptyMap() } - def 'should return env environment'() { + def 'should return env environment with SAS token config when accountKey is provided'() { given: + def NAME = 'myaccount' + def KEY = 'myaccountkey' Global.session = Mock(Session) { - getConfig() >> [azure: [storage: [accountName: 'x1']]] + getConfig() >> [azure: [storage: [accountName: NAME, accountKey: KEY]]] } - and: when: def config = Mock(FusionConfig) - def env = new AzFusionEnv().getEnvironment('az', config) - then: - env == [AZURE_STORAGE_ACCOUNT: 'x1'] + def fusionEnv = Spy(AzFusionEnv) + 1 * fusionEnv.getOrCreateSasToken() >> 'generatedSasToken' + def env = fusionEnv.getEnvironment('az', config) + then: + env.AZURE_STORAGE_ACCOUNT == NAME + env.AZURE_STORAGE_SAS_TOKEN + env.size() == 2 } - def 'should return env environment with SAS token config when accountKey is provided'() { + def 'should return env environment with SAS token config when a Service Principal is provided'() { given: + def NAME = 'myaccount' + def CLIENT_ID = 'myclientid' + def CLIENT_SECRET = 'myclientsecret' + def TENANT_ID = 'mytenantid' Global.session = Mock(Session) { - getConfig() >> [azure: [storage: [accountName: 'myaccount', accountKey: 'myaccountkey']]] + getConfig() >> [ + azure: [ + activeDirectory: [ + servicePrincipalId: CLIENT_ID, + servicePrincipalSecret: CLIENT_SECRET, + tenantId: TENANT_ID + ], + storage: [ + accountName: NAME + ] + ] + ] } - def mockStorageObject = Mock(AzStorageOpts) { - getOrCreateSasToken() >> 'generatedSasToken' + + when: + def config = Mock(FusionConfig) + def fusionEnv = Spy(AzFusionEnv) + 1 * fusionEnv.getOrCreateSasToken() >> 'generatedSasToken' + def env = fusionEnv.getEnvironment('az', config) + + then: + env.AZURE_STORAGE_ACCOUNT == NAME + env.AZURE_STORAGE_SAS_TOKEN == 'generatedSasToken' + env.size() == 2 + } + + def 'should return env environment with SAS token config when a user-assigned Managed Identity is provided'() { + given: + def NAME = 'myaccount' + def CLIENT_ID = 'myclientid' + Global.session = Mock(Session) { + getConfig() >> [ + azure: [ + managedIdentity: [ + clientId: CLIENT_ID, + ], + storage: [ + accountName: NAME + ] + ] + ] } - def config = Mock(FusionConfig) { - storage() >> mockStorageObject + + when: + def config = Mock(FusionConfig) + def fusionEnv = Spy(AzFusionEnv) + 1 * fusionEnv.getOrCreateSasToken() >> 'generatedSasToken' + def env = fusionEnv.getEnvironment('az', config) + + then: + env.AZURE_STORAGE_ACCOUNT == NAME + env.AZURE_STORAGE_SAS_TOKEN == 'generatedSasToken' + env.size() == 2 + } + + def 'should return env environment with SAS token config when a system-assigned Managed Identity is provided'() { + given: + def NAME = 'myaccount' + Global.session = Mock(Session) { + getConfig() >> [ + azure: [ + managedIdentity: [ + system: true + ], + storage: [ + accountName: NAME + ] + ] + ] } when: - def env = new AzFusionEnv().getEnvironment('az', config) + def config = Mock(FusionConfig) + def fusionEnv = Spy(AzFusionEnv) + 1 * fusionEnv.getOrCreateSasToken() >> 'generatedSasToken' + def env = fusionEnv.getEnvironment('az', config) then: - env.AZURE_STORAGE_ACCOUNT == 'myaccount' - env.AZURE_STORAGE_SAS_TOKEN + env.AZURE_STORAGE_ACCOUNT == NAME + env.AZURE_STORAGE_SAS_TOKEN == 'generatedSasToken' env.size() == 2 } - - def 'should return env environment with SAS token config'() { + + def 'should return env environment with SAS token config when a sasToken is provided'() { given: Global.session = Mock(Session) { getConfig() >> [azure: [storage: [accountName: 'x1', sasToken: 'y1']]] From dd20318f6e7597a04759ec837584521cbb2204d0 Mon Sep 17 00:00:00 2001 From: Paolo Di Tommaso Date: Mon, 16 Sep 2024 11:04:09 +0200 Subject: [PATCH 7/8] test [platform stage] Signed-off-by: Paolo Di Tommaso From 61374f81dcae6daa0c206d4d2b0dd0414b004794 Mon Sep 17 00:00:00 2001 From: Paolo Di Tommaso Date: Mon, 16 Sep 2024 11:14:47 +0200 Subject: [PATCH 8/8] test [platform stage] Signed-off-by: Paolo Di Tommaso