Avoid leaking keys by mistake with --upload-test-report
#4877
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,6 +55,7 @@ | |
| from easybuild.tools.github import det_pr_title, fetch_easyconfigs_from_commit, fetch_files_from_commit | ||
| from easybuild.tools.github import is_patch_for, pick_default_branch | ||
| from easybuild.tools.testing import create_test_report, post_pr_test_report, session_state | ||
| from easybuild.tools.testing import exclude_env_from_report_add, exclude_env_from_report_clear | ||
| import easybuild.tools.github as gh | ||
|
|
||
| try: | ||
|
|
@@ -1317,26 +1318,98 @@ def test_github_create_test_report(self): | |
| 'log_file': logfile, | ||
| }), | ||
| ] | ||
| environ = { | ||
| 'USER': 'test', | ||
| 'DONT_REMOVE_ME': 'test-123', | ||
| } | ||
| JWT_HDR = 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9' | ||
| JWT_PLD = 'eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNzA4MzQ1MTIzLCJleHAiOjE3MDgzNTUxMjN9' | ||
| JWT_SIG = 'SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c' | ||
| secret_environ = { | ||
| 'REMOVEME': 'test-abc', | ||
| 'ReMoVeMe2': 'TeSt-xyz', | ||
|
|
||
| # Test default removal based on variable value | ||
| 'TOTALLYPUBLICVAR1': 'AKIAIOSFODNN7EXAMPLE', # AWS_ACCESS_KEY | ||
| 'TOTALLYPUBLICVAR2': 'wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY', # AWS_SECRET_KEY | ||
| 'TOTALLYPUBLICVAR3': '.'.join([JWT_HDR, JWT_PLD, JWT_SIG]), # JWT | ||
| 'TOTALLYPUBLICVAR4': 'ghp_123456789_ABCDEFGHIJKlmnopqrstuvwxyz', # GH_TOKEN | ||
| 'TOTALLYPUBLICVAR5': 'xoxb-1234567890-1234567890123-ABCDEFabcdef', # SLACK_TOKEN | ||
|
|
||
| # Test default removal based on variable name | ||
| 'API_SOMETHING': '1234567890', | ||
| 'MY_PASSWORD': '1234567890', | ||
| 'ABC_TOKEN': '1234567890', | ||
| 'AUTH_XXX': '1234567890', | ||
| 'LICENSE': '1234567890', | ||
| 'WORLD_KEY': '1234567890', | ||
| 'PRIVATE_INFO': '1234567890', | ||
| 'SECRET_SECRET': '1234567890', | ||
| 'INFO_CREDENTIALS': '1234567890', | ||
| } | ||
| init_session_state = { | ||
| 'easybuild_configuration': ['EASYBUILD_DEBUG=1'], | ||
| 'environment': {**environ, **secret_environ}, | ||
| 'module_list': [{'mod_name': 'test'}], | ||
| 'system_info': {'name': 'test'}, | ||
| 'time': gmtime(0), | ||
| } | ||
|
|
||
| # Test exclude_env_from_report_add/clear | ||
| exclude_env_from_report_add('REMOVEME'.lower()) # Also check that the name is uppercased in the check | ||
|
|
||
| res = create_test_report("just a test", ecs_with_res, init_session_state) | ||
| patterns = [ | ||
| "**SUCCESS** _test.eb_", | ||
| "**FAIL (build issue)** _fail.eb_", | ||
| "01 Jan 1970 00:00:00", | ||
| "EASYBUILD_DEBUG=1", | ||
| "DONT_REMOVE_ME = test-123", | ||
| ] | ||
| for pattern in patterns: | ||
| self.assertIn(pattern, res['full']) | ||
|
|
||
| # Test that excluded patterns works by matching also partial strings | ||
| exclude_patterns1 = [ | ||
| 'REMOVEME', | ||
| 'ReMoVeMe2', | ||
| ] | ||
| # Test that known token regexes for ENV vars are excluded by default | ||
| exclude_patterns2 = [ | ||
| 'TOTALLYPUBLICVAR1', | ||
| 'TOTALLYPUBLICVAR2', | ||
| 'TOTALLYPUBLICVAR3', | ||
| 'TOTALLYPUBLICVAR4', | ||
| 'TOTALLYPUBLICVAR5', | ||
|
|
||
| 'API_SOMETHING', | ||
| 'MY_PASSWORD', | ||
| 'ABC_TOKEN', | ||
| 'AUTH_XXX', | ||
| 'LICENSE', | ||
| 'WORLD_KEY', | ||
| 'PRIVATE_INFO', | ||
| 'SECRET_SECRET', | ||
| 'INFO_CREDENTIALS', | ||
| ] | ||
| for pattern in exclude_patterns1 + exclude_patterns2: | ||
| # .lower() test that variable name is not case sensitive for excluding | ||
| self.assertNotIn(pattern.lower(), res['full']) | ||
|
|
||
| exclude_env_from_report_clear() | ||
| patterns += exclude_patterns1 | ||
|
|
||
| res = create_test_report("just a test", ecs_with_res, init_session_state) | ||
| for pattern in patterns: | ||
| self.assertIn(pattern, res['full']) | ||
|
|
||
| for pattern in patterns[:2]: | ||
| self.assertIn(pattern, res['overview']) | ||
|
|
||
| for pattern in exclude_patterns2: | ||
| # .lower() test that variable name is not case sensitive for excluding | ||
| self.assertNotIn(pattern.lower(), res['full']) | ||
|
|
||
| # mock create_gist function, we don't want to actually create a gist every time we run this test... | ||
| def fake_create_gist(*args, **kwargs): | ||
| return 'https://gist.github.com/%s/test' % GITHUB_TEST_ACCOUNT | ||
|
|
@@ -1353,7 +1426,7 @@ def fake_create_gist(*args, **kwargs): | |
| self.assertIn(pattern, res['full']) | ||
|
|
||
| for pattern in patterns[:3]: | ||
| self.assertIn(pattern, res['overview']) | ||
|
|
||
| self.assertIn("**SUCCESS** _test.eb_", res['overview']) | ||
|
|
||
|
|
||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3390,26 +3390,46 @@ def toy(extra_args=None): | |
| return test_report_txt | ||
|
|
||
| # define environment variables that should (not) show up in the test report | ||
| # The name contains an auto-excluded pattern `SECRET` | ||
| test_var_secret_always = 'THIS_IS_JUST_A_SECRET_ENV_VAR_FOR_EASYBUILD' | ||
| os.environ[test_var_secret_always] = 'thisshouldremainsecretonrequest' | ||
| test_var_secret_always_regex = re.compile(test_var_secret_always) | ||
| # The name contains an autoexcluded value as a recognized GH token | ||
| test_var_secret_always2 = 'THIS_IS_JUST_A_TOTALLY_PUBLIC_ENV_VAR_FOR_EASYBUILD' | ||
| os.environ[test_var_secret_always2] = 'ghp_123456789_ABCDEFGHIJKlmnopqrstuvwxyz' | ||
| test_var_secret_always_regex2 = re.compile(test_var_secret_always2) | ||
| # This should be in general present and excluded on demand | ||
| test_var_secret_ondemand = 'THIS_IS_A_CUSTOM_ENV_VAR_FOR_EASYBUILD' | ||
| os.environ[test_var_secret_ondemand] = 'thisshouldbehiddenondemand' | ||
| test_var_secret_ondemand_regex = re.compile(test_var_secret_ondemand) | ||
| test_var_public = 'THIS_IS_JUST_A_PUBLIC_ENV_VAR_FOR_EASYBUILD' | ||
| os.environ[test_var_public] = 'thisshouldalwaysbeincluded' | ||
| test_var_public_regex = re.compile(test_var_public) | ||
|
|
||
| # default: no filtering | ||
| test_report_txt = toy() | ||
| self.assertTrue(test_var_secret_ondemand_regex.search(test_report_txt)) | ||
| self.assertTrue(test_var_public_regex.search(test_report_txt)) | ||
| for rgx in [ | ||
| test_var_secret_always_regex, | ||
| test_var_secret_always_regex2, | ||
| ]: | ||
| res = rgx.search(test_report_txt) | ||
| self.assertFalse(res, "No match for %s in %s" % (rgx.pattern, test_report_txt)) | ||
|
|
||
| # filter out env vars that match specified regex pattern | ||
| filter_arg = "--test-report-env-filter=.*_IS_A_CUSTOM_ENV_VAR_FOR_EASYBUILD" | ||
| test_report_txt = toy(extra_args=[filter_arg]) | ||
| for rgx in [ | ||
| test_var_secret_ondemand_regex, | ||
| test_var_secret_always_regex, | ||
| test_var_secret_always_regex2, | ||
| ]: | ||
| res = rgx.search(test_report_txt) | ||
| self.assertFalse(res, "No match for %s in %s" % (rgx.pattern, test_report_txt)) | ||
| self.assertTrue(test_var_public_regex.search(test_report_txt)) | ||
| # make sure that used filter is reported correctly in test report | ||
| filter_arg_regex = re.compile(r"--test-report-env-filter='.\*_IS_A_CUSTOM_ENV_VAR_FOR_EASYBUILD'") | ||
| tup = (filter_arg_regex.pattern, test_report_txt) | ||
| self.assertTrue(filter_arg_regex.search(test_report_txt), "%s in %s" % tup) | ||
|
|
||
|
|
||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the only reason that this is added is so we have something to play with in the tests?
That's a bad pattern, I think it's better pull tricks in the tests (like changing constants in place and than restoring their original value in the
tearDownof the tests) rather than introducing global variables that are only there to play with in the test suite.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea was for this functions to also be used outside of framework, eg for an easyblock to excluded a specific environment variable from reports that is know to contain a secret
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if there would be a better solution to implement excluding variables on demand.
In case we want more discussion around that, i think the default excludes should be included ASAP, so i could split this PR in 2 if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that
_exclude_env_from_reportis a global variable here is a problem, since that means it'll be shared across multiple easyblocks used in a single EasyBuild session...