fix(cluster): redact the Builder password behind cassandra.expose_credentials - #144
Merged
Conversation
php_scylladb_cluster_builder_properties copied the stored password into the object properties table in clear text. Every operation that reads object properties printed it: var_dump(), print_r(), get_object_vars(), (array) casts, and the debug renderer of any framework or exception handler. This defeated the #[\SensitiveParameter] attribute on Cassandra\Cluster\Builder::withCredentials(), which only redacts the value in stack traces. Put the fixed string "***" in the table instead. The key stays present, and the null-when-unset branch does not change, so the shape of a dump is the same as before. src/SSLOptions/ registers no get_properties handler, so the private key passphrase was never exposed. No change is needed there.
|
Tick the box to add this pull request to the merge queue (same as
|
The previous commit always replaced the password in the Builder properties table with "***". That also removed a value that is useful when you debug a connection problem. Add the cassandra.expose_credentials INI setting. It is off by default, so the redaction stays the safe default. Turn it on to put the real password back in the properties table. The setting is PHP_INI_SYSTEM. A request cannot call ini_set() to turn the redaction off, so a library or a debug handler cannot unredact the value. Only the operator can, through php.ini or the -d option.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
php_scylladb_cluster_builder_propertiescopied the stored password into the object properties table in clear text (src/Cluster/BuilderHandlers.c). Every operation that reads object properties printed it:var_dump($builder)print_r($builder)get_object_vars($builder)(array) $builderThis defeated the
#[\SensitiveParameter]attribute onCassandra\Cluster\Builder::withCredentials(). That attribute only redacts the value in stack traces.Fix
The properties table now holds
"***"instead of the credential. Thepasswordkey stays present and thenull-when-unset branch does not change, so the shape of a dump is the same as before.The real value is still reachable for debugging, behind a new INI setting:
cassandra.expose_credentials = OnOff by default, so redaction is the safe default. Turn it on to put the real password back in the properties table.
The setting is
PHP_INI_SYSTEM. A request cannot callini_set()to turn the redaction off, so a library, a template, or a debug handler cannot unredact the value. Only the operator can, throughphp.iniorphp -d cassandra.expose_credentials=1.Scope
src/SSLOptions/registers noget_propertieshandler at all, so the private key passphrase never reached a properties table. No change was needed there.passwordproperty.tests/Support/helpers.phponly callswithCredentials(), it never reads the value back.get_propertiesin the tree that touched a secret.Tests
New file
tests/Unit/ClusterBuilderCredentialsTest.phpwith 6 cases:(array) $builderkeeps thepasswordkey and returns"***".print_r()andvar_export()do not contain the credential.usernameandpasswordare bothnullwhen no credentials are set.cassandra.expose_credentialsdefaults to off.ini_set()on the setting returnsfalseand the value stays redacted.-d cassandra.expose_credentials=1prints the real password, and the same child without the flag prints***.Case 6 needs a fresh process, because a
PHP_INI_SYSTEMsetting cannot change inside a request. The helper readsSCYLLADB_EXTENSION_PATHto find the module to load, and marks the test skipped if the child cannot load it. Both paths were checked:With the variable set: 6 passed. Without it: 5 passed, 1 skipped.
Full Unit suite: 809 passed, 1 failed. The failure is the pre-existing
UuidTestcase attests/Unit/Uuid/UuidTest.php:112. It spawns a child with-d extension=cassandra, which resolves throughextension_dirand therefore loads an installed build instead of the build under test. It is unrelated to this change.Also changed
cassandra.inidocuments the new setting.