Page MenuHomeSoftware Heritage

Migrate deposit to use keycloak as authentication mechanism
ClosedPublic

Authored by ardumont on Feb 24 2021, 4:57 PM.

Details

Summary

This reworks the internal authentication mechanism to delegate that part to keycloak.
Through a Direct Grant access.

It's transparent for the deposit clients. They will continue their authentication against
the deposit server. Internally, the deposit server will require authentication on their
behalf.

What works [5]:

  • tryouts from within docker (user hal/test):
    • rejects unknown user or invalid creds
    • service document is served with the right creds
    • metadata-only deposit...
  • actual (manual) deposit tests are ok (curl on servicedocument url, metadata-only deposit, status, ... )
  • use cache to avoid authentication at each http connection (cf. django cache mechanism)
  • leverage keycloak scope to allow keycloak user with the right "swh.deposit.api" permission (decrypt token and read role there to authorize/reject authenticated user)
  • adapt user django model to permission (-> swh.auth)

Remains to:

  • Installs actual users (with their password) to keycloak (either manual or script)
  • update docs
  • split this one diff into multiple ones or squash all incremental commits into one (incremental

code got rewritten anyway and pushed into swh-auth).

  • (?) adapt the deposit admin cli so it creates user in keycloak

Note that I checked other python modules [1] [2] [3].

They seem to have little movement (for some, last actions dates back at best 2 years
ago). They don't entertain in their documentation the authentication scheme [4] we want to
use ([1] source code seems to be able to support it though). They are not packaged for
debian.

So in the end, a new module swh-auth (T3079) reused most of what has been done for
that purpose. And then slightly adapted (incrementally) to allow the Direct Grant flow
for the deposit.

[1] https://github.com/Peter-Slump/django-keycloak

[2] https://github.com/marcospereirampj/django-rest-framework-keycloak

[3] https://github.com/marcelo225/django-keycloak-auth

[4] https://tools.ietf.org/html/rfc6749#section-4.3

[5] P964

Related to T2858

Test Plan

tox

P964

Diff Detail

Repository
rDDEP Push deposit
Branch
keycloak-integration
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19873
Build 30865: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 30864: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Add coverage on the swh.deposit.auth backend class

Build has FAILED

Patch application report for D5137 (id=18765)

Rebasing onto 4353a323f6...

Current branch diff-target is up to date.
Changes applied before test
commit 7b68fb0efd3405e5aebd383515a13e55db345e8b
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Mar 11 22:33:43 2021 +0100

    Delegate authentication to keycloak 2/2
    
    Related to T2858

commit f42e3dfed400d73fe604ebe6369051e3e84a1a40
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Feb 24 16:28:07 2021 +0100

    Delegate authentication to keycloak 1/2
    
    Related to T2858

commit d664c76502a7ad021b4f5f78c7238728685bfeb8
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Feb 24 11:43:46 2021 +0100

    auth: Make keycloak instantiable from configuration
    
    Related to T2858

commit 29ae8f35baadda60247b258a4570690c3338f245
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Feb 18 16:57:55 2021 +0100

    Add KeycloakOpenIDConnectDirectGrant class for keycloak authentication
    
    Related to T2858

Link to build: https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/559/
See console output for more information: https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/559/console

Build has FAILED

please try again, swh.auth >= 0.3.2 exist, i assure you ;)

Build is green

Patch application report for D5137 (id=18765)

Rebasing onto 4353a323f6...

Current branch diff-target is up to date.
Changes applied before test
commit 7b68fb0efd3405e5aebd383515a13e55db345e8b
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Mar 11 22:33:43 2021 +0100

    Delegate authentication to keycloak 2/2
    
    Related to T2858

commit f42e3dfed400d73fe604ebe6369051e3e84a1a40
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Feb 24 16:28:07 2021 +0100

    Delegate authentication to keycloak 1/2
    
    Related to T2858

commit d664c76502a7ad021b4f5f78c7238728685bfeb8
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Feb 24 11:43:46 2021 +0100

    auth: Make keycloak instantiable from configuration
    
    Related to T2858

commit 29ae8f35baadda60247b258a4570690c3338f245
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Feb 18 16:57:55 2021 +0100

    Add KeycloakOpenIDConnectDirectGrant class for keycloak authentication
    
    Related to T2858

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/560/ for more details.

