I don't have a good alternative since we need to have a fixed reference point and this is the most common way to get time relative to the Epoch. However, it is worth noting that this is not monotonic so you could get subsequent calls with the same value returned even if the time difference is more than 1 ms. It should not matter if we have some clock skew tolerance but just worth mentioning.

I think it would be worthwhile to document why we picked some of the defaults in the code next to them, HMAC was picked because ...

(BTW think HMAC is a good choice for our scenario)

Here is a simple T-chart of HMAC vs RSA based on the research I did:

Another algorithm, ECDSA (Elliptic Curve Digital Signature Algorithm), is pretty similar to RSA, but a little bit faster (the performance ranking from most efficiency to less efficiency should be: HMAC > ECDSA > RSA). However, back to our case, I agree that HMAC is good for our scenario, due to its performance and simplicity. More importantly, it meets our requirement of supporting both data integrity and authenticity, so that the payload of our token cannot be tampered.

Remove this else {...} section. There should be a single code path for the way these tokens are generated, and this is a case where customization shouldn't be allowed.

Note; if we have a user-facing usage of token generated, we could allow other options / customizations, but we should expose it via a seperate API.

@peternied This else clause is what makes this flexible to support creating JWKs with any properties that are not the default HMAC SHA512 symmetric signing key.

This would allow user's to configure asymmetric encryption for the JWT signing if desired.

i.e.

config:
    signing_key: base64.encode(<secret here>)

vs

config:
    key:
        kid: Gps4Ea8bRzBNXMrzE8ciJZKNrlTKPP2MPEBPDSUXPpo
        kty: RSA
        alg: RS256
        use: sig
        n: pGGGyC01Krfq4kR6ebiFm8L3OLdAIL7KCA4gw9iVCdo-12aAftxwTIfv59bhlktOlOhsTQ883wDn4XnquMUBW5DffZUXyf81wLP6aWR-iySANF7_bEnu-HFyl40X8QmpJImXADHjDL3D4C5ckhRqUnIqET3eQ6TWcWGnoEG6wpmE5UlZinB7koAFcLnucPcHBvLLvpMDKxN6GW6jjwn5PKQqfim5TF_xQCXlACfe-dd5x2ZVSzKmErfim-ZhLDr4D83kKSJjch7iROhs7sbh6bj_6OvIeiTDUHDN7dMZZJr-LCXyvRpJZVEZXXRlxgj9WV6UEq7UbKwmkc5653RBRw
        e: AQAB
        x5c:
            [
                "MIICmzCCAYMCBgGHCWjKXDANBgkqhkiG9w0BAQsFADARMQ8wDQYDVQQDDAZtYXN0ZXIwHhcNMjMwMzIyMTI1OTM1WhcNMzMwMzIyMTMwMTE1WjARMQ8wDQYDVQQDDAZtYXN0ZXIwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCkYYbILTUqt+riRHp5uIWbwvc4t0AgvsoIDiDD2JUJ2j7XZoB+3HBMh+/n1uGWS06U6GxNDzzfAOfheeq4xQFbkN99lRfJ/zXAs/ppZH6LJIA0Xv9sSe74cXKXjRfxCakkiZcAMeMMvcPgLlySFGpScioRPd5DpNZxYaegQbrCmYTlSVmKcHuSgAVwue5w9wcG8su+kwMrE3oZbqOPCfk8pCp+KblMX/FAJeUAJ97513nHZlVLMqYSt+Kb5mEsOvgPzeQpImNyHuJE6GzuxuHpuP/o68h6JMNQcM3t0xlkmv4sJfK9GkllURlddGXGCP1ZXpQSrtRsrCaRznrndEFHAgMBAAEwDQYJKoZIhvcNAQELBQADggEBAFuycpje7FnIGBwurU/RwylGO1yx5NX/7LORv1q1fzaQoz4ti3BZTSwTM/K2NsWv3xJAYNm5sqL5CwcJ9PlSOVWRpUt0ce/zBQmAylYUWnfyym7p+JXV9317eT3BeKV04LfGTvVPSmFQRigOuyrihOQ7AQg8zFRJUWvfGFrt3Jl8XRQ0qZAFLCoi9177onEVtdCXSiyIdjIEFE8GTyeRyqm0ed7l8HyLRWIjXCud17qGxZyaL1VqiCHfJGgJBES2LqCau8vKqnN8sLO+gnj/jE8QsJQ6mF+kWKK1/JMUwWscrnsB+rxktttzqo/WfKQiB1CjmoGkYIBlMljLpdaUdQo="
            ]
        x5t: 3nw2MTcS2gwFcGdnlGaB0RPOYG8
        x5t#S256: 2VUpjQgUQW1TyPGP6PSt0wLUDkTINRqmJxLBr2F-Ps0

