Closed byronwolfman closed 8 months ago
This is a great security improvement for the Slack backend!
What do you think about using a pattern detection strategy instead of explicit toggle? If the string begins with @
use the self.username
verses strings starting with U
use the self.userid
.
Pattern detection would maintain compatibility with existing errbot installations and allow people to use the unique identification method in an intuitive way (no need to explicitly lookup the configuraiton toggle to enable it, just write to correct string into the configuration file and you're good to go).
Some documentation explain the difference between @
and U
in the Slack documentation would help users understand what's going in the background and why they should care about this difference.
What do you think about using a pattern detection strategy instead of explicit toggle? If the string begins with
@
use theself.username
verses strings starting withU
use theself.userid
.
From a UX perspective there's a strong case to be made for using pattern-detection to allow both methods. I've opted for the toggle in this case because it allows for a smaller code change (I suppose optimizing for the developer flow vs the operator flow).
get_acl_usr
ultimately asks the pluggable backend to return a canonical identifier for the user, and then compares that to BOT_ADMINS, e.g.
glob(get_acl_usr(msg), self.bot_config.BOT_ADMINS)
# True if '@byronwolfman' is in BOT_ADMINS
To pattern-detect the type, the backend itself would have to perform the comparison, e.g. we'd do something like
get_acl_usr(msg, self.bot_config.BOT_ADMINS)
# Pluggable backend makes comparison and returns True/False
It could be done, but it's a bit more involved, and I don't know if I have the complete means to not break everything. If someone is able to help test this though, I can give it a shot. :)
Speaking of breaking stuff: it looks like this toggle will have to also apply to def person(self):
in the Slack backend, otherwise ACLs will also fail in errbot/core_plugins/flows.py. I'm hoping someone more familiar with errbot can confirm or clarify if this is the case.
@byronwolfman I really like @nzlosh idea of a detection strategy. The suggestion of using @
as the key indicator seems like a fair one.
This would only apply for the Slack backend. Other backends would still behave as expected and not be impacted by this.
Hey! Sorry for not following up on this sooner; it's been a bit of a crazy month/months/pandemic. The internal team ended up forking errbot to achieve their outcome. It turns out that switching to unique IDs versus @handles
required a couple of other minor changes, so this PR shouldn't be accepted as-if even if a detection mechanism were not preferred (and a detection mechanism would definitely be better).
Anyway I'll see what I can do about a detection mechanism!
Hey so I've run into some design issues and have to come out of the rabbit hole. The main issue is that the SlackPerson
object and that Person
object it is based on does not actually have access to the bot_config
property. If it did (or if it at least had access to BOT_ADMINS
), then I can think of a couple different ways to implement a detection mechanism. Since it doesn't have access to this property, aclattr
for SlackPerson
can't choose to return a username or user ID based on the config that it can't access.
Anyway, the question is whether additional properties should be passed to each SlackPerson
object initialized, or if there is a potentially better way for a SlackPerson
to access this information that I'm not familiar with. By the time the ACLs are examined, it is done by errbot/core_plugins/acls.py
which is backend agnostic, so any information used to detect username vs user ID must be passed to SlackPerson
ahead of time.
Thinking out loud here, I was wondering if it would be possible to support handling a username lookup by adding a new method to the Person
base class. E.g. a new method resolve_to_id
, that would take a string representation of a user property as the argument. In the base class method it simply does a pass
so any backends that don't support/need this feature have the correct interface but in the Slack backend would override the base method to perform a test of the string for @
or U
and in the case of @
, look up the user_id so that resolve_to_id
will always return a valid user_id.
I would have expected this sort of behaviour from aclattr
but the problem I see with aclattr
method in SlackPerson
, is that it isn't a guaranteed return a Slack user_id. In the case the lookup fails, the Slack username is returned. I don't know in what context a user lookup would fail to be confident about changing the aclattr
, which is why I'm suggesting a new method.
It's certainly possible to add a new method to Person
but I'm not sure where it would be called. Perhaps in get_acl_usr
? It could work, but it would probably also read a bit confusingly. A bit pseudo-cody:
def get_acl_usr(msg):
"""Return the ACL attribute of the sender of the given message"""
+ hasattr(msg.frm, 'resolve_to_id') and if detect_user_ids_in_acls() :
+ return msg.frm.resolve_to_id
if hasattr(msg.frm, 'aclattr'): # if the identity requires a special field to be used for acl
return msg.frm.aclattr
return msg.frm.person # default
It strikes me as confusing because we would be returning a non-acl attribute in certain circumstances. Also, detect_user_ids_in_acls
would be somewhat Slack-specific, and probably does not belong in a core plugin that is backend agnostic. The get_acl_usr
method is classless, so it would have to perform this detection on every invocation (potentially slowly).
I've got one more thing I can try. As far as I can tell, the only place that a SlackUser
object is created is inside the SlackBackend
class, and the latter has access to the bot config. I can probably do something goofy like having the backend itself do the username/userid detection once at startup, and then inform all SlackUser
objects on creation which one they are to return.
~Right, so, I've just tested this and it appears to work, but it requires changing the signature of SlackPerson
's initializer:~
Never mind-- I think I've found a better way forward (see below).
Ok pretty sure I've figured out a non-terrible way of making this work without breaking stuff for existing users; new commits incoming!
Thanks everyone for your patience. I've pushed some new commits and have re-written the opening PR notes to describe the new changes that will detect user IDs vs usernames at startup.
https://github.com/errbotio/errbot/pull/1421#issue-390111630
I noticed that https://github.com/errbotio/errbot/pull/1416 has since been merged and so I'm looking for some more guidance here... should these ACL changes be added to the slack_rtm
backend as well?
There's some heavy ongoing work with that backend (see also: https://github.com/errbotio/errbot/pull/1451) so let me know if you would prefer that I wait on https://github.com/errbotio/errbot/pull/1451 to be merged first.
I wouldn't worry about that other pr. If you could add these acl changes to the slack rtm that would be good
I wouldn't worry about that other pr. If you could add these acl changes to the slack rtm that would be good
Thanks @sijis. I've rebased against master and have added the change to the Slack RTM backend as well. You have the final say, but I think we're good to go!
Apologies for creating PR comment clutter - but just checking in on the status. Thanks for adding this @byronwolfman - since our slack users can change their handle at will this is a big improvement for us! Are you still looking to contribute this - do you need help to get it over the line?
Hey! The clutter (and requisite apology) is all on me. :)
As far as contributing goes, all that's left to do is to have this merged/approved by @sijis -- or alternatively, more review if there are still unsatisfied problems with this PR. But, pending any new review, the feature is code complete!
Is this still relevant?
/cc @nzlosh
I don't think so. The slackv3 backend only accepts Slack ID and Slack Usernames are not guaranteed to be unique which precludes them from being used for security purposes like ACLs.
Hey friends, it's been a minute! I'd nearly forgotten about this PR.
I opened this back in 2020 because at the time, errbot admins were identified using Slack usernames, and this allowed me to (at the time) successfully impersonate an admin. This PR would address that by optionally allowing user IDs, but as pointed out by @nzlosh, user IDs are now required by the v3 backend itself so this PR is no longer applicable.
Looks like it would be a heck of a merge conflict to untangle anyway!
Despite the patch not making it into master, thank you taking the time to write it and make it available. :+1:
Note that this PR has been rewritten a bit since the first commit, owing to this request. In this new and improved version,
BOT_ADMINS
can be specified via immutable Slack user IDs rather than usernames:Because of how the
aclattr
property works, however, it is not possible to mix them:The reason for this change is that a couple years ago, Slack declared that usernames were no longer guaranteed to be unique. It's now possible for multiple Slack users in a given workspace to have the same username, which means someone who isn't a bot admin can become one pretty easily just by changing their username. This may also apply to Slack guests and multi-org channels but I've not dug in that deeply yet.
It is therefore much safer to use user IDs which are unique per user, and cannot be changed, to prevent admin impersonation (especially in community Slacks). I think it would be even safer to not allow mutable
@usernames
for ACLs, but that would break existing installations and I can appreciate the desire for backwards compatibility. Interestingly, theSlackBot
already does this for the same reasons (impersonation) so without wanting to over-step my bounds too much, I'd recommend that such a breaking change be planned for whenever the next major update takes place.Code and doc diffs should hopefully cover all the changes but I'm happy to walk through them if there are any questions or further review asks.
Original PR notes below, which no longer apply:
~Hey friends, hopefully this PR is appropriate! Some teammates are hoping to build some stuff with errbot, but we're a bit concerned about Slack's approach to mutability when it comes to usernames. Specifically, errbot admins are configured via:~
~A couple years ago, Slack declared that usernames were no longer guaranteed to be unique. It's now possible for multiple Slack users in a given workspace to have the same username, which means someone who isn't a bot admin can become one pretty easily just by changing their username. This may also apply to Slack guests and multi-org channels but I've not dug in that deeply yet.~
~Anyhow, the safe thing to do would be to instead use Slack User IDs which are unique and immutable, and not subject to impersonation. I think that would probably break errbot for a lot of its users if such a change were introduced as default so instead of doing that I'd like to at least introduce a toggle and hopefully convince someone to switch to Slack User IDs as default at a later date.~
~To enable the toggle:~
~Once again I hope this is the right scope and format for such a proposal. If the conventions aren't quite right or other changes are needed please let me know!~