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

add .ssh/authorized_keys support for --sshcopyid #153

Merged
merged 1 commit into from Jun 3, 2020

Conversation

anarcat
Copy link
Contributor

@anarcat anarcat commented Feb 25, 2020

We retain backwards compatibility, that is: we use keys from the agent
by default. But if unavailable, we tap into the ~/.ssh/authorized_keys
file (or whatever is specified by the $AUTHORIZED_KEYS_SOURCE
environment).

The target SSH directory can be changed with $AUTHORIZED_KEYS_TARGET.

Closes: #151

We retain backwards compatibility, that is: we use keys from the agent
by default. But if unavailable, we tap into the ~/.ssh/authorized_keys
file (or whatever is specified by the $AUTHORIZED_KEYS_SOURCE
environment).

The target SSH directory can be changed with $AUTHORIZED_KEYS_TARGET.

Closes: grml#151
Copy link
Member

@mika mika 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 providing the PR, @anarcat ! Finally managed to find some time looking into this, sorry for the delay!

I'm wondering though, whether we're putting too much magic into this, because what if someone expects --ssh-copy-id to only use keys provided via agent, but it fails and then it suddenly uses ~/.ssh/authorized_keys instead? Also maybe it might not be clear upfront that .ssh/authorized_keys needs to be set up?

On my very first try (without looking into the code ahead) I assumed it would take my existing ~/.ssh/id_rsa.pub file, so I didn't set up a ~/.ssh/authorized_keys file, while this didn't work as I thought it would. :)

Hmmmmm, what about instead introducing an option like --ssh-authfile or so (not really sure + happy with the naming yet :-/) which does what you implemented here?

Any other opinions (/cc @jkirk + @suntong)?
How would you like to use it in actual production usage?

eerror "Error: copying '$AUTHORIZED_KEYS_SOURCE' to '$AUTHORIZED_KEYS_TARGET' failed"
eend 1
bailout 1
fi
else
eerror "Could not open a connection to your authentication agent or the agent has no identites."
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to adjust the error message in line 1813 accordingly, so it's clear that either ssh-add -L or taking over ~/.ssh/ stuff didn't work?

@@ -1809,6 +1811,17 @@ iface ${interface} inet dhcp
eend 1
bailout 1
fi
elif [ -f "$AUTHORIZED_KEYS_SOURCE" ]; then
einfo "copying '$AUTHORIZED_KEYS_SOURCE' to '$AUTHORIZED_KEYS_TARGET' as requested via --sshcopyid option."
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: s/copying/Copying/ to make it consistent?

einfo "copying '$AUTHORIZED_KEYS_SOURCE' to '$AUTHORIZED_KEYS_TARGET' as requested via --sshcopyid option."
mkdir -p "$AUTHORIZED_KEYS_TARGET"
chmod 0700 "$AUTHORIZED_KEYS_TARGET"
if cp "$AUTHORIZED_KEYS_SOURCE" "$AUTHORIZED_KEYS_TARGET" ; then
Copy link
Member

Choose a reason for hiding this comment

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

Should we ensure via chmod call that the authorized_keys file has the appropriate permissions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes, or cp -p and just assume?

Copy link
Member

Choose a reason for hiding this comment

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

The cp -p sounds like a good idea! 👍

