PHPC-2356: Remove explicit env variables #1514
Conversation
| @@ -19,7 +19,7 @@ buildvariants: | |||
| - ".sharded .local !.3.6 !.4.0 !.4.2 !.4.4 !.5.0" | |||
| - ".loadbalanced .local !.3.6 !.4.0 !.4.2 !.4.4 !.5.0" | |||
| - "test-atlas-connectivity" | |||
| - ".ocsp !.4.4" | |||
| - ".ocsp !.4.4 !.5.0" | |||
There was a problem hiding this comment.
IIUC, this drops OCSP testing on 5.0. What the impetus for this change? The OCSP spec test README doesn't talk about server versions.
There was a problem hiding this comment.
We only test 6.0 and newer on Debian 11 - 5.0 and older are tested on Debian 9.2. OCSP tests for 5.0 are included in the test-debian92-php82-local matrix below.
| - command: shell.exec | ||
| params: | ||
| script: | | ||
| openssl aes-256-cbc -S "${encrypted_uris_salt}" -K "${encrypted_uris_key}" -iv "${encrypted_uris_iv}" -in ${PROJECT_DIRECTORY}/.evergreen/atlas-uris.txt.enc -out ${PROJECT_DIRECTORY}/.evergreen/atlas-uris.txt -d |
There was a problem hiding this comment.
This makes it available for us to use in tests without having to decrypt a file and potentially leak the encryption credentials.
Are other drivers doing this as well? I don't have an opinion on which is more secure, but storing the URIs directly in the Evergreen project config is certainly more convenient.
Note: you can also remove .evergreen/atlas-uris.txt from .gitignore.
| ${PROJECT_DIRECTORY}/.evergreen/run-ocsp-responder.sh | ||
|
|
||
| "run tests": | ||
| - command: shell.exec | ||
| type: test | ||
| params: | ||
| include_expansions_in_env: | ||
| - API_VERSION | ||
| - ATLAS_CONNECTIVITY_URIS |
There was a problem hiding this comment.
Should this always be specified? In the test-atlas-connectivity task, run tests is invoked with vars: { TESTS: "tests/atlas.phpt" }, so I would expect that's the only case where you'd actually want to pass this potentially sensitive environment variable onwards.
There was a problem hiding this comment.
I could create a separate function to run just the connectivity tests that also passes the necessary expansion to the env, but this would entail duplicating all of the logic in this function. I don't think it's worth it, as any attacker who might modify tests to get access to this environment variable would be able to do so in the particular test that includes this variable.
| @@ -167,9 +167,19 @@ functions: | |||
| "bootstrap mongo-orchestration": | |||
| - command: shell.exec | |||
| params: | |||
| include_expansions_in_env: | |||
There was a problem hiding this comment.
Noted that vars (when calling a func) actually define expansions (per Functions). That distinction wasn't clear to me before this PR.
.evergreen/run-tests.sh
Outdated
| @@ -4,7 +4,7 @@ set -o errexit # Exit the script with error if any of the commands fail | |||
| # Supported environment variables | |||
| API_VERSION=${API_VERSION:-} # Optional API_VERSION environment variable for run-tests.php | |||
| CRYPT_SHARED_LIB_PATH="${CRYPT_SHARED_LIB_PATH:-}" # Optional path to crypt_shared library | |||
| MONGODB_URI=${MONGODB_URI:-} # Connection string (including credentials and topology info) | |||
| MONGODB_URI=${MONGODB_URI:-}${APPEND_URI:-} # Connection string (including credentials and topology info) | |||
There was a problem hiding this comment.
Nit: the "Supported environment variables" section here doubles as documentation, so I think it'd be clearer to take APPEND_URI separately in the same form and then merge them below.
That said, it's only essential that this merging happens before the "Determine if MONGODB_URI already has a query string" logic below.
There was a problem hiding this comment.
Good point. I've moved the merging of APPEND_URI to before detecting the suffix.
Co-authored-by: Jeremy Mikola <[email protected]>
PHPC-2356
This will prevent accidental leaking of environment variables.
Note that the URI list for Atlas connectivity tests are now contained in an environment variable in the project config, which is added to the environment via the
include_expansions_in_envoption inrun tests. This makes it available for us to use in tests without having to decrypt a file and potentially leak the encryption credentials.