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

Reporting process errors leaks DOCUMENTER_KEY #1958

Closed
maleadt opened this issue Sep 30, 2022 · 2 comments · Fixed by #1962
Closed

Reporting process errors leaks DOCUMENTER_KEY #1958

maleadt opened this issue Sep 30, 2022 · 2 comments · Fixed by #1962

Comments

@maleadt
Copy link
Contributor

maleadt commented Sep 30, 2022

Documenter.jl currently wraps git commands in try/catch, reporting the failed process exception when something goes wrong. This is very bad, because it may leak the DOCUMENTER_KEY environment variable that the user has set:

Initialized empty Git repository in /tmp/jl_SChxvR/.git/
Load key "/var/lib/buildkite-agent/builds/gpuci-16/julialang/scimlsensitivity-dot-jl/docs/.documenter": invalid format
[email protected]: Permission denied (publickey).
fatal: Could not read from remote repository.
 
Please make sure you have the correct access rights
and the repository exists.
┌ Error: Git failed to fetch [email protected]:SciML/SciMLSensitivity.jl.git
│ This can be caused by a DOCUMENTER_KEY variable that is not correctly set up.
│ Make sure that the environment variable is properly set up as a Base64-encoded string
│ of the SSH private key. You may need to re-generate the keys with DocumenterTools.
└ @ Documenter ~/.cache/julia-buildkite-plugin/depots/ea52448d-f230-4619-b27a-2d98107bd215/packages/Documenter/yf96B/src/Documenter.jl:652
┌ Error: Failed to push:
│   exception =
│    failed process: Process(setenv(`/usr/bin/git fetch upstream`,["DOCUMENTER_KEY=XXXXXXXX", "PATH=YYYY", ...]), ProcessExited(128)) [128]

It would be best to not just dump these exceptions to STDOUT (even if you scrub DOCUMENTER_KEY it may contain other sensitive information). In addition, it's probably best to scrub DOCUMENTER_KEY from the environment as soon as its parsed by Documenter.jl to prevent it from leaking to any other spawned commands.

cc @ChrisRackauckas

@ericphanson
Copy link
Contributor

This doesn't help in the general case, but GitHub Actions scrubs secrets, so at least it doesn't show up there: JuliaEcosystem/PackageAnalyzer.jl#68 (comment)

@fredrikekre
Copy link
Member

Travis did too, even scrubbed clever things like base64 encodings of the secret. That is probably a reasonable feature request for Buildkite, but we can of course try harder in Documenter too.

fredrikekre added a commit that referenced this issue Oct 10, 2022
This patch removes DOCUMENTER_KEY from the environment when it is not
needed. In particular, it is removed from all of makedocs(...) and
removed from all git commands in deploydocs, fixes #1958.
fredrikekre added a commit that referenced this issue Oct 10, 2022
This patch removes DOCUMENTER_KEY from the environment when it is not
needed. In particular, it is removed from all of makedocs(...) and
removed from all git commands in deploydocs, fixes #1958.
fredrikekre added a commit that referenced this issue Oct 10, 2022
This patch removes DOCUMENTER_KEY from the environment when it is not
needed. In particular, it is removed from all of makedocs(...) and
removed from all git commands in deploydocs, fixes #1958.
fredrikekre added a commit that referenced this issue Oct 16, 2022
This patch removes DOCUMENTER_KEY from the environment when it is not
needed. In particular, it is removed from all of makedocs(...) and
removed from all git commands in deploydocs, fixes #1958.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants