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

JWT: support both certs and raw public keys #6601

Open
6 tasks done
maxtropets opened this issue Oct 29, 2024 · 11 comments · May be fixed by #6680
Open
6 tasks done

JWT: support both certs and raw public keys #6601

maxtropets opened this issue Oct 29, 2024 · 11 comments · May be fixed by #6680
Assignees

Comments

@maxtropets
Copy link
Collaborator

maxtropets commented Oct 29, 2024

For instance, FB openid conf:
https://www.facebook.com/.well-known/oauth/openid/jwks/

Format:

        {
            "kid": "dcd214c095176e06454dfe832254b0cde52b6052",
            "kty": "RSA",
            "alg": "RS256",
            "use": "sig",
            "n": "6GkDCgRsMiCb2Zsjge86oXAFwvRIOTEAlLCtxtFzsb2x6alKYa1ycSEmiwj_hlGXUQnbrj8VEWkT9ZigNC-WAGxhCe-RikzkmUG99_xACWzoI9zUN50Qj6jZM8-P-pmoYEnKK_7yj2gJSlzMtWYBRzJihz5-zN3Ed75GVQOuANytYbAclPmhm2-g-gfH9g6JRqYsBY6k-MMP0d5VLk8u_nAg6jf0Kw4Ii-PndlNHsyG6aXHXteyFsZ7bBOjb_nUs9C0xgiJPVqMoOtMRhelDLuj4W4N7CQVxoCEvkW6g0932eCOzteOvbawXutx18kF2bGrSgFvIOLCbnzL8dgf7zw",
            "e": "AQAB"
        },

