feat(integrations): add endpoint for rotating client secret of Internal/Public integrations #69015

oioki · 2024-04-16T18:20:38Z

Adding an endpoint that allows to rotate clientSecret of Internal/Public integrations.
Inspired by #53124

ykamo001 · 2024-04-17T19:36:48Z

src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py

+    def has_object_permission(self, request: Request, view: object, sentry_app: SentryApp):
+        # organization that owns an integration
+        org_context = organization_service.get_organization_by_id(id=sentry_app.owner_id)
+        if org_context is None:
+            raise Http404
+
+        organization = org_context.organization
+        if organization is None:
+            raise Http404
+
+        # if user is not a member of an organization owning an integration,
+        # return 404 to avoid leaking integration slug
+        organizations = (
+            user_service.get_organizations(user_id=request.user.id)
+            if request.user.id is not None
+            else ()
+        )
+        if not any(organization.id == org.id for org in organizations):
+            raise Http404
+
+        # permission check inside an organization
+        self.determine_access(request, organization)
+        allowed_scopes = set(self.scope_map.get(request.method or "", []))
+        return any(request.access.has_scope(s) for s in allowed_scopes)


Would it make more sense to just return false/boolean and have the parent/calling method decide what error code it should use? I think instead of 404, a failed permission is more of a 403?

I think maybe even a 400 bad request status code might be more accurate? That being said, do we think this is plausible? Can a SentryApp (id or object) exist without a representational organization?

Because technically a 404 would mean the sentry_app itself doesn't exist, which isn't really true in this case right?

Technically 403 means failed permission, but in this case we don't want to leak info if a specific integration slug exists with different status codes. Take a look from an attacker's perspective:

POST /integration-aaa/rotate-secret/ 404 -- integration does not exist POST /integration-bb/rotate-secret/ 403 -- aha, so it exists but I don't have permissions. I can try to build some attack on top of this info

We're taking a similar approach in other places, for example:

sentry/src/sentry/api/bases/sentryapps.py

Lines 223 to 224 in 0491f7d

if not any(sentry_app.owner_id == org.id for org in organizations):

raise Http404

Ahhh, I see what you're saying, that makes sense!

I doubt we are, but hope we're that consistent across our other API endpoints in terms of behavior and expected status codes

ykamo001 · 2024-04-17T19:40:48Z

src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py

+
+    def has_object_permission(self, request: Request, view: object, sentry_app: SentryApp):
+        # organization that owns an integration
+        org_context = organization_service.get_organization_by_id(id=sentry_app.owner_id)


since this is an RPC call, which could have network/side effect errors, would it be worth wrapping in a try/catch, and defaulting to return false for safety?

I checked other uses of organization_service.get_organization_by_id, and none of them are using this try-catch approach. I see only checks like if not org_context or if org_context is None, so I presume network failures are handled somewhere else (in RPC abstraction?).

ykamo001 · 2024-04-17T19:44:24Z

src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py

+        "POST": ["org:write", "org:admin"],
+    }
+
+    def has_object_permission(self, request: Request, view: object, sentry_app: SentryApp):


Something to help us understand our decisions and debug in the future: should we log when we make a decision in this function?

i.e.

if org_context is None: log.info("org does not exist for request", extra={"app_id": app_id, "user_id": user_id, ...etc}) return False

Or, maybe the exception we raise has a better error message to help both the user and us (for showing in the UI and debugging when users come to us for support)

Added logging for now.

ykamo001 · 2024-04-17T19:47:21Z

src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py

+
+    def post(self, request: Request, sentry_app: SentryApp) -> Response:
+        if sentry_app.application is None:
+            return Response(status=404)


should we add some more details to the response?

ykamo001 · 2024-04-17T19:47:58Z

src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py

+            return Response(status=404)
+
+        new_token = generate_token()
+        sentry_app.application.update(client_secret=new_token)


should we add a try/catch here and respond accordingly? And also log the error? Because this is a DB transaction, there could be errors

ykamo001 · 2024-04-17T19:49:12Z

src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py

+        if sentry_app.application is None:
+            return Response(status=404)
+
+        new_token = generate_token()


@mdtro I know we're making some changes to this method in another PR, is this still safe/intended to be used in this flow?

@ykamo001 Yes, this is okay for now. :) When we come back through to improve the app API tokens we can adjust these calls then.

ykamo001 · 2024-04-17T19:51:17Z

src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py

+        # if user is not a member of an organization owning an integration,
+        # return 404 to avoid leaking integration slug
+        organizations = (
+            user_service.get_organizations(user_id=request.user.id)


Same question here regarding the RPC try/catch

ykamo001 · 2024-04-17T19:52:45Z

src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py

+from sentry.services.hybrid_cloud.user.service import user_service
+
+
+class SentryAppRotateSecretPermission(SentryPermission):


@cathteng should superusers be allowed to do this for other orgs on their behalf for support cases?

fair. maybe we only let superusers with write privileges do this?

ykamo001 · 2024-04-17T19:54:55Z

Since you pulled the FE changes to another PR, you can probably remove the FE label on the PR, I am not sure it will auto update and remove for you haha

…integrations, frontend part (#69115) To be merged after #69015 <img width="1167" alt="image" src="https://github.com/getsentry/sentry/assets/1127549/eb8ba4bf-2596-4ce5-be74-8386d1bacbc1">

Follow-up on #69015 #69115

…al/Public integrations (#69015) Adding an endpoint that allows to rotate `clientSecret` of Internal/Public integrations. Inspired by #53124

…integrations, frontend part (#69115) To be merged after #69015 <img width="1167" alt="image" src="https://github.com/getsentry/sentry/assets/1127549/eb8ba4bf-2596-4ce5-be74-8386d1bacbc1">

Follow-up on #69015 #69115

oioki added 2 commits April 16, 2024 19:38

feat(integrations): add endpoint for rotating sentry app clientSecret

29d60fb

add tests and fix HC case

5ec75fb

oioki requested review from mdtro and a team April 16, 2024 18:20

oioki requested review from a team as code owners April 16, 2024 18:20

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 16, 2024

cathteng requested a review from ykamo001 April 16, 2024 18:57

oioki added 2 commits April 16, 2024 21:17

add new control silo URL pattern

2e4aa9c

fix typing issues

a8c75ed

github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 16, 2024

vercel bot deployed to Preview April 16, 2024 19:34 View deployment

oioki mentioned this pull request Apr 17, 2024

feat(integrations): allow to rotate client secret of Internal/Public integrations, frontend part #69115

Merged

ykamo001 reviewed Apr 17, 2024

View reviewed changes

oioki added 4 commits April 18, 2024 10:25

cleanup

c429586

allow superuser access

6588418

more details in case of malformed sentry app

4310cc2

add logging to has_object_permission

6c64156

oioki removed the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 18, 2024

vercel bot deployed to Preview April 18, 2024 09:19 View deployment

oioki requested a review from ykamo001 April 18, 2024 12:28

getsentry deleted a comment from github-actions bot Apr 18, 2024

ykamo001 approved these changes Apr 18, 2024

View reviewed changes

oioki merged commit 1a732c3 into master Apr 19, 2024
52 checks passed

oioki deleted the feat/sentry-app-rotate-secret-endpoint branch April 19, 2024 07:29

oioki mentioned this pull request Apr 19, 2024

feat(sentry-apps): hide clientSecret in 1 day after creation #69289

Merged

oioki added a commit that referenced this pull request Apr 22, 2024

feat(sentry-apps): hide clientSecret in 1 day after creation (#69289)

5d785d6

Follow-up on #69015 #69115

MichaelSun48 pushed a commit that referenced this pull request Apr 25, 2024

feat(sentry-apps): hide clientSecret in 1 day after creation (#69289)

87df077

Follow-up on #69015 #69115

github-actions bot locked and limited conversation to collaborators May 4, 2024

	if not any(sentry_app.owner_id == org.id for org in organizations):
	raise Http404

		from sentry.services.hybrid_cloud.user.service import user_service


		class SentryAppRotateSecretPermission(SentryPermission):

Uh oh!

feat(integrations): add endpoint for rotating client secret of Internal/Public integrations #69015

feat(integrations): add endpoint for rotating client secret of Internal/Public integrations #69015

Uh oh!

Conversation

oioki commented Apr 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oioki Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ykamo001 commented Apr 17, 2024

Uh oh!

Uh oh!

Uh oh!

oioki Apr 18, 2024 •

edited

Loading