Verify nicks #424

Natorator · 2023-02-10T01:09:36Z

This pr is an alternative implementation of a feature to reserve chat nicks. It is more conservative, simpler, does fewer things, and makes fewer changes to the code base than previous prs #420 and #423 . Thanks to @shazow for many good points of advice from the previous pr thread.

The first commit only loads ssh key comments onto the Auth object. Key comments are any string (trimmed for spaces) that follow the public key on each line of the admin and allowlist key files. Each comment is associated with the key it appears with. This commit does not attempt to parse the comments or use them in any way; it simply stores them for use in future commits.

The second commit implements an injectable checkName() function on the Room object, which is then used during Join() and Rename() operations to reject attempts by users to make use of a nick that is associated with another key. A nick is parsed from the first word of a key comment, and sanitized.

That's it. Things that are not in the implementation thus far:

Reserved nicks are //not// assigned automatically to connecting users; this should significantly reduce user surprise that would accompany such behaviour.
- If a user attempts to connect with a nick reserved by another key, they are simply assigned a "guestX" nick, in the same way they would if they previously attempted to connect with a nick that was empty or already in use.
- If a user attempts to use /nick or /rename to set a nick for a user reserved to a different key, the attempt fails with an error. The current implementation cannot distinguish between renaming attempts made by users from those made by admins, so all attempts are rejected.
command line options. The nick guard is currently enabled by default. Since user surprise is reduced significantly by the fact that nicks are not assigned automatically upon connection, a command line option to enable the behaviour is less necessary. If a command line option is desired to enable this behavior, it is easy to add.
symbol-setting. This, too, is easy to add in the future if desired. The essentials of the nick reservation feature can be considered in isolation from the aesthetics of how to present it to users.

I also added go fmt and test adjustments to this pr.

…nts of reserved nicks.

Natorator · 2023-02-12T04:53:46Z

Here's something I noticed yesterday, that is worth discussing: how should we handle a situation where the same nick is reserved by multiple keys?

When a single fingerprint is reserving multiple nicks, the solution seems pretty obvious: the owner of the key can use any nick associated with it. The inverse case has a legible use case, but could also result from inadvertent mistakes made while configuring the admin and allowlist files.

First, the understandable use case: a user connects from different accounts on different hosts with different keys, at different times, but wants to use the same nick no matter where they connect from. So they register the same nick with both keys. In this case, the presumption is that either key would be able to use the nick (barring the case where it's already in use).

There's also the error case: A user is invited to select a nick to register, and the admin registers one that is already registered, without checking to see if it is already registered in the admin or allowlist files. Obviously allowing both users to use the nick would contradict user expectations of what the feature is intended to do. So what do we do in this case?

The current implementation allows NEITHER user to use the nick. I didn't plan it that way, but I'm happy with that at the moment. It will inspire users, and admins alike to discover the problem closer the the even that caused it - the extra reservation. It also averts a potential security issue (impersonating another user) at the expense of serving a legitimate use case, which I think is an appropriate "fail-closed" reaction to an unconsidered edge case. And, if you want to change the permissiveness of a feature, it's better for backwards compatibility to loosen constraints than tighten them.

There's another way to handle it, though: We could simply allow the //first// user in the files to use the nick, and prohibit subsequent reservations. Reservations in the admins file would be considered before reservations in the allowlist file, for this case.

Thoughts?

shazow

Sorry I don't have much time to work on ssh-chat lately, it may take me a while to review new PRs.

Some high level thoughts:

I understand your desire to do a half-way feature and get it merged, but I'm reluctant to merge things that are not going to be forward-compatible, especially if they break backward compatibility in the exported interface of the code and we'll likely need to break it again in the future to fix shortcomings.
To that end, let's try to avoid changing exported interfaces when possible? That makes the PR easier to merge, because that means things can change "behind the scenes" without breaking too many things depending on it looking a specific way (also other serves maintain ssh-chat forks and it'll be annoying for them to change things back and forth).

I appreciate the direction you're heading in, and I think we can use what you've made if you want to leave it until I have time to work on ssh-chat more and I'll build more changes on top of it. :)

Or if you want to continue iterating, that's cool too.

shazow · 2023-02-12T15:24:48Z

chat/room.go


 	Members *set.Set
 }

 // NewRoom creates a new room.
