Issue/personal keys #5859

bharath-orchestral · 2023-01-05T06:51:00Z

Code to add feature where Users(non-admin) can manage their own keys.
Added a key in config file(personal_keys under rbac). When the key is enabled, Users(non-admin) can create/update/delete/view their own keys and cannot create/update/delete/view other user's keys.

If the 'personal_keys' key is disabled everything behaves as before. Nothing is changed in regards to admins.

My client requirement is such that every user(non-admin) in system should manage their own keys. Current implementation of StackStorm is such that if a user(non-admin) is given apikey 'create' permission that user can create apikeys for every other user. But we need it such that every user(non-admin) manages only their own keys(should not be able to create for others).

Example: Assume 3 users: 'user1', 'user2','user3'.
'user1' is admin, 'user2' has apikey create permission, 'user3' has no permissions attached to him. We need a scenario:

'user1' can create apikeys for everyone since he is admin
'user2', 'user3' can only create apikeys for themselves.

…sonal_keys' flag is added

Merging master into this branch

bharath-orchestral · 2023-01-05T12:25:17Z

Can someone help me on 'ci/circleci:packages' and 'Orquesta CI/Integration Tests' errors? And also where to add changes in CHANGELOG.rst?

amanda11

Some minor comments but in addition:

It would be good if the PR could mention the use-case for this
Need to add changelog entry - needs to go into the in development section, in the added section. Just a new bullet that matches the others in there for formatting
Not sure why the init.py from dummy_pack_23 has been removed
I think it needs some more tests to verify behaviour depending on value of personal_keys and whether the rbac backend allows user or not.
I think we need to consider the cases where RBAC is not enabled. That may just be a reverse of some logic you have added, but I've not considered it deeply - just think that it is missing
In general, then should this be a global setting. Why is it a global setting as to whether personal key is amended. Perhaps that would be explained by more information in the PR, but need to understand use-case and why it has been added as a global setting rather than say another RBAC permission that is given to individual users.

on the failres. I've re-run the failing integration job and the CircleCI jobs, let's see if they pass. As it didn'nt seem to be related to the code changes.

amanda11 · 2023-01-05T16:39:16Z

st2api/st2api/controllers/v1/auth.py

@@ -126,18 +136,42 @@ def get_all(self, requester_user, show_secrets=None, limit=None, offset=0):

        return resp

+    def checkPersonalAPIKeyPermission(self, api_key_api, requester_user):
+        """
+        Checks whether requested user is the creating/updating/fetching/deleting the api key. This is used when


Suggested change

Checks whether requested user is the creating/updating/fetching/deleting the api key. This is used when

Checks whether requested user is creating/updating/fetching/deleting the api key. This is used when

amanda11 · 2023-01-05T16:39:41Z

st2api/st2api/controllers/v1/auth.py

+    def checkPersonalAPIKeyPermission(self, api_key_api, requester_user):
+        """
+        Checks whether requested user is the creating/updating/fetching/deleting the api key. This is used when
+        'personal_keys' flag is set of 'rbac' group in conf file.


Suggested change

'personal_keys' flag is set of 'rbac' group in conf file.

'personal_keys' flag is set in 'rbac' group in conf file.

amanda11 · 2023-01-05T16:40:14Z

st2api/st2api/controllers/v1/auth.py

@@ -126,18 +136,42 @@ def get_all(self, requester_user, show_secrets=None, limit=None, offset=0):

        return resp

+    def checkPersonalAPIKeyPermission(self, api_key_api, requester_user):


api_key_api seems an odd name, is there a more expressive name for variable we can use?

amanda11 · 2023-01-05T16:40:38Z

st2api/st2api/controllers/v1/auth.py

+            )
+            if not isKeyCreationAllowed:
+                LOG.exception(
+                    "User does not have permission to view apikey=%s.", api_key_db


Can we use python 3 f-strings instead of the older method of formatting up strings?

amanda11 · 2023-01-05T16:43:56Z

conf/st2.dev.conf

@@ -43,6 +43,7 @@ stream_output = True

 [rbac]
 enable = False
+personal_keys = True


Do we want the default to be True, or should it be False for backwards compatibility?

amanda11 · 2023-01-05T16:46:45Z

st2common/st2common/config.py

@@ -72,6 +72,11 @@ def register_opts(ignore_errors=False):
            "executions. All resources can only be viewed or executed by the owning user "
            "except the admin and system_user who can view or run everything.",
        ),
