[PATCH 1/1] support adding SSH keys with pre-existing options

[Philip Paeps]

From: Philip Paeps <phi...@FreeBSD.org>

When prepending our options to a key, check if the key already has any
options. In this case put a comma after the end of our options instead
of a space.

To differentiate between keys with and without options, we check if they
start with (ecdsa|(sk-)?ssh)-. This supports all keys supported by
OpenSSH at the time of this commit. If a different algorithm is
introduced to OpenSSH in future, this regular expression should be
updated. Adding a full key parser to this subroutine would be overkill.

Submitted by: Gleb Smirnoff <gle...@FreeBSD.org>
---
src/triggers/post-compile/ssh-authkeys | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/triggers/post-compile/ssh-authkeys b/src/triggers/post-compile/ssh-authkeys
index cd59aec..c4cb39b 100755
--- a/src/triggers/post-compile/ssh-authkeys
+++ b/src/triggers/post-compile/ssh-authkeys
@@ -137,6 +137,7 @@ sub optionise {
return '';
}
chomp(@line);
- return "command=\"$glshell $user" . ( $kfn ? " $f" : "" ) . "\",$auth_options $line[0]";
+ return "command=\"$glshell $user" . ( $kfn ? " $f" : "" ) . "\",$auth_options" .
+ ($line[0] =~ /^(ecdsa|(sk-)?ssh)-/ ? " " : "," ) . $line[0];
}

--
2.39.3 (Apple Git-145)

[Sitaram Chamarty]

On Mon, Sep 25, 2023 at 09:30:43PM +0800, Philip Paeps wrote:
> From: Philip Paeps <phi...@FreeBSD.org>
>
> When prepending our options to a key, check if the key already has any
> options. In this case put a comma after the end of our options instead
> of a space.

First, consider that ssh-authkeys, like a lot of non-core
gitolite, can be overridden by any installation:

- set LOCAL_CODE in ~/.gitolite.rc on the server to, say,
$ENV{HOME}/local
- add/edit ~/local/triggers/post-compile/ssh-authkeys and it
will override the shipped ssh-authkeys file.

I know that raises the concern that upstream may change in some
manner at some point in future, and that will need to be rolled
in to the local-code version, but ssh-authkeys is unlikely to
have significant functionality enhancements in its limited role.

But if you *really* need this, it's best to whitelist the
options allowed. I am not comfortable allowing a user to
specify anything he wants in that line, unchecked.

I just added a key with this option

port-forwarding,permitopen="127.0.0.1:22"

in it and it overrode the no-port-forwarding in the default auth
options gitolite sets up.

sitaram

>
> To differentiate between keys with and without options, we check if they
> start with (ecdsa|(sk-)?ssh)-. This supports all keys supported by
> OpenSSH at the time of this commit. If a different algorithm is
> introduced to OpenSSH in future, this regular expression should be
> updated. Adding a full key parser to this subroutine would be overkill.
>
> Submitted by: Gleb Smirnoff <gle...@FreeBSD.org>
> ---
> src/triggers/post-compile/ssh-authkeys | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/triggers/post-compile/ssh-authkeys b/src/triggers/post-compile/ssh-authkeys
> index cd59aec..c4cb39b 100755
> --- a/src/triggers/post-compile/ssh-authkeys
> +++ b/src/triggers/post-compile/ssh-authkeys
> @@ -137,6 +137,7 @@ sub optionise {
> return '';
> }
> chomp(@line);
> - return "command=\"$glshell $user" . ( $kfn ? " $f" : "" ) . "\",$auth_options $line[0]";
> + return "command=\"$glshell $user" . ( $kfn ? " $f" : "" ) . "\",$auth_options" .
> + ($line[0] =~ /^(ecdsa|(sk-)?ssh)-/ ? " " : "," ) . $line[0];
> }
>
> --
> 2.39.3 (Apple Git-145)
>

> --
> You received this message because you are subscribed to the Google Groups "gitolite" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to gitolite+u...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/gitolite/20230925133043.61343-2-philip%40trouble.is.

[Philip Paeps]

On 2023-09-26 00:56:52 (+0800), Sitaram Chamarty wrote:
> On Mon, Sep 25, 2023 at 09:30:43PM +0800, Philip Paeps wrote:
>> From: Philip Paeps <phi...@FreeBSD.org>
>>
>> When prepending our options to a key, check if the key already has
>> any
>> options. In this case put a comma after the end of our options
>> instead
>> of a space.
>
> First, consider that ssh-authkeys, like a lot of non-core
> gitolite, can be overridden by any installation:
>
> - set LOCAL_CODE in ~/.gitolite.rc on the server to, say,
> $ENV{HOME}/local
> - add/edit ~/local/triggers/post-compile/ssh-authkeys and it
> will override the shipped ssh-authkeys file.

This is good to know. Thank you!

I wonder why we didn't go that route. We're generally wary of local
patches. We have plenty of accumulated hacks already. ;-)

If we can make it work with a local override, the patch is clearly
unnecessary.

> I know that raises the concern that upstream may change in some
> manner at some point in future, and that will need to be rolled
> in to the local-code version, but ssh-authkeys is unlikely to
> have significant functionality enhancements in its limited role.

Adapting configuration to upstream changes is a lot easier than adapting
patches. And, as you write, this is not likely to become a volatile
area of the codebase.

> But if you *really* need this, it's best to whitelist the
> options allowed. I am not comfortable allowing a user to
> specify anything he wants in that line, unchecked.

I agree.

I'll see if I can make configuration changes to make the need for this
patch go away. If it turns out we do still need it, I'll expand the
patch to include a (configurable) whitelist of options. I'll report
back either way.

> I just added a key with this option
>
> port-forwarding,permitopen="127.0.0.1:22"
>
> in it and it overrode the no-port-forwarding in the default auth
> options gitolite sets up.

I'll test this. We have system-wide configuration to disable things
like port-forwarding, but I believe in defence in depth.

Thanks for the review and the tips.

Best wishes.
Philip

--
Philip Paeps
Senior Reality Engineer
Alternative Enterprises

[Sitaram Chamarty]

I forgot to add... I had to cheat by adding a "sleep 30" at the
top of gitolite-shell, but in real life a potential attacker
might choose some long running operation to keep the connection
open.

> Thanks for the review and the tips.

You're welcome!