backend: Add test for insufficient check conditional

Build is green

Patch application report for D5137 (id=18766)

Rebasing onto 4353a323f6...

Current branch diff-target is up to date.
Changes applied before test
commit 545d1bfce1d0d79e445871d41d1146f604aa4687
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Mar 11 22:33:43 2021 +0100

    Delegate authentication to keycloak 2/2
    
    Related to T2858

commit f42e3dfed400d73fe604ebe6369051e3e84a1a40
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Feb 24 16:28:07 2021 +0100

    Delegate authentication to keycloak 1/2
    
    Related to T2858

commit d664c76502a7ad021b4f5f78c7238728685bfeb8
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Feb 24 11:43:46 2021 +0100

    auth: Make keycloak instantiable from configuration
    
    Related to T2858

commit 29ae8f35baadda60247b258a4570690c3338f245
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Feb 18 16:57:55 2021 +0100

    Add KeycloakOpenIDConnectDirectGrant class for keycloak authentication
    
    Related to T2858

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/561/ for more details.

anlambert added inline comments.
swh/deposit/auth.py
143–150

Agreed, authentication backend should only login a user.

153–156

You should rather use:

deposit_client, created = DepositClient.objects.get_or_create(username=user_id)

because at this point, the user has been successfully authenticated with Keycloak so
it is safe to add it in django db.

You should never have to create user manually through the CLI now this is delegated
to Keycloak.

swh/deposit/cli/admin.py
84

When the migration to Keycloak will be done, you should never have to use that CLI again
as user registration will be delegated to Keycloak.

swh/deposit/exception.py
18

why moving that import here ?

swh/deposit/models.py
123

s/_state/session_state/

133

same here

swh/deposit/tests/conftest.py
254–260

Use DepositClient.objects.get_or_create instead.

283–284

This setup could be moved to swh-auth later as swh-web will also need to do such kind of mocking.

304

s/Returned/Return/

This revision now requires changes to proceed.Mar 12 2021, 11:30 AM
swh/deposit/exception.py
18