@@ -1809,6 +1811,17 @@ iface ${interface} inet dhcp
eend 1
bailout 1
fi
elif [ -f "$AUTHORIZED_KEYS_SOURCE" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

Hm, what about installing ~/.ssh/id*.pub as ~/.ssh/authorized_keys if no ~/.ssh/authorized_keys exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean all of them? that's kind of a different use case

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd agree with @anarcat here, but @mika's question on ~/.ssh/authorized_keys does make sense, as it exists more on the receiving end. From a dev machine that only connects out, to vpn, git repo etc, it'll be not unusual to be empty.

grml-debootstrap Show resolved Hide resolved
@anarcat
Copy link
Contributor Author

anarcat commented Apr 2, 2020

Hmmmmm, what about instead introducing an option like --ssh-authfile or so (not really sure + happy with the naming yet :-/) which does what you implemented here?

i'm fine with that.

How would you like to use it in actual production usage?

we use this to bootstrap hosts on hetzner. we give them ssh keys, they give us a rescue shell with ~/.ssh/authorized_keys populated with those keys, and we debootstrap from there. it's the reason why we use that file.

the other reason is that we might not want to use ~/.ssh/*.pub because those can mean a lot of things. i have a bunch of public keys here and they're not all relevant to a box i'm building. so we leave the creation of that file to the caller.

@mika
Copy link
Member

mika commented Apr 2, 2020

Hmmmmm, what about instead introducing an option like --ssh-authfile or so (not really sure + happy with the naming yet :-/) which does what you implemented here?

i'm fine with that.

Excellent! Do you have a nice naming for the option in mind? Similar to --ssh-copyid it should be like --ssh-copyauth or so?

How would you like to use it in actual production usage?

we use this to bootstrap hosts on hetzner. we give them ssh keys, they give us a rescue shell with ~/.ssh/authorized_keys populated with those keys, and we debootstrap from there. it's the reason why we use that file.

Ah, excellent use case + idea!

the other reason is that we might not want to use ~/.ssh/*.pub because those can mean a lot of things. i have a bunch of public keys here and they're not all relevant to a box i'm building. so we leave the creation of that file to the caller.

Fair enough, let's forget my ~/.ssh/*.pub idea and let's make sure we have proper documentation and usage instructions for this instead!

@anarcat
Copy link
Contributor Author

anarcat commented Apr 2, 2020

--ssh-copyauth

sounds fine.

Fair enough, let's forget my ~/.ssh/*.pub idea and let's make sure we have proper documentation and usage instructions for this instead!

i admit i'm a bit under the weather now so it might take some time before i get the time to look at this again. for now, we do this as a hook and i just figured i would bring that idea back upstream to see if it could land.

i'll see if i can find the time to reshuffle this, but in the meantime feel free to beat me to it. :)

@mika mika merged commit b5efac4 into grml:master Jun 3, 2020
@jkirk
Copy link
Contributor

jkirk commented Jun 3, 2020

Fair enough, let's forget my ~/.ssh/*.pub idea and let's make sure we have proper documentation and usage instructions for this instead!

i admit i'm a bit under the weather now so it might take some time before i get the time to look at this again. for now, we do this as a hook and i just figured i would bring that idea back upstream to see if it could land.

i'll see if i can find the time to reshuffle this, but in the meantime feel free to beat me to it. :)

What about naming this feature / option --copypubkeys (or so). But this would be a different use case then.

mika added a commit that referenced this pull request Jun 3, 2020
…opyauth

Related to commit 07e835e and the discussion within
#153

If execution of --sshcopyid fails, then user might want to be aware of
it. So instead of implementing the copying of .ssh/authorized_keys as
fallback of --sshcopyid, let's provide it via cmdline option
--sshcopyauth.

Closes: #153
@anarcat anarcat deleted the ssh-authorized-keys branch June 3, 2020 13:23
mika added a commit that referenced this pull request Jun 3, 2020
…opyauth

Related to commit 07e835e and the discussion within
#153

If execution of --sshcopyid fails, then user might want to be aware of
it. So instead of implementing the copying of .ssh/authorized_keys as
fallback of --sshcopyid, let's provide it via cmdline option
--sshcopyauth.

Reviewed-by: Chris Hofstaedtler
Closes: #153
mika added a commit that referenced this pull request Jun 3, 2020
…opyauth

Related to commit 07e835e and the discussion within
#153

If execution of --sshcopyid fails, then user might want to be aware of
it. So instead of implementing the copying of .ssh/authorized_keys as
fallback of --sshcopyid, let's provide it via cmdline option
--sshcopyauth.

Reviewed-by: Chris Hofstaedtler
Closes: #153
mika added a commit that referenced this pull request Jun 3, 2020
…opyauth

Related to commit 07e835e and the discussion within
#153

If execution of --sshcopyid fails, then user might want to be aware of
it. So instead of implementing the copying of .ssh/authorized_keys as
fallback of --sshcopyid, let's provide it via cmdline option
--sshcopyauth.

Reviewed-by: Chris Hofstaedtler
Reviewed-by: Darshaka Pathirana
Closes: #153
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

find /root/.ssh/authorized_keys on top of agent keys
4 participants