Allow Slack backend to optionally use immutable userids for ACLs

[byronwolfman]

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:

BOT_ADMINS = ('@gbin', '@zoni') # Can still specify via usernames
BOT_ADMINS = ('U01AKS5RT1T', 'U01A872QUQP') # Can also specify via user IDs now!

Because of how the aclattr property works, however, it is not possible to mix them:

BOT_ADMINS = ('U01AKS5RT1T', '@gbin') # Fatal error at startup

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, the SlackBot 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:

BOT_ADMINS = ('@bob', '@alice',)

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:

ACCESS_CONTROLS_USE_SLACK_USERID = True
BOT_ADMINS = ('U1234ABCD', 'U5678EFGH',)

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!

[nzlosh]

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.

[byronwolfman]

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.

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.

[sijis]

@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.

[sijis]

Would prefer to have a detection mechanism instead of a flag.

[byronwolfman]

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!

[byronwolfman]

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.

[nzlosh]

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.

[byronwolfman]

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.

[byronwolfman]

Right, so, I've just tested this and it appears to work, but it requires changing the signature of SlackPerson's initializer:

Expand!

-    def __init__(self, sc, userid=None, channelid=None):
+    def __init__(self, sc, userid_as_acl, userid=None, channelid=None):
         if userid is not None and userid[0] not in ('U', 'B', 'W'):
             raise Exception(f'This is not a Slack user or bot id: {userid} (should start with U, B or W)')

         if channelid is not None and channelid[0] not in ('D', 'C', 'G'):
             raise Exception(f'This is not a valid Slack channelid: {channelid} (should start with D, C or G)')

+        self._userid_as_acl = userid_as_acl
         self._userid = userid
         self._channelid = channelid
         self._sc = sc

There are some other changes, but that's the one most likely to affect other errbot users. I've not used errbot before or authored any integrations so I'm not sure if changing the SlackPerson init signature will either

1. Cause no problems, hooray, lemme push up my commits or
2. Require everyone with custom integrations to rewrite their integrations to support the new parameter

Thoughts?

Never mind-- I think I've found a better way forward (see below).

[byronwolfman]

Ok pretty sure I've figured out a non-terrible way of making this work without breaking stuff for existing users; new commits incoming!

[byronwolfman]

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.

#1421 (comment)

[byronwolfman]

I noticed that #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: #1451) so let me know if you would prefer that I wait on #1451 to be merged first.

[sijis]

I wouldn't worry about that other pr. If you could add these acl changes to the slack rtm that would be good

[byronwolfman]

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!

[etherops]

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?

[byronwolfman]

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!

[sijis]

Is this still relevant?

/cc @nzlosh

[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.

[byronwolfman]

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!

[nzlosh]

Despite the patch not making it into master, thank you taking the time to write it and make it available. 👍