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

[Security/Extension] JWT Vendor for extensions #2567

Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
168 changes: 168 additions & 0 deletions src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*
* Modifications Copyright OpenSearch Contributors. See
* GitHub history for details.
*/

package org.opensearch.security.authtoken.jwt;

import java.time.Instant;
import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import java.util.function.LongSupplier;

import com.google.common.base.Strings;
import org.apache.cxf.jaxrs.json.basic.JsonMapObjectReaderWriter;
import org.apache.cxf.rs.security.jose.jwk.JsonWebKey;
import org.apache.cxf.rs.security.jose.jwk.KeyType;
import org.apache.cxf.rs.security.jose.jwk.PublicKeyUse;
import org.apache.cxf.rs.security.jose.jws.JwsUtils;
import org.apache.cxf.rs.security.jose.jwt.JoseJwtProducer;
import org.apache.cxf.rs.security.jose.jwt.JwtClaims;
import org.apache.cxf.rs.security.jose.jwt.JwtToken;
import org.apache.cxf.rs.security.jose.jwt.JwtUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

import org.opensearch.common.settings.Settings;
import org.opensearch.common.transport.TransportAddress;
import org.opensearch.common.util.concurrent.ThreadContext;
import org.opensearch.security.securityconf.ConfigModel;
import org.opensearch.security.support.ConfigConstants;
import org.opensearch.security.user.User;
import org.opensearch.threadpool.ThreadPool;