I recommend we start exposing as little surface area as possible around these new features.

As this is part of flow where generating the sensitive tokens e.g. on-behalf-of user tokens for extensions. I would advocate that we minimize the risk of misconfiguration by locking down these values in code.

@peternied and @cwperks what was the verdict here?

For the experimental release we are aiming for sensible defaults. In this case HMAC SHA512 is chosen as the sensible defaults. Customizability will be added, but less of a priority then making sure all key functionality works first.

I'm not sure we've fully locked down the design - but I am in the camp that we should not include any mapped roles/backend roles onto this claims. @RyanL1997 Could you find out where we are making this decision and reference it?

If we are not including any AuthZ information we should make sure we remove it from here.

Yes, we can continue our discussion over here: #2545. For this one, I was just write up the function we may need for implementing it. 100% we can change/remove it anytime. And also, this function should be locate in a separate class if we choose to go down this path.

We've got another issue about user information parity, that might cover it, but I don't think it clearly calls out that we don't include this information inside the authentication token. Or is there another issue where we have this called out, if not want to make one so we can track?

• [Question] How to support extensions parity for user information #2536

Sure, thanks for the information! I will create separate issue attach to this for the tracking.

@peternied @RyanL1997 I'm curious to get your thoughts on an idea of encrypting the value of the claim itself for sensitive claims. For cases with an external IdP, I think we will need to store the roles in a claim of the token because they cannot be looked up when the security plugin receives the token as part of a request. Since they cannot be looked up, they will need to be part of the claims of the token. The current JWT backend (and OIDC and SAML since they also use JWTs) already assumes that roles are included as a claim of the token.

Encryption of the sensitive claim can be done using a utility like (Reference SO post: https://stackoverflow.com/a/57902503):

import java.util.Arrays;
import java.util.Base64;

import javax.crypto.Cipher;
import javax.crypto.SecretKey;
import javax.crypto.spec.SecretKeySpec;

public class EncryptionDecryptionUtil {

    public static String encrypt(final String secret, final String data) {


        byte[] decodedKey = Base64.getDecoder().decode(secret);

        try {
            Cipher cipher = Cipher.getInstance("AES");
            // rebuild key using SecretKeySpec
            SecretKey originalKey = new SecretKeySpec(Arrays.copyOf(decodedKey, 16), "AES");
            cipher.init(Cipher.ENCRYPT_MODE, originalKey);
            byte[] cipherText = cipher.doFinal(data.getBytes("UTF-8"));
            return Base64.getEncoder().encodeToString(cipherText);
        } catch (Exception e) {
            throw new RuntimeException(
                    "Error occured while encrypting data", e);
        }

    }

    public static String decrypt(final String secret,
            final String encryptedString) {


        byte[] decodedKey = Base64.getDecoder().decode(secret);

        try {
            Cipher cipher = Cipher.getInstance("AES");
            // rebuild key using SecretKeySpec
            SecretKey originalKey = new SecretKeySpec(Arrays.copyOf(decodedKey, 16), "AES");
            cipher.init(Cipher.DECRYPT_MODE, originalKey);
            byte[] cipherText = cipher.doFinal(Base64.getDecoder().decode(encryptedString));
            return new String(cipherText);
        } catch (Exception e) {
            throw new RuntimeException(
                    "Error occured while decrypting data", e);
        }
    }
}

The secret that this function takes is different than the signing_key that this PR introduces and would be another setting on the same level. I wrote a test with this behavior here: RyanL1997/security@jwt-generator-for-extensions...cwperks:security:jwt-generator-for-extensions

Here's the output of the test. The first line is the encrypted claim that the extension would see and below is decrypted that the security plugin would be able to decrypt from the claim inside the token.

org.opensearch.security.authtoken.jwt.JwtVendorTest > testCreateJwtWithRoles STANDARD_OUT
    rolesClaim: U5CjroB/LS95E5nrKl+WMw==
    roles: IT,HR

Hi @cwperks, thanks for putting this together. This is making sense to me.

(Assuming we are keeping AuthZ info in the token, otherwise this should be deleted) We shouldn't be attributing any roles based on the remote address because the request will be performed from that address but via the extension - which is elsewhere. We might want to break this out into a separate issue to discuss.

@RyanL1997, did this get split into the separate issue? It is the last area where I have a concern around merging. As long as there is an issue, I am fine with following-up in this case but could you share the link please? Thank you.

We may want to add further negative test cases down the road. Since you are only looking at the vending this seems fine for now, but once we have verification as well, we will want to test expiration compliance and signature mismatches.

100% We need an authc backend to do the verification, so that we can test them. I''m working on that.