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

fix: Azure Fusion env misses credentials when no key or SAS provided #5287

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,4 @@ class AzStorageOpts {
}
return result
}

synchronized String getOrCreateSasToken() {
if( !sasToken )
sasToken = AzHelper.generateAccountSas(accountName, accountKey, tokenDuration)
return sasToken
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,47 +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 <[email protected]>
*/
@Extension
@CompileStatic
@Slf4j
class AzFusionEnv implements FusionEnv {

@Override
Map<String, String> getEnvironment(String scheme, FusionConfig config) {
if (scheme != 'az')
if (scheme != 'az') {
return Collections.<String, String> 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()

// If a Managed Identity or Service Principal is configured, Fusion only needs to know the account name
if (cfg.managedIdentity().isConfigured() || cfg.activeDirectory().isConfigured()) {
return result
}
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() {

final cfg = AzConfig.config

// If a SAS token is configured, instead, Fusion also requires the token value
// If a SAS token is already defined in the configuration, just return it
if (cfg.storage().sasToken) {
result.AZURE_STORAGE_SAS_TOKEN = cfg.storage().getOrCreateSasToken()
return cfg.storage().sasToken
}

return result
// 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)
}

// Shared Key authentication can use an account SAS token
return AzHelper.generateAccountSasWithAccountKey(Global.session.workDir, cfg.storage().tokenDuration)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import nextflow.Global
import nextflow.Session
import nextflow.SysEnv
import nextflow.fusion.FusionConfig

import spock.lang.Specification

/**
Expand All @@ -46,22 +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)
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 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: [
activeDirectory: [
servicePrincipalId: CLIENT_ID,
servicePrincipalSecret: CLIENT_SECRET,
tenantId: TENANT_ID
],
storage: [
accountName: NAME
]
]
]
}

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
]
]
]
}

when:
def config = Mock(FusionConfig)
def fusionEnv = Spy(AzFusionEnv)
1 * fusionEnv.getOrCreateSasToken() >> 'generatedSasToken'
def env = fusionEnv.getEnvironment('az', config)

then:
env == [AZURE_STORAGE_ACCOUNT: 'x1']
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 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'() {
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']]]
Expand Down Expand Up @@ -99,4 +195,5 @@ class AzFusionEnvTest extends Specification {
then:
thrown(IllegalArgumentException)
}

}
Loading