Extend auth to secure chatops #2920

tonybaloney · 2016-09-23T05:40:39Z

To start enabling better security controls over the chatops process I'd like to propose the following changes (to be added to this PR).

- Add a 'is_service' flag to the user db. To distinguish actual users and services (like the chatops daemon)
- Add an API method to issue user ID tokens on behalf of the service principal, these can be requested from an existing token only if the requestor is a service principal (see previous)
- Extend the UserDB model to include the users' chatops username.
- Adapt chatops to first request a user token for the user that issued the chatops command, then make the command match and execute requests with that token, not the token for hubot.
- Create a flag to turn all of this off by default
- Add a StackStorm pack to create users!

tonybaloney · 2016-09-23T06:36:11Z

…er optional but on by default to not break backward compat

Kami · 2016-09-23T07:35:01Z

Thanks for the PR.

I will look into it some time in the next couple of days.

We did already had some discussion about how to handle RBAC in ChatOps (how to map ChatOps users to StackStorm users) in the past.

There are some things we haven't decided on yet, but by quickly looking at your description it looks that some base changes (username field, is_service, etc.) are generic and good enough and something we can re-use in any case.

In short, my idea for authenticating and mapping ChatOps users back to StackStorm was for the user on the chat to authenticate to StackStorm via bot using a ChatOps specific token. Then we would also need some logic in the adapters on how to handle the user uniqueness (on IRC for example, you need to use username+host since the nickname is not unique, in other cases, IDs are available so we can just leverage that...).

emedvedev · 2016-09-23T10:07:44Z

Extend the UserDB model to include the users' chatops username.

I would like this field to be more generic. We shouldn't assume just a single ChatOps bot and a single adapter in this architecture, especially when we're starting the work on dumbing down chatops clients and moving alias-related features to the server side. Maybe I want an e-mail service parsing incoming e-mails as aliases and executing them, or a twitter-based adapter alongside with the bot? Or some other interface, or even multi-tenancy support for ChatOps (which is quite possible in the future). Long story short, a user could have more than one "chatops id."

Here's my proposal: why don't we make this field a dict, where the key is the service, and the value is the username in the service? Something like this would probably make sense in the UserDB model:

nicknames:
  slack: anthonypjshaw
  hipchat: xXx_CoolTony_xXx
  email: [email protected]

And if we're considering ChatOps multi-tenancy (we should IMO), we should include tenant ID as well:

nicknames:
  slack@stackstorm: tonybaloney
  slack@stackstorm_community: anthonypjshaw

Other than that, the proposal looks fine to me on the high level. Another thing I'd advise is breaking the feature into multiple PRs: username aliases would be a no-brainer to merge, but other things could become more tricky—and it would make sense to keep the discussion on different parts separate as well.

emedvedev · 2016-09-23T10:26:53Z

On @Kami's comment: I think that we can safely assume username uniqueness, because, firstly, IRC is a clear outcast here, and secondly, I think this should be handled on the ChatOps client/adapter level: i.e. Hubot should check if the user's authenticated with NickServ to execute a protected command. If we're not assuming ChatOps username uniqueness on the st2 back-end level, we'll just descend into madness from here. And it's outside the scope of this PR anyway, let's keep it small and manageable.

Speaking of IRC: if we don't check NickServ, there's just no good option for a unique user ID. If we settle on username+host and later, at some point in my life, I'll have to use IRC with StackStorm, then I will seek you out and make you experience the rage of a thousand white hot blinding suns of hateful fury, seeing as I connect to the internet from a hundred places a year and my host will just change all the time. 😛 It's not that secure either: if you're using NAT, or a shared pool of IPs (i.e. a mobile network), or work from a cafe every once in a while, you're screwed.

tonybaloney · 2016-09-23T23:53:04Z

If you want to expose secure processes or data over IRC you need your head inspecting anyway.
I did also consider extending the auth API for JWT or OAuth. I see pecan supports neither :-)

Store a dict of nickname-origin pairs instead of just one chatops_id string to make the field more future-proof. We may not need it immediately, but some features that will require this change are already planned.

Replace the `chatops_id` string with `nicknames` dict

enykeev · 2016-09-26T10:22:03Z

I did also consider extending the auth API for JWT or OAuth. I see pecan supports neither :-)

There's a good chance we not going to stick to pecan for long anyway. See #2727.

Switching authentication scheme should be also discussed prior to implementation. Some of the limitations that led to current design are not relevant anymore, that should make it easier. On the other hand, we had requests for SSO in the past. And with that in mind, single size is surely not going to fit all, so we would need to find a way to make auth check pluggable.

enykeev · 2016-09-29T06:55:04Z

We've talked through that on community, but it seems useful to have a more permanent record. The current implementation of impersonalization only works for 'standalone' auth while tests only cover 'proxy' auth. Both problems need to be addressed. "By whom?" is another question.

I'd say that the way we branch between proxy and standalone auth at the beginning of the controller is not acceptable anymore (and likely never was). My vision is that we need to break the controller in three parts:

one that does authentication and determines username (for proxy, we just get it off pecan.request.remote_user)
one that does impersonalization (this PR)
one that generates the token for the username

On the tests side, we need to make sure we also test 'standalone' code path with a different set of tests.

tonybaloney · 2016-09-30T04:54:43Z

ko @enykeev and @emedvedev this is done and ready for review. I've setup all the logic for st2auth.handlers to seperate concern from pecan implementation.

emedvedev · 2016-09-30T05:43:16Z

I love you. ❤️ ❤️ ❤️

Overall, looks good to me. I'm satisfied with both design and proposed implementation (especially since we were able to get nicknames addressed as part of this PR, too), and I'll try to find some time to make a thorough review later tonight. I'd also like @Kami and/or @lakshmi-kannan to review this.

lakshmi-kannan · 2016-10-04T02:37:22Z

st2auth/st2auth/handlers.py

+class ProxyAuthHandler(AuthHandlerBase):
+    def handle_auth(self, request, headers=None, remote_addr=None,
+                    remote_user=None, authorization=None, **kwargs):
+        remote_addr = headers.get('x-forwarded-for',


s/x-forwarded-for/X-Forwarded-For for consistency with other headers?

it was already like that.

lakshmi-kannan · 2016-10-04T02:39:21Z

st2common/st2common/services/access.py

-            extra = {'username': username, 'user': user}
-            LOG.audit('Registered new user "%s".' % (username), extra=extra)
+            if add_missing_user:
+                user = UserDB(name=username)


user_db to be consistent across the code.

lakshmi-kannan · 2016-10-04T02:40:12Z

st2common/st2common/util/pecan_util.py

+]
+
+
+def abort_request(status_code=http_client.UNAUTHORIZED,


Seems un-necessary redirection and the default message is not great.

tonybaloney added 2 commits September 23, 2016 15:34

add service principal flag to auth model

362a198

add chatops_id to user model

7db97bf

tonybaloney added 3 commits September 23, 2016 17:09

add new options to the token auth API

5c78afe

extend auth API for impersonation by username or chatops_id

55fb8c8

when creating tokens, make the (weird) behaviour of creating a new us…

9c90b80

…er optional but on by default to not break backward compat

emedvedev and others added 3 commits September 25, 2016 16:27

Replace the chatops_id string with nicknames dict

f0d94a1

Store a dict of nickname-origin pairs instead of just one chatops_id string to make the field more future-proof. We may not need it immediately, but some features that will require this change are already planned.

Make the code great again! Also pretty.

7c7f386

Merge pull request #1 from emedvedev/oauth

0a9d45a

Replace the `chatops_id` string with `nicknames` dict

updated tests for impersonation

5b73c1a

tonybaloney added 14 commits September 30, 2016 08:18

work on the tests

4498b80

refactor the auth process

17b33f4

start a seperate test case

f587085

rename pecan util module to stop import errors

06a1bbd

fix import

554147c

decouple the auth handlers from the pecan structure

e4229c0

Merge branch 'oauth' of github.com:tonybaloney/st2 into oauth

484e9c2

fix first test

4cbc0cd

unit tests for the new handlers

a5dbace

moar tests

038641e

pylint fixes

ea874df

fix logging import

78f5e20

final tests

8729eea

linting fixes

52acd1e

tonybaloney added 5 commits September 30, 2016 13:45

remove module execution shortcut

ffa24e7

add a service user and remove unused import

3c085dd

add impersonation to the proxy and standalone handlers

3f04e4f

rename to nicknames

403b10f

fix test by mocking lookup

6d3bea9

forgot the auth param

08cc80b

lakshmi-kannan reviewed Oct 4, 2016

View reviewed changes

tonybaloney added 2 commits October 4, 2016 14:10

rename local var

13f87ff

kwarg mapping to field names, of course.

0037afd

tonybaloney changed the title ~~[WIP] Extend auth to secure chatops~~ Extend auth to secure chatops Oct 6, 2016

emedvedev merged commit b8d4caf into StackStorm:master Oct 6, 2016

emedvedev mentioned this pull request Nov 8, 2016

RBAC for ChatOps in StackStorm StackStorm/hubot-stackstorm#128

Open

arm4b added RBAC chatops enterprise labels Jun 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend auth to secure chatops #2920

Extend auth to secure chatops #2920

tonybaloney commented Sep 23, 2016 •

edited

Loading

tonybaloney commented Sep 23, 2016

Kami commented Sep 23, 2016

emedvedev commented Sep 23, 2016 •

edited

Loading

emedvedev commented Sep 23, 2016 •

edited

Loading

tonybaloney commented Sep 23, 2016

enykeev commented Sep 26, 2016

enykeev commented Sep 29, 2016 •

edited

Loading

tonybaloney commented Sep 30, 2016

emedvedev commented Sep 30, 2016

lakshmi-kannan Oct 4, 2016

tonybaloney Oct 4, 2016

lakshmi-kannan Oct 4, 2016

tonybaloney Oct 4, 2016

lakshmi-kannan Oct 4, 2016

Extend auth to secure chatops #2920

Extend auth to secure chatops #2920

Conversation

tonybaloney commented Sep 23, 2016 • edited Loading

tonybaloney commented Sep 23, 2016

Kami commented Sep 23, 2016

emedvedev commented Sep 23, 2016 • edited Loading

emedvedev commented Sep 23, 2016 • edited Loading

tonybaloney commented Sep 23, 2016

enykeev commented Sep 26, 2016

enykeev commented Sep 29, 2016 • edited Loading

tonybaloney commented Sep 30, 2016

emedvedev commented Sep 30, 2016

lakshmi-kannan Oct 4, 2016

Choose a reason for hiding this comment

tonybaloney Oct 4, 2016

Choose a reason for hiding this comment

lakshmi-kannan Oct 4, 2016

Choose a reason for hiding this comment

tonybaloney Oct 4, 2016

Choose a reason for hiding this comment

lakshmi-kannan Oct 4, 2016

Choose a reason for hiding this comment

tonybaloney commented Sep 23, 2016 •

edited

Loading

emedvedev commented Sep 23, 2016 •

edited

Loading

emedvedev commented Sep 23, 2016 •

edited

Loading

enykeev commented Sep 29, 2016 •

edited

Loading