Ae v2 extended tests #1077
Ae v2 extended tests #1077
Conversation
There was a problem hiding this comment.
Beginning with php 7.4, nested ternary operations must explicitly use parentheses to dictate the order of the operations. Even though you may not have operations nested, add parentheses to ensure no ambiguity. FYI, see https://www.php.net/manual/en/migration74.deprecated.php
Also, before accessing any array values, do check if the array is empty first. Will continue reviewing after you have fixed all these.
| } elseif ($attestationType == 'invalid') { | ||
| die("Connection should have failed for invalid protocol\n"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Please indicate the conditions under which connection should have failed or should not have failed in the error messages such that if this happens we know which combination it is.
| } | ||
| } elseif ($targetAttestationType == 'invalid') { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Please add an else case as well to catch all.
There was a problem hiding this comment.
The else case is just to connect successfully and proceed with the rest of the test - the continue and die statements ensure that it won't proceed with the code below when it shouldn't.
| } | ||
| } elseif ($attestationType == 'invalid') { | ||
| die("Connection should have failed for invalid protocol\n"); | ||
| } |
| } | ||
|
|
||
| $ceDisabled = $attestationType == 'disabled' ? true : false; | ||
| insertValues($conn, $insertQuery, $dataTypes, $testValues, $ceDisabled); |
There was a problem hiding this comment.
Add parentheses around the if condition.
| $sameKeyAndType = false; | ||
| if ($key == $targetKey and $encryptionType == $targetType and $isEncrypted) { | ||
| $sameKeyAndType = true; | ||
| } |
There was a problem hiding this comment.
Same here, add parentheses around testing if conditions
test/extended/pdo_AE_functions.inc
Outdated
| // Connect and clear the procedure cache | ||
| function connect($attestation_info) | ||
| { | ||
| include("MsSetup.inc"); |
| { | ||
| include("MsSetup.inc"); | ||
| $options = "sqlsrv:Server=$server;Database=$databaseName;ColumnEncryption=$attestation_info"; | ||
|
|
There was a problem hiding this comment.
Also, be consistent with your variable names. Stick with camel case please
There was a problem hiding this comment.
I vaguely recall there were others. Please double check and fix those as well.
test/extended/pdo_AE_functions.inc
Outdated
| $stmt = $conn->query($query); | ||
| $info = $stmt->fetch(); | ||
| if ($info['value'] != 1 or $info['value_in_use'] != 1) { | ||
| die("Error: enclave computations are not enabled on the server!"); |
There was a problem hiding this comment.
Add parentheses and check $stmt and $info is null or false
|
|
||
| function insertValues($conn, $insertQuery, $dataTypes, $testValues, $ceDisabled=false) | ||
| { | ||
| for ($v = 0; $v < sizeof($testValues['bigint']); ++$v) { |
There was a problem hiding this comment.
Test if $testValues is empty or null first
| $stmt->bindParam($i, $val, $PDOType, 0, PDO::SQLSRV_ENCODING_BINARY); | ||
| $stmt->bindParam($i+1, $val, $PDOType, 0, PDO::SQLSRV_ENCODING_BINARY); | ||
| } | ||
| $i+=2; |
There was a problem hiding this comment.
Put curly braces to the end of line for if, foreach or else
| @@ -307,22 +307,30 @@ function connect($attestation_info) | |||
| return false; | |||
| } | |||
|
|
|||
| $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); | |||
| $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_WARNING); | |||
| $conn->setAttribute(PDO::SQLSRV_ATTR_FETCHES_DATETIME_TYPE, true); | |||
There was a problem hiding this comment.
Please justify why ERRMODE_WARNING is used rather than ERRMODE_EXCEPTION here (I know it will be set to using the latter mode later but add a comment to ease future maintenance)
There was a problem hiding this comment.
I think that's from an earlier version - we can set ERRMODE_EXCEPTION immediately. Will fix.
There was a problem hiding this comment.
Please note later in this test you reset to ERRMODE_EXCEPTION I was asking you to justify this in a comment
| { | ||
| include("MsSetup.inc"); | ||
| $options = "sqlsrv:Server=$server;Database=$databaseName;ColumnEncryption=$attestation_info"; | ||
|
|
There was a problem hiding this comment.
I vaguely recall there were others. Please double check and fix those as well.
test/extended/pdo_AE_functions.inc
Outdated
| die("Error: enclave computations are not enabled on the server!"); | ||
| if (!$stmt) { | ||
| print_r($conn->errorInfo()); | ||
| die("Error when checking if enclave computations are enabled. This should never happen!\n"); |
There was a problem hiding this comment.
Please add a simple comment to explain why this should never happen. e.g. the right sql server, etc.
| $info = sqlsrv_fetch_array($stmt); | ||
| if (empty($info) or ($info['value'] != 1) or ($info['value_in_use'] != 1)) { | ||
| die("Error: enclave computations are not enabled on the server!"); | ||
| } |
There was a problem hiding this comment.
Wondering if this should have been verified in a skipif instead?
There was a problem hiding this comment.
I'd rather check it here, so that the test clearly fails if enclave computations are not enabled (the skipif checks that the server is a HGS enabled VM - if it is, then these tests should proceed and work as expected).
| # populate these tables | ||
| populateTables(conn_options, args.DBNAME) | ||
| print("About to set up encryption...\n") | ||
| # setup AE (certificate, column master key and column encryption key) | ||
| setupAE(conn_options, args.DBNAME) | ||
|
|
There was a problem hiding this comment.
This reminds me. This setup_dbs.py can be used to set up an Azure database as well. Do we need special handling with the AE part when it's Azure?
No description provided.