luci-app-sshtunnel: Improve keys

[stokito]

Maintainer: me (?)
Tested: OpenWrt master on VirtualBox amd64
Description:

The luci-app-sshtunnel has a problem.
I made a button to generate an ssh key. Internally it calls ssh-keygen from openssh.
The ssh-keygen generates two files: an id key like id_ed25519 and its public key id_ed25519.pub.
Then the luci app shows the pub key to a user so it can be copied and pasted on a SSH server.

But then I added the dropbear support. The app checks if the openssh ssh-keygen is installed then calls it but if not then it will call the dropbearkey to generate a key. The problem is that the dropbearkey generates only a private key file without a .pub.

That's why luci app can't show the pub key and a user can't copy it. Still the dropbearkey provides a command dropbearkey -y privatekey that extracts and shows a public key. Things are getting worse because the luci app detects keys by presents of the pub key.

The key files may have any name and no any specific extension. But usually they starts with id_ e.g. id_rsa. I made some workaround to show all files starting with id_ and even call the dropbearkey -y privatekey to get a raw pubkey.

A screenshot of the new Keys page:

Here you can see that for the id_dropbear its public key shown even when there is no the id_dropbear.pub file.
The ed25519 key doesn't have the id_ prefix but it has a .pub file so it was recognized as a key.
The id_stokito+turris@gmail.com file name contains my email so it's allowed now.
Also the "Generate" key moved to the bottom.

Additionally I sent a PR to the Dropbear to generate the pub file: mkj/dropbear#267

CC @nunojpg @stweil @kvuorine @kzyapkov @systemcrash @lars18th @marazmarci please have a look and test

[systemcrash]

So this is looking better. But my first thoughts when I look at this as a 'newbie': perhaps 'Name' should be 'Private Key' or even 'Private Key Name'? Anyone using openwrt today probably understands this difference, but we should practice clarity here.

If possible, Public Key column should get some iconography/button which serves as a 'copy' to clipboard.

Some of the code you execute in the JS callbacks will not pass muster. It's rather unsafe.

Take a look at how the yggdrasil guys did it using rpc calls. #6689

[stokito]

I simplified intentionally: Key name is anyway the same as a private/identity key.
We have acl that limits commands. Anyway this is something that is used by an admin.
For the record: it is possible an XSS because the pub file content is printed directly to html without escaping. But it's you who generate the key so it's not possible to use.

A copy button would be great. I decided this as a low priority cosmetic feature will add it later.

[knarrff]

I think the logic you use to detect public keys probably catches all likely cases. However, it depending so much on the file name might be either inconvenient or rather problematic if someone does not follow the usual conventions. Wouldn't it be possible to not use the file name, but rather look for files containing only one line and starting with 'ssh-'?

In addition, since you look in folders that contain private keys (and especially if you also read files not only containing one line): what would you think about an extra check that the file content that would go to the client does not contain the string "PRIVATE KEY" as an additional precaution?

As an additional side-note, but maybe that is already how sshtunnel does this: the presence of a public key does not necessarily mean the corresponding private key is present as well. Would the 'correct' way here not be to generate all public keys from all present private keys and only show those - instead of all public keys that happen to be present on the system?

[stokito]

Thank you for the detailed feedback. In 99.99999% of real life usage we just need to show a pubkey for the id_dropbear. We don't have to worry about oyher cases that much. I even concidered to just generate the .pub durring the sshtunnel upgrade. On the next Dropbear upgrade all new keys will be generated with a .pub too. Keep in mind that previously the sshtunnel used an openssh so all old users definetely have a .pub. 20 декабря 2023 г. 23:45:32 GMT+02:00, "Frank Löffler" ***@***.***> пишет:

…

I think the logic you use to detect public keys probably catches all likely cases. However, it depending so much on the file name might be either inconvenient or rather problematic if someone does not follow the usual conventions. Wouldn't it be possible to not use the file name, but rather look for files containing only one line and starting with 'ssh-'? In addition, since you look in folders that contain private keys (and especially if you also read files not only containing one line): what would you think about an extra check that the file content that would go to the client does not contain the string "PRIVATE KEY" as an additional precaution? As an additional side-note, but maybe that is already how sshtunnel does this: the presence of a public key does not necessarily mean the corresponding private key is present as well. Would the 'correct' way here not be to generate all public keys from all present private keys and only show those - instead of all public keys that happen to be present on the system? -- Reply to this email directly or view it on GitHub: #6772 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>

[systemcrash]

I think the logic you use to detect public keys probably catches all likely cases. However, it depending so much on the file name might be either inconvenient or rather problematic if someone does not follow the usual conventions. Wouldn't it be possible to not use the file name, but rather look for files containing only one line and starting with 'ssh-'?

The assumption is that we only handle the .pub files with this pattern. Whereas if I've got this whole shebang right, we need to pick a private key at some point to config the tunnel so as to decrypt received traffic, but have the pub key handy to give to the remote party at some step.

In addition, since you look in folders that contain private keys (and especially if you also read files not only containing one line): what would you think about an extra check that the file content that would go to the client does not contain the string "PRIVATE KEY" as an additional precaution?

The assumption here is that we do not handle the binary files, but expect base64 encoded pkey files which is not the case here (at least for some private keys).

As an additional side-note, but maybe that is already how sshtunnel does this: the presence of a public key does not necessarily mean the corresponding private key is present as well. Would the 'correct' way here not be to generate all public keys from all present private keys and only show those - instead of all public keys that happen to be present on the system?

Seeing/knowing which priv key this pub key came from (in a private key name column) is important. It's a strong hint which key is to be used.

[stokito]

I added a few commits before this wasn't merged.

703c5db I added a ProxyCommand field that will be added in openwrt/packages#20163

Other commits are minor description/i18n improve.

Please merge

[systemcrash]

Please add the separate commits tied to the proxy part in a separate PR so they don't tie progress up here. You may keep those prerequisites here but it will undoubtedly take longer.

[stokito]

ok, I removed the ProxyCommand commit.
Saved it to a separate branch https://github.com/stokito/luci/tree/luci-app-sshtunnel-proxycomd

Will try to push the PR with the ProxyCommand myself but that may take more time.
So please merge the PR

[stokito]

I created a PR for the ProxyCommand openwrt/packages#22967

[stokito]

I force pushed a new version with the key detection that won't add id_rsa if it's not exists but only it's id_rsa.pub.

[systemcrash]

OK. Could you please split out the common functions to a common file in the same folders so that there are not multiple copies. Put all functions in the various files which are outside of return view.extend({... there. Then add a header 'require ...'; whatever you named it. Then I think we can merge.

[stokito]

The require expects a class. So it's not so simple. Let's keep the duplication for now

[stokito]

I changed Ubuntu man pages to Debian man pages. I didn't even now that they are exists because they looks like not indexed by a Google.
Reasons tho switch:

1. It shows links to same article in different languages.
2. It looks like it even has a lang negotiation but it looks broken. The language-indep(endant) URL just redirect to English version.
3. Anchors are supported
4. It more open source https://github.com/Debian/debiman