CCF now only supports x5c field though. It has to support both x5c and n + e combination.

  • Construct public key from n + e fields
  • Save raw public key first OR x5c in jwt_management.h
  • Raw key verification impl (currently OpenSSL_Verifier only supports certs)
  • Use public key first in jwt_auth.cpp, fallback to cert if needed (used public keys in the KV only, extracting ones from the certs on a fly when setting
  • Test-cover public key verifier (unit tests) - tested in e2e (auth)
  • Test-cover third-party key provider (e2e) - server refresh with public keys
@maxtropets maxtropets self-assigned this Oct 29, 2024
@maxtropets
Copy link
Collaborator Author

@achamayou, one design question so far, shall we

  • create a self-endorsed certificate and keep it in the existing schema
  • OR extend the table to contain pubkeys too
  • OR replace all certs with pubkeys?

Self-endorsed cert looks, as we may set expiry date explicitly, however, it may look a little weird. On the other hand, it's a smaller change, no schema changes involved.

@maxtropets
Copy link
Collaborator Author

maxtropets commented Nov 4, 2024

Talked on-site. Decided to move on with extending

  struct OpenIDJWKMetadata
  {
    Cert cert;
    JwtIssuer issuer;
    std::optional<JwtIssuer> constraint;
    // new key: raw public_key
  };

This avoids breaking old entries format and simplifies transition to the new code version.

@maxtropets
Copy link
Collaborator Author

maxtropets commented Nov 15, 2024

Wondering which key types to support in here. Checked Google's and Facebook's well-knows configs, both use RSA256 keys.

@PallabPaul do you have a list of providers you intend to support? At least those which you think may be a must-have, just to double-check the key types.

@PallabPaul
Copy link
Member

So the overall goal would be to support the same IdP's that other Azure services like Azure Function supports listed here and also allow customers to add their custom IdP as long as they follow the OIDC protocol:

Image

From this list it looks like Apple also follows RSA256 but couldn't find the well-known endpoint for Github or Twitter. IMO, RSA256 keys should suffice and open up many common IdP's that we can support.

We have also started exploring alternatives such as inviting external IdP's to Microsoft Entra Id which would allow the external user to be part of AAD and have a JWT token provided by Microsoft which includes the x5c field but it seems like this is only limited to Google, Facebook and IdP's that follow a SAML/ WS-Fed based authentication protocol.

@maxtropets
Copy link
Collaborator Author

maxtropets commented Nov 15, 2024

supports listed here

That's a useful one, thank you!

@maxtropets
Copy link
Collaborator Author

maxtropets commented Dec 2, 2024

When testing against the existing implementation, I found out we're unable to verify signature produced by python jwt package.

def create_jwt(body_claims: dict, key_priv_pem: str, key_id: str) -> str:
    return jwt.encode(
        body_claims, key_priv_pem, algorithm="RS256", headers={"kid": key_id}
    )

Recently the RSA sign/verify has been altered (#6405) to use a more up-to-date default padding, so verification code now looks like this

    CHECK1(EVP_PKEY_verify_init(pctx));
    CHECK1(EVP_PKEY_CTX_set_rsa_padding(pctx, RSA_PKCS1_PSS_PADDING));
    CHECK1(EVP_PKEY_CTX_set_rsa_pss_saltlen(pctx, RSA_PSS_SALTLEN_AUTO));
    CHECK1(EVP_PKEY_CTX_set_signature_md(pctx, get_md_type(md_type)));
    return EVP_PKEY_verify(
             pctx, signature, signature_size, hash.data(), hash.size()) == 1;

However, the tokens signed by the current python jwt library seem to use different padding, because changing RSA_PKCS1_PSS_PADDING to RSA_PKCS1_PADDING helps resolve the issue.

The OpenID spec says it's recommended to use RSASSA-PKCS1-v1_5, but doesn't enforce it.

I plan to proceed in the following way

  • check what padding existing providers use
  • support all needed paddings, probably get it from the metadata if possible

@maxtropets
Copy link
Collaborator Author

Noticed that cert verification worked, but raw keys didn't, wondered why.

Revealed that we always used EC-specific code (PublicKey_OpenSSL::verify() -> PublicKey_OpenSSL::verify_hash() for RSA keys in JWT authentication.

It worked because of the implementation similarity, and because we specified md as an argument to the function.

Created #6673 to tidy-up the interfaces, meanwhile will try to sort out the paddings supported.

@maxtropets
Copy link
Collaborator Author

Carefully read the RFC, so the important quote from section 8 is:

Of the signature and MAC algorithms specified in JSON Web Algorithms
[JWA], only HMAC SHA-256 ("HS256") and "none" MUST be implemented by
conforming JWT implementations. It is RECOMMENDED that
implementations also support RSASSA-PKCS1-v1_5 with the SHA-256 hash
algorithm ("RS256") and ECDSA using the P-256 curve and the SHA-256
hash algorithm ("ES256"). Support for other algorithms and key sizes
is OPTIONAL.

I checked the .well-known Entra config:

    "id_token_signing_alg_values_supported": [
        "RS256"
    ],

As per JWA spec:

          +-------------------+---------------------------------+
          | "alg" Param Value | Digital Signature Algorithm     |
          +-------------------+---------------------------------+
          | RS256             | RSASSA-PKCS1-v1_5 using SHA-256 |
          | RS384             | RSASSA-PKCS1-v1_5 using SHA-384 |
          | RS512             | RSASSA-PKCS1-v1_5 using SHA-512 |
          +-------------------+---------------------------------+

Also checked Facebook's and Google's endpoints, all use RS256

@achamayou
Copy link
Member

@maxtropets we have to continue to support it at least on the validation path then, which also means that test code must be able to emit it unfortunately.

@maxtropets
Copy link
Collaborator Author

maxtropets commented Dec 5, 2024

Tried FB token authentication, noticed a problem with nbf tokens.

We now require it as mandatory, Entra seems to always provide it: https://learn.microsoft.com/en-us/entra/identity-platform/access-token-claims-reference. However, other providers don't seem to.

Suggested solution: I'll make this claim optional to allow working with 3rd-party providers.

@maxtropets
Copy link
Collaborator Author

https://www.rfc-editor.org/rfc/rfc7518.txt

   The table below is the set of "alg" (algorithm) Header Parameter
   values defined by this specification for use with JWS, each of which
   is explained in more detail in the following sections:

   +--------------+-------------------------------+--------------------+
   | "alg" Param  | Digital Signature or MAC      | Implementation     |
   | Value        | Algorithm                     | Requirements       |
   +--------------+-------------------------------+--------------------+
   | HS256        | HMAC using SHA-256            | Required           |
   | HS384        | HMAC using SHA-384            | Optional           |
   | HS512        | HMAC using SHA-512            | Optional           |
   | RS256        | RSASSA-PKCS1-v1_5 using       | Recommended        |
   |              | SHA-256                       |                    |
   | RS384        | RSASSA-PKCS1-v1_5 using       | Optional           |
   |              | SHA-384                       |                    |
   | RS512        | RSASSA-PKCS1-v1_5 using       | Optional           |
   |              | SHA-512                       |                    |
   | ES256        | ECDSA using P-256 and SHA-256 | Recommended+       |
   | ES384        | ECDSA using P-384 and SHA-384 | Optional           |
   | ES512        | ECDSA using P-521 and SHA-512 | Optional           |
   | PS256        | RSASSA-PSS using SHA-256 and  | Optional           |
   |              | MGF1 with SHA-256             |                    |
   | PS384        | RSASSA-PSS using SHA-384 and  | Optional           |
   |              | MGF1 with SHA-384             |                    |
   | PS512        | RSASSA-PSS using SHA-512 and  | Optional           |
   |              | MGF1 with SHA-512             |                    |
   | none         | No digital signature or MAC   | Optional           |
   |              | performed                     |                    |
   +--------------+-------------------------------+--------------------+

   The use of "+" in the Implementation Requirements column indicates
   that the requirement strength is likely to be increased in a future
   version of the specification.

   See Appendix A.1 for a table cross-referencing the JWS digital
   signature and MAC "alg" (algorithm) values defined in this
   specification with the equivalent identifiers used by other standards
   and software packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants