-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Support multiple SSH keys for the same host #1433
Conversation
The following is the coverage report on pkg/.
|
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this is gonna work, see my comment 👼
@@ -305,8 +307,8 @@ func TestSSHFlagHandlingThrice(t *testing.T) { | |||
HostName github.com | |||
IdentityFile %s | |||
Port 22 | |||
Host gitlab.com | |||
HostName gitlab.com | |||
Host github.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does having multiple exact same Host
works ? I read a bit man ssh_config
and I am not sure it's documented (either works or not), but I have a fear it doesn't as it's gonna pick up the first one and if it fails, it will considered it failed.
I think it's possible to pass multiple IdentityFile
though, like
Host github.com
IdentityFile /etc/ssh/my_project_1_github_deploy_key
IdentityFile /etc/ssh/my_project_2_github_deploy_key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right. Reading https://superuser.com/q/366649 it looks like the correct way to handle this is with multiple differently-named Host
s which each specify the same HostName
I've updated the PR to do this, where each Host
includes the secret name to dedupe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems I was wrong! Thanks e2e tests! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔
b4838fc
to
fcd4d41
Compare
The following is the coverage report on pkg/.
|
fcd4d41
to
b853cc6
Compare
The following is the coverage report on pkg/.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Before this change, if users provided two secrets annotated to target the same host,
creds-init
would fail to generate the ssh config.This use case is entirely valid, to allow users to rotate keys for instance.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Release Notes