Keynames for Nick reservation #423

Natorator · 2023-02-06T00:16:43Z

This pr supersedes and replaces #420

So here we have a full-blown chat nick reservation implementation.

Chat nicks are reserved for users by passing the --keynames option to ssh-chat at startup, and placing the desired nickname at the end of the public key line (separated by a space) in the allow list or admin key files. This is the "comment" field is traditionally filled out with $user@$host information when OpenSSH generates an ssh keypair. This makes the public key file suitable for simply copying and pasting into the allowlist or admin files, line for line, in order to reserve usernames. Only the first word of the comment field is used for the reserved nick.

When a user with a reserved nick connects, they are automatically assigned the username assigned to their key along with a checkmark symbol prefix to indicate that their key fingerprint is verified by the server.

The /nick and /rename commands have been reimplemented to prevent users from receiving reserved nicks unless their ssh key matches that recorded in the reservation lists.

Natorator · 2023-02-06T00:21:35Z

Two things bug me about this:

reloading the allowlist does not override the nick reservations. Adding new reservations should work, but removing or changing reservations saved to the admin or allowlist key files will require restarting the server.
traditionally, when openssh generates a public key, the comment is of the format user@host. ssh-chat currently sanitizes nicks by removing any but the most basic characters, including @ . This means that if I tried to reserve nato@graham, the actual nick I'd end up with would be natograham.

shazow

Thanks for working on this!

Wrote some ideas for changes, I think we're on the right track and hopefully these changes will address your concerns too.

shazow · 2023-02-06T18:55:04Z

auth.go

+func (a *Auth) SetKeynamesMode(value bool) {
+	a.settingsMu.Lock()
+	defer a.settingsMu.Unlock()
+	a.keynamesMode = value


Do we need to track the mode on the Auth struct if we can just avoid populating it when the flag is off?

Not really. I was just following existing convention for the allowlistMode

shazow · 2023-02-06T18:58:09Z

auth.go

@@ -168,6 +192,17 @@ func (a *Auth) Op(key ssh.PublicKey, d time.Duration) {
 	} else {
 		a.ops.Set(authItem)
 	}
+
+	// nick registration


What do you think of pulling this out into a helper that can be run independently? Something like func parseAuthComment(comment string) (nick string, prefix string)

This would also be a good place to parse out the @... part of the nick and only use the first part, which would address your second concern. :)

(That is, foo@bar would become foo)

That occurred to me the instant I copied and pasted between these two functions, naturally :)

wrt to @ , I actually want to keep the @host suffix. I tend to generate keys on each account on each host, rather than copying them around, so using them to identify accounts on particular hosts, rather than just individual people, seems natural to me.

shazow · 2023-02-06T19:37:05Z

host.go

@@ -378,6 +387,60 @@ func (h *Host) InitCommands(c *chat.Commands) {
 		},
 	})

