From cbfc7638e294085e51f1a80d6e6e6ac7213827dd Mon Sep 17 00:00:00 2001 From: Jenny Tam Date: Tue, 23 Nov 2021 12:52:08 -0800 Subject: [PATCH] Let ODBC driver verify azure ad authentications (#1326) --- source/pdo_sqlsrv/pdo_parser.cpp | 25 -------- source/pdo_sqlsrv/pdo_util.cpp | 8 --- source/pdo_sqlsrv/php_pdo_sqlsrv_int.h | 4 -- source/shared/core_conn.cpp | 64 ++----------------- source/shared/core_sqlsrv.h | 6 -- source/sqlsrv/conn.cpp | 10 --- source/sqlsrv/php_sqlsrv_int.h | 1 - source/sqlsrv/util.cpp | 8 --- .../pdo_azure_ad_authentication.phpt | 18 ------ .../pdo_azure_ad_managed_identity.phpt | 55 ---------------- .../sqlsrv_azure_ad_authentication.phpt | 26 -------- .../sqlsrv_azure_ad_managed_identity.phpt | 31 --------- 12 files changed, 6 insertions(+), 250 deletions(-) diff --git a/source/pdo_sqlsrv/pdo_parser.cpp b/source/pdo_sqlsrv/pdo_parser.cpp index 6d1608933..2574a5ad2 100644 --- a/source/pdo_sqlsrv/pdo_parser.cpp +++ b/source/pdo_sqlsrv/pdo_parser.cpp @@ -173,31 +173,6 @@ void conn_string_parser::validate_key( _In_reads_(key_len) const char *key, _Ino THROW_PDO_ERROR( this->ctx, PDO_SQLSRV_ERROR_INVALID_DSN_KEY, static_cast( key_name ) ); } -void conn_string_parser::add_key_value_pair( _In_reads_(len) const char* value, _In_ int len ) -{ - // if the keyword is 'Authentication', check whether the user specified option is supported - bool valid = true; - if ( stricmp( this->current_key_name, ODBCConnOptions::Authentication ) == 0 ) { - if (len <= 0) - valid = false; - else { - // extract option from the value by len - sqlsrv_malloc_auto_ptr option; - option = static_cast( sqlsrv_malloc( len + 1 ) ); - memcpy_s( option, len + 1, value, len ); - option[len] = '\0'; - - valid = AzureADOptions::isAuthValid(option, len); - } - } - if( !valid ) { - THROW_PDO_ERROR( this->ctx, PDO_SQLSRV_ERROR_INVALID_AUTHENTICATION_OPTION, this->current_key_name ); - } - - string_parser::add_key_value_pair( value, len ); -} - - inline bool sql_string_parser::is_placeholder_char( char c ) { // placeholder only accepts numbers, upper and lower case alphabets and underscore diff --git a/source/pdo_sqlsrv/pdo_util.cpp b/source/pdo_sqlsrv/pdo_util.cpp index cb0b06206..1edec3623 100644 --- a/source/pdo_sqlsrv/pdo_util.cpp +++ b/source/pdo_sqlsrv/pdo_util.cpp @@ -377,10 +377,6 @@ pdo_error PDO_ERRORS[] = { PDO_SQLSRV_ERROR_EMULATE_INOUT_UNSUPPORTED, { IMSSP, (SQLCHAR*) "Statement with emulate prepare on does not support output or input_output parameters.", -72, false } }, - { - PDO_SQLSRV_ERROR_INVALID_AUTHENTICATION_OPTION, - { IMSSP, (SQLCHAR*) "Invalid option for the Authentication keyword. Only SqlPassword, ActiveDirectoryPassword, ActiveDirectoryMsi or ActiveDirectoryServicePrincipal is supported.", -73, false } - }, { SQLSRV_ERROR_CE_DRIVER_REQUIRED, { IMSSP, (SQLCHAR*) "The Always Encrypted feature requires Microsoft ODBC Driver 17 for SQL Server (or above) for %1!s!.", -78, true } @@ -441,10 +437,6 @@ pdo_error PDO_ERRORS[] = { SQLSRV_ERROR_INVALID_DECIMAL_PLACES, { IMSSP, (SQLCHAR*) "Expected an integer to specify number of decimals to format the output values of decimal data types.", -92, false} }, - { - SQLSRV_ERROR_AAD_MSI_UID_PWD_NOT_NULL, - { IMSSP, (SQLCHAR*) "When using ActiveDirectoryMsi Authentication, PWD must be NULL. UID can be NULL, but if not, an empty string is not accepted.", -93, false} - }, { SQLSRV_ERROR_DATA_CLASSIFICATION_PRE_EXECUTION, { IMSSP, (SQLCHAR*) "The statement must be executed to retrieve Data Classification Sensitivity Metadata.", -94, false} diff --git a/source/pdo_sqlsrv/php_pdo_sqlsrv_int.h b/source/pdo_sqlsrv/php_pdo_sqlsrv_int.h index 18a65acb0..f4915de8b 100644 --- a/source/pdo_sqlsrv/php_pdo_sqlsrv_int.h +++ b/source/pdo_sqlsrv/php_pdo_sqlsrv_int.h @@ -149,9 +149,6 @@ class conn_string_parser : private string_parser int discard_trailing_white_spaces( _In_reads_(len) const char* str, _Inout_ int len ); void validate_key( _In_reads_(key_len) const char *key, _Inout_ int key_len); - protected: - void add_key_value_pair( _In_reads_(len) const char* value, _In_ int len); - public: conn_string_parser( _In_ sqlsrv_context& ctx, _In_ const char* dsn, _In_ int len, _In_ HashTable* conn_options_ht ); void parse_conn_string( void ); @@ -391,7 +388,6 @@ enum PDO_ERROR_CODES { PDO_SQLSRV_ERROR_INVALID_OUTPUT_PARAM_TYPE, PDO_SQLSRV_ERROR_INVALID_CURSOR_WITH_SCROLL_TYPE, PDO_SQLSRV_ERROR_EMULATE_INOUT_UNSUPPORTED, - PDO_SQLSRV_ERROR_INVALID_AUTHENTICATION_OPTION, PDO_SQLSRV_ERROR_CE_DIRECT_QUERY_UNSUPPORTED, PDO_SQLSRV_ERROR_CE_EMULATE_PREPARE_UNSUPPORTED, PDO_SQLSRV_ERROR_EXTENDED_STRING_TYPE_INVALID diff --git a/source/shared/core_conn.cpp b/source/shared/core_conn.cpp index f4733fe6d..4fb2caff8 100644 --- a/source/shared/core_conn.cpp +++ b/source/shared/core_conn.cpp @@ -704,41 +704,6 @@ bool core_is_conn_opt_value_escaped( _Inout_ const char* value, _Inout_ size_t v return true; } -namespace AzureADOptions { - enum AAD_AUTH_TYPE { - MIN_AAD_AUTH_TYPE = 0, - SQL_PASSWORD = 0, - AAD_PASSWORD, - AAD_MSI, - AAD_SPA, - MAX_AAD_AUTH_TYPE - }; - - const char *AADAuths[] = { "SqlPassword", "ActiveDirectoryPassword", "ActiveDirectoryMsi", "ActiveDirectoryServicePrincipal" }; - - bool isAuthValid(_In_z_ const char* value, _In_ size_t value_len) - { - if (value_len <= 0) - return false; - - bool isValid = false; - for (short i = MIN_AAD_AUTH_TYPE; i < MAX_AAD_AUTH_TYPE && !isValid; i++) - { - if (!stricmp(value, AADAuths[i])) { - isValid = true; - } - } - - return isValid; - } - - bool isAADMsi(_In_z_ const char* value) - { - return (value != NULL && !stricmp(value, AADAuths[AAD_MSI])); - } -} - - // *** internal connection functions and classes *** namespace { @@ -792,10 +757,11 @@ void build_connection_string_and_set_conn_attr( _Inout_ sqlsrv_conn* conn, _Inou access_token_used = true; } - // Check if Authentication is ActiveDirectoryMSI + // Check if Authentication is ActiveDirectoryMSI because we have to handle this case differently // https://docs.microsoft.com/en-ca/azure/active-directory/managed-identities-azure-resources/overview bool activeDirectoryMSI = false; if (authentication_option_used) { + const char aadMSIoption[] = "ActiveDirectoryMSI"; zval* auth_option = NULL; auth_option = zend_hash_index_find(options, SQLSRV_CONN_OPTION_AUTHENTICATION); @@ -804,34 +770,16 @@ void build_connection_string_and_set_conn_attr( _Inout_ sqlsrv_conn* conn, _Inou option = Z_STRVAL_P(auth_option); } - //if (option != NULL && !stricmp(option, AzureADOptions::AZURE_AUTH_AD_MSI)) { - activeDirectoryMSI = AzureADOptions::isAADMsi(option); - if (activeDirectoryMSI) { - // There are two types of managed identities: - // (1) A system-assigned managed identity: UID must be NULL - // (2) A user-assigned managed identity: UID defined but must not be an empty string - // In both cases, PWD must be NULL - - bool invalid = false; - if (pwd != NULL) { - invalid = true; - } else { - if (uid != NULL && strnlen_s(uid) == 0) { - invalid = true; - } - } - - CHECK_CUSTOM_ERROR(invalid, conn, SQLSRV_ERROR_AAD_MSI_UID_PWD_NOT_NULL ) { - throw core::CoreException(); - } + if (option != NULL && !stricmp(option, aadMSIoption)) { + activeDirectoryMSI = true; } } // Add the server name common_conn_str_append_func( ODBCConnOptions::SERVER, server, strnlen_s( server ), connection_string ); - // If uid is not present then we use trusted connection -- but not when access token or ActiveDirectoryMSI is used, - // because they are incompatible + // If uid is not present then we use trusted connection -- but not when connecting + // using the access token or Authentication is ActiveDirectoryMSI if (!access_token_used && !activeDirectoryMSI) { if (uid == NULL || strnlen_s(uid) == 0) { connection_string += CONNECTION_OPTION_NO_CREDENTIALS; // "Trusted_Connection={Yes};" diff --git a/source/shared/core_sqlsrv.h b/source/shared/core_sqlsrv.h index 8209bf3d0..766b3f457 100644 --- a/source/shared/core_sqlsrv.h +++ b/source/shared/core_sqlsrv.h @@ -190,11 +190,6 @@ const int SQL_SERVER_2005_DEFAULT_DATETIME_SCALE = 3; const int SQL_SERVER_2008_DEFAULT_DATETIME_PRECISION = 34; const int SQL_SERVER_2008_DEFAULT_DATETIME_SCALE = 7; -namespace AzureADOptions { - bool isAuthValid(_In_z_ const char* value, _In_ size_t value_len); - bool isAADMsi(_In_z_ const char* value); -} - // the message returned by ODBC Driver for SQL Server const char ODBC_CONNECTION_BUSY_ERROR[] = "Connection is busy with results for another command"; @@ -2011,7 +2006,6 @@ enum SQLSRV_ERROR_CODES { SQLSRV_ERROR_INVALID_OPTION_WITH_ACCESS_TOKEN, SQLSRV_ERROR_EMPTY_ACCESS_TOKEN, SQLSRV_ERROR_INVALID_DECIMAL_PLACES, - SQLSRV_ERROR_AAD_MSI_UID_PWD_NOT_NULL, SQLSRV_ERROR_DATA_CLASSIFICATION_PRE_EXECUTION, SQLSRV_ERROR_DATA_CLASSIFICATION_NOT_AVAILABLE, SQLSRV_ERROR_DATA_CLASSIFICATION_FAILED, diff --git a/source/sqlsrv/conn.cpp b/source/sqlsrv/conn.cpp index c582c774e..d83f4458f 100644 --- a/source/sqlsrv/conn.cpp +++ b/source/sqlsrv/conn.cpp @@ -1362,16 +1362,6 @@ int get_conn_option_key( _Inout_ sqlsrv_context& ctx, _In_ zend_string* key, _In throw ss::SSException(); } - bool valid = true; - if( stricmp( SS_CONN_OPTS[i].sqlsrv_name, SSConnOptionNames::Authentication ) == 0 ) { - valid = AzureADOptions::isAuthValid(value, value_len); - } - - CHECK_CUSTOM_ERROR( !valid, ctx, SS_SQLSRV_ERROR_INVALID_AUTHENTICATION_OPTION, SS_CONN_OPTS[i].sqlsrv_name ) { - - throw ss::SSException(); - } - break; } case CONN_ATTR_INVALID: diff --git a/source/sqlsrv/php_sqlsrv_int.h b/source/sqlsrv/php_sqlsrv_int.h index 8937e821b..d4011d203 100644 --- a/source/sqlsrv/php_sqlsrv_int.h +++ b/source/sqlsrv/php_sqlsrv_int.h @@ -206,7 +206,6 @@ enum SS_ERROR_CODES { SS_SQLSRV_ERROR_CONNECT_BRACES_NOT_ESCAPED, SS_SQLSRV_ERROR_INVALID_OUTPUT_PARAM_TYPE, SS_SQLSRV_ERROR_PARAM_VAR_NOT_REF, - SS_SQLSRV_ERROR_INVALID_AUTHENTICATION_OPTION, SS_SQLSRV_ERROR_AE_QUERY_SQLTYPE_REQUIRED }; diff --git a/source/sqlsrv/util.cpp b/source/sqlsrv/util.cpp index f7f50f9d5..eaa801e09 100644 --- a/source/sqlsrv/util.cpp +++ b/source/sqlsrv/util.cpp @@ -363,10 +363,6 @@ ss_error SS_ERRORS[] = { "Output or bidirectional variable parameters (SQLSRV_PARAM_OUT and SQLSRV_PARAM_INOUT) passed to sqlsrv_prepare or sqlsrv_query should be passed by reference, not by value." , -61, true } }, - { - SS_SQLSRV_ERROR_INVALID_AUTHENTICATION_OPTION, - { IMSSP, (SQLCHAR*)"Invalid option for the Authentication keyword. Only SqlPassword, ActiveDirectoryPassword, ActiveDirectoryMsi or ActiveDirectoryServicePrincipal is supported.", -62, false } - }, { SS_SQLSRV_ERROR_AE_QUERY_SQLTYPE_REQUIRED, { IMSSP, (SQLCHAR*)"Must specify the SQL type for each parameter in a parameterized query when using sqlsrv_query in a column encryption enabled connection.", -63, false } @@ -429,10 +425,6 @@ ss_error SS_ERRORS[] = { SQLSRV_ERROR_INVALID_DECIMAL_PLACES, { IMSSP, (SQLCHAR*) "Expected an integer to specify number of decimals to format the output values of decimal data types.", -117, false} }, - { - SQLSRV_ERROR_AAD_MSI_UID_PWD_NOT_NULL, - { IMSSP, (SQLCHAR*) "When using ActiveDirectoryMsi Authentication, PWD must be NULL. UID can be NULL, but if not, an empty string is not accepted.", -118, false} - }, { SQLSRV_ERROR_DATA_CLASSIFICATION_PRE_EXECUTION, { IMSSP, (SQLCHAR*) "The statement must be executed to retrieve Data Classification Sensitivity Metadata.", -119, false} diff --git a/test/functional/pdo_sqlsrv/pdo_azure_ad_authentication.phpt b/test/functional/pdo_sqlsrv/pdo_azure_ad_authentication.phpt index 3d5147b53..8171625fd 100644 --- a/test/functional/pdo_sqlsrv/pdo_azure_ad_authentication.phpt +++ b/test/functional/pdo_sqlsrv/pdo_azure_ad_authentication.phpt @@ -34,22 +34,6 @@ if ($stmt === false) { unset($conn); -/////////////////////////////////////////////////////////////////////////////////////////// -// Test Azure AD with integrated authentication. This should fail because -// we don't support it. -// -$connectionInfo = "Authentication = ActiveDirectoryIntegrated; TrustServerCertificate = true;"; - -try { - $conn = new PDO("sqlsrv:server = $server ; $connectionInfo"); - echo "Connected successfully with Authentication=ActiveDirectoryIntegrated.\n"; - unset($conn); -} catch (PDOException $e) { - echo "Could not connect with Authentication=ActiveDirectoryIntegrated.\n"; - print_r($e->getMessage()); - echo "\n"; -} - /////////////////////////////////////////////////////////////////////////////////////////// // Test Azure AD on an Azure database instance. Replace $azureServer, etc with // your credentials to test, or this part is skipped. @@ -95,6 +79,4 @@ if ($azureServer != 'TARGET_AD_SERVER') { --EXPECTF-- Connected successfully with Authentication=SqlPassword. string(1) "%d" -Could not connect with Authentication=ActiveDirectoryIntegrated. -SQLSTATE[IMSSP]: Invalid option for the Authentication keyword. Only SqlPassword, ActiveDirectoryPassword, ActiveDirectoryMsi or ActiveDirectoryServicePrincipal is supported. %s with Authentication=ActiveDirectoryPassword. diff --git a/test/functional/pdo_sqlsrv/pdo_azure_ad_managed_identity.phpt b/test/functional/pdo_sqlsrv/pdo_azure_ad_managed_identity.phpt index 00a0c9507..23271a368 100644 --- a/test/functional/pdo_sqlsrv/pdo_azure_ad_managed_identity.phpt +++ b/test/functional/pdo_sqlsrv/pdo_azure_ad_managed_identity.phpt @@ -18,58 +18,6 @@ function verifyErrorMessage($exception, $expectedError, $msg) } } -function connectWithInvalidOptions() -{ - global $server; - - $message = 'AzureAD Managed Identity test: expected to fail with '; - $expectedError = 'When using ActiveDirectoryMsi Authentication, PWD must be NULL. UID can be NULL, but if not, an empty string is not accepted'; - - $uid = ''; - $connectionInfo = "Authentication = ActiveDirectoryMsi;"; - $testCase = 'empty UID provided'; - try { - $conn = new PDO("sqlsrv:server = $server; $connectionInfo", $uid); - echo $message . $testCase . PHP_EOL; - } catch(PDOException $e) { - verifyErrorMessage($e, $expectedError, $testCase); - } - unset($connectionInfo); - - $pwd = ''; - $connectionInfo = "Authentication = ActiveDirectoryMsi;"; - $testCase = 'empty PWD provided'; - try { - $conn = new PDO("sqlsrv:server = $server; $connectionInfo", null, $pwd); - echo $message . $testCase . PHP_EOL; - } catch(PDOException $e) { - verifyErrorMessage($e, $expectedError, $testCase); - } - unset($connectionInfo); - - $pwd = 'dummy'; - $connectionInfo = "Authentication = ActiveDirectoryMsi;"; - $testCase = 'PWD provided'; - try { - $conn = new PDO("sqlsrv:server = $server; $connectionInfo", null, $pwd); - echo $message . $testCase . PHP_EOL; - } catch(PDOException $e) { - verifyErrorMessage($e, $expectedError, $testCase); - } - unset($connectionInfo); - - $expectedError = 'When using Azure AD Access Token, the connection string must not contain UID, PWD, or Authentication keywords.'; - $connectionInfo = "Authentication = ActiveDirectoryMsi; AccessToken = '123';"; - $testCase = 'AccessToken option'; - try { - $conn = new PDO("sqlsrv:server = $server; $connectionInfo"); - echo $message . $testCase . PHP_EOL; - } catch(PDOException $e) { - verifyErrorMessage($e, $expectedError, $testCase); - } - unset($connectionInfo); -} - function connectInvalidServer() { global $server, $driver, $uid, $pwd; @@ -102,9 +50,6 @@ function connectInvalidServer() require_once('MsSetup.inc'); -// Test some error conditions -connectWithInvalidOptions(); - // Make a connection to an invalid server connectInvalidServer(); diff --git a/test/functional/sqlsrv/sqlsrv_azure_ad_authentication.phpt b/test/functional/sqlsrv/sqlsrv_azure_ad_authentication.phpt index 5614ab00a..903ce9dcb 100644 --- a/test/functional/sqlsrv/sqlsrv_azure_ad_authentication.phpt +++ b/test/functional/sqlsrv/sqlsrv_azure_ad_authentication.phpt @@ -34,22 +34,6 @@ if (sqlsrv_fetch($stmt)) { sqlsrv_free_stmt($stmt); sqlsrv_close($conn); -/////////////////////////////////////////////////////////////////////////////////////////// -// Test Azure AD with integrated authentication. This should fail because -// we don't support it. -// -$connectionInfo = array( "Authentication"=>"ActiveDirectoryIntegrated", "TrustServerCertificate"=>true ); - -$conn = sqlsrv_connect($server, $connectionInfo); -if ($conn === false) { - echo "Could not connect with Authentication=ActiveDirectoryIntegrated.\n"; - $errors = sqlsrv_errors(); - print_r($errors[0]); -} else { - echo "Connected successfully with Authentication=ActiveDirectoryIntegrated.\n"; - sqlsrv_close($conn); -} - /////////////////////////////////////////////////////////////////////////////////////////// // Test Azure AD on an Azure database instance. Replace $azureServer, etc with // your credentials to test, or this part is skipped. @@ -99,14 +83,4 @@ if ($azureServer != 'TARGET_AD_SERVER') { --EXPECTF-- Connected successfully with Authentication=SqlPassword. string(1) "%d" -Could not connect with Authentication=ActiveDirectoryIntegrated. -Array -( - [0] => IMSSP - [SQLSTATE] => IMSSP - [1] => -62 - [code] => -62 - [2] => Invalid option for the Authentication keyword. Only SqlPassword, ActiveDirectoryPassword, ActiveDirectoryMsi or ActiveDirectoryServicePrincipal is supported. - [message] => Invalid option for the Authentication keyword. Only SqlPassword, ActiveDirectoryPassword, ActiveDirectoryMsi or ActiveDirectoryServicePrincipal is supported. -) %s with Authentication=ActiveDirectoryPassword. diff --git a/test/functional/sqlsrv/sqlsrv_azure_ad_managed_identity.phpt b/test/functional/sqlsrv/sqlsrv_azure_ad_managed_identity.phpt index 644731eb7..e7abba6f2 100644 --- a/test/functional/sqlsrv/sqlsrv_azure_ad_managed_identity.phpt +++ b/test/functional/sqlsrv/sqlsrv_azure_ad_managed_identity.phpt @@ -19,34 +19,6 @@ function verifyErrorMessage($conn, $expectedError, $msg) } } -function connectWithInvalidOptions() -{ - global $server; - - $expectedError = 'When using ActiveDirectoryMsi Authentication, PWD must be NULL. UID can be NULL, but if not, an empty string is not accepted'; - - $connectionInfo = array("UID"=>"", "Authentication" => "ActiveDirectoryMsi"); - $conn = sqlsrv_connect($server, $connectionInfo); - verifyErrorMessage($conn, $expectedError, 'empty UID provided'); - unset($connectionInfo); - - $connectionInfo = array("PWD"=>"", "Authentication" => "ActiveDirectoryMsi"); - $conn = sqlsrv_connect($server, $connectionInfo); - verifyErrorMessage($conn, $expectedError, 'empty PWD provided'); - unset($connectionInfo); - - $connectionInfo = array("PWD"=>"pwd", "Authentication" => "ActiveDirectoryMsi"); - $conn = sqlsrv_connect($server, $connectionInfo); - verifyErrorMessage($conn, $expectedError, 'PWD provided'); - unset($connectionInfo); - - $expectedError = 'When using Azure AD Access Token, the connection string must not contain UID, PWD, or Authentication keywords.'; - $connectionInfo = array("Authentication"=>"ActiveDirectoryMsi", "AccessToken" => "123"); - $conn = sqlsrv_connect($server, $connectionInfo); - verifyErrorMessage($conn, $expectedError, 'AccessToken option'); - unset($connectionInfo); -} - function connectInvalidServer() { global $server, $driver, $userName, $userPassword; @@ -76,9 +48,6 @@ function connectInvalidServer() } } -// Test some error conditions -connectWithInvalidOptions($server); - // Make a connection to an invalid server connectInvalidServer();