port number added to AKV urls breaks things

[reythia]

Describe the bug

When driver attempts to retrieve encryption key from Azure Key Vault it passes a key identifier url plus port number causing unhandled failures where checks expect only a host name.

Example: https://xxx.vault.azure.net:443/keys/xxxxxxx/xxxxxxxxxxxxxxxxxxxx

First bug:

akv.KeyProvider.AllowedLocations check fails if following the documentation (host without port)

Constrain the provider to an allowed list of key vaults by appending vault host strings like "mykeyvault.vault.azure.net" to akv.KeyProvider.AllowedLocations.

if strings.HasSuffix(strings.ToLower(url.Host), strings.ToLower(l)) {
        allowed = true
        break loop
    }

Temporary workaround: include port number in location: akv.KeyProvider.AllowedLocations = append(akv.KeyProvider.AllowedLocations, "xxx.vault.azure.net:443")

Second bug:

Panic at akv > keyprovider.go:225 because r.Key returns an unhandled nil

Cause: akv > keyprovider.go:274 > getAKVClient() azkeys.NewClient(endpoint, credential, nil)

Where endpoint again includes port ie https://xxx.vault.azure.net:443 and again the check relies on looking for a suffix which the port number breaks:

if !strings.HasSuffix(req.URL.Host, "."+parsed.Host) {
    return &challengePolicyError{err: fmt.Errorf(challengeMatchError, scope)}
}

Temporary workaround update getAKVClient():

options := &azkeys.ClientOptions{
    DisableChallengeResourceVerification: true,
}
client, err = azkeys.NewClient(endpoint, credential, options)

To Reproduce

OpenDB then try to access an Always Encrypted column secured with keys from Azure Key Vault

config, err := azuread.NewConnector(connString)
config.RegisterCekProvider(mssql.AzureKeyVaultKeyProvider, &akv.KeyProvider)
db := sql.OpenDB(config)
...
query code

Expected behavior Column key should be retrieved from vault to proceed with encryption/decryption. Code should error not panic if a key can not be retrieved

Other Azure SQL Server github.com/microsoft/go-mssqldb v1.7.0

[shueybubbles]

@reythia thx for opening an issue. If we were to change the test case that sets the master key path to include the port number in the vaultURL , then fixed the code so the test passes, would that likely cover your issue?

I'm not sure this scenario affects many customers, though. Is there any AKV SDK that emits URLs with the port number in them? I wouldn't expect the port to be included in such URLs commonly.