one test about this failed otherwise
(i don't recall exactly why)

  • Rename _state to session_state
  • Move permission check concern to dedicated HasDepositPermission class and declare it (\m/)
  • Refactor pytest fixtures a bit

Build is green

Patch application report for D5137 (id=18771)

Rebasing onto 4353a323f6...

Current branch diff-target is up to date.
Changes applied before test
commit 98d9963c907069661fc038c3a3cc6a95e05cadf6
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Mar 11 22:33:43 2021 +0100

    Delegate authentication to keycloak 2/2
    
    Related to T2858

commit f42e3dfed400d73fe604ebe6369051e3e84a1a40
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Feb 24 16:28:07 2021 +0100

    Delegate authentication to keycloak 1/2
    
    Related to T2858

commit d664c76502a7ad021b4f5f78c7238728685bfeb8
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Feb 24 11:43:46 2021 +0100

    auth: Make keycloak instantiable from configuration
    
    Related to T2858

commit 29ae8f35baadda60247b258a4570690c3338f245
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Feb 18 16:57:55 2021 +0100

    Add KeycloakOpenIDConnectDirectGrant class for keycloak authentication
    
    Related to T2858

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/563/ for more details.

ardumont added inline comments.
swh/deposit/cli/admin.py
84

it's not entirely clear yet ;)

collection, provider-url and domain are the deposit's concern for now.

It has been discussed to move this over to the keycloak but same it's not
entirely if we want to also do that...

¯\_(ツ)_/¯

ardumont added inline comments.
swh/deposit/models.py
123

The id should not be filtered out.

Build is green

Patch application report for D5137 (id=18775)

Rebasing onto 4353a323f6...

Current branch diff-target is up to date.
Changes applied before test
commit b729ed05ef76ceb9d1c9da7f88021a2cdc3de477
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Mar 11 22:33:43 2021 +0100

    Delegate authentication to keycloak 2/2
    
    Related to T2858

commit f42e3dfed400d73fe604ebe6369051e3e84a1a40
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Feb 24 16:28:07 2021 +0100

    Delegate authentication to keycloak 1/2
    
    Related to T2858

commit d664c76502a7ad021b4f5f78c7238728685bfeb8
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Feb 24 11:43:46 2021 +0100

    auth: Make keycloak instantiable from configuration
    
    Related to T2858

commit 29ae8f35baadda60247b258a4570690c3338f245
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Feb 18 16:57:55 2021 +0100

    Add KeycloakOpenIDConnectDirectGrant class for keycloak authentication
    
    Related to T2858

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/564/ for more details.

swh/deposit/auth.py
153–156

keycloak has, for now, only the responsibility to log the deposit users.

We still need other information on the deposit client though (provider_url,
domain, collection...). Those are stored in the db backend. Thus why it's here.

swh/deposit/models.py
117

this should rather be named set_oidc_profile as it is not a factory method and moved to swh.auth.models.OIDCUser

118–127

This should rather be moved in the OIDCUser class with the following signature and name:

def set_oidc_profile(self, oidc_profile: Dict[str, Any]) -> "OIDCUser":

Then in your backend code:

deposit_client.set_oidc_profile(oidc_user.oidc_profile())
129

this could be moved as an oidc_profile method in the OIDCUser class.

133–137

if you move that code to OIDCUser, you only need to return attributes proper to that class in a dict.

Oh and you should keep session_state, I thought you made a typo in my first review round (did not know about _state storing a django ModelState).

swh/deposit/models.py
118–127

correction de signature

def set_oidc_profile(self, oidc_profile: Dict[str, Any]) -> None
swh/deposit/auth.py
130

how could that have worked...?
It's used below but it's not an oidc_profile which is returned, it's an OIDCUser...
¯\_(ツ)_/¯

Rework the DepositClient model and move away from inheritance. It's a pain in the neck
and force some kind of useless migration. Instead move towards a collaborator pattern.
DepositClient has one collaboration, the oidc_user.

This also fixes a couple of buggy issues neither detected by test nor mypy!

  1. Cache entry, in certain circumstance could have outlive its time (each time a cache

hit was done, we updated again the tll with new date).

  1. the get_user method outputted with cache hit an OIDCUser and somehow, the rest of the

code manipulated it as dict without any issues... ¯\_(ツ)_/¯

Both fixed!

Build is green

Patch application report for D5137 (id=18786)

Rebasing onto 4353a323f6...

Current branch diff-target is up to date.
Changes applied before test
commit bc6820031356f7b6df4c2878fca07e7d54c4ad3a
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Mar 12 17:07:13 2021 +0100

    Delegate authentication to keycloak 2/2
    
    Related to T2858

commit f42e3dfed400d73fe604ebe6369051e3e84a1a40
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Feb 24 16:28:07 2021 +0100

    Delegate authentication to keycloak 1/2
    
    Related to T2858

commit d664c76502a7ad021b4f5f78c7238728685bfeb8
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Feb 24 11:43:46 2021 +0100

    auth: Make keycloak instantiable from configuration
    
    Related to T2858

commit 29ae8f35baadda60247b258a4570690c3338f245
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Feb 18 16:57:55 2021 +0100

    Add KeycloakOpenIDConnectDirectGrant class for keycloak authentication
    
    Related to T2858

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/568/ for more details.

ardumont added inline comments.
swh/deposit/models.py
154

about that, manage makemigrations complained about it... something got misses, it's actually the case in db...

swh/deposit/tests/conftest.py
254–260

good idea.

Some code from swh-auth should be reused here.

swh/deposit/auth.py
152–154
from swh.web.auth.utils import oidc_user_from_profile

oidc_user = oidc_user_from_profile(oidc_profile)
158–159
from django.utils import timezone

ttl = int(oidc_user.refresh_expires_at.timestamp() - timezone.now().timestamp())
This revision now requires changes to proceed.Mar 12 2021, 5:34 PM
swh/deposit/auth.py
152–154

corrected code

oidc_user = oidc_user_from_profile(self.client(), oidc_profile)
ardumont marked 3 inline comments as done.

Adapt according to the awesome suggestion

Build is green

Patch application report for D5137 (id=18789)

Rebasing onto 4353a323f6...

Current branch diff-target is up to date.
Changes applied before test
commit d693a934d5fcc2a437a8ee923fed6179bd85269c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Mar 12 17:07:13 2021 +0100

    Delegate authentication to keycloak 2/2
    
    Related to T2858

commit f42e3dfed400d73fe604ebe6369051e3e84a1a40
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Feb 24 16:28:07 2021 +0100

    Delegate authentication to keycloak 1/2
    
    Related to T2858

commit d664c76502a7ad021b4f5f78c7238728685bfeb8
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Feb 24 11:43:46 2021 +0100

    auth: Make keycloak instantiable from configuration
    
    Related to T2858

commit 29ae8f35baadda60247b258a4570690c3338f245
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Feb 18 16:57:55 2021 +0100

    Add KeycloakOpenIDConnectDirectGrant class for keycloak authentication
    
    Related to T2858

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/569/ for more details.

swh/deposit/auth.py
164

nitpick, you could drop the exceptions. prefix by adapting the imports

Build is green

Patch application report for D5137 (id=18790)

Rebasing onto 4353a323f6...

Current branch diff-target is up to date.
Changes applied before test
commit 78650f85260fe173a9400a77ce9c12b9f799d13a
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Mar 12 17:07:13 2021 +0100

    Delegate authentication to keycloak 2/2
    
    Related to T2858

commit f42e3dfed400d73fe604ebe6369051e3e84a1a40
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Feb 24 16:28:07 2021 +0100

    Delegate authentication to keycloak 1/2
    
    Related to T2858

commit d664c76502a7ad021b4f5f78c7238728685bfeb8
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Feb 24 11:43:46 2021 +0100

    auth: Make keycloak instantiable from configuration
    
    Related to T2858

commit 29ae8f35baadda60247b258a4570690c3338f245
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Feb 18 16:57:55 2021 +0100

    Add KeycloakOpenIDConnectDirectGrant class for keycloak authentication
    
    Related to T2858

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/570/ for more details.

Looks good !

I guess we could have some kind of generic direct grant flow django auth backend in swh-auth that could be reused by the deposit but that will be for later.

This revision is now accepted and ready to land.Mar 12 2021, 5:57 PM

I guess we could have some kind of generic direct grant flow django auth backend in swh-auth that could be reused by the deposit but that will be for later.

yes, agreed and agreed ;)

