Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: don't crash if none is a supported request signing alg #297

Merged
merged 2 commits into from
Dec 5, 2023

Conversation

paulswartz
Copy link
Collaborator

Instead, catch the error and allow the upstream to fall back to a different approach. For the oidcc_authorization module, that means normal query parameters rather than a signed request object.

In particular, I ran into this when calling create_redirect_url with a client ID and client secret (not :unauthenticated) when none is a supported request signing alg.

This might be eliminating support for none signed request JWTs entirely, but it wasn't clear whether that's required for clients.

Instead, catch the error and allow the upstream to fall back to a
different approach. For the `oidcc_authorization` module, that means
normal query parameters rather than a signed request object.

In particular, I ran into this when calling `create_redirect_url` with a
client ID and client secret (not `:unauthenticated`) when `none` is a
supported request signing alg.

This might be eliminating support for `none` signed request JWTs
entirely, but it wasn't clear whether that's required for clients.
@maennchen
Copy link
Member

@paulswartz Thanks for the PR.

I however believe that we should support none for request_object_signing_alg_values_supported. (Especially if combined with encryption.)

What error did you get exactly?

@paulswartz
Copy link
Collaborator Author

Request: GET /auth/keycloak
** (exit) an exception was raised:
    ** (ErlangError) Erlang error: :not_supported
        (jose 1.11.6) src/jws/jose_jws_alg_none.erl:52: :jose_jws_alg_none.sign/3
        (jose 1.11.6) src/jws/jose_jws.erl:311: :jose_jws.sign/4
        (jose 1.11.6) src/jwt/jose_jwt.erl:173: :jose_jwt.sign/3
        (oidcc 3.1.0) src/oidcc_jwt_util.erl:189: anonymous fn/4 in :oidcc_jwt_util.sign/3
        (stdlib 5.1.1) lists.erl:1599: :lists.foldl_1/3
        (oidcc 3.1.0) src/oidcc_jwt_util.erl:194: :oidcc_jwt_util.sign/3
        (oidcc 3.1.0) src/oidcc_authorization.erl:241: :oidcc_authorization.attempt_request_object/2
        (oidcc 3.1.0) src/oidcc_authorization.erl:83: :oidcc_authorization.create_redirect_url/2
        (ueberauth_oidcc 0.2.0) lib/ueberauth_oidcc.ex:73: UeberauthOidcc.handle_request/2
        (ueberauth_oidcc 0.2.0) lib/ueberauth/strategy/oidcc.ex:17: Ueberauth.Strategy.Oidcc.handle_request!/1

@maennchen
Copy link
Member

maennchen commented Dec 5, 2023

@paulswartz My initial plan was to support none everywhere where the standard does not explicitly forbid it. But I wanted to use the jose config value for the user to decide if he wants to allow this behavior.

Looking at it now (also considering none id tokens only being valid in combination with a userinfo request) I'm not sure if this is a good solution.

We should for sure catch this error.

About allowing none, I see two options:

  1. Rely on jose:unsecured_signing to detect if the user allows it (and document that behavior somewhere)
  2. Take the providers none as a signal that we should allow it and don't care about jose:unsecured_signing

@maennchen
Copy link
Member

(Don't know though how we would do option 2 without overriding the config globally at least while we sign a token, so that's probably not really feasible.)

@maennchen maennchen self-assigned this Dec 5, 2023
@maennchen maennchen added the bug label Dec 5, 2023
@paulswartz
Copy link
Collaborator Author

I agree on not overriding the global configuration; that doesn't seem like a safe option. @maennchen I added back the support, but handling the exception so that we'll still use the fallback path if unsecured_signing isn't enabled.

@maennchen maennchen merged commit e4ceee8 into erlef:main Dec 5, 2023
23 of 25 checks passed
@paulswartz paulswartz deleted the catch-error-signing-none branch December 5, 2023 18:14
@maennchen maennchen added this to the v3.1.1 milestone Dec 7, 2023
paulswartz added a commit to paulswartz/oidcc that referenced this pull request Dec 7, 2023
Previously, it would return only two items: the first non-`none`
algorithm, and `none`. This doesn't appear to be what we want: instead,
we want the full list in their regular order, but with `none` at the
end.

Related to erlef#297, as the root cause of
the crash I was seeing was the the first algorithm in the authorizer's
list wasn't supported by JOSE, so we went immediately to `none`.
maennchen pushed a commit that referenced this pull request Dec 7, 2023
Previously, it would return only two items: the first non-`none`
algorithm, and `none`. This doesn't appear to be what we want: instead,
we want the full list in their regular order, but with `none` at the
end.

Related to #297, as the root cause of
the crash I was seeing was the the first algorithm in the authorizer's
list wasn't supported by JOSE, so we went immediately to `none`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants