Added repo-mappings and drop-extra-header options

[shaunco]

Updated README.md to include the two new options
Fixed build.js to work on windows
Fixed homedir lookup for windows
Moved param names to const vars at the top and replaced all references
Resolves #30

[shaunco]

This solved a 3 day nightmare of a private repo pulling in two other private repos through go modules. Hope it helps!

[mpdude]

@shaunco Thank you for the time you invested to understand this issue and for coming up with a PR! And sorry it took me so long to get back to it, I was a bit busy.

What I like about your approach is that it does not require to use special URLs with made-up hostnames in the packages.json, composer.json etc. files. The different repos are mapped to different hostnames at the git config level, and these hostnames back to the original ones + matching keys in the ssh config. So this is way better than other approaches I've seen where everyone needs to have special ssh config in order to install dependencies.

However, I still feel this makes the action way more complicated and thus harder to maintain (also given the fact that we don't have test coverage :-().

So, wouldn't the wrapper script suggested in #30 solve the issue as well, and possibly be usable in other situations (no GH actions) as well when you need multiple deploy keys?

What is this extra HTTP header thing about?

[mpdude]

@shaunco,

I've seen your comments over at #30, but I'd like to continue the discussion here since it applies to the proposal you made.

I agree there's value in using configuration techniques built into git to avoid an additional wrapper script that's platform-specific and needs to be shipped somehow.

So, could we – for the time being – try to reduce this PR somewhat in complexity, by removing things that are not absolutely necessary? Or, to put it another way, things where I'm not yet sure if they are Go-specific?

Apart from that, the main two points I'd like to dicuss are a) private keys on disk and b) action configuration/syntax.

a) Regarding private keys, I might be wrong, but I've read this PR as if the private keys are written to keyfiles on disk. This is something I've tried to avoid in this action in the first place: Keys are decrytped by GitHub (from secrets), passed into the worker and from there directly into ssh-agent. I think that's important so no other (rogue) action can "steal" the private keys.

This should not be a show-stopper for this PR, however, since #30 showed that choosing the preferred key in ssh can also be done based on the public key part. That is, the corresponding line from ssh-add -l can be put into a file and be used as the key file (ssh -i ...) as well. The same would probably work for SSH config files.

b) Regarding action configuration/syntax, I find the new repo_mappings key hard to understand. I'm not sure why this is – maybe it's that the configuration lines must match the order of keys given before; or that the lines to this argument have too little context?

So, what if we used key comments like in my suggestion in #30? We could go through ssh-add -l, and for every line that contains a key comment in a particular format (e. g. Depoy key for git@github.com/org/repo.git?) we'd issue the git config --global ... command and append a section to the SSH config?

That might make the order of keys irrelevant and remove the repo_mappings altogether?

What do you think?

[shaunco]

Nothing in this is go specific, it just happens to be a technique that works everywhere - including go.

The "extra HTTP header thing" is described in the README.md

For writing keys to disk. I tried a few different ways to get ssh-add to take the keys via stdin, which would be preferred, but was unsuccessful. The technique in #30 still requires that you construct SSH keys in a very particular way, which is something I would like to avoid. I can spend some more time this week trying to get ssh-add from stdin to work.

For repo-mappings, it is also fully documented in the README.md. I created a second section for these to avoid breaking backward compatibility with anyone already using this github action.

I'm really not a fan of hiding configuration inside ssh key comments. That makes it really hard for anyone to look at their github action and figure out what is going on, as they'd need to also have access to secrets to see those comments. Having simple configuration of key x is for repo x, key y is for repo y, etc, keeps all knowledge in one place. That said, if having two side by side lists is adding to the confusion, we can combine them:

            - uses: webfactory/ssh-agent@v0.4.0
              with:
                  ssh-private-key: |
                        github.com/OWNERX/REPO1 ${{ secrets.FIRST_KEY }}
                        bitbucket.com/OWNERY/REPO2 ${{ secrets.NEXT_KEY }}
                        github.com/OWNERX/REPO3 ${{ secrets.ANOTHER_KEY }}

however, that will add quite a bit of new complexity to this action's parsing of the keys.

[ryanzidago]

@shaunco

I tried your fork and it worked elegantly, I really like the idea of having to map the SSH-keys to their corresponding repositories. It is easy to understand and I like the expliciteness of this approach.

The instructions you provided were clear enough that I knew the keys needed to be in the same order as their repositories. I didn't even need to drop the extra http header.

Many thanks 🙏

[Sinclert]

Hey @mpdude 👋🏻

I have been following this PR for a while, and reading through the fantastic work that @shaunco put together. Being able to set up multiple SSH keys to access a wide range of private repositories, instead of being limited to one, would be quite convenient for a lot of us.

Obviously, we could use the git-repo-mapping branch on Shaunco's fork, but it seems a bit hacky. It would be nicer if these changes were merged into this nice GitHub Action.

Is this going to happen anytime soon?

[mpdude]

Dear @shaunco, dear @Sinclert, @ryanzidago and possibly others,

thank you all for your patience and sorry it sometimes takes me longer to get back to this than I'd like to see myself. The deploy key issue is not a pressing issue for my own organisation, so sometimes other issues grab more attention and I am limited in the time I can allocate to this. Please do not understand this as disinterest in your contributions, I definitely appreciate the time and energy you invest!

In order to get this along, could we first address a few small changes that seem not 100% related to the basic issue?

@shaunco if you'd like to create dedicated PRs for those, we'd have appropriate attribution in the code.

1. The improvements to the README file when it comes to documenting for other developers how the input variables can be passed
2. The change from HOME to os.homedir() – it that all it takes to support running on Windows, or what does this improve?
3. The Authorization extra header thing – not sure if this is really related to this action and the basic goal of setting up the SSH agent. I don't really understand (yet) what this is about, Auth in extraheader makes it difficult to override actions/checkout#162 seems to be related. (If it is about a disputed way how actions/checkout configures things, I am not sure this action here should be the place to contain a switch to revert/change these things.)

[shaunco]

1. My PR had a fully updated README with substantial details on internal workings, configuration options, and more
2. Yes, this was the change needed to get it working on Windows for me
3. For the extra header, again, it is fully documented in the updated README

If this is really something that is not of interest to you or your team, or you don't have time to merge it, I will happily fork. We have about 40 repos that now depend on my branch at this point, so it isn't a huge deal to make it an proper fork.

[shaunco]

@mpdude #59 isn't working for me in go get or pip operations, and it requires that I rebuild about 20 deploy keys to have the required comment. I will dig in to it later this week.

[mpdude]

Ok, looking forward to your reply regarding pip/go.

Note that the current code does not contain that HTTP header thing I did not fully understand and which I am unsure about has/should have anything to do with this action.

Regarding keys: What if we added an additional configuration option using URL -> key fingerprints?

[shaunco]

The HTTP header was unrelated. That was to fix issues with git tag creation in a different repo.

The keys all come from org secrets. Our developers don't have access to the fingerprints, and the fingerprint could change at any time (if a key gets rolled). It makes no sense to hardcode a key fingerprint in a github action yml file when the yml is not in control of the key. The method I used in #38 of just having a matched list worked great and was very easy to understand.

[mpdude]

Glad to hear that HTTP header thing was unrelated 👍.

I understand that an explicit mapping is not an option in your case. I am sorry when the transition to how it's implemented right now causes additional work for you.

However, I don't think that correlating keys and URLs from two input arguments via line number offset is a sane way of configuring things, sorry.

[shaunco]

Two options, that are side by side, is not sane ... but tying together 1 option in a yml file with 1 option that is configured in repo settings, is?