public class JwtVendor {
private static final Logger logger = LogManager.getLogger(JwtVendor.class);

private static JsonMapObjectReaderWriter jsonMapReaderWriter = new JsonMapObjectReaderWriter();

private JsonWebKey signingKey;
private JoseJwtProducer jwtProducer;
private final LongSupplier timeProvider;

//TODO: Relocate/Remove them at once we make the descisions about the `roles`
private ConfigModel configModel;
private ThreadContext threadContext;
RyanL1997 marked this conversation as resolved.
Show resolved Hide resolved

public JwtVendor(Settings settings) {
JoseJwtProducer jwtProducer = new JoseJwtProducer();
try {
this.signingKey = createJwkFromSettings(settings);
} catch (Exception e) {
throw new RuntimeException(e);
}
this.jwtProducer = jwtProducer;
timeProvider = System::currentTimeMillis;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

//For testing the expiration in the future
public JwtVendor(Settings settings, final LongSupplier timeProvider) {
JoseJwtProducer jwtProducer = new JoseJwtProducer();
try {
this.signingKey = createJwkFromSettings(settings);
} catch (Exception e) {
throw new RuntimeException(e);
}
this.jwtProducer = jwtProducer;
this.timeProvider = timeProvider;
}

static JsonWebKey createJwkFromSettings(Settings settings) throws Exception {
RyanL1997 marked this conversation as resolved.
Show resolved Hide resolved
String signingKey = settings.get("signing_key");

if (!Strings.isNullOrEmpty(signingKey)) {

JsonWebKey jwk = new JsonWebKey();

jwk.setKeyType(KeyType.OCTET);
jwk.setAlgorithm("HS512");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

jwk.setPublicKeyUse(PublicKeyUse.SIGN);
peternied marked this conversation as resolved.
Show resolved Hide resolved
jwk.setProperty("k", signingKey);

return jwk;
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@cwperks cwperks Mar 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peternied and @cwperks what was the verdict here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Settings jwkSettings = settings.getAsSettings("jwt").getAsSettings("key");

if (jwkSettings.isEmpty()) {
throw new Exception(
"Settings for key is missing. Please specify at least the option signing_key with a shared secret.");
}

JsonWebKey jwk = new JsonWebKey();

for (String key : jwkSettings.keySet()) {
jwk.setProperty(key, jwkSettings.get(key));
}

return jwk;
}
}

//TODO:Getting roles from User
public Map<String, String> prepareClaimsForUser(User user, ThreadPool threadPool) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@RyanL1997 RyanL1997 Mar 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@cwperks cwperks Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Map<String, String> claims = new HashMap<>();
this.threadContext = threadPool.getThreadContext();
final TransportAddress caller = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Set<String> mappedRoles = mapRoles(user, caller);
RyanL1997 marked this conversation as resolved.
Show resolved Hide resolved
claims.put("sub", user.getName());
claims.put("roles", String.join(",", mappedRoles));
return claims;
}

public Set<String> mapRoles(final User user, final TransportAddress caller) {
return this.configModel.mapSecurityRoles(user, caller);
}

public String createJwt(String issuer, String subject, String audience, Integer expirySeconds) throws Exception {
long timeMillis = timeProvider.getAsLong();
Instant now = Instant.ofEpochMilli(timeProvider.getAsLong());

jwtProducer.setSignatureProvider(JwsUtils.getSignatureProvider(signingKey));
JwtClaims jwtClaims = new JwtClaims();
RyanL1997 marked this conversation as resolved.
Show resolved Hide resolved
JwtToken jwt = new JwtToken(jwtClaims);

jwtClaims.setIssuer(issuer);

jwtClaims.setIssuedAt(timeMillis);

jwtClaims.setSubject(subject);

jwtClaims.setAudience(audience);

jwtClaims.setNotBefore(timeMillis);

if (expirySeconds == null) {
long expiryTime = timeProvider.getAsLong() + (300 * 1000);
jwtClaims.setExpiryTime(expiryTime);
} else if (expirySeconds > 0) {
long expiryTime = timeProvider.getAsLong() + (expirySeconds * 1000);
jwtClaims.setExpiryTime(expiryTime);
} else {
throw new Exception("The expiration time should be a positive integer");
}

// TODO: Should call preparelaims() if we need roles in claim;

String encodedJwt = jwtProducer.processJwt(jwt);

if (logger.isDebugEnabled()) {
logger.debug(
"Created JWT: "
+ encodedJwt
+ "\n"
+ jsonMapReaderWriter.toJson(jwt.getJwsHeaders())
+ "\n"
+ JwtUtils.claimsToJson(jwt.getClaims())
);
}

return encodedJwt;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c
if (!checkAndAuthenticateRequest(request, channel, client)) {
User user = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER);
if (userIsSuperAdmin(user, adminDNs) || (whitelistingSettings.checkRequestIsAllowed(request, channel, client) && allowlistingSettings.checkRequestIsAllowed(request, channel, client))) {
//TODO: If the request is going to the extension, issue a JWT for authenticated user.
original.handleRequest(request, channel, client);
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*
* Modifications Copyright OpenSearch Contributors. See
* GitHub history for details.
*/

package org.opensearch.security.authtoken.jwt;

import java.util.function.LongSupplier;

import org.apache.cxf.rs.security.jose.jwk.JsonWebKey;
import org.apache.cxf.rs.security.jose.jws.JwsJwtCompactConsumer;
import org.apache.cxf.rs.security.jose.jwt.JwtToken;
import org.junit.Assert;
import org.junit.Test;

import org.opensearch.common.settings.Settings;

public class JwtVendorTest {

@Test
public void testCreateJwkFromSettings() throws Exception {
Settings settings = Settings.builder()
.put("signing_key", "abc123").build();

JsonWebKey jwk = JwtVendor.createJwkFromSettings(settings);
Assert.assertEquals("HS512", jwk.getAlgorithm());
Assert.assertEquals("sig", jwk.getPublicKeyUse().toString());
Assert.assertEquals("abc123", jwk.getProperty("k"));
}

@Test (expected = Exception.class)
public void testCreateJwkFromSettingsWithoutSigningKey() throws Exception{
Settings settings = Settings.builder()
.put("jwt", "").build();
JwtVendor.createJwkFromSettings(settings);
}

@Test
public void testCreateJwt() throws Exception {
String issuer = "cluster_0";
String subject = "admin";
String audience = "extension_0";
Integer expirySeconds = 300;
LongSupplier currentTime = () -> (int)100;
Settings settings = Settings.builder().put("signing_key", "abc123").build();
Long expectedExp = currentTime.getAsLong() + (expirySeconds * 1000);

JwtVendor jwtVendor = new JwtVendor(settings, currentTime);
String encodedJwt = jwtVendor.createJwt(issuer, subject, audience, expirySeconds);

JwsJwtCompactConsumer jwtConsumer = new JwsJwtCompactConsumer(encodedJwt);
JwtToken jwt = jwtConsumer.getJwtToken();

Assert.assertEquals("cluster_0", jwt.getClaim("iss"));
Assert.assertEquals("admin", jwt.getClaim("sub"));
Assert.assertEquals("extension_0", jwt.getClaim("aud"));
Assert.assertNotNull(jwt.getClaim("iat"));
Assert.assertNotNull(jwt.getClaim("exp"));
Assert.assertEquals(expectedExp, jwt.getClaim("exp"));
}

@Test (expected = Exception.class)
public void testCreateJwtWithBadExpiry() throws Exception {
String issuer = "cluster_0";
String subject = "admin";
String audience = "extension_0";
Integer expirySeconds = -300;

Settings settings = Settings.builder().put("signing_key", "abc123").build();
JwtVendor jwtVendor = new JwtVendor(settings);

jwtVendor.createJwt(issuer, subject, audience, expirySeconds);
}
}
RyanL1997 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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