Uploaded image for project: 'Keycloak'
  1. Keycloak
  2. KEYCLOAK-13933 Client Policies
  3. KEYCLOAK-17929

SecureSigningAlgorithmForSignedJwtEnforceExecutor polishing for FAPI

    XMLWordPrintable

    Details

    • Type: Sub-task
    • Status: Resolved (View Workflow)
    • Priority: Major
    • Resolution: Done
    • Affects Version/s: None
    • Fix Version/s: 13.0.0
    • Component/s: None
    • Labels:
    • Docs QE Status:
      NEW
    • QE Status:
      NEW

      Description

      Regarding `SecureSigningAlgorithmForSignedJwtEnforceExecutor`, it contains these potential issues, which I think will be good to address for the FAPI:

      • It will fail if "client_assertion" parameter is not provided. However FAPI allows also client authentication with x509 client authenticator, which does not use "client_assertion" parameter. Hence all clients using x509 will currently fail this executor even if client is FAPI compliant. I wonder that we can possibly add option "Require Client Assertion". If false, the executor will just allow request to continue in case that "client_assertion" parameter is not present . I think that this does not contradicts FAPI as it means that request is authenticated with x509 client authenticator then (they are supposed to be other executors to check client authentication method)
      • Just for the sake of simplicity (and maybe slightly better security protection), it will be probably good to remove this check from method `SecureSigningAlgorithmForSignedJwtEnforceExecutor.VerifySecureSigningAlgorithm`
                if (signatureAlgorithm == null) {
                    logger.trace("Signing algorithm not specified explicitly.");
                    return;
                }
        

        The reason is that "alg" claim must be present in JWS according to the specification https://tools.ietf.org/html/rfc7515#section-4.1.1 . For the case it is not present, I guess there may be even earlier the NullPointerException on this line

        String alg = jws.getHeader().getAlgorithm().name();
        

        Attachments

          Issue Links

            Activity

              People

              Assignee:
              Unassigned Unassigned
              Reporter:
              mposolda@redhat.com Marek Posolda
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: