github/azure-sdk-for-cpp
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Create separate lists of characters that are allowed within a tenant id vs scopes in AzureCliCredential. (#5086)

Browse Source 
* Create separate lists of characters that are allowed within a tenant id vs scopes in AzureCliCredential.

* Update test to catch the particular exception we expect to be thrown for
tenant id but not for scopes.

* Address PR feedback.
This commit is contained in:
Ahson Khan 2023-11-01 19:41:50 -07:00 committed by GitHub
parent 677a1da61e
commit 82de63902d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 160 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
sdk/identity/azure-identity/CHANGELOG.md
View File

@ -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

5
sdk/identity/azure-identity/inc/azure/identity/azure_cli_credential.hpp
View File

@ -74,7 +74,10 @@ namespace Azure { namespace Identity {
DateTime::duration cliProcessTimeout,
std::vector<std::string> 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:
/**

30
sdk/identity/azure-identity/src/azure_cli_credential.cpp
View File

@ -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())

146
sdk/identity/azure-identity/test/ut/azure_cli_credential_test.cpp
View File

@ -344,19 +344,33 @@ TEST(AzureCliCredential, UnsafeChars)
}
}
TEST(AzureCliCredential, SpaceNotAllowed)
class ParameterizedTestForDisallowedChars : public ::testing::TestWithParam<std::string> {
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<void>(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<void>(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<std::string> {
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<void>(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<std::string> {
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\","