Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Update README.rst #11

Merged
merged 9 commits into from
Mar 22, 2021
Merged

Update README.rst #11

merged 9 commits into from
Mar 22, 2021

Conversation

altendky
Copy link
Member

@altendky altendky commented Mar 14, 2021

Draft for:

  • Clarification that secrets are not masked in the log file. (or some other response to the hazard)

@altendky altendky marked this pull request as ready for review March 14, 2021 14:25
@altendky altendky requested a review from a team March 14, 2021 14:25
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

I am ok with this change. It can be merged. Thanks!

Do we really need to full dump inside README?

README.rst Outdated
Comment on lines 143 to 160
'PIPX_BIN_DIR' : '/opt/pipx_bin'
'PIPX_HOME' : '/opt/pipx'
'POWERSHELL_DISTRIBUTION_CHANNEL' : 'GitHub-Actions-ubuntu20'
'PWD' : '/home/runner/work/python-info-action/python-info-action'
'RUNNER_OS' : 'Linux'
'RUNNER_PERFLOG' : '/home/runner/perflog'
'RUNNER_TEMP' : '/home/runner/work/_temp'
'RUNNER_TOOL_CACHE' : '/opt/hostedtoolcache'
'RUNNER_TRACKING_ID' : 'github_eb40fa08-b43b-4cc2-9f54-726413848d06'
'RUNNER_USER' : 'runner'
'RUNNER_WORKSPACE' : '/home/runner/work/python-info-action'
'SELENIUM_JAR_PATH' : '/usr/share/java/selenium-server-standalone.jar'
'SHLVL' : '1'
'SWIFT_PATH' : '/usr/share/swift/usr/bin'
'USER' : 'runner'
'VCPKG_INSTALLATION_ROOT' : '/usr/local/share/vcpkg'
'_' : '/opt/hostedtoolcache/Python/3.9.2/x64/bin/python'
'pythonLocation' : '/opt/hostedtoolcache/Python/3.9.2/x64'
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just start with 2 or 3 env vars and then cut the output ... same for workflow details

below is just an example.,. but i would cut a lot :)

Suggested change
'PIPX_BIN_DIR' : '/opt/pipx_bin'
'PIPX_HOME' : '/opt/pipx'
'POWERSHELL_DISTRIBUTION_CHANNEL' : 'GitHub-Actions-ubuntu20'
'PWD' : '/home/runner/work/python-info-action/python-info-action'
'RUNNER_OS' : 'Linux'
'RUNNER_PERFLOG' : '/home/runner/perflog'
'RUNNER_TEMP' : '/home/runner/work/_temp'
'RUNNER_TOOL_CACHE' : '/opt/hostedtoolcache'
'RUNNER_TRACKING_ID' : 'github_eb40fa08-b43b-4cc2-9f54-726413848d06'
'RUNNER_USER' : 'runner'
'RUNNER_WORKSPACE' : '/home/runner/work/python-info-action'
'SELENIUM_JAR_PATH' : '/usr/share/java/selenium-server-standalone.jar'
'SHLVL' : '1'
'SWIFT_PATH' : '/usr/share/swift/usr/bin'
'USER' : 'runner'
'VCPKG_INSTALLATION_ROOT' : '/usr/local/share/vcpkg'
'_' : '/opt/hostedtoolcache/Python/3.9.2/x64/bin/python'
'pythonLocation' : '/opt/hostedtoolcache/Python/3.9.2/x64'
...

Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

There is still one issue with the readme.

The python_path example is using v1.0.1 which does not support python_path :)

I forgot about this PR and i have created a branch that can be removed once this is merged.

https://github.com/twisted/python-info-action/compare/fix-readme-example-version

@altendky
Copy link
Member Author

I didn't forget about it but... yeah, didn't get back to it. Is snipping the env vars sufficient? Or should other big bits be snipped as well? How about @v1 instead of trying to track the minor or patch releases?

@adiroiban
Copy link
Member

My branch was just an experiment :). I think that is best to track the major version in the examples. so v1.

Also, the env vars snip was just an example. I think that everything should be snipped :)

@altendky
Copy link
Member Author

How about just making it collapsible like in the real thing? If not, I'll snip whenever I get back here.

Some Section
made ya look!

@adiroiban
Copy link
Member

How about just making it collapsible like in the real thing? If not, I'll snip whenever I get back here.

Looks nice ;)

If you don't mind the extra work for this formatting, why not ? :)

But I think a SNIP is good enough .

@altendky
Copy link
Member Author

Oops, I forgot the readme is .rst not .md... (which I picked but... forgot) Oh well, <snip> it is.

@altendky altendky requested a review from adiroiban March 19, 2021 15:16
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

Only a minor comment about the visibility of the GitHub Token.

README.rst Outdated
------

{
"token": "v1.0c46741a32436f591105a1c12ee7000683bfb58e",
Copy link
Member

Choose a reason for hiding this comment

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

Was this token present in the output of the action?

I double checked and the token is hidden in the output.

      "token": "***", 

If this is the real github API token used by the action, and it is displayed in plain text in the output, then this might be a security bug.


Suggested change
"token": "v1.0c46741a32436f591105a1c12ee7000683bfb58e",
"token": "***",

Copy link
Member Author

Choose a reason for hiding this comment

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

I just checked the latest build and as you say it has been masked. Both in the normal log view and the raw log view.

image
image

As could reasonably be expected (though sure, I forgot about this) it is _not_ masked in the log file the action generates. Apparently that's where I grabbed this from. This should definitely be noted in the readme, at a minimum. Perhaps it should be actively masked by our own code.

https://docs.github.com/en/actions/reference/context-and-expression-syntax-for-github-actions#github-context

A token to authenticate on behalf of the GitHub App installed on your repository. This is functionally equivalent to the GITHUB_TOKEN secret. For more information, see "Authenticating with the GITHUB_TOKEN."

https://docs.github.com/en/actions/reference/authentication-in-a-workflow#about-the-github_token-secret

Before each job begins, GitHub fetches an installation access token for the job. The token expires when the job is finished.

I did check and the latest CPython 3.9 Linux log does have a different token as I would expect. So, as long as github.token expires "functionally equivalent" to GITHUB_TOKEN then there isn't any _lasting_ security hazard by this being shown now. I would think that artifacts are the most likely escape hole for the logs after the regular GitHub logs. Since they are not available until after all the builds complete it would seem that this is not a flagrant security issue that we should be up in arms about. But, I would like more consideration of how seriously it should be taken and what steps ought to be taken to mitigate it. I'll raise it in #twisted-dev.

Thanks for pointing this out @adiroiban.

Copy link
Member

Choose a reason for hiding this comment

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

True. Thanks for the analysis.

You can always write the token in a file and upload it the Internet.
You don't need github-info-action for that :)

So at this point, I think that is enough just to add a warning.

Co-authored-by: Adi Roiban <adiroiban@gmail.com>
@altendky altendky marked this pull request as draft March 20, 2021 15:12
@altendky altendky marked this pull request as ready for review March 22, 2021 15:15
@altendky altendky merged commit 7e93916 into main Mar 22, 2021
@altendky altendky deleted the altendky-patch-3 branch March 22, 2021 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants