Mount the entire user SSH directory into build container #489

jacksgt · 2020-03-09T08:38:11Z

This enables the user to use an key file format (RSA, ED25519, ...).
Additionally, it allows more complex workflows (such as different SSH keys for
specfic sites, such as Github or Bitbucket), since the .ssh/config
file is also mounted into the container.

Fixes #488

miketheman

Looks like this may work, however I don’t think any of our existing tests cover this behavior. Have you considered adding some to ensure this works as intended? The tests.js file has a bunch tagged with canUseDocker as examples.

lib/pip.js

jacksgt · 2020-03-09T15:56:08Z

Adding some tests sounds like a good idea, however I'm not very experienced with JS tests.
Basically what I would do is the following:

create a file in ~/.ssh/id_foobar
run sls with dockerizePip option
check if the file is mounted inside the docker container

jacksgt · 2020-03-16T12:05:39Z

@miketheman Hey, I tried adding a test, but I'm not sure why the test does not produce a ZIP file (and subsequently fails).
Could you look into it?

jacksgt · 2020-03-18T16:17:19Z

@miketheman After a few more attempts I got it working. Could you review the PR again when you find the time? Thanks.

miketheman

This looks great, thanks for taking the time to write the test!
I know that this branch is out of date from master and @bsamuel-ui has been focusing on changing the CI/CD from CircleCI to GitHub Actions - so I'd recommend rebasing/merging after that PR is on master so as to exercise this one more time in the new CI before merging.

jacksgt · 2020-04-14T04:54:10Z

Since there hasn't been any activity on the github actions PR for a two weeks now, could we merge this one in anyway?

bsamuel-ui · 2020-09-19T14:53:17Z

@jacksgt I was looking at that merge conflict, I could sort of resolve that through the editor, but I'm just as likely to screw it up. You just need to make sure your new test is present and is async, and as soon as you can do that we can merge this.

This enables the user to use an key file format (RSA, ED25519, ...). Additionally, it allows more complex workflows (such as different SSH keys for specfic sites, such as Github or Bitbucket), since the .ssh/config file is also mounted into the container. Fixes serverless#488

This test makes sure the plugin actually mounts the SSH directory into the container, if the dockerizePip and dockerSsh are given. It does this by creating a new known_hosts file and placing the RSA fingerprint of Github inside, then installing a package via Git, which requires this fingerprint to be present in known_hosts. If it is not present, the process will fail. This patch also adds minor debugging output, in case no requirements.txt is found.

jacksgt · 2020-09-21T06:37:05Z

@bsamuel-ui Hey, thanks for getting back on this one and keeping the project going!

I rebased on master, but now the test is failing with a git error code that seems to indicate there is no public/private ssh key present on the host machine.
Since the testcase worked on the old CI, I assume that something changed in the setup.
Any idea how to fix this?

bsamuel-ui · 2020-09-21T14:05:33Z

Seeing a bunch of issues here that aren't in other branches. I'm going to take some time to dig into it on Tuesday per #550

bsamuel-ui · 2020-09-22T20:17:37Z

test.js

+    t.true(zipfiles.includes(`boto3/__init__.py`), 'boto3 is packaged via ssh');
+    t.end();
+  },
+  { skip: !canUseDocker() }


Have you tested this on Windows?

Nope, not sure if its gonna work on Windows.

Then the skip section should be skipping the test on Windows, and you can create an issue noting that this functionality doesn't work on Windows yet.

tonythree · 2021-04-15T08:46:04Z

Hey, having this issue as well.
Is this going to be merged at some point?
also, is there any workaround for this?
Thanks a lot for the hard work.

EDIT
Tried to Workaround by simply creating an id_rsa key and attaching as a new key to github.
https://docs.github.com/en/github/authenticating-to-github/generating-a-new-ssh-key-and-adding-it-to-the-ssh-agent

ssh-keygen -t rsa -b 4096 -C "[email protected]"