+	c.Add(chat.Command{


Hmm I'd much prefer to keep the /nick command in the chat module.

Could we do the extra checking inside the room.Rename function?

Also preferably the Room shouldn't know about details of the Host's implementation, so ideally the Host would pass in something like a CheckAvailableID(string) error interface to the Room, and that can do the checking.

The reason I moved /nick to host.go was so that the Command was able to get a reference to an Auth instance (through the host), which is where the reservation data lives.

At first I saw that Host is an extension of a Room, and tried a type assertion on it, but that didn't seem to work. Perhaps there's a way to do that without moving the nick Command. My Go knowledge isn't the best, so pointers welcome!

shazow · 2023-02-06T19:37:45Z

host.go

+			if h.auth.keynamesMode {
+				identity, identified := member.Identifier.(*Identity)
+				if identified {
+					identity.SetSymbol(strings.Replace(identity.symbol, "✓", "", -1 ))


We shouldn't hardcode symbols here. If we want a checkmark symbol for registered names, we can have them in the key comment, right?

We can have cases where people wish to reserve names but not want to have a checkmark, or maybe a checkmark is earned in some other way on some servers.

Given how contentious this issue has been, the symbol prefix may also be the wrong place to do this.

Instead, what if we added a field to the /whois listing that displays the ssh key comment, in it's entirety?

The situation I'm trying to avoid by enforcing the symbol appearance is one where a user claims their nick is reserved, when it isn't. Other non-admin users should be able to depend on it, so long as they can trust the operators (who can still assign any symbol string with /rename).

If we put this in a /whois field, instead, other users will always have access to that, and will always be able to depend on it, whether the user has a registered nick or not. I can then undo all the symbol-setting business; operators can continue to /rename with any symbols they want, and future developers can brainstorm and implement new features there as they please.

shazow · 2023-02-06T19:40:16Z

host.go

+			oldID := member.ID()
+			newID := sanitize.Name(args[1])
+
+			// check nick registration


Don't think we need this here, right? /rename can only be called by admins, and it should override any default behaviour anyway.

There are two levels of authority - the users in the admin list (operators), including those granted ops by other ops, and the folks who have write privileges to the admin and allowlist files. I thought it more trustworthy to limit the class of people who can override the nick reservations, to maximize the amount trust users could put in the system.

shazow · 2023-02-06T19:44:02Z

host.go

+		name := h.auth.Keyname(id.PublicKey())
+		if len(name) >0 {
+			id.SetName(name)
+			id.SetSymbol("✓")


Same as https://github.com/shazow/ssh-chat/pull/423/files#diff-7717c52956f97079739dd32cd1d2c7a9ff80173a3895ac11d9e845d1b6c5ca41R417

We shouldn't need to hard-code symbols, but rather load them from the comment.

shazow · 2023-02-06T19:45:09Z

cmd/ssh-chat/cmd.go

@@ -38,6 +38,7 @@ type Options struct {
 	Version    bool     `long:"version" description:"Print version and exit."`
 	Allowlist  string   `long:"allowlist" description:"Optional file of public keys who are allowed to connect."`
 	Whitelist  string   `long:"whitelist" dexcription:"Old name for allowlist option"`
+	Keynames   bool     `long:"keynames" description:"Reserve usernames by appending them (after a space) to their keys in the admin or allowlist files."`


Maybe ReserveNames and reserve-names?

--reserve-names does sound better - it says more directly what it does. --keynames says more about how it's done. I'll change that.

shazow · 2023-02-06T19:46:18Z

auth.go

@@ -61,9 +62,12 @@ type Auth struct {
 	banned         *set.Set
 	allowlist      *set.Set
 	ops            *set.Set
+	keynamesByFingerprint map[string]string
+	fingerprintsByKeyname map[string]string


Why do we need fingerprintsByKeyname? I don't think we're actually using it?

see Auth.Keyname() and Auth.KeynameFingerprint(). The former is used to determine what registereed name to assign a newly-connected user, based on their fingerprint. The latter is used to find the fingerprint that a nick may be registered to, to compare it with the fingerprint of the user's key in /nick and /rename.

I suppose we could just build one map at launch, then implement one or the other function to iterate over the same map instead, instead of building two inverted maps to do separate lookups with.

shazow · 2023-02-06T19:47:56Z

auth.go

@@ -61,9 +62,12 @@ type Auth struct {
 	banned         *set.Set
 	allowlist      *set.Set
 	ops            *set.Set
+	keynamesByFingerprint map[string]string


Would it be better to make this fingerprintComments, and store the comment section of the fingerprint altogether here? This way we can extract both the name and the symbol later.

My intent was to just parse the keyname (first word) from the comment, and drop the rest. I'd rather leave future features to future developers.

Natorator · 2023-02-10T01:11:35Z

I've incorporated most of the changes discussed here to #424 . I'll leave this one open for the moment, but I think both of us will like the new one better.

nato added 5 commits February 5, 2023 15:44

Auth.go: parse and record nick registrations at startup

043b45d

Host.go: assign registered nicks to users upon connection.

fedad0f

Host.go: move /nick command and add nick registration check.

71a0376

Host.go: add nick registration check to /rename command.

233898f

add and document --keynames options

aba5ca0

Natorator mentioned this pull request Feb 6, 2023

Key comments as symbols #420

Closed

shazow requested changes Feb 6, 2023

View reviewed changes

Natorator mentioned this pull request Feb 10, 2023

Verify nicks #424

Closed

shazow closed this Feb 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keynames for Nick reservation #423

Keynames for Nick reservation #423

Natorator commented Feb 6, 2023

Natorator commented Feb 6, 2023

shazow left a comment

shazow Feb 6, 2023

Natorator Feb 7, 2023

shazow Feb 6, 2023

Natorator Feb 7, 2023

shazow Feb 6, 2023

Natorator Feb 7, 2023 •

edited

Loading

shazow Feb 6, 2023

Natorator Feb 7, 2023

shazow Feb 6, 2023

Natorator Feb 7, 2023

shazow Feb 6, 2023

shazow Feb 6, 2023

Natorator Feb 7, 2023

shazow Feb 6, 2023

Natorator Feb 7, 2023

shazow Feb 6, 2023

Natorator Feb 7, 2023

Natorator commented Feb 10, 2023

Keynames for Nick reservation #423

Keynames for Nick reservation #423

Conversation

Natorator commented Feb 6, 2023

Natorator commented Feb 6, 2023

shazow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Natorator Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Natorator commented Feb 10, 2023

Natorator Feb 7, 2023 •

edited

Loading