frontend/oidc: add OIDC_UNIQUE_PREFERRED_USERNAMES #3023

Bob131 · 2023-11-23T18:21:04Z

The current OpenID Connect code assumes the existence of a non-standard field in the UserInfo returned by the provider, limiting the ability to use non-Ipsilon OIDC providers; however, the standard forbids using corresponding standardised field (preferred_username) as a unique user identifier, a property on which Copr relies.

This commit adds a fall back path to the OIDC code that will use preferred_username anyway if the administrator asserts that the values will be unique in practice with a new configuration option. The upshot of this change is that administrators can now use OIDC providers like Gitea/Forgejo.

Fixes #3008.

Bob131 · 2023-11-23T18:40:14Z

Testing this is kind of a pain in the neck. This is my attempt at a rough guide to reproduce my testing environment:

Apply the following patch:

diff --git a/docker-compose.dev.yaml b/docker-compose.dev.yaml
index d8eec3de..d55d3486 100644
--- a/docker-compose.dev.yaml
+++ b/docker-compose.dev.yaml
@@ -11,24 +11,24 @@ services:
   backend-log:
     environment:
       - PYTHONPATH=/opt/copr/backend
-    command: /usr/sbin/runuser -u copr -g copr -- /opt/copr/backend/run/copr_run_logger.py
+    command: /opt/copr/backend/run/copr_run_logger.py
 
   backend-build:
     environment:
       - PYTHONPATH=/opt/copr/backend
-    command: /usr/sbin/runuser -u copr -g copr -- /usr/bin/copr-run-dispatcher builds
+    command: /usr/bin/copr-run-dispatcher builds
 
   backend-action:
     environment:
       - PYTHONPATH=/opt/copr/backend
-    command: /usr/sbin/runuser -u copr -g copr -- /usr/bin/copr-run-dispatcher actions
+    command: /usr/bin/copr-run-dispatcher actions
 
   frontend:
     environment:
       - PYTHONPATH=/opt/copr/frontend/coprs_frontend
-    command: /usr/sbin/runuser -u copr-fe -g copr-fe -- /opt/copr/frontend/coprs_frontend/manage.py runserver -p 5000 -h 0.0.0.0 --without-threads --no-reload
+    command: sh -c 'while true; do /opt/copr/frontend/coprs_frontend/manage.py runserver -p 5000 -h 0.0.0.0 --without-threads --no-reload; done'
 
   distgit:
     environment:
       - PYTHONPATH=/opt/copr/dist-git
-    command: /usr/sbin/runuser -u copr-dist-git -g copr-dist-git -- /opt/copr/dist-git/run/importer_runner.py
+    command: /opt/copr/dist-git/run/importer_runner.py
diff --git a/docker-compose.forgejo.yaml b/docker-compose.forgejo.yaml
new file mode 100644
index 00000000..be543162
--- /dev/null
+++ b/docker-compose.forgejo.yaml
@@ -0,0 +1,12 @@
+version: '3'
+services:
+  forgejo:
+    image: codeberg.org/forgejo/forgejo:1.20.5-0
+    environment:
+      - USER_UID=1000
+      - USER_GID=1000
+    volumes:
+      - ./forgejo:/data
+    ports:
+      - '3000:3000'
+      - '2222:22'
diff --git a/frontend/coprs_frontend/coprs/oidc.py b/frontend/coprs_frontend/coprs/oidc.py
index d965186e..f22c054f 100644
--- a/frontend/coprs_frontend/coprs/oidc.py
+++ b/frontend/coprs_frontend/coprs/oidc.py
@@ -99,6 +99,7 @@ def init_oidc_app(app):
                 access_token_url=app.config.get("OIDC_TOKEN_URL"),
                 authorize_url=app.config.get("OIDC_AUTH_URL"),
                 userinfo_endpoint=app.config.get("OIDC_USERINFO_URL"),
+                jwks_uri=app.config.get("OIDC_JWKS_URL"),
                 client_kwargs=client_kwargs,
             )
     return oidc

Start up Forgejo: podman compose -f docker-compose.yaml -f docker-compose.dev.yaml -f docker-compose.forgejo.yaml up forgejo
Navigate to localhost:3000 and click through the setup UI
Stop Forgejo and enable the OIDC provider by adding ENABLE = true under [oauth] in forgejo/gitea/conf/app.ini
Start up everything: podman compose -f docker-compose.yaml -f docker-compose.dev.yaml -f docker-compose.forgejo.yaml up
Create an OAuth application client and token from the Forgejo site administration UI. Add http://localhost:5000/oidc_auth/ to the redirect URI whitelist.
Edit /etc/copr/copr.conf in copr_frontend_1 to include:

