Skip to content

Add /allowlist command#399

Merged
shazow merged 27 commits intoshazow:masterfrom
mikitsu:whitelist-command
Jan 6, 2022
Merged

Add /allowlist command#399
shazow merged 27 commits intoshazow:masterfrom
mikitsu:whitelist-command

Conversation

@mikitsu
Copy link
Copy Markdown
Contributor

@mikitsu mikitsu commented Jul 9, 2021

closes #270
also closes #256

user-facing changes

Ops have a new /whitelist command to modify the whitelist. Help text:

Usage: /whitelist help | on | off | add {PUBKEY|USER}... | remove {PUBKEY|USER}... | import [AGE] | reload {keep|flush} | reverify | status
help: this help message
on, off: set whitelist mode (applies to new connections)
add, remove: add or remove keys from the whitelist
import: add all keys of users connected since AGE (default 0) ago to the whitelist
reload: re-read the whitelist file and keep or discard entries in the current whitelist but not in the file
reverify: kick all users not in the whitelist if whitelisting is enabled
status: show status information

Essentially #270 commands plus reload, reverify (which might have been implicit in on?) and status.

This whitelist is enabled by default at startup if a file is specified (previous behaviour: whitelist enabled == people in the whitelist set). This means that you will see the whitelist enabled when it wasn't before iff you specify a whitelist file without keys.

inside changes

Files (op and whitelist) are now loaded from auth.go instead of from cmd/ssh-chat/cmd.go. Apart from moving, I also put more of the loading code in the same place.

The whitelist mode is saved as a bool, instead of being determined by the whitelist set.

Obviously, there is a new command, /whitelist. The handler uses three inner functions for convenience:

  • sendMsg adds a string to a slice that is sent at the end. This makes sure the lines arrive in order.
  • forConnectedUsers executes a function for every connected user. Used for "import", "reverify" and "status"
  • forPubkeyUser executes a function for every public key specified as argument to the command. A key may be specified either as ssh-whatever base64== (with the space, i.e. as two arguments) or as username of a connected user with public key. Used for "add" and "remove".

open questions

  • The command handler is quite large. If that isn't OK, how should I break it up?
  • Should there be a "panic" (or something else) subcommand for "on" + "import" + "reverify" (which is the use-case mentioned in Admin: Ability to toggle whitelist-only mode #270)?
  • Which commands should print a reply? Currently, some are quite detailed and others are silent.
  • Should "add" and "remove" accept other arguments (e.g. fingerprints or a duration)? Fingerprints would require editing the whitelist set directly, while a duration would complicate the argument handling.

In addition to these major questions, there are also some TODO comments in the code. Most of them are changes/additions I don't consider necessary. If there isn't anything you'd like to have in them, I'll just silently remove them in one of my next commits.

@shazow
Copy link
Copy Markdown
Owner

shazow commented Jul 10, 2021

@mik2k2 Thank you for doing all of this work, you've clearly put a lot of effort into it. :)

I'll take a closer look a bit later, but some answers for your questions:

The command handler is quite large. If that isn't OK, how should I break it up?

Yea, it's quite huge. Would be nice to keep the command add sections relatively small, so it would be great if you could pull it apart into some helpers that live adjacently somewhere.

Should there be a "panic" (or something else) subcommand for "on" + "import" + "reverify"

I think it's fine as it is. Let's keep the PRs as small as possible, we can always add more later but it's always an uphill battle for me to ask you to remove things. :)

Which commands should print a reply? Currently, some are quite detailed and others are silent.

Good question! Let's err on the side of less for now, can always add more later.

Should "add" and "remove" accept other arguments (e.g. fingerprints or a duration)? Fingerprints would require editing the whitelist set directly, while a duration would complicate the argument handling.

Yea this part is tricky. This is partly why I avoid add/remove and instead overload durations for remove.

IMO fingerprints should be supported. We can detect if it's a fingerprint or a pubkey. Fingerprints start with SHA256:... whereas pubkeys start with ssh-ed5519 .... Just checking whether there is a colon is in there is probably safe to assume it's a fingerprint. (We don't need to support legacy fingerprints IMO). Pubkeys can be converted to fingerprints for consistent storage in the set.

Aside: I've been thinking of renaming the whole feature to allowlist. Unfortunately it was a bit more work than I hoped, but if you wanted to change the /whitelist command to /allowlist, I would support that.

shazow
shazow reviewed Jul 10, 2021
View reviewed changes
@mikitsu
Copy link
Copy Markdown
Contributor Author

mikitsu commented Jul 10, 2021

Thank you for looking at my work.

The "problem" with keys/fingerprints (regarding add, remove and print) is that currently, the set itself contains the fingerprints while Whitelist() takes a public key. If fingerprints are accepted for add/remove, we won't be able to

  • switch the whitelist set to keys, which has to be done for print or
  • use Whitelist, which is bad if we'd like to add durations.

Allowing fingerprints is probably easier, but it's not trivial.

Should just the command name change to allowlist, just the code I've touched, or all the code?

@shazow
Copy link
Copy Markdown
Owner

shazow commented Jul 10, 2021

You're right, we'll need to convert the Whitelist set to contain fingerprints rather than pubkeys first. We can add fingerprint support in a later PR. :) It would be a backwards-compatible change.

Long-term I'd love to change all of the user-facing stuff to use allowlist rather than whitelist, but I think it'd be fine to just have this command be /allowlist for now.

@mikitsu mikitsu changed the title Add /whitelist command Add /allowlist command Jul 16, 2021
@shazow
Copy link
Copy Markdown
Owner

shazow commented Dec 18, 2021

Thank you for continuing to work on this. :) Please ping when it's ready to review!

@mikitsu mikitsu marked this pull request as ready for review December 22, 2021 15:17
@mikitsu
Copy link
Copy Markdown
Contributor Author

mikitsu commented Dec 22, 2021

@shazow I think it's mostly done now. Two remaining questions I have:

  • Do we want to completely ignore lines without keys when reading them from files? I understand we'd like to allow empty lines/comments.
  • I haven't written a test that actually reloads keys from a file. Is that OK?

@shazow
Copy link
Copy Markdown
Owner

shazow commented Dec 22, 2021

Do we want to completely ignore lines without keys when reading them from files? I understand we'd like to allow empty lines/comments.

Sounds good but not blocking. Feel free to add that if you want, or we can add it in the future. :)

I haven't written a test that actually reloads keys from a file. Is that OK?

That's fine. :) Someday should refactor the loader to read from an io interface so we can test it that way.

shazow
shazow requested changes Dec 22, 2021
View reviewed changes
Copy link
Copy Markdown
Owner

@shazow shazow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions, but overall I think this is a very valuable change and we should merge/deploy soon. :) Thank you for your work!!

host.go Outdated
msgs = append(msgs, fmt.Sprintf("The following connected users are on the allowlist: %v", whitelistedUsers))
}
if len(whitelistedKeys) != 0 {
msgs = append(msgs, fmt.Sprintf("The following keys of not connected users are on the allowlist: %v", whitelistedKeys))
Copy link
Copy Markdown
Owner

@shazow shazow Dec 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
msgs = append(msgs, fmt.Sprintf("The following keys of not connected users are on the allowlist: %v", whitelistedKeys))
msgs = append(msgs, fmt.Sprintf("Connected user keys not included on the allowlist: %v", whitelistedKeys))

Copy link
Copy Markdown
Contributor Author

@mikitsu mikitsu Dec 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually different: These are keys with inAllowlist && !userConnected. (I read your suggestion as !inAllowlist && userConncted)

Copy link
Copy Markdown
Contributor Author

@mikitsu mikitsu Dec 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we switch the loops, we can get connected unlisted users instead (we can also get both, but that means a bit more bookkeeping). What do you prefer?

voldyman
voldyman reviewed Dec 22, 2021
View reviewed changes
@mikitsu
Copy link
Copy Markdown
Contributor Author

mikitsu commented Jan 4, 2022

I changed the messages and did a complete whitelist -> allowlist rename. As options to the executable we now accept both, with a warning printed on --whitelist (which you might want to phrase differently?).

shazow
shazow approved these changes Jan 4, 2022
View reviewed changes
Copy link
Copy Markdown
Owner

@shazow shazow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor phrasing suggestions but overall looks good! Let me know when you're ready to merge. :)

@mikitsu
Copy link
Copy Markdown
Contributor Author

mikitsu commented Jan 5, 2022

I just have one final question: on /allowlist status, do you prefer getting keys in the list whose users are not connected or connected users whose keys are not in the list?

@shazow
Copy link
Copy Markdown
Owner

shazow commented Jan 5, 2022

Hm. I think we want the set in the allowlist, and (optionally) their connected status.

Honestly I might tweak that code a bit after merging and playing with it. :)

@mikitsu
Copy link
Copy Markdown
Contributor Author

mikitsu commented Jan 6, 2022 via email

@shazow
Copy link
Copy Markdown
Owner

shazow commented Jan 6, 2022

@mik2k2 Would you mind please updating the test so that the build passes before I merge? :)

@mikitsu
Copy link
Copy Markdown
Contributor Author

mikitsu commented Jan 6, 2022

Oh, sorry.

@shazow shazow merged commit 621ae1b into shazow:master Jan 6, 2022
@shazow
Copy link
Copy Markdown
Owner

shazow commented Jan 6, 2022

@mik2k2 Thank you for working on this!!

@mikitsu mikitsu deleted the whitelist-command branch January 6, 2022 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shazow shazow shazow approved these changes

+1 more reviewer

@voldyman voldyman voldyman left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Admin: Ability to toggle whitelist-only mode Add /whitelist command

3 participants

@mikitsu @shazow @voldyman