From 5aa7d7a22f764340f5782681d955f9684f658d2b Mon Sep 17 00:00:00 2001 From: yitam Date: Wed, 3 Nov 2021 11:05:30 -0700 Subject: [PATCH 1/2] Let ODBC driver verify azure ad authentications --- source/pdo_sqlsrv/pdo_parser.cpp | 35 +++--- source/pdo_sqlsrv/pdo_util.cpp | 12 +- source/pdo_sqlsrv/php_pdo_sqlsrv_int.h | 6 +- source/shared/core_conn.cpp | 112 +++++++++--------- source/shared/core_sqlsrv.h | 10 +- source/sqlsrv/conn.cpp | 10 -- source/sqlsrv/php_sqlsrv_int.h | 1 - source/sqlsrv/util.cpp | 12 +- .../pdo_azure_ad_authentication.phpt | 22 ++-- .../pdo_azure_ad_managed_identity.phpt | 2 +- .../sqlsrv_azure_ad_authentication.phpt | 30 ++--- .../sqlsrv_azure_ad_managed_identity.phpt | 2 +- 12 files changed, 112 insertions(+), 142 deletions(-) diff --git a/source/pdo_sqlsrv/pdo_parser.cpp b/source/pdo_sqlsrv/pdo_parser.cpp index 6d1608933..23e8e8e9c 100644 --- a/source/pdo_sqlsrv/pdo_parser.cpp +++ b/source/pdo_sqlsrv/pdo_parser.cpp @@ -173,30 +173,31 @@ 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 ); - } + //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 ) { diff --git a/source/pdo_sqlsrv/pdo_util.cpp b/source/pdo_sqlsrv/pdo_util.cpp index cb0b06206..d3cd505e6 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,10 @@ 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_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..12431191c 100644 --- a/source/pdo_sqlsrv/php_pdo_sqlsrv_int.h +++ b/source/pdo_sqlsrv/php_pdo_sqlsrv_int.h @@ -149,8 +149,8 @@ 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); + //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 ); @@ -391,7 +391,7 @@ 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_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..b8cade3bb 100644 --- a/source/shared/core_conn.cpp +++ b/source/shared/core_conn.cpp @@ -704,39 +704,39 @@ 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])); - } -} +//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 *** @@ -792,10 +792,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 +805,33 @@ 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; // 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(); + // } } } // 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 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..d27edc854 100644 --- a/source/shared/core_sqlsrv.h +++ b/source/shared/core_sqlsrv.h @@ -190,10 +190,10 @@ 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); -} +//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 +2011,7 @@ 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_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..6febc6403 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,10 @@ 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_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..27f331dff 100644 --- a/test/functional/pdo_sqlsrv/pdo_azure_ad_authentication.phpt +++ b/test/functional/pdo_sqlsrv/pdo_azure_ad_authentication.phpt @@ -38,17 +38,17 @@ unset($conn); // Test Azure AD with integrated authentication. This should fail because // we don't support it. // -$connectionInfo = "Authentication = ActiveDirectoryIntegrated; TrustServerCertificate = true;"; +// $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"; -} +// 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 @@ -95,6 +95,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..36d7e8f06 100644 --- a/test/functional/pdo_sqlsrv/pdo_azure_ad_managed_identity.phpt +++ b/test/functional/pdo_sqlsrv/pdo_azure_ad_managed_identity.phpt @@ -103,7 +103,7 @@ function connectInvalidServer() require_once('MsSetup.inc'); // Test some error conditions -connectWithInvalidOptions(); +// 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..159b0e27c 100644 --- a/test/functional/sqlsrv/sqlsrv_azure_ad_authentication.phpt +++ b/test/functional/sqlsrv/sqlsrv_azure_ad_authentication.phpt @@ -38,17 +38,17 @@ sqlsrv_close($conn); // Test Azure AD with integrated authentication. This should fail because // we don't support it. // -$connectionInfo = array( "Authentication"=>"ActiveDirectoryIntegrated", "TrustServerCertificate"=>true ); +// $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); -} +// $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 @@ -99,14 +99,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..15c147254 100644 --- a/test/functional/sqlsrv/sqlsrv_azure_ad_managed_identity.phpt +++ b/test/functional/sqlsrv/sqlsrv_azure_ad_managed_identity.phpt @@ -77,7 +77,7 @@ function connectInvalidServer() } // Test some error conditions -connectWithInvalidOptions($server); +// connectWithInvalidOptions($server); // Make a connection to an invalid server connectInvalidServer(); From ad91fa1d47f01a0f651737c4982aeda9247f86cc Mon Sep 17 00:00:00 2001 From: yitam Date: Mon, 8 Nov 2021 13:24:16 -0800 Subject: [PATCH 2/2] Cleaned up --- source/pdo_sqlsrv/pdo_parser.cpp | 26 --------- source/pdo_sqlsrv/pdo_util.cpp | 4 -- source/pdo_sqlsrv/php_pdo_sqlsrv_int.h | 4 -- source/shared/core_conn.cpp | 58 +------------------ source/shared/core_sqlsrv.h | 6 -- source/sqlsrv/util.cpp | 4 -- .../pdo_azure_ad_authentication.phpt | 16 ----- .../pdo_azure_ad_managed_identity.phpt | 55 ------------------ .../sqlsrv_azure_ad_authentication.phpt | 16 ----- .../sqlsrv_azure_ad_managed_identity.phpt | 31 ---------- 10 files changed, 3 insertions(+), 217 deletions(-) diff --git a/source/pdo_sqlsrv/pdo_parser.cpp b/source/pdo_sqlsrv/pdo_parser.cpp index 23e8e8e9c..2574a5ad2 100644 --- a/source/pdo_sqlsrv/pdo_parser.cpp +++ b/source/pdo_sqlsrv/pdo_parser.cpp @@ -173,32 +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 d3cd505e6..1edec3623 100644 --- a/source/pdo_sqlsrv/pdo_util.cpp +++ b/source/pdo_sqlsrv/pdo_util.cpp @@ -437,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 12431191c..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 b8cade3bb..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 { @@ -806,32 +771,15 @@ void build_connection_string_and_set_conn_attr( _Inout_ sqlsrv_conn* conn, _Inou } if (option != NULL && !stricmp(option, aadMSIoption)) { - activeDirectoryMSI = true; // 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(); - // } + 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 connecting access token or Authentication is ActiveDirectoryMSI + // 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 d27edc854..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/util.cpp b/source/sqlsrv/util.cpp index 6febc6403..eaa801e09 100644 --- a/source/sqlsrv/util.cpp +++ b/source/sqlsrv/util.cpp @@ -425,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 27f331dff..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. 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 36d7e8f06..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 159b0e27c..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. diff --git a/test/functional/sqlsrv/sqlsrv_azure_ad_managed_identity.phpt b/test/functional/sqlsrv/sqlsrv_azure_ad_managed_identity.phpt index 15c147254..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();