But having the same issue. So this does not solve the problem :(

martinezpl · 2022-02-03T10:19:59Z

Any move with this? Something I can help with to have this eligible for merge? This is the current source code:

// Mount necessary ssh files to work with private repos
      if (options.dockerSsh) {
        // Mount necessary ssh files to work with private repos
        dockerCmd.push(
          '-v',
          `${process.env.HOME}/.ssh/id_rsa:/root/.ssh/id_rsa:z`,
          '-v',
          `${process.env.HOME}/.ssh/known_hosts:/root/.ssh/known_hosts:z`,
          '-v',
          `${process.env.SSH_AUTH_SOCK}:/tmp/ssh_sock:z`,
          '-e',
          'SSH_AUTH_SOCK=/tmp/ssh_sock'
        );
      }

RSA keys are considered legacy, I don't think it's reasonable to continue expecting (or forcing, pretty much) this format.

EDIT Tried to Workaround by simply creating an id_rsa key and attaching as a new key to github. https://docs.github.com/en/github/authenticating-to-github/generating-a-new-ssh-key-and-adding-it-to-the-ssh-agent
But having the same issue. So this does not solve the problem :(

Creating a new RSA key and registering it to my GH account worked for me.

pgrzesik · 2022-02-03T10:30:52Z

Hey @martinezpl - this PR has been dead for a long time - if you'd like to take over and propose a new one, feel free to do so 💯

pgrzesik · 2022-09-27T18:28:03Z

Hey @jacksgt - it's been a long time since this PR was proposed. I'm going to close it, if you feel like the issue is valid, please open a new issue or a new PR against the latest main branch. Thanks 🙇

jacksgt force-pushed the mount-ssh-dir branch from b14b862 to 50992c0 Compare March 9, 2020 08:39

miketheman reviewed Mar 9, 2020

View reviewed changes

lib/pip.js Show resolved Hide resolved

miketheman added the Docker label Mar 9, 2020

jacksgt force-pushed the mount-ssh-dir branch from 50992c0 to 80faab9 Compare March 9, 2020 09:09

jacksgt force-pushed the mount-ssh-dir branch 7 times, most recently from e1d74fb to 43d0469 Compare March 16, 2020 10:42

jacksgt force-pushed the mount-ssh-dir branch 8 times, most recently from d285c58 to 3be471f Compare March 18, 2020 15:38

miketheman previously approved these changes Mar 29, 2020

View reviewed changes

miketheman added the bug-fix PR that fixes a bug. label Mar 29, 2020

jacksgt force-pushed the mount-ssh-dir branch 2 times, most recently from 040adee to 12326b5 Compare April 3, 2020 06:44

jacksgt force-pushed the mount-ssh-dir branch from 12326b5 to 2c12a7a Compare April 23, 2020 15:05

jacksgt force-pushed the mount-ssh-dir branch from 270ab71 to 0217946 Compare September 20, 2020 09:18

bsamuel-ui mentioned this pull request Sep 21, 2020

5.1.1 / 5.2.0 release planning #550

Closed

bsamuel-ui reviewed Sep 22, 2020

View reviewed changes

Merge branch 'master' into mount-ssh-dir

814f728

pgrzesik dismissed miketheman’s stale review via 814f728 August 11, 2021 13:27

martinezpl mentioned this pull request Feb 17, 2022

feat: Support dockerPrivateKey to specify path to SSH key #674

Merged

pgrzesik closed this Sep 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mount the entire user SSH directory into build container #489

Mount the entire user SSH directory into build container #489

jacksgt commented Mar 9, 2020

miketheman left a comment

jacksgt commented Mar 9, 2020

jacksgt commented Mar 16, 2020

jacksgt commented Mar 18, 2020

miketheman left a comment

jacksgt commented Apr 14, 2020

bsamuel-ui commented Sep 19, 2020

jacksgt commented Sep 21, 2020

bsamuel-ui commented Sep 21, 2020

bsamuel-ui Sep 22, 2020

jacksgt Sep 24, 2020

bsamuel-ui Sep 24, 2020

tonythree commented Apr 15, 2021 •

edited

Loading

martinezpl commented Feb 3, 2022 •

edited

Loading

pgrzesik commented Feb 3, 2022

pgrzesik commented Sep 27, 2022

Mount the entire user SSH directory into build container #489

Mount the entire user SSH directory into build container #489

Conversation

jacksgt commented Mar 9, 2020

miketheman left a comment

Choose a reason for hiding this comment

jacksgt commented Mar 9, 2020

jacksgt commented Mar 16, 2020

jacksgt commented Mar 18, 2020

miketheman left a comment

Choose a reason for hiding this comment

jacksgt commented Apr 14, 2020

bsamuel-ui commented Sep 19, 2020

jacksgt commented Sep 21, 2020

bsamuel-ui commented Sep 21, 2020

bsamuel-ui Sep 22, 2020

Choose a reason for hiding this comment

jacksgt Sep 24, 2020

Choose a reason for hiding this comment

bsamuel-ui Sep 24, 2020

Choose a reason for hiding this comment

tonythree commented Apr 15, 2021 • edited Loading

martinezpl commented Feb 3, 2022 • edited Loading

pgrzesik commented Feb 3, 2022

pgrzesik commented Sep 27, 2022

tonythree commented Apr 15, 2021 •

edited

Loading

martinezpl commented Feb 3, 2022 •

edited

Loading