Issues with Mapkey Function

[krishmoodbidri]

Description
The Mapkey function in the Workingdir struct (tg123/sshpiper/plugin/internal/workingdir/workingdir.go) is misaligned with the intended configuration for SSH authentication. The current implementation reads the entire userAuthorizedKeysFile and iterates through each key using ssh.ParseAuthorizedKey, comparing each key with the provided pub parameter. This approach is redundant and inefficient, given that the configuration structure only allows specifying a single private key for upstream SSH authentication, which is hardcoded to id_rsa.

Current Behavior

1. The function reads the entire userAuthorizedKeysFile.
2. It iterates through each key in the file using ssh.ParseAuthorizedKey.
3. For each key, it compares it with the provided pub parameter.
4. If a match is found, it returns the contents of userKeyFile.
5. If no match is found after checking all keys, it returns an error.

Proposed Change

1. Mapkey function shouldn’t be looking through authorized_keys file
2. It should just use the defined key to authenticate to the upstream.

We have demonstrated this with our code change.  We can set the key to id_ecdsa and just return that from mapkey and we get the desired behavior of using ecdsa for the upstream auth. 

Overall we'd like to know the intended purpose of this function and even the necessity of it.

[tg123]

it is nice to have configs for working dir

that is the very first version of sshpiper like back in 2014

everything was designed under convention over configuration

yaml plugin was introduced later to cover the dark side of working dir

[krishmoodbidri]

Thank you for responding.
But im not sure I follow the need for the Mapkey Function. We changed it to return a success each time and that worked just fine.

func (w *Workingdir) Mapkey(pub []byte) ([]byte, error) {
        }

        var authedPubkey ssh.PublicKey
+      log.Debugf("pub: %v", pub)
        for len(rest) > 0 {
                authedPubkey, _, _, rest, err = ssh.ParseAuthorizedKey(rest)

                if err != nil {
                        return nil, err
                }
+               log.Debugf("authedPubkey: %v",authedPubkey)

                if bytes.Equal(authedPubkey.Marshal(), pub) {
                        log.Infof("found mapping key %v", w.fullpath(userKeyFile))
                        return w.Readfile(userKeyFile)
                }
        }
+        return w.Readfile(userKeyFile)
        return nil, fmt.Errorf("no matching key found")
 }

[tg123]

seem your Mapkey does not check downstream's key, no matter who the client is, you just map it to your private key
this makes your ssh open to public, anyone can ssh with any random key

[krishmoodbidri]

Our motivation for the code change was that the original code is hardcoded to look for id_rsa and our users could have any of the other key options. The code change snippet is just for our dev cluster and is definitely not something we'd have in production.

Ideally Mapkey should look at the downstream key and map it accordingly based on keytype.

I tested with id_rsa and id_ecdsa between downstream to proxy; and that worked fine; but from proxy to upstream, it always defaults to id_rsa (which is where original code breaks - since we're using id_ecdsa [bright cluster])

[tg123]

maybe change to
for key in [id_rsa, id_ecdsa, ... ] -> return readfile if exists?

[krishmoodbidri]

So here’s a follow up to our workflow; we proceeded with suggestion re. yaml plugin.

Yaml plugin worked fine for almost everything we’re trying to accomplish with sshpiper.

The only functionality it doesn’t support is the lookup of a user in groups and we’ve added that to our codebase, will submit a PR soon.