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

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

Merged
merged 14 commits into from
Mar 8, 2022

Conversation

martinezpl
Copy link
Contributor

@martinezpl martinezpl commented Feb 17, 2022

Problem

The SSH key that is being mounted to a dockerized pip is hardcoded as ~/.ssh/id_rsa. This key format is considered legacy and there's no way to use a different key. Moreover, to get the user's home directory, home env var is being used, which does not cover 100% of cases.

Solution

Compared to previously proposed solution of mounting the whole .ssh directory , adding an option to specify the path is a lightweight approach that shouldn't be problematic.

process.env.HOME is switched to require('os').homedir() to increase system compatibility.

The test I came up with is using a read-only deploy key to download a simple pip package from a private repo created for this purpose. Perhaps such repo should exist in /serverless profile?

Solves: #488, #617
Potentially helps: #560, #272

-edit

Some permission denied errors popped up in the second test run, not sure why?

@pgrzesik
Copy link
Collaborator

Hey @martinezpl, thanks a lot for the proposal here. I think in this particular case we could skip the integration test as it has a bit more complicated setup - otherwise we should create a repo owned by serverless to host that package to install. As for the errors - it seems like they're only happening for this PR, do you have any idea if any change introduced might be causing it?

@martinezpl
Copy link
Contributor Author

Hi @pgrzesik!
Regarding the errors - you can see that the first test batch has only one fail each. Two commits later (prettier + .at() -> .splice() + adding brokenOn('win32')) all tests fail because of the same reason:

EACCES: permission denied, rmdir '/home/runner/.cache/serverless-python-requirements/downloadCacheslspyc/http'

I don't see the code-related reason here?

As for the test, I invited you to that private repo so you can fork it to /serverless and substitute the key if you wish. This also serves as the only test for dockerSsh currently.

Otherwise, I guess it's pretty safe to assume it's gonna work if dockerizePip tests pass, as we're just appending docker commands there.

@pgrzesik
Copy link
Collaborator

Thanks @martinezpl - I'm not sure what is happening then - I'll dive into that probably next week as I'm pretty busy with other things, sorry about the delay

@maxpaj
Copy link

maxpaj commented Feb 24, 2022

Please merge this!

@pgrzesik
Copy link
Collaborator

Hey @martinezpl - I believe I've fixed the CI issue, could you please rebase your branch against current master?

@martinezpl
Copy link
Contributor Author

@pgrzesik permission denied error persists

@pgrzesik
Copy link
Collaborator

pgrzesik commented Mar 2, 2022

Thanks @martinezpl - I'll look into what might be causing this 😬

// Mount necessary ssh files to work with private repos
dockerCmd.push(
'-v',
`${process.env.HOME}/.ssh/id_rsa:/root/.ssh/id_rsa:z`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you check if that change to os.homedir() could be causing these issues with permissions on runner? AFAIK it still takes the result from process.env.HOME but nothing else in this PR looks like the cause of this problem and all other PRs are passing without problems 😕

@martinezpl
Copy link
Contributor Author

@pgrzesik so turns out the new test is blocking all the tests. Perhaps it's some sort of a security measure related to the SSH connection to an external repo?

@pgrzesik
Copy link
Collaborator

pgrzesik commented Mar 7, 2022

Hello @martinezpl - thanks for investigating - it looks like it might be the case. In that situation, I think we should skip adding such test - could we have a simpler test that just validates proper resolution of the command when custom key is involved?

@martinezpl martinezpl requested a review from pgrzesik March 7, 2022 22:11
Copy link
Collaborator

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

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

Thanks @martinezpl - let's keep it this way - thanks a lot for your contribution 👍

@pgrzesik pgrzesik changed the title feature: dockerPrivateKey option to specify path to mounted SSH key feat: Support dockerPrivateKey to specify path to SSH key Mar 8, 2022
@pgrzesik pgrzesik merged commit 915bcad into serverless:master Mar 8, 2022
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.

None yet

3 participants