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

Explicitly avoid committing SSH key to gh-pages #2018

Merged
merged 13 commits into from
Jan 20, 2023
Merged

Conversation

mortenpi
Copy link
Member

Rather than writing the key to the repository, let's write it to homedir(). I think ideally we would avoid writing a temporary file altogether, but I think the only way to do it then is with ssh-agent?

cc @simeonschaub

@@ -379,14 +379,16 @@ function git_push(
# Get the parts of the repo path and create upstream repo path
user, host, upstream = user_host_upstream(repo)

keyfile = abspath(joinpath(root, ".documenter"))
keyfile = abspath(joinpath(homedir(), ".documenter-identity-file.tmp"))
Copy link
Member

Choose a reason for hiding this comment

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

Better with temp?

Suggested change
keyfile = abspath(joinpath(homedir(), ".documenter-identity-file.tmp"))
keyfile = abspath(joinpath(mktempdir(), ".documenter-identity-file.tmp"))

Copy link
Contributor

@simeonschaub simeonschaub Jan 17, 2023

Choose a reason for hiding this comment

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

Yeah, I think it's not inconceivable someone might try to upload the entire home directory, e.g. on a CI service where there's nothing much there anyways. If we really wanted to be on the safe side, maybe even check if tempdir is a subdirectory of the target and error loudly if it is?

Copy link
Member Author

@mortenpi mortenpi Jan 17, 2023

Choose a reason for hiding this comment

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

tempdir would indeed be the most convenient, but what I was concerned about is that, if somehow it doesn't get cleaned up, it's better to leave the file into /home (where hopefully only the current user can read it), rather than /tmp.

Checking for the subdirectory seems reasonable though.

Copy link
Member

Choose a reason for hiding this comment

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

Could the file not be created as read-write only for current user, then after the key is written, change to read-only for current user?

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 guess the permissions are fine already (user-only rw protects as much). I guess my main concern is some shared system where you might have the keyfile sitting around in the /tmp of some random node. In your home directory you might be more likely to notice it.

@fredrikekre
Copy link
Member

fredrikekre commented Jan 17, 2023

Good idea to add the file to $GIT_DIR/info/exclude (or perhaps use git add -A -- :!.documenter-identity-file.tmp)

diff --git a/src/deploydocs.jl b/src/deploydocs.jl
index 02d572c4f..140d0e09e 100644
--- a/src/deploydocs.jl
+++ b/src/deploydocs.jl
@@ -357,6 +357,9 @@ function git_push(
         end

         # Add, commit, and push the docs to the remote.
+        open(".git/info/exclude", "a") do io
+            println(io, ".documenter-identity.file.tmp")
+        end
         run(`$(git()) add -A .`)
         if !success(`$(git()) diff --cached --exit-code`)
             if !isnothing(archive)

Copy link
Contributor

@simeonschaub simeonschaub left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix! Not very familiar with Documenter internals, but this seems like the right approach to me

@@ -357,7 +365,7 @@ function git_push(
end

# Add, commit, and push the docs to the remote.
run(`$(git()) add -A .`)
run(`$(git()) add -A -- ':!.documenter-identity-file.tmp' ':!**/.documenter-identity-file.tmp'`)
Copy link
Member Author

Choose a reason for hiding this comment

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

@fredrikekre Does this look right to you? :!.documenter-identity-file.tmp' seemed to only exclude the top-level file.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like that works, but I wonder why the first isn't sufficient. In a standard .gitignore file a line with .documenter-identity-file.tmp would exclude all files with that name.

@mortenpi
Copy link
Member Author

Okay, I kinda changed my mind now. I think checking that tmpdir/homedir is a subdirectory of target is not too useful, since it doesn't catch most bad cases. What we might want to do is to check that target is a subdirectory of root though.

On the other hand, the git solution seems to nicely avoid pushing the key file, so I changed back to writing it into root (i.e. docs/ in most cases). I think between homedir() and root, the latter is preferred.

@mortenpi mortenpi changed the title Write SSH keyfile to homedir() Explicitly avoid committing SSH key to gh-pages Jan 18, 2023
@mortenpi mortenpi merged commit 7560548 into master Jan 20, 2023
@mortenpi mortenpi deleted the mp/documenter-key branch January 20, 2023 03:08
mortenpi added a commit that referenced this pull request Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants