erlef / oidcc

OpenId Connect client library in Erlang & Elixir
https://hexdocs.pm/oidcc
Apache License 2.0
184 stars 49 forks source link

Default algorithms for `client_secret_jwt` auth #301

Closed maennchen closed 6 months ago

maennchen commented 11 months ago

@maennchen I did have one question about this. My read of the spec was that RS256 should be allowed but not HS256, but there's a specific test case which relies on HS256 being allowed. This was introduced to fix https://github.com/erlef/oidcc/issues/287 but I don't totally understand the original issue.

@paulswartz Looking into it I'm not sure if this is correct the other way around.

The idea of client_secret_jwt is to infer the signature from the client_secret. This therefore operates on a shared secret & a MAC. With RS256 we need a public & private key.

Therefore I'm questioning if RS256 should be here and not HS256.

What part of the specification are you referring to?

_Originally posted by @paulswartz in https://github.com/erlef/oidcc/pull/300#discussion_r1421225761_

paulswartz commented 11 months ago

Some spec references:

OAuth 2.0 Authorization Server Metadata

https://datatracker.ietf.org/doc/html/rfc8414

token_endpoint_auth_signing_alg_values_supported [...] No default algorithms are implied if this entry is omitted. Servers SHOULD support "RS256". The value "none" MUST NOT be used.

JWT Profile

https://datatracker.ietf.org/doc/html/rfc7523#section-5

The "RS256" algorithm, from [JWA], is a mandatory-to-implement JSON Web Signature algorithm for this profile.

paulswartz commented 11 months ago

Given that, I think there shouldn't be a default for client_secret_jwt or private_key_jwt. The SHOULD is applying to whether there's implementation support, not that it's a default value if the configuration is undefined.

maennchen commented 11 months ago

I think there shouldn't be a default for client_secret_jwt or private_key_jwt.

@paulswartz I tend to agree. I think I remember some certification issue though without a default.

I’ll rerun the certification profiles without the default to be sure. (Or you can as well if you want.)

If we can pass everything without a default, we should assume no supported algorithms and fall back to the next auth method.

paulswartz commented 11 months ago

I added a patch for that to #300. I should have some time in the next few days to check it against the profiles if you don't get to it first.

paulswartz commented 11 months ago

Looking at the conformance suite, it's hard to tell what's expected here. The client_secret_jwt tests don't include any HS* algorithms in token_endpoint_auth_signing_alg_values_supported:

  "token_endpoint_auth_methods_supported": [
    "client_secret_jwt"
  ],
  "token_endpoint_auth_signing_alg_values_supported": [
    "RS256",
    "RS384",
    "RS512",
    "PS256",
    "PS384",
    "PS512",
    "ES256",
    "ES256K",
    "ES384",
    "ES512",
    "EdDSA"
  ],
maennchen commented 11 months ago

@paulswartz Are we able to sign the token using the algoithms provided? On a first glance, they all look like they are asymmetric…

I’ll have a look myself in the next few days when I have some time available.

paulswartz commented 11 months ago

I'm not (in the conformance suite) and it doesn't look like that was a case covered in the 3.0.0 certification. I am with an actual OP (Keycloak).

maennchen commented 11 months ago

@paulswartz I see the reason why it was in:

Certification Setup

image

Test Command:

mix run_certification  \
  --profile test \
  --alias test \
  --version v3.2.0 \
  --auto-open \
  --auto-screenshot \
  --token-endpoint-auth-method client_secret_jwt

Test Name: oidcc-client-test

Logged Metadata:

19:18:39.334 [info] Loaded Provider Configuration: %Oidcc.ProviderConfiguration{
  issuer: "https://www.certification.openid.net/test/a/test/",
  authorization_endpoint: "https://www.certification.openid.net/test/a/test/authorize",
  token_endpoint: "https://www.certification.openid.net/test/a/test/token",
  userinfo_endpoint: "https://www.certification.openid.net/test/a/test/userinfo",
  jwks_uri: "https://www.certification.openid.net/test/a/test/jwks",
  registration_endpoint: "https://www.certification.openid.net/test/a/test/register",
  scopes_supported: ["openid", "phone", "profile", "email", "address",
   "offline_access"],
  response_types_supported: ["code", "id_token code", "token code id_token",
   "id_token", "token id_token", "token code", "token"],
  response_modes_supported: ["query", "fragment", "form_post"],
  grant_types_supported: ["authorization_code", "implicit"],
  acr_values_supported: ["PASSWORD"],
  subject_types_supported: [:public, :pairwise],
  id_token_signing_alg_values_supported: ["none", "RS256", "RS384", "RS512",
   "PS256", "PS384", "PS512", "ES256", "ES256K", "ES384", "ES512", "EdDSA"],
  id_token_encryption_alg_values_supported: ["RSA1_5", "RSA-OAEP",
   "RSA-OAEP-256", "RSA-OAEP-384", "RSA-OAEP-512", "ECDH-ES", "ECDH-ES+A128KW",
   "ECDH-ES+A192KW", "ECDH-ES+A256KW", "A128KW", "A192KW", "A256KW",
   "A128GCMKW", "A192GCMKW", "A256GCMKW", "dir"],
  id_token_encryption_enc_values_supported: ["A128CBC-HS256", "A192CBC-HS384",
   "A256CBC-HS512", "A128GCM", "A192GCM", "A256GCM"],
  userinfo_signing_alg_values_supported: ["RS256", "RS384", "RS512", "PS256",
   "PS384", "PS512", "ES256", "ES256K", "ES384", "ES512", "EdDSA"],
  userinfo_encryption_alg_values_supported: ["RSA1_5", "RSA-OAEP",
   "RSA-OAEP-256", "RSA-OAEP-384", "RSA-OAEP-512", "ECDH-ES", "ECDH-ES+A128KW",
   "ECDH-ES+A192KW", "ECDH-ES+A256KW", "A128KW", "A192KW", "A256KW",
   "A128GCMKW", "A192GCMKW", "A256GCMKW", "dir"],
  userinfo_encryption_enc_values_supported: ["A128CBC-HS256", "A192CBC-HS384",
   "A256CBC-HS512", "A128GCM", "A192GCM", "A256GCM"],
  request_object_signing_alg_values_supported: ["none", "RS256", "RS384",
   "RS512", "PS256", "PS384", "PS512", "ES256", "ES256K", "ES384", "ES512",
   "EdDSA"],
  request_object_encryption_alg_values_supported: ["RSA1_5", "RSA-OAEP",
   "RSA-OAEP-256", "RSA-OAEP-384", "RSA-OAEP-512", "ECDH-ES", "ECDH-ES+A128KW",
   "ECDH-ES+A192KW", "ECDH-ES+A256KW", "A128KW", "A192KW", "A256KW",
   "A128GCMKW", "A192GCMKW", "A256GCMKW", "dir"],
  request_object_encryption_enc_values_supported: ["A128CBC-HS256",
   "A192CBC-HS384", "A256CBC-HS512", "A128GCM", "A192GCM", "A256GCM"],
  token_endpoint_auth_methods_supported: ["client_secret_jwt"],
  token_endpoint_auth_signing_alg_values_supported: ["RS256", "RS384", "RS512",
   "PS256", "PS384", "PS512", "ES256", "ES256K", "ES384", "ES512", "EdDSA"],
  display_values_supported: :undefined,
  claim_types_supported: [:normal, :aggregated, :distributed],
  claims_supported: ["sub", "name", "given_name", "family_name", "middle_name",
   "nickname", "preferred_username", "gender", "birthdate", "address",
   "zoneinfo", "locale", "phone_number", "phone_number_verified", "email",
   "email_verified", "website", "profile", "updated_at", "txn"],
  service_documentation: :undefined,
  claims_locales_supported: :undefined,
  ui_locales_supported: :undefined,
  claims_parameter_supported: true,
  request_parameter_supported: false,
  request_uri_parameter_supported: true,
  require_request_uri_registration: false,
  op_policy_uri: :undefined,
  op_tos_uri: :undefined,
  revocation_endpoint: :undefined,
  revocation_endpoint_auth_methods_supported: ["client_secret_basic"],
  revocation_endpoint_auth_signing_alg_values_supported: :undefined,
  introspection_endpoint: :undefined,
  introspection_endpoint_auth_methods_supported: ["client_secret_basic"],
  introspection_endpoint_auth_signing_alg_values_supported: :undefined,
  code_challenge_methods_supported: :undefined,
  end_session_endpoint: :undefined,
  extra_fields: %{}
}

The test passes with the latest published oidcc release but fails with the current main.

However: It seems like this is a bug in the certification tooling. If I specify client_secret_jwt_alg HS256 in the config, I would expect that to show up in request_object_signing_alg_values_supported.

Since you were already in contact with the certification team: Would you mind to open an issue to see if this is intentional?

paulswartz commented 11 months ago

https://gitlab.com/openid/conformance-suite/-/issues/1267