+        cfg.BoolOpt(
+            "personal_keys",
+            default=True,


As above do we want default to be True? As that's not backwards compatible with previous versions of STackStorm?

amanda11 · 2023-01-05T16:57:57Z

st2api/st2api/controllers/v1/auth.py

-            resource_db=api_key_db,
-            permission_type=permission_type,
-        )
+        if cfg.CONF.rbac.personal_keys:


I think having this in here rather than in rbac_utils doesn't work with the idea that the rbac is optional.
So for instance if we were configured without rbac, then the no-op rbac backend would be being used, and then the options like this would always be allowed.
This now means that if I don't have RBAC backend I have to have the personal_api_keys set in the config.

Perhaps would be better that we always use rbac_utils to ask if user has db permission, and only if that returns false then you'd want to check the personal keys.

Can you help me as to where to add the code? I cannot understand where the rbac_utils methods are implemented.I have checked base.py in st2common/st2common/rbac/backends/ . Do I need to add the method there, check if conf.rbac.enabled is true and proceed?

I was more suggesting that you reverse the order in this method. e.g. First call rbac_utils method to see if they have permission to access database. Then if the rbac_utils calls says No, you add in your check for personal keys.
That way when the noop backend is used (which will always say yes to permission), you don't need to check personal keys - and can keep personal keys default to false.

There are multiple rbac backends, so if you wanted to add a new method you would need to add into all implementations. The base.py is just the base rbac class which always says not implemented. The noop.py is a always yes utils. Then in the st2-rbac-backend GIT repo you will find the implementation that is used when you really do use RBAC - which uses the rbac roles and auth files.

st2api/st2api/controllers/v1/auth.py

bharath-orchestral · 2023-01-06T06:37:56Z

@amanda11
I have added some more context in the PR.
Added changes in CHANGELOG file.
Made the flag false by default
Added init.py file. Don't know why it got removed in the first place
@nzlosh
Made the suggested code change

amanda11 · 2023-01-06T08:08:26Z

In general, I'd like some consideration from the @StackStorm/tsc as to whether it makes sense to keep this as is - eg. global option that allows if set, so that all users can manage their own keys. OR if there is a use-case where you want to make it an RBAC permission, such that some users can create their own keys but others can't. For example, if we are adding the ability to give permission to manage your own api keys, would it be better to be added as a RBAC permission, e.g. USER_API_KEY_VIEW etc - which allows user to manage their own keys. That way, you can allow some non-admins to manage their keys, but others not. This would be more flexible, and more in keeping with having the permissions scheme we already have. For example, if you want all non-admins to have that, then you'd give that permission to a role that all users had.

I'm just not sure about starting trend of having some permissions controlled by role, and others by system flags. Think it might complicate things long term, and the change to introduce a new permission in my view would be more in sync with current system. If we went down this route, then this would mean that the change would move to the st2-rbac-backend repo, in the implementation of the assert_user_has_resource_db_permission method (though may need a similar method, or extend the signature of the method so you can see what user's api key they are trying to manage). This would be my preference, but I'd wait for feedback from others, as it will require a bigger change.

But I'd welcome other TSC members views.

nzlosh · 2023-01-06T09:48:25Z

I agree with the points @amanda11 has made to keep the feature in the RBAC security model. It's unfortunate the user ownership feature can't be used directly https://docs.stackstorm.com/rbac.html#restricting-users-to-only-view-resources-they-own-or-created to allow users to manage their own keys but perhaps it could be used to inspire an implementation.

arm4b · 2023-01-06T13:19:23Z

@bharath-orchestral Fine-grained control for users to manage their own API keys is an excellent addition! It would be nice to have it in the st2 core 👍

I totally agree about the interface and implementation with @amanda11 and @nzlosh that it's best to have this functionality as a new RBAC permission, instead of the st2.conf option.

bishopbm1 · 2023-01-06T13:53:09Z

I agree with @amanda11 also. I think this fits the bill more in RBAC then in the st2.conf.

We are seeing more and more people turn on RBAC controls to help manage users coming into the platform and using it. I think this falls right inline with that. I like @nzlosh idea about the ownership feature and would be a great implementation as well.

amanda11 · 2023-01-06T14:04:13Z

Thanks for the updates... @bharath-orchestral I think its worth taking a look at the ownership feature that @nzlosh mentioned, and also take a look at the st2-rbac-backend GIT repo, in there you'll see in there a ./st2rbac_backend/utils.py which is where the current checking on the role permissions is done. So if its implemented as a new role permission then it would be around there that would be added.
For the ownership feature then I'm not sure how that's implemented, to advise how easy that would be. But feel free to ask on here, or on the StackStorm thread to help, and I'm sure one of us from the @StackStorm/tsc can help answer any queries.

It would be great to get this feature in, but consensus seems to be agreed to try and move this from the st2.conf into a proper RBAC feature. Hope, this has given some guidance of how to move that into the main RBAC, but we'll help where we can to assist you moving this.

bharath-orchestral added 3 commits December 21, 2022 05:40

Added implementation for crud operation of personal apikeys when 'per…

602bf2b

…sonal_keys' flag is added

Merge branch 'master' into issue/personal_keys

f45874c

Merging master into this branch

Removed test data in htpasswd file

b4c16f3

pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Jan 5, 2023

bharath-orchestral added 2 commits January 5, 2023 07:21

Small changes in auth.py

9e79c80

Disabling rbac, auth in config due to errors in integration tests

1b1d70c

amanda11 requested changes Jan 5, 2023

View reviewed changes

nzlosh reviewed Jan 5, 2023

View reviewed changes

st2api/st2api/controllers/v1/auth.py Outdated Show resolved Hide resolved

Small changes as suggested by reviewer

94b895b

arm4b added the feature label Jan 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/personal keys #5859

Issue/personal keys #5859

bharath-orchestral commented Jan 5, 2023 •

edited

bharath-orchestral commented Jan 5, 2023

amanda11 left a comment

amanda11 Jan 5, 2023

amanda11 Jan 5, 2023

amanda11 Jan 5, 2023

amanda11 Jan 5, 2023

amanda11 Jan 5, 2023

amanda11 Jan 5, 2023

amanda11 Jan 5, 2023

bharath-orchestral Jan 6, 2023

amanda11 Jan 6, 2023

bharath-orchestral commented Jan 6, 2023

amanda11 commented Jan 6, 2023 •

edited

nzlosh commented Jan 6, 2023

arm4b commented Jan 6, 2023

bishopbm1 commented Jan 6, 2023

amanda11 commented Jan 6, 2023

	Checks whether requested user is the creating/updating/fetching/deleting the api key. This is used when
	Checks whether requested user is creating/updating/fetching/deleting the api key. This is used when

	'personal_keys' flag is set of 'rbac' group in conf file.
	'personal_keys' flag is set in 'rbac' group in conf file.

		@@ -126,18 +136,42 @@ def get_all(self, requester_user, show_secrets=None, limit=None, offset=0):

		return resp

		def checkPersonalAPIKeyPermission(self, api_key_api, requester_user):

Issue/personal keys #5859

Are you sure you want to change the base?

Issue/personal keys #5859

Conversation

bharath-orchestral commented Jan 5, 2023 • edited

bharath-orchestral commented Jan 5, 2023

amanda11 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

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

bharath-orchestral commented Jan 6, 2023

amanda11 commented Jan 6, 2023 • edited

nzlosh commented Jan 6, 2023

arm4b commented Jan 6, 2023

bishopbm1 commented Jan 6, 2023

amanda11 commented Jan 6, 2023

bharath-orchestral commented Jan 5, 2023 •

edited

amanda11 commented Jan 6, 2023 •

edited