diff --git a/sdk/identity/azure-identity/CHANGELOG.md b/sdk/identity/azure-identity/CHANGELOG.md index ad04a3931..c8658bb32 100644 --- a/sdk/identity/azure-identity/CHANGELOG.md +++ b/sdk/identity/azure-identity/CHANGELOG.md @@ -12,6 +12,8 @@ ### Other Changes +- Create separate lists of characters that are allowed within tenant ids and scopes in `AzureCliCredential`. + ## 1.6.0-beta.3 (2023-10-12) ### Bugs Fixed diff --git a/sdk/identity/azure-identity/inc/azure/identity/azure_cli_credential.hpp b/sdk/identity/azure-identity/inc/azure/identity/azure_cli_credential.hpp index d04c9909f..b7b563619 100644 --- a/sdk/identity/azure-identity/inc/azure/identity/azure_cli_credential.hpp +++ b/sdk/identity/azure-identity/inc/azure/identity/azure_cli_credential.hpp @@ -74,7 +74,10 @@ namespace Azure { namespace Identity { DateTime::duration cliProcessTimeout, std::vector additionallyAllowedTenants); - void ThrowIfNotSafeCmdLineInput(std::string const& input, std::string const& description) const; + void ThrowIfNotSafeCmdLineInput( + std::string const& input, + std::string const& allowedChars, + std::string const& description) const; public: /** diff --git a/sdk/identity/azure-identity/src/azure_cli_credential.cpp b/sdk/identity/azure-identity/src/azure_cli_credential.cpp index 3c5ab0cb8..87f8f7707 100644 --- a/sdk/identity/azure-identity/src/azure_cli_credential.cpp +++ b/sdk/identity/azure-identity/src/azure_cli_credential.cpp @@ -57,26 +57,20 @@ using Azure::Identity::_detail::TokenCredentialImpl; void AzureCliCredential::ThrowIfNotSafeCmdLineInput( std::string const& input, + std::string const& allowedChars, std::string const& description) const { for (auto const c : input) { - switch (c) + if (allowedChars.find(c) != std::string::npos) { - case ':': - case '/': - case '.': - case '-': - case '_': - break; - - default: - if (!StringExtensions::IsAlphaNumeric(c)) - { - throw AuthenticationException( - GetCredentialName() + ": Unsafe command line input found in " + description + ": " - + input); - } + continue; + } + if (!StringExtensions::IsAlphaNumeric(c)) + { + throw AuthenticationException( + GetCredentialName() + ": Unsafe command line input found in " + description + ": " + + input); } } } @@ -119,8 +113,10 @@ AzureCliCredential::AzureCliCredential(const Core::Credentials::TokenCredentialO std::string AzureCliCredential::GetAzCommand(std::string const& scopes, std::string const& tenantId) const { - ThrowIfNotSafeCmdLineInput(scopes, "Scopes"); - ThrowIfNotSafeCmdLineInput(m_tenantId, "TenantID"); + // The OAuth 2.0 RFC (https://datatracker.ietf.org/doc/html/rfc6749#section-3.3) allows space as + // well for a list of scopes, but that isn't currently required. + ThrowIfNotSafeCmdLineInput(scopes, ".-:/_", "Scopes"); + ThrowIfNotSafeCmdLineInput(m_tenantId, ".-", "TenantID"); std::string command = "az account get-access-token --output json --scope \"" + scopes + "\""; if (!tenantId.empty()) diff --git a/sdk/identity/azure-identity/test/ut/azure_cli_credential_test.cpp b/sdk/identity/azure-identity/test/ut/azure_cli_credential_test.cpp index 6e9f6abd3..670b6d01c 100644 --- a/sdk/identity/azure-identity/test/ut/azure_cli_credential_test.cpp +++ b/sdk/identity/azure-identity/test/ut/azure_cli_credential_test.cpp @@ -344,19 +344,33 @@ TEST(AzureCliCredential, UnsafeChars) } } -TEST(AzureCliCredential, SpaceNotAllowed) +class ParameterizedTestForDisallowedChars : public ::testing::TestWithParam { +protected: + std::string value; +}; + +TEST_P(ParameterizedTestForDisallowedChars, DisallowedCharsForScopeAndTenantId) { - std::string const invalid = "space character"; + std::string const InvalidValue = GetParam(); { AzureCliCredentialOptions options; options.TenantId = "01234567-89AB-CDEF-0123-456789ABCDEF"; - options.TenantId += invalid; + options.TenantId += InvalidValue; AzureCliCredential azCliCred(options); TokenRequestContext trc; trc.Scopes.push_back(std::string("https://storage.azure.com/.default")); EXPECT_THROW(static_cast(azCliCred.GetToken(trc, {})), AuthenticationException); + + try + { + auto const token = azCliCred.GetToken(trc, {}); + } + catch (AuthenticationException const& e) + { + EXPECT_TRUE(std::string(e.what()).find("Unsafe") != std::string::npos) << e.what(); + } } { @@ -365,12 +379,134 @@ TEST(AzureCliCredential, SpaceNotAllowed) AzureCliCredential azCliCred(options); TokenRequestContext trc; - trc.Scopes.push_back(std::string("https://storage.azure.com/.default") + invalid); - + trc.Scopes.push_back(std::string("https://storage.azure.com/.default") + InvalidValue); EXPECT_THROW(static_cast(azCliCred.GetToken(trc, {})), AuthenticationException); + + try + { + auto const token = azCliCred.GetToken(trc, {}); + } + catch (AuthenticationException const& e) + { + EXPECT_TRUE(std::string(e.what()).find("Unsafe") != std::string::npos) << e.what(); + } } } +INSTANTIATE_TEST_SUITE_P( + AzureCliCredential, + ParameterizedTestForDisallowedChars, + ::testing::Values(" ", "|", "`", "\"", "'", ";", "&")); + +class ParameterizedTestForCharDifferences : public ::testing::TestWithParam { +protected: + std::string value; +}; + +TEST_P(ParameterizedTestForCharDifferences, ValidCharsForScopeButNotTenantId) +{ + std::string const ValidScopeButNotTenantId = GetParam(); + + { + AzureCliCredentialOptions options; + options.TenantId = "01234567-89AB-CDEF-0123-456789ABCDEF"; + options.TenantId += ValidScopeButNotTenantId; + AzureCliCredential azCliCred(options); + + TokenRequestContext trc; + trc.Scopes.push_back(std::string("https://storage.azure.com/.default")); + EXPECT_THROW(static_cast(azCliCred.GetToken(trc, {})), AuthenticationException); + + try + { + auto const token = azCliCred.GetToken(trc, {}); + } + catch (AuthenticationException const& e) + { + EXPECT_TRUE(std::string(e.what()).find("Unsafe") != std::string::npos) << e.what(); + } + } + + { + AzureCliCredentialOptions options; + options.CliProcessTimeout = std::chrono::hours(24); + AzureCliCredential azCliCred(options); + + TokenRequestContext trc; + trc.Scopes.push_back( + std::string("https://storage.azure.com/.default") + ValidScopeButNotTenantId); + + // We expect the GetToken to fail, but not because of the unsafe chars. + try + { + auto const token = azCliCred.GetToken(trc, {}); + } + catch (AuthenticationException const& e) + { + EXPECT_TRUE(std::string(e.what()).find("Unsafe") == std::string::npos) << e.what(); + } + } +} + +INSTANTIATE_TEST_SUITE_P( + AzureCliCredential, + ParameterizedTestForCharDifferences, + ::testing::Values(":", "/", "_")); + +class ParameterizedTestForAllowedChars : public ::testing::TestWithParam { +protected: + std::string value; +}; + +TEST_P(ParameterizedTestForAllowedChars, ValidCharsForScopeAndTenantId) +{ + std::string const ValidChars = GetParam(); + + { + AzureCliCredentialOptions options; + options.TenantId = "01234567-89AB-CDEF-0123-456789ABCDEF"; + options.TenantId += ValidChars; + AzureCliCredential azCliCred(options); + + TokenRequestContext trc; + trc.Scopes.push_back(std::string("https://storage.azure.com/.default")); + + // We expect the GetToken to fail, but not because of the unsafe chars. + try + { + auto const token = azCliCred.GetToken(trc, {}); + } + catch (AuthenticationException const& e) + { + EXPECT_TRUE(std::string(e.what()).find("Unsafe") == std::string::npos) << e.what(); + } + } + + { + AzureCliCredentialOptions options; + options.CliProcessTimeout = std::chrono::hours(24); + AzureCliCredential azCliCred(options); + + TokenRequestContext trc; + trc.Scopes.push_back(std::string("https://storage.azure.com/.default") + ValidChars); + + // We expect the GetToken to fail, but not because of the unsafe chars. + try + { + auto const token = azCliCred.GetToken(trc, {}); + } + catch (AuthenticationException const& e) + { + EXPECT_TRUE(std::string(e.what()).find("Unsafe") == std::string::npos) << e.what(); + } + } +} + +INSTANTIATE_TEST_SUITE_P( + AzureCliCredential, + ParameterizedTestForAllowedChars, + ::testing::Values(".", "-", "A", "9")); + TEST(AzureCliCredential, StrictIso8601TimeFormat) { constexpr auto Token = "{\"accessToken\":\"ABCDEFGHIJKLMNOPQRSTUVWXYZ\","