Added support to load a custom keystore provider for Column Encryption #511
Conversation
source/shared/core_conn.cpp
Outdated
| core::SQLSetConnectAttr( conn, SQL_COPT_SS_CEKEYSTOREDATA, reinterpret_cast<SQLPOINTER>( pKsd ), SQL_IS_POINTER ); | ||
| } | ||
|
|
||
| void common_conn_str_append_func( const char* odbc_name, const char* val, size_t val_len, std::string& conn_str TSRMLS_DC ) |
There was a problem hiding this comment.
Missing SAL annotations for this function - they were previously included here
source/shared/core_conn.cpp
Outdated
| @@ -776,7 +778,68 @@ void determine_server_version( _Inout_ sqlsrv_conn* conn TSRMLS_DC ) | |||
| conn->server_version = version_major; | |||
| } | |||
|
|
|||
| void common_conn_str_append_func( _In_z_ const char* odbc_name, _In_reads_(val_len) const char* val, _Inout_ size_t val_len, _Inout_ std::string& conn_str TSRMLS_DC ) | |||
| // Column Encryption feature: if a custom keystore provider is specified, | |||
| // load and configure it when column encryption is enabled, but this step have | |||
source/shared/core_conn.cpp
Outdated
|
|
||
| ksp_data = reinterpret_cast<unsigned char*>( sqlsrv_malloc( sizeof( CEKEYSTOREDATA ) + key_size ) ); | ||
|
|
||
| CEKEYSTOREDATA *pKsd = (CEKEYSTOREDATA*) ksp_data.get(); |
There was a problem hiding this comment.
ksp_data appears to be some sort of intermediary - why is it needed? Why not just have CEKEYSTOREDATA *pKsd;?
There was a problem hiding this comment.
yes I use sqlsrv_malloc_auto_ptr to take care of releasing the memory afterwards
There was a problem hiding this comment.
OK. Why use a C-style cast though? Wouldn't a static_cast be preferable?
source/shared/core_conn.cpp
Outdated
| throw core::CoreException(); | ||
| } | ||
|
|
||
| pKsd->name = (wchar_t *) wksp_name.get(); |
There was a problem hiding this comment.
Is it necessary to cast to wchar_t? wchar_t is 16 bits on Windows and 32 on Linux, and wksp_name is defined with SQLWCHAR, which should resolve to wchar_t iirc. But if not, can this lead to cross-platform incompatibility?
There was a problem hiding this comment.
Please check the keystore definitions in msodbcsql.h, and they used wchar_t in quite a few places. In Linux/mac, when compiling the KSP source and the app, we use -fshort-wchar
|
|
||
| // Next, extract the character string from conn->ce_option.ksp_encrypt_key into encrypt_key | ||
| char* encrypt_key = Z_STRVAL_P( conn->ce_option.ksp_encrypt_key ); | ||
| memcpy_s( pKsd->data, key_size * sizeof( char ) , encrypt_key, key_size ); |
There was a problem hiding this comment.
Does key_size account for the null terminator?
There was a problem hiding this comment.
Good question. The answer is no. Only the length of the key.
source/pdo_sqlsrv/pdo_dbh.cpp
Outdated
| sizeof(ODBCConnOptions::CEKeystoreEncryptKey), | ||
| CONN_ATTR_STRING, | ||
| ce_ksp_provider_set_func::func | ||
| }, |
There was a problem hiding this comment.
The indentation is off...
| break; | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Are all three keywords required if the user wants to use a custom keystore?
There was a problem hiding this comment.
yes, as you can see in the helper function load_configure_ksp()
source/sqlsrv/conn.cpp
Outdated
| sizeof(ODBCConnOptions::CEKeystoreEncryptKey), | ||
| CONN_ATTR_STRING, | ||
| ce_ksp_provider_set_func::func | ||
| }, |
There was a problem hiding this comment.
Indentation is off
| if( ! $conn ) | ||
| { | ||
| die( "skip - could not connect during SKIPIF." ); | ||
| } |
There was a problem hiding this comment.
Do we really want to skip if we can't connect?? Seems like this should be an error...
There was a problem hiding this comment.
I got the idea from skipif_protocol_not_tcp.inc :)
There was a problem hiding this comment.
hmmmm I agree with David too. If somehow it can't connect, the test should fail instead of being skipped. Should fix this in skipif_protocol_not_tcp.inc too (but maybe in a separate PR to the dev branch)
test/functional/setup/ksp_app.c
Outdated
| } | ||
| while (pKsp = *ppKsp++) { | ||
| if (!memcmp(KSPNAME, pKsp->Name, sizeof(KSPNAME))) | ||
| goto FoundProv; |
There was a problem hiding this comment.
I basically got this from the sample code, but of course I can avoid using goto in this case.
test/functional/setup/build_ksp.py
Outdated
| if platform.system() == 'Windows': | ||
| command = 'cl /MD {0}.c /link odbc32.lib /out:ksp_app.exe'.format(app_name) | ||
| batchfile = 'build_app.bat' | ||
| create_batch_file('x86', batchfile, command) |
There was a problem hiding this comment.
Why do you need to write the commands to a batch file and then executing the whole thing? Why can't you just run the command one by one in the script?
There was a problem hiding this comment.
because I need to invoke vcvarsall.bat first with the appropriate option (for x64 or x86) in order to use the c compiler cl.exe
test/functional/setup/build_ksp.py
Outdated
| def configure_KSP(app_name): | ||
| print('Compiling ', app_name) | ||
| if platform.system() == 'Windows': | ||
| command = 'cl /MD {0}.c /link odbc32.lib /out:ksp_app.exe'.format(app_name) |
There was a problem hiding this comment.
not a big deal, but would be nice if the order of these arguments are consistent with the ones in compile_KSP_windows
There was a problem hiding this comment.
I followed the ODBC sample codes on how to compile the source code. Besides, one is to compile the source into a DLL and the other is to create an *.exe, so the switches are different.
test/functional/setup/build_ksp.py
Outdated
| os.system(batchfile) | ||
| os.remove(batchfile) | ||
| else: | ||
| os.system('gcc -o ksp_app -fshort-wchar {0}.c -lodbc -ldl'.format(app_name)) |
There was a problem hiding this comment.
make the ordering of the arguments consistent with the ones in conpile_KSP, but make sure -lodbc comes after the c file.
There was a problem hiding this comment.
again, I basically followed the examples given in the ODBC sample code
test/functional/setup/build_ksp.py
Outdated
| args = parser.parse_args() | ||
|
|
||
| ksp_name = args.KSPNAME | ||
| app_name = args.APPNAME |
There was a problem hiding this comment.
these variable names are a little confusing. These variables correspond to the name of the C file that is used to compile, but the variable names sound like they are the names of the dlls or executables. Maybe rename it to something like ksp_src_name and app_src_name.
Another not major thing at all is you may want to consider making the names to args.KSPNAME + '.c' so you don't need to append the '.c' everywhere.
There was a problem hiding this comment.
I changed the arguments to be KSPSRC and APPSRC respectively
test/functional/setup/build_ksp.py
Outdated
| # ksp_app.c (or any equivalent) | ||
| # msodbcsql.h (odbc header file) | ||
| # | ||
| # Execution: Run with command line with required options |
| if (task == set_up) { | ||
| printf("Setting up KSP...\n"); | ||
| setKSPLibrary(stmt); | ||
| populateTestTable(dbc, stmt); |
There was a problem hiding this comment.
Is there a reason why we're creating and populating a table here? wouldn't it be better to make and populate the tables on fly in the tests?
There was a problem hiding this comment.
I agree, but as of today I can only insert data into the test table with encryption using sqlsrv...
For now the KSP tests show that they can retrieve data from a pre-populated table with encryption
There was a problem hiding this comment.
hmmm this may change very soon though once the ODBC driver is ready. We shall see
There was a problem hiding this comment.
No we will keep using a test table because we want to show that we can retrieve data encrypted by a 3rd party (here in this case it's odbc )
| SQLExecDirect(stmt, "DROP TABLE CustomKSPTestTable", SQL_NTS); | ||
| SQLExecDirect(stmt, "DROP COLUMN ENCRYPTION KEY CustomCEK", SQL_NTS); | ||
| SQLExecDirect(stmt, "DROP COLUMN MASTER KEY CustomCMK", SQL_NTS); | ||
| printf("Removed table, CEK, and CMK\n"); |
There was a problem hiding this comment.
If the database is dropped at the end, don't need this part
There was a problem hiding this comment.
yes I agree but I provided this as a handy way to remove the KSP related entities in case the user doesn't want to drop the entire database
| if ($version < 13) | ||
| { | ||
| die( "skip - feature not supported in this version of SQL Server." ); | ||
| } |
There was a problem hiding this comment.
Should we not check for ODBC version too? at least exclude 11 and lower.
.travis.yml
Outdated
| - PDOSQLSRV_DBNAME=msphpsql_pdosqlsrv | ||
| - PDOSQLSRV_DBNAME=msphpsql_pdosqlsrv | ||
| - TEST_PHP_SQL_UID=sa | ||
| - TEST_PHP_SQL_PWD=Password12@ |
There was a problem hiding this comment.
yml file should be synced with dev branch
Dockerfile-msphpsql
Outdated
| @@ -74,9 +79,6 @@ RUN sed -i -e 's/TARGET_DATABASE/msphpsql_sqlsrv/g' MsSetup.inc | |||
| RUN sed -i -e 's/TARGET_USERNAME/'"$TEST_PHP_SQL_UID"'/g' MsSetup.inc | |||
| RUN sed -i -e 's/TARGET_PASSWORD/'"$TEST_PHP_SQL_PWD"'/g' MsSetup.inc | |||
|
|
|||
| WORKDIR $PHPSQLDIR | |||
| RUN chmod +x ./entrypoint.sh | |||
| CMD /bin/bash ./entrypoint.sh | |||
There was a problem hiding this comment.
the deleted lines are actually the latest dev branch, Docker file should be the latest on dev
No description provided.