Let ODBC driver verify azure ad authentications (#1326)

microsoft · Nov 23, 2021 · cbfc763 · cbfc763
1 parent a570522
commit cbfc763
Show file tree

Hide file tree

Showing 12 changed files with 6 additions and 250 deletions.
diff --git 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<char*>( 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<char> option;
-            option = static_cast<char*>( 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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.