Use project-scoped temporal tokens to interact with the API from the builders #10378

stsewd · 2023-06-01T20:58:18Z

This implements the design document from https://dev.readthedocs.io/en/latest/design/secure-api-access-from-builders.html

The api.v2 package was converted into a real django app, so we can add models to it.
A custom API key model was created to hold the relationship of the key with a project
A /api/v2/revoke/ endpoint was added to revoke an API key after it has been used.
The old super-user permission based still works, this is to avoid breaking the builds while we do the deploy,
that code can be removed in the next deploy.
All endpoints use the project attached to the API key to filter the resources
API keys expire after 3 hours

Closes https://github.com/readthedocs/meta/issues/21

For #10289 we are going to need to pass a new argument (build_api_key). And since we deploy webs first, builders will have the old task that doesn't match the new signature, and the task will fail. To avoid this, we can just accept any kwargs, this obviously only works if the change is backwards compatible with the old code from the builders (in this case it will be).

We aren't using the LocalEnvironment class, only the BuildEnvironment, so there is no need to keep BaseEnvironment seperated from BuildEnvironment.

…d-api-token-access

With #10378 we now need to always pass an environment, we can't just create a default one.

ericholscher

This looks like a good start, I think there's some places where we need to be more clear about how auth is done in comments and perhaps dev docs somewhere? I don't fully understand where the actual flow of auth is getting checked, likely because some of that logic is living in the API key package? But how do we easily explain to readers of our code what it's depending on the API key package for?

ericholscher · 2023-06-19T21:44:28Z

readthedocs/api/v2/permissions.py

@@ -65,3 +67,37 @@ def has_permission(self, request, view):
            .exists()
        )
        return has_access
+
+
+class TokenKeyParser(KeyParser):


Odd this isn't builtin... seems pretty standard?

They default to Api-Key {token}

readthedocs/api/v2/views/core_views.py

ericholscher · 2023-06-19T21:48:58Z

readthedocs/api/v2/views/model_views.py

@@ -151,7 +155,7 @@ class ProjectViewSet(DisableListEndpoint, UpdateModelMixin, UserSelectViewSet):

    """List, filter, etc, Projects."""

-    permission_classes = [APIRestrictedPermission]
+    permission_classes = [HasBuildAPIKey | APIRestrictedPermission]


Weird syntax... I feel like this could use a comment explaining the logic. Guessing it means either can be used to validate permissions (eg. OR)?

Yeah, this is an or (it supports & too), it's standard from REST framework. I can add a comment if it's useful.

ericholscher · 2023-06-19T21:49:47Z

readthedocs/api/v2/views/model_views.py

        return Response({
            'url': project.get_docs_url(),
        })

+    def get_queryset(self):


Could use a docstring, since it seems like this is the core of the logic that checks authz in the queryset?

ericholscher · 2023-06-19T21:50:23Z

readthedocs/api/v2/views/model_views.py

+        build_api_key = request.build_api_key
+        if build_api_key:
+            if project_slug != build_api_key.project.slug:
+                raise Http404()


We should probably have a log message here, at least for our debugging use.

Why isn't this check performed in other places, because the queryset filter is doing the work there, and this code isn't using the get_queryset method for some reason?

yeah, all others just use the queryset.

readthedocs/core/views/hooks.py

readthedocs/projects/tasks/builds.py

ericholscher · 2023-06-19T21:59:28Z

readthedocs/api/v2/permissions.py

+
+    def has_permission(self, request, view):
+        build_api_key = None
+        has_permission = super().has_permission(request, view)


I don't fully understand what code this is calling... What is actually being validated here? Is it in our code, or a third-party package? And is it checking the project matches the API key somehow?

We rely on the permission class from the package to make sure that the API key given is valid, we just override it to attach the key to the request, otherwise we need to parse the key and get it from the db every time we want to use from a view.

readthedocs.org/readthedocs/api/v2/permissions.py

Lines 85 to 91 in f261ecc

"""

Custom permission to inject the build API key into the request.

This avoids having to parse the key again on each view.

The key is injected in the ``request.build_api_key`` attribute

only if it's valid, otherwise it's set to ``None``.

"""

Gotcha, so it's basically just checking that the key exists? Not checking it against any specific user or anything.

yeah, api keys aren't attached to a user, authorization for a given project is done via the queryset.

readthedocs/api/v2/views/model_views.py

ericholscher

Think I'm 👍 on this after the comments. Looks like a huge win.

humitos

This is really cool! I know this is already in production, but I haven't had the time to take a look before. I made some comments and questions that can be applied in another PR if you consider

readthedocs/api/v2/models.py

humitos · 2023-07-05T10:28:31Z

readthedocs/api/v2/models.py

+
+    project = models.ForeignKey(
+        Project,
+        on_delete=models.CASCADE,


This adds more overhead to the deletion of the project. We may want to make this SET_NULL or similar. See #10040

readthedocs/api/v2/views/model_views.py

humitos · 2023-07-05T10:36:56Z

readthedocs/api/v2/views/model_views.py

+    def perform_create(self, serializer):
+        """Restrict creation to builds attached to the project from the api key."""
+        build_pk = serializer.validated_data["build"].pk
+        api_key = self.request.build_api_key
+        if api_key and not api_key.project.builds.filter(pk=build_pk).exists():
+            raise PermissionDenied()
+        # If the request isn't attached to a build api key,
+        # the user doing the request is a superuser, so it has access to all projects.
+        return super().perform_create(serializer)


Why do we need this only for this view but it's not required when updating a Version or Build object via the API from the builder as well?

When updating, we know the object (since it already exists), so the queryset already filters the objects the token has access to. When creating, we don't know where the object belongs since it hasn't been created, but we know that it should be attached to the project the token grants access to.

humitos · 2023-07-05T10:38:32Z

readthedocs/projects/tasks/builds.py

+        try:
+            self.data.api_client.revoke.post()
+        except Exception:
+            log.exception("Failed to revoke build api key.", exc_info=True)


I would make this just an INFO. There is no need to log this as an exception. The key will expire anyways in 3 hours.

stsewd added 7 commits May 31, 2023 20:00

Build: merge BaseEnvironment with BuildEnvironment

58aed74

We aren't using the LocalEnvironment class, only the BuildEnvironment, so there is no need to keep BaseEnvironment seperated from BuildEnvironment.

Linter

31387db

Merge branch 'avoid-breaking-builds-on-task-params-changes' into buil…

b966c2e

…d-api-token-access

Fix tests

bc5a648

Merge branch 'merge-build-env' into build-api-token-access

1cff5bf

Create API token model

49e8eb6

stsewd added a commit that referenced this pull request Jun 5, 2023

Build: pass environment explicitly

ba652de

With #10378 we now need to always pass an environment, we can't just create a default one.

stsewd mentioned this pull request Jun 5, 2023

Build: pass environment explicitly #10388

Merged

stsewd added a commit that referenced this pull request Jun 5, 2023

Build: pass environment explicitly (#10388)

ccb24e9

With #10378 we now need to always pass an environment, we can't just create a default one.

stsewd added 14 commits June 13, 2023 18:02

Merge branch 'main' into build-api-token-access

ba7356b

Updates

eddc6a4

Black

93a493e

Fix tests

1169421

Updates tests

4a67956

Black

623bf20

Linter

d3a5d22

Merge branch 'main' into build-api-token-access

2467464

Update just one dep

8878394

Linter

4cd5242

Custom manager + revoke view

7ca4819

Update docstring

c352aa6

Test revoke calls

9da76fa

Add tests

475fee7

stsewd changed the title ~~Build api token access~~ Use project-scoped temporal tokens to interact with the API from the builders Jun 15, 2023

stsewd marked this pull request as ready for review June 16, 2023 03:26

stsewd requested a review from a team as a code owner June 16, 2023 03:26

stsewd requested a review from humitos June 16, 2023 03:26

auto-assign bot assigned stsewd Jun 16, 2023

Merge branch 'main' into build-api-token-access

f261ecc

ericholscher reviewed Jun 19, 2023

View reviewed changes

ericholscher approved these changes Jun 20, 2023

View reviewed changes

stsewd added 3 commits June 22, 2023 14:38

Merge branch 'main' into build-api-token-access

df55b2c

Updates from review

dd00fc2

Fix

5f1dde4

stsewd merged commit 5508303 into main Jun 22, 2023

stsewd deleted the build-api-token-access branch June 22, 2023 23:24

humitos mentioned this pull request Jul 3, 2023

Add project-scoped API tokens to limit the risks #7928

Open

humitos reviewed Jul 5, 2023

View reviewed changes

	"""
	Custom permission to inject the build API key into the request.

	This avoids having to parse the key again on each view.
	The key is injected in the ``request.build_api_key`` attribute
	only if it's valid, otherwise it's set to ``None``.
	"""

Uh oh!

Use project-scoped temporal tokens to interact with the API from the builders #10378

Use project-scoped temporal tokens to interact with the API from the builders #10378

Uh oh!

Conversation

stsewd commented Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericholscher left a comment

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!

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!

ericholscher Jun 19, 2023 • 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!

Uh oh!

Uh oh!

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!

Uh oh!

ericholscher left a comment

Choose a reason for hiding this comment

Uh oh!

humitos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

stsewd commented Jun 1, 2023 •

edited

Loading

ericholscher Jun 19, 2023 •

edited

Loading