-func NewRoom() *Room {
+func NewRoom( checkName func( *message.User ) error ) *Room {


Please avoid changing public function signatures when possible (for backwards compatibility and also minimizing interface concerns). In this case, we can avoid passing in checkName as an argument by making it a new public element on the Room struct that can be set after NewRoom() is called.

shazow · 2023-02-12T15:25:27Z

chat/room.go

@@ -200,11 +206,15 @@ func (r *Room) Leave(u *message.User) error {
 }

 // Rename member with a new identity. This will not call rename on the member.
-func (r *Room) Rename(oldID string, u message.Identifier) error {
+func (r *Room) Rename(oldID string, u *Member) error {


Can we avoid changing this public function signature? I believe the host has everything it needs to look up users by ID?

The Identifier interface technically doesn't fulfill the needs of checkName(). But... this could probably be fixed best with a signature like func ( nick string, fingerprint string) error instead, so that it's up to the caller to handle the lookups and type assertions. I'll try that.

shazow · 2023-02-12T15:26:08Z

host.go

@@ -58,7 +58,30 @@ type Host struct {

 // NewHost creates a Host on top of an existing listener.
 func NewHost(listener *sshd.SSHListener, auth *Auth) *Host {
-	room := chat.NewRoom()
+
+	checkName := func(user *message.User) error {


Would be nice to pull this out into a helper rather than inline it, just to keep the NewHost function easier to follow.

I thought the same; I've got an implementation of this in progress (unpushed). Look for it soon.

shazow · 2023-02-12T15:37:55Z

Here's something I noticed yesterday, that is worth discussing: how should we handle a situation where the same nick is reserved by multiple keys? [...]

I agree with your analysis. :)

Any of those options are fine for now, that's something we can change/improve in the future.

shazow · 2023-02-12T15:39:37Z

auth.go

@@ -158,7 +162,8 @@ func (a *Auth) CheckPassphrase(passphrase string) error {
 }

 // Op sets a public key as a known operator.
-func (a *Auth) Op(key ssh.PublicKey, d time.Duration) {
+// comment is the string that follows the public key in openssh authorized_keys format
+func (a *Auth) Op(key ssh.PublicKey, comment string, d time.Duration) {


Can we avoid changing Op(...) and Allow(...) by introducing a new Reserve(key ssh.PublicKey, id string, symbol rune) function?

[Edit: Expanded on the signature]

If you want to keep this a half-way solution, could maintain the same internal storage as a reconstructed "comment" for now, but I would appreciate if can at least converge on a "final" public code interface. :)

Natorator · 2023-02-12T22:35:39Z

No worries about pacing; I think I'm going to set this down for a bit, too. You've taught me several good things about Go, maintaining OSS projects, and compatibility. Thank you for that!

shazow · 2023-02-13T20:13:33Z

Alright. :) Let me know if you want to resume this another time.

I appreciate the effort you put into this!

nato added 2 commits February 8, 2023 20:34

Auth.go: parse and record ssh key comments at startup

de92057

room.go: inject a function into NewRoom() that verifies key fingerpri…

e4127bf

…nts of reserved nicks.

Natorator mentioned this pull request Feb 10, 2023

Keynames for Nick reservation #423

Closed

shazow requested changes Feb 12, 2023

View reviewed changes

shazow reviewed Feb 12, 2023

View reviewed changes

Natorator closed this Feb 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify nicks #424

Verify nicks #424

Natorator commented Feb 10, 2023

Natorator commented Feb 12, 2023 •

edited

Loading

shazow left a comment

shazow Feb 12, 2023

shazow Feb 12, 2023

Natorator Feb 12, 2023 •

edited

Loading

shazow Feb 12, 2023

Natorator Feb 12, 2023

shazow commented Feb 12, 2023

shazow Feb 12, 2023 •

edited

Loading

shazow Feb 12, 2023

Natorator commented Feb 12, 2023

shazow commented Feb 13, 2023

Verify nicks #424

Verify nicks #424

Conversation

Natorator commented Feb 10, 2023

Natorator commented Feb 12, 2023 • edited Loading

shazow left a comment

Choose a reason for hiding this comment

shazow Feb 12, 2023

Choose a reason for hiding this comment

shazow Feb 12, 2023

Choose a reason for hiding this comment

Natorator Feb 12, 2023 • edited Loading

Choose a reason for hiding this comment

shazow Feb 12, 2023

Choose a reason for hiding this comment

Natorator Feb 12, 2023

Choose a reason for hiding this comment

shazow commented Feb 12, 2023

shazow Feb 12, 2023 • edited Loading

Choose a reason for hiding this comment

shazow Feb 12, 2023

Choose a reason for hiding this comment

Natorator commented Feb 12, 2023

shazow commented Feb 13, 2023

Natorator commented Feb 12, 2023 •

edited

Loading

Natorator Feb 12, 2023 •

edited

Loading

shazow Feb 12, 2023 •

edited

Loading