OIDC_LOGIN = True
OIDC_PROVIDER_NAME = "asdf" # e.g "openEuler ID"
OIDC_CLIENT = "<OID client ID from Forgejo>"
OIDC_SECRET = "<OID client secret from Forgejo>"
OIDC_SCOPES = "openid username profile email" # e.g. "openid username profile email"
OIDC_TOKEN_AUTH_METHOD="client_secret_post" # possible: client_secret_post, client_secret_basic, none

# The mixing of hostnames is intentional and required
OIDC_AUTH_URL="http://localhost:3000/login/oauth/authorize"
OIDC_TOKEN_URL="http://forgejo:3000/login/oauth/access_token"
OIDC_USERINFO_URL="http://forgejo:3000/login/oauth/userinfo"
OIDC_JWKS_URL="http://forgejo:3000/login/oauth/keys"

Restart the frontend with podman exec -ti copr_frontend_1 bash and kill 2 3
Try logging in. It should fail with an exception that hints towards a misconfiguration.
Add OIDC_UNIQUE_PREFERRED_USERNAMES = True to copr.conf, restart the frontend and try again. It should work this time 🙂

praiskup · 2023-11-29T08:17:06Z

Testing this is kind of a pain in the neck. This is my attempt at a rough guide to reproduce my testing environment:

I think @FrostyX may comment on the docker-compose scripts if https://docs.pagure.org/copr.copr/contribute.html isn't helpful.

I must say the change is pretty straight-forward and non-intrusive, I like it.

frontend/coprs_frontend/coprs/oidc.py

praiskup · 2023-11-29T08:28:04Z

NB, neither Fedora Copr nor Red Hat Copr (internal) have been using OIDC so far, it would be nice to let @pkking from openEuler comment on this PR

praiskup · 2023-11-29T08:33:20Z

Is @ character part of the preferred_username?

FrostyX

I think @FrostyX may comment on the docker-compose scripts if https://docs.pagure.org/copr.copr/contribute.html isn't helpful.

Not sure if I can be of help here. I have no experience with Forgejo. When it comes to the Copr development, my workflow is kind of described here https://frostyx.cz/posts/copr-docker-compose-without-supervisord#running-services-from-git

frontend/coprs_frontend/config/copr.conf

Bob131 · 2023-12-05T14:18:45Z

Is @ character part of the preferred_username?

I'm not sure what you mean. There were no @s in my testing, but the OIDC spec §5.1 says of preferred_username

This value MAY be any valid JSON string including special characters such as @, /, or whitespace

pkking · 2023-12-05T14:39:55Z

LGTM, since the behavior is opt-in :)

Bob131 · 2023-12-05T17:12:19Z

This should address the suggestions that have been raised. With respect to @praiskup's suggested changes, I have kept username as the default rather than preferred_username because

Defaulting to behaviour that the standard explicitly disallows seems a bit iffy, and
I'm concerned about possibly breaking existing deployments

frontend/coprs_frontend/coprs/oidc.py

Bob131 · 2023-12-14T12:41:40Z

Having a look at the lint warnings, I'm not convinced that addressing them directly would improve the code; however, I'm unsure. Am I okay to just add lint suppression comments to squash those warnings?

FrostyX · 2023-12-14T13:39:21Z

Having a look at the lint warnings, I'm not convinced that addressing them directly would improve the code;

Error: PYLINT_WARNING:
coprs_frontend/coprs/oidc.py:16: R0911[too-many-return-statements]: oidc_enabled: Too many return statements (7/6)

There is a lot of repeated code like this

if not config.get("OIDC_CLIENT"):
    app.logger.error("OIDC_CLIENT is empty")
    return False

We could do something like (in a separate commit):

for key in ["OIDC_LOGIN", "OIDC_CLIENT", "OIDC_SECRET", "OIDC_SCOPES"]:
    if not config.get(key):
        app.logger.error("%s is empty", key)
        return False

but if you'd rather avoid changing the code, feel free to add pylint: disable statement.

Error: PYLINT_WARNING:
coprs_frontend/coprs/oidc.py:67:8: W0707[raise-missing-from]: oidc_username_from_userinfo: Consider explicitly re-raising using 'except KeyError as exc' and 'raise RuntimeError("Can't get unique username, see OIDC_USERNAME_CLAIM configuration docs") from exc'

This is easy to fix, we usually respect this warning.

Bob131 · 2023-12-14T14:39:00Z

There is a lot of repeated code like this

if not config.get("OIDC_CLIENT"):
    app.logger.error("OIDC_CLIENT is empty")
    return False

We could do something like (in a separate commit):

