Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

censor authorization part of headers before logging ReST API request #3248

Merged
merged 7 commits into from
Mar 16, 2020

Conversation

boegel
Copy link
Member

@boegel boegel commented Mar 16, 2020

GitHub tokens were found to be "leaking" into the top-level log file when using --from-pr combined with --debug, as reported by @zao:

# relevant part of log for "eb --from-pr 10064 --debug"
== 2020-03-15 19:15:28,551 github.py:389 DEBUG Fetching easyconfigs from easybuilders/easybuild-easyconfigs PR #10064 into /tmp/eb-nqIFwJ/files_pr10064
== 2020-03-15 19:15:30,187 github.py:1843 INFO Successfully obtained GitHub token for user zao from keyring.
== 2020-03-15 19:15:30,188 rest.py:165 DEBUG cli request: GET, /repos/easybuilders/easybuild-easyconfigs/pulls/10064, None, {'Authorization': u'Token deadbeefdeadbeefdeadbeefdeadbeefde', 'User-Agent': 'vsc-rest-client'}
== 2020-03-15 19:15:30,188 rest.py:207 DEBUG opening request:  https://api.github.com/repos/easybuilders/easybuild-easyconfigs/pulls/10064
== 2020-03-15 19:15:30,691 rest.py:177 DEBUG reponse len: 46 

That's clearly not desirable, so the changes in this PR censor the Authorization part of the headers before the debug log statement.

To clarify the scope of this a bit:

  • the log message only appears in the top-level log file, not in the individual software installation logs (see https://easybuild.readthedocs.io/en/latest/Logfiles.html);

    • as a consequence, tokens are not included in the partial log files that are uploaded into a gist when using --upload-test-report in combination with --from-pr, nor in the installation logs that are copied to the software installation directories;
  • the message is only logged when using --debug, so it will not appear when using the default EasyBuild configuration (only info messages are logged by default);

  • the log message is triggered via --from-pr, but also via various other GitHub integration options like --new-pr, --merge-pr, --close-pr, etc., but usually only appears in the temporary log file that is cleaned up automatically as soon as eb completes successfully;

  • you may have several debug log files that include your GitHub token in /tmp (or a different location if you've set the --tmpdir EasyBuild configuration option) on the systems where you use EasyBuild, but they are located in a subdirectory that is only accessible to your account (permissions set to 700);

  • the only way that a log file that may include your token could have been made public is if you shared it yourself, for example by copying the contents of the log file into a gist manually, or by sending a log file to someone;

  • for log files uploaded to GitHub, your token would be revoked automatically when GitHub notices it (which is what happened to @zao)

We strongly encourage that you revoke the GitHub tokens you are using currently, via https://github.com/settings/tokens, and to replace them using a new token (using eb --install-github-token --force).

(this PR also includes the fixes from #3212 and #3226 which is required to get the full test suite to pass)

@boegel boegel added the bug fix label Mar 16, 2020
@boegel boegel added this to the 4.1.2 milestone Mar 16, 2020
Copy link
Member

@migueldiascosta migueldiascosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@migueldiascosta migueldiascosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@migueldiascosta
Copy link
Member

Going in, thanks @boegel!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants