Key comments as symbols

[Natorator]

After having bookmarked this project a few years ago, I finally took a stab at implementing a feature I wanted to see. After a review of the issues, I see it's a common ask. It fills some of the needs in issues #367 , #265 , #246 , and even #74.

This PR attempts to implement tying of arbitrary chat usernames to a connecting user's ssh keys. This is implemented in a simple, lightweight way by using the "comment" field that follows the ssh public keys on each line of the files already loaded through the --allowlist and --admin options. The comment field is standard in the openssh authorized_keys format, so lines of public keys from existing files can often be used as-is after trivial review from the server admins.

When a user connects using a key in the admin or allowlist files, any comment on the used key is pre-pended to their nick in chat. This string is not part of the user's nick; it's the "symbol" prefix that is customarily used to indicate users with ops privileges. As a result, while users can continue to change their nick suffix as usual, their "symbol" prefix can only be changed by admins using the /rename command. This makes the prefix a reliable means of tying the user to a specific key without needing to take away their ability to use or change nicks freely.

This implementation requires no additional persistent state management, fits in well with existing use patterns for ssh and irc/chat, requires no new command line options, and has minimal impact on existing ssh-chat use patterns.

What do you think?

[shazow]

I like this idea for overloading the ssh key comments, the ergonomics are nice.

A couple of questions:

1. The issues you referenced are about reserving a username, not about setting a prefix symbol. Do you think this should support that? (Or perhaps punt to a future PR?) -- I was imagining that syntax would be similar to what gets passed to /rename, so something like NAME [PREFIX] (prefix being optional).
2. Could you please add some comments and documentation about this behaviour? Specifically in the functions that are changed to accept comments, how are comments threated? And also perhaps in the README or --help text or somewhere that people can discover this behaviour.

[Natorator]

What I like about ssh-chat is being able to connect a chat user with the strong authentication guarantees offered by ssh keys. Currently, if I'm a user that isn't an op, I have to use /whois $nick, track and verify the key fingerprint myself in order to take advantage of that, because we all know that nicks can change willy-nilly at the whim of their users, or of any op.

What I'm trying to get at with this pr, is to increase the convenience with which a user can exercise the authentication guarantees they can already get at. That not only fits my use case; I think it's also what our other users, who filed those other issues, are looking for - even if the way we imagined that feature could be implemented isn't the same.

I, too, started looking at this by thinking about nailing down chat nicks so that they couldn't be changed as easily. But then I realized that the symbol prefix already existed, and met the requirements:

• it can be any arbitrary string
• it was already adequately out of reach of non-op users, unlike nicks. only ops can change the symbols; so if you can trust the ops, then you can trust the symbol prefix. And ops can also change the prefix arbitrarily as well, using /rename, if they really need to.
• it was displayed RIGHT NEXT to the user's nick, making it unnecessary to fumble around with /whois and verify a key fingerprint. As long as the prefix is the same as the prefix you've interacted with in the past, and you can trust the ops haven't swapped it on you, you can trust that.

So since we're using the prefixed "symbol" instead of the nick, we don't have to remove the flexibility of using /nick or /rename to get the guarantees we imagined trying to pull from the name we use for a chat user.

So that's why I think this implementation fills the same needs that those issues raise, even though it doesn't implement changes to how nicks actually work, as they had imagined. Think of the prefix as the name the //ops// call a user by, based on their ssh key. The nick is just a bit of ephemeral whimsy the user chooses to go by at that moment. :)

[Natorator]

As for docs and discoverability, sure; I'll keep working on that...

[Natorator]

Another issue I realized today is that the admin file is loaded before the allowlist file. That being the case, the comment that appears on a key in the allowlist with the same fingerprint as a key (presumably the same) in the admin list would override the comment in the latter. It should probably be the other way around.

[shazow]

Your rationale sounds fine, I like this as a starting point. But would be good to plan to support usernames in the future, so the syntax should ideally be forward-compatible with that idea.

Another scenario worth considering is that some people put in their actual .pub files in the allowlists (e.g. cat ~/.ssh/id_rsa.pub >> allowlist.txt) which includes the username@hostname for their device. Will that have unexpected behaviour with this enabled by default? How do we make that not happen by default or make it easily discoverable of what went wrong?

Another issue I realized today is that the admin file is loaded before the allowlist file. That being the case, the comment that appears on a key in the allowlist with the same fingerprint as a key (presumably the same) in the admin list would override the comment in the latter. It should probably be the other way around.

Could change the order if it makes sense, or could make it avoid setting the prefix if it's already set. Whatever makes more sense. :)

[Natorator]

I don't see including the comments from the authorized_keys files as a problem; in fact, I see this as exactly the kind of application that the comment fields were created to enable!

If a user provides the authorized_keys or their .pub file with a comment, but doesn't want it displayed on with their chat nick, they can provide the keyline without the comment. If admins don't want them, they can simply not include them in the files. A command line option could be added to either introduce or prevent the prefix-initialization behavior, but I think the fact that it won't show unless deliberately added to the files is configurable enough.

[Natorator]

With regard to comment precedence between the wadmin and allowlist, care needs to be taken that reloading those files while the server is running still yields reasonable results. I'll look at that...

[shazow]

I don't see including the comments from the authorized_keys files as a problem; in fact, I see this as exactly the kind of application that the comment fields were created to enable!

I'm not saying it's a problem when it's intended, but I guarantee 90% of people are going to do it unintentionally and then get really confused why they have rogue prefixes in their names and won't be able to figure out where they came from. :) How do we make it easy for them to figure that out, without having to reply to a bunch of github issues every week asking the same thing?

Perhaps adding a console log in the server informing of the prefixes loading would be a start, but would be nice to have something more if you can thin of anything.

The other option I would propose is to make the prefix an additional space-separated field in the comment. That is, username@hostname or foo alone won't set a prefix, but username@hostname A or foo A would set the prefix A.

Then later we can utilize the first component to also reserve usernames. :) This would match the syntax of /rename.

[Natorator]

without having to reply to a bunch of github issues every week asking the same thing?

Ah! I hadn't thought of this - and now I can appreciate it.

I suppose enabling the new behavior with a command line option would accomplish two things:

• it would prevent surprises to the existing user base, and
• it would provide an obvious place to document the feature, in the --help page documenting the options.

What do you think we should call the option? --show-key-comments?

[shazow]

What do you think of the proposal of just making the format backwards-compatible (making the comment FOO <PREFIX>) so that it doesn't surprise existing users?

[Natorator]

I feel like chopping the keyfile's comment up by spaces, and only using the second item, overcomplicates things a touch for people that intend to use the key comment as their identifer as-is.

Going the way of the command line option instead avoids surprises, too, but additionally helps document the feature, as mentioned.

[Natorator]

I had a look at the behavior for reloading the keyfiles, and how that impacts key comments.
It's possible to reload the allowlist using /allowlist reload ... after the server starts

there are two problems:

• the allowlist key file is loaded after the admin key file. Currently, this would mean that allowlist comments for the same key fingerprint would override the one in the admins key file. I think that's backwards.
• you can't remove key comments when you /allowlist reload, because empty comment strings are ignored rather than saved.

I think we can solve this by storing two maps for comments instead of one. Each key file would have its own comment map, built when the server starts. The comments map for the allowlist is initialized from a copy of the comments map for the admin comments, after the latter is built. Then, when /allowlist reload happens, the allowlist is flushed, re-initialized from the admin comments, and rebuilt from the allowlist file again. Finally, we change the replacement behavior from "only replace if the replacement is non-empty" to "only replace if the replaced value is non-empty".

[shazow]

Sure, if we can't get it backward and forward compatible, then a flag sounds fine.

I feel like chopping the keyfile's comment up by spaces, and only using the second item, overcomplicates things a touch for people that intend to use the key comment as their identifer as-is.

Even if we assume that we're going to add handle reservation for the first component, so it can be used as-is?

One other syntax option we could consider is similar to the /ban query syntax we've put together. The parser is a little clumsy but it allows for named arguments:

ssh-chat/auth.go

Line 303 in aaf0671

func (a *Auth) BanQuery(q string) error {

So it could look something like ... prefix=🦇, and later it could be ... prefix=🦇 name=Batman

I'm fine whichever you'd like to implement. :)

I agree with your assessment about the desired reload behaviour!

[Natorator]

I've gone back over your comments about directly reserving usernames. I didn't quite understand them at the time, but I think I'm getting it now. So in the interest of forward compatibility, let's think about how that might be implemented...

As mentioned, I was originally thinking in terms of the user's nick, but I wanted something that wasn't as easy to change (for users, wrt their own nicks, as well as ops). Maybe we can compromise here...

How about this?

• on connection, write the key comment to the nick, instead of the symbol field
• reject hijacking attempts, by adding a check to /nick and /rename that rejects attempts to set nicks on users with mismatching key fingerprints
• add a symbol "r" (registered) or "v" (verified) or unicode "✓" (instead of the actual comment) to indicate to users that the user's nick is indeed tied to their ssh key, in the opinion of the server admins.

I think the option flag would still be needed here to avoid surprises. An additional text could be added to /help to explain the meaning of the additional symbol.

[shazow]

I think we could have both? That is, we can still have the symbol field in addition to the nick field (symbol fields follows the nick field separated by a space). That way the admin can be explicit whether it's desirable to add a symbol for registered names. For example:

ssh-ed25519 AAAAA... shazow ✔
ssh-ed25519 AAAAB... batman 🦇
ssh-rsa AAAAC... nosymbol
ssh-rsa AAAAD...

Last line wouldn't make any changes to either nick nor symbol.

Otherwise that sounds good. :)

[Natorator]

I think adding the "✓" symbol explicitly is critical, because we want to avoid a situation where a user can claim that their nick is registered, but no other users can tell for sure. Sure, an op could /rename it away, but the symbol can be regained with a simple reconnection.

In the future, if you want to start adding key=value parameters to the comment, we can easily pinch off the first word as the nick, and handle the rest however you like.

Technically, your "flair" examples can be implemented with the same effect with just /nick 🦇 batman, if you replaced the space after the bat with an alternate blank unicode.

... derp. Which, now that I think of it, is also possible with the checkmark. A user could simply set a unicode nick that starts with the checkmark, and a non-space blank character, and their nick, and fool people.

...back to the drawing board...

[shazow]

To be clear, non-admins can't add their own prefix symbols right now. Only admins can run /rename. Normal names are limited to an ASCII alphabet.

Code here: https://github.com/shazow/ssh-chat/blob/master/internal/sanitize/sanitize.go#L6

[Natorator]

Yeah, you're right. I should have known better than to think you wouldn't sanitize the daylights out of user input for nicks. My apologies.

In that case, my proposal above stands. Adding the symbol on connect needs to be done for all registrations. For the initial implementation, I'll chop the key comment off after the first space, and apply that to the nick. The rest will be stored in the comments maps, and you can do anything you want with them in the future.

How's that sound?

[shazow]

No apologies needed. :)

Sounds good! If you'd like to give the nick reservation a try afterwards also, that's welcome.

[Natorator]

pr #423 implements what we've talked about so far here. We can close this one without merging, if that one works out better.

[shazow]

Sounds good, let's continue work in the other PR. :)