for key in ["OIDC_LOGIN", "OIDC_CLIENT", "OIDC_SECRET", "OIDC_SCOPES"]:
    if not config.get(key):
        app.logger.error("%s is empty", key)
        return False

Ah, that makes sense. I can do that.

Error: PYLINT_WARNING:
coprs_frontend/coprs/oidc.py:67:8: W0707[raise-missing-from]: oidc_username_from_userinfo: Consider explicitly re-raising using 'except KeyError as exc' and 'raise RuntimeError("Can't get unique username, see OIDC_USERNAME_CLAIM configuration docs") from exc'

This is easy to fix, we usually respect this warning.

Ack. Glad I asked 🙂

A small snippet of code checking the existence of a particular key is duplicated several times in oidc_enabled(). This commit is a small clean up to reduce some of the duplicated code.

The current OpenID Connect code assumes the existence of a non-standard field in the UserInfo returned by the provider, limiting the ability to use non-Ipsilon OIDC providers; however, the standard forbids using corresponding standardised field (preferred_username) as a unique user identifier, a property on which Copr relies. This commit allows the administrator to specify which claim from UserInfo should be used as a username; they may, for instance, choose preferred_username if they know that the values from their OIDC provider will be unique. The upshot of this change is that administrators can now use OIDC providers like Gitea/Forgejo. Fixes fedora-copr#3008.

FrostyX

Thank you very much for the changes.

nikromen assigned nikromen and unassigned nikromen Nov 27, 2023

praiskup reviewed Nov 29, 2023

View reviewed changes

frontend/coprs_frontend/coprs/oidc.py Outdated Show resolved Hide resolved

FrostyX reviewed Nov 29, 2023

View reviewed changes

frontend/coprs_frontend/config/copr.conf Outdated Show resolved Hide resolved

Bob131 force-pushed the oidc-unique-preferred-usernames branch from 53c6871 to a9dc527 Compare December 5, 2023 17:09

Bob131 force-pushed the oidc-unique-preferred-usernames branch from a9dc527 to bfdeb76 Compare December 5, 2023 17:19

github-advanced-security bot found potential problems Dec 11, 2023

View reviewed changes

frontend/coprs_frontend/coprs/oidc.py Fixed Show fixed Hide fixed

Bob131 added 2 commits December 15, 2023 01:50

frontend/oidc: clean up duplicate return code

024c668

A small snippet of code checking the existence of a particular key is duplicated several times in oidc_enabled(). This commit is a small clean up to reduce some of the duplicated code.

Bob131 force-pushed the oidc-unique-preferred-usernames branch from bfdeb76 to e609005 Compare December 14, 2023 14:58

FrostyX approved these changes Dec 19, 2023

View reviewed changes

FrostyX merged commit bcf645d into fedora-copr:main Dec 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

frontend/oidc: add OIDC_UNIQUE_PREFERRED_USERNAMES #3023

frontend/oidc: add OIDC_UNIQUE_PREFERRED_USERNAMES #3023

Uh oh!

Bob131 commented Nov 23, 2023

Uh oh!

Bob131 commented Nov 23, 2023

Uh oh!

praiskup commented Nov 29, 2023

Uh oh!

Uh oh!

praiskup commented Nov 29, 2023

Uh oh!

praiskup commented Nov 29, 2023

Uh oh!

FrostyX left a comment

Uh oh!

Uh oh!

Bob131 commented Dec 5, 2023

Uh oh!

pkking commented Dec 5, 2023

Uh oh!

Bob131 commented Dec 5, 2023

Uh oh!

Uh oh!

Bob131 commented Dec 14, 2023

Uh oh!

FrostyX commented Dec 14, 2023

Uh oh!

Bob131 commented Dec 14, 2023

Uh oh!

FrostyX left a comment

Uh oh!

Uh oh!

frontend/oidc: add OIDC_UNIQUE_PREFERRED_USERNAMES #3023

frontend/oidc: add OIDC_UNIQUE_PREFERRED_USERNAMES #3023

Uh oh!

Conversation

Bob131 commented Nov 23, 2023

Uh oh!

Bob131 commented Nov 23, 2023

Uh oh!

praiskup commented Nov 29, 2023

Uh oh!

Uh oh!

praiskup commented Nov 29, 2023

Uh oh!

praiskup commented Nov 29, 2023

Uh oh!

FrostyX left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Bob131 commented Dec 5, 2023

Uh oh!

pkking commented Dec 5, 2023

Uh oh!

Bob131 commented Dec 5, 2023

Uh oh!

Uh oh!

Bob131 commented Dec 14, 2023

Uh oh!

FrostyX commented Dec 14, 2023

Uh oh!

Bob131 commented Dec 14, 2023

Uh oh!

FrostyX left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!