[Security/Extension] JWT Vendor for extensions

[RyanL1997]

Description

Initial commit of JWT token generator for extensions

• Category New feature

Issues Resolved

• Relate: [Question] What capabilities should a token that is passed to an extension have? Should it be a JWT? #2545
• Relate: Generate an auth token for an Extension Request

Check List

• New functionality includes testing
• New functionality has been documented
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[RyanL1997]

While we are having the discussion in this PR, I will create a META issue for Generate an auth token for an Extension Request and add a unit test.

[codecov-commenter]

Codecov Report

Merging #2567 (367752b) into main (a5c73f9) will decrease coverage by 0.07%.
The diff coverage is 63.23%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main    #2567      +/-   ##
============================================
- Coverage     61.29%   61.23%   -0.07%     
  Complexity     3334     3334              
============================================
  Files           260      261       +1     
  Lines         18509    18577      +68     
  Branches       3269     3275       +6     
============================================
+ Hits          11346    11375      +29     
- Misses         5571     5601      +30     
- Partials       1592     1601       +9     

Impacted Files	Coverage Δ
...opensearch/security/filter/SecurityRestFilter.java	77.77% <ø> (ø)
...g/opensearch/security/authtoken/jwt/JwtVendor.java	63.23% <63.23%> (ø)

... and 6 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[peternied]

Thanks for sending this out @RyanL1997

[peternied]

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)

[RyanL1997]

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

Name/Features	Pros	Cons
HMAC (Hash-based Message Authentication Code)	- Performance: HMAC is generally faster than RSA, as it uses symmetric key cryptography, which is computationally less expensive. - Flexibility: HMAC can work with various hash functions, such as SHA-256, SHA-384, or SHA-512, depending on the desired security level and performance.	- Shared secret key: HMAC relies on a shared secret key between the sender and receiver. - Not suitable for non-repudiation, as anyone with the secret key can create a valid signature.
RSA (Rivest-Shamir-Adleman)	- Widely adopted: RSA is a widely-used and well-established public key cryptography algorithm, with extensive support in various libraries and systems. - Public key cryptography: RSA uses a public and private key pair, allowing for easier key management and distribution. The public key can be shared openly, while the private key remains secret.	- Performance: RSA is generally slower than HMAC, as it involves more complex mathematical operations and requires larger key sizes to achieve the same level of security. - Key size: RSA requires larger keys for equivalent security levels compared to symmetric algorithms like HMAC. For example, a 3072-bit RSA key provides roughly the same level of security as a 256-bit symmetric key.

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.

[peternied]

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.

[cwperks]

@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

[peternied]

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.

[stephen-crawford]

@peternied and @cwperks what was the verdict here?

[cwperks]

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.

[peternied]

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.

[RyanL1997]

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.

[peternied]

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

[RyanL1997]

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

[cwperks]

@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

[RyanL1997]

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

[peternied]

(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.

[stephen-crawford]

@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.

[stephen-crawford]

Hi Ryan, thank you for putting this together. Overall, I don't think there was much that seemed like it needed re-visiting. The obvious questions will be around the claims we include in the tokens but I did not leave comments for those. Great job.

[stephen-crawford]

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.

[peternied]

Hey, @RyanL1997, I noticed that this code is generating a JWT, but it doesn't seem to be connected with any user scenarios. Can you provide some insight into what you had in mind for the evolution of this pull request so it would be merged?

To enhance the functionality of this pull request, I recommend that we add the ability to generate a token for an authenticated user and accept that generated token and validate it.

Let me know your thoughts on this.

[RyanL1997]

Hey, @RyanL1997, I noticed that this code is generating a JWT, but it doesn't seem to be connected with any user scenarios. Can you provide some insight into what you had in mind for the evolution of this pull request so it would be merged?

To enhance the functionality of this pull request, I recommend that we add the ability to generate a token for an authenticated user and accept that generated token and validate it.

Let me know your thoughts on this.

Hi @peternied, thanks for the advice! I just had a conversation with @cwperks about the JIT concept related to authentication backend, and I'm looking forward into some details for the authentication backend next week after our meetups. After that, I will post our findings here/a new issue.

[RyanL1997]

Hi, @peternied. I had a conversation with @cwperks today about the authentication backend. For answering your questions:

Can you provide some insight into what you had in mind for the evolution of this pull request so it would be merged

I think the purpose of this PR is to setup a basic structure of this JIT token, such as, some of the components we discussed above for token claims: subject, audience ..etc. I guess the only tricky part for now is about the how are we gonna deal with the roles, encrypted/not-encrypted/include/not-included. For these parts, we can discuss it in another review session with a documentations and modify them later, but I think we can use this basic structure for future development. For now, I just started to write a draft PR for authentication backend, and I do need some of the basic concepts from this PR for token verification.

[cwperks]

Hi @peternied , since this a component of a larger feature (Security for Extensions), should we merge this into a feature branch until the feature is ready to merge to main?

[peternied]

Let's merge changes into the main branch when they benefit security plugin customers. I recommend moving this pull request to a feature/jwt branch, and once it's functional enough for customer use and end-to-end validation, merge it into the main branch.

[stephen-crawford]

Thanks for putting this together Ryan. It looks like all my previous comments were addressed. I am happy to merge this PR and then follow up as needed for velocity.

[stephen-crawford]

@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.

[stephen-crawford]

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.

[RyanL1997]

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

[RyanL1997]

Hi @scrawfor99, here is a link for place holder (#2619). Me and Craig are working on a documentation, and I will transfer that into this issue. Btw I just change the base branch into the feature branch I created. As Peter said before we can go through the entire functionality before we merge into main.