Build is green

Patch application report for D5137 (id=18791)

Rebasing onto 4353a323f6...

Current branch diff-target is up to date.
Changes applied before test
commit 17ccda4b58fb1fed55cbc597ef96320c8c0a54f8
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Mar 12 17:07:13 2021 +0100

    Delegate authentication to keycloak 2/2
    
    Related to T2858

commit f42e3dfed400d73fe604ebe6369051e3e84a1a40
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Feb 24 16:28:07 2021 +0100

    Delegate authentication to keycloak 1/2
    
    Related to T2858

commit d664c76502a7ad021b4f5f78c7238728685bfeb8
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Feb 24 11:43:46 2021 +0100

    auth: Make keycloak instantiable from configuration
    
    Related to T2858

commit 29ae8f35baadda60247b258a4570690c3338f245
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Feb 18 16:57:55 2021 +0100

    Add KeycloakOpenIDConnectDirectGrant class for keycloak authentication
    
    Related to T2858

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/571/ for more details.

ardumont retitled this revision from wip: Migrate deposit to use keycloak as authentication mechanism to Migrate deposit to use keycloak as authentication mechanism.Mar 15 2021, 9:58 AM
  • Rebase
  • Squash commit into 1

Rework some more the commit message

Build is green

Patch application report for D5137 (id=18823)

Rebasing onto 3a9b2fc4ba...

Current branch diff-target is up to date.
Changes applied before test
commit 114d96ef9ea502d0d224876558ec49c45bbf9911
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Feb 18 16:57:55 2021 +0100

    Delegate authentication to keycloak
    
    Related to T2858

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/576/ for more details.

Build is green

Patch application report for D5137 (id=18825)

Rebasing onto 3a9b2fc4ba...

Current branch diff-target is up to date.
Changes applied before test
commit 27efcccbfaea7188b05e6ce18e201ec9a2ddfb79
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Feb 18 16:57:55 2021 +0100

    Delegate authentication to keycloak
    
    This reworks the internal authentication mechanism to delegate that part to keycloak.
    Through a Direct Grant access flow.
    
    It's transparent for the deposit clients. They will continue their authentication
    against the deposit server. Internally, the deposit server will require authentication
    on their behalf.
    
    Related to T2858

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/577/ for more details.

This revision was automatically updated to reflect the committed changes.