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.

Sign up for GitHub

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

Allow using GenericSecret for HmacSHA* #935

Merged
merged 2 commits into from
Apr 22, 2024
Merged

Allow using GenericSecret for HmacSHA* #935

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eb703c3
Allow using GenericSecret for HmacSHA*
mnylen Apr 16, 2024
1fa8ccb
Review fixes
mnylen Apr 22, 2024
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
Next Next commit
Allow using GenericSecret for HmacSHA* 
Extend the pre-existing check for SUN PKCS11 generic secret to allow all
SecretKeys where getAlgorithm() returns "GenericSecret" to bypass the
algorithm validation.

This matches at least with AWS CloudHSM JCE provider, but likely others
as well.
  • Loading branch information
@mnylen
mnylen committed Apr 16, 2024
commit eb703c3abb69563f171966039a0902047c6273f2
6 changes: 3 additions & 3 deletions impl/src/main/java/io/jsonwebtoken/impl/security/DefaultMacAlgorithm.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,9 @@ private void assertAlgorithmName(SecretKey key, boolean signing) {
throw new InvalidKeyException(msg);
}

// We can ignore PKCS11 key name assertions because HSM module key algorithm names don't always align with
// JCA standard algorithm names:
boolean pkcs11Key = KeysBridge.isSunPkcs11GenericSecret(key);
// We can ignore key name assertions for generic secrets, because HSM module key algorithm names
// don't always align with JCA standard algorithm names:
boolean pkcs11Key = KeysBridge.isGenericSecret(key);

//assert key's jca name is valid if it's a JWA standard algorithm:
if (!pkcs11Key && isJwaStandard() && !isJwaStandardJcaName(name)) {
Expand Down
14 changes: 10 additions & 4 deletions impl/src/main/java/io/jsonwebtoken/impl/security/KeysBridge.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public final class KeysBridge {

private static final String SUNPKCS11_GENERIC_SECRET_CLASSNAME = "sun.security.pkcs11.P11Key$P11SecretKey";
private static final String SUNPKCS11_GENERIC_SECRET_ALGNAME = "Generic Secret"; // https://github.com/openjdk/jdk/blob/4f90abaf17716493bad740dcef76d49f16d69379/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyStore.java#L1292
private static final String GENERIC_SECRET_ALGNAME = "GenericSecret"; // AWS CloudHSM JCE provider and possibly other HSMs

// prevent instantiation
private KeysBridge() {
Expand Down Expand Up @@ -95,10 +96,15 @@ public static byte[] findEncoded(Key key) {
return encoded;
}

public static boolean isSunPkcs11GenericSecret(Key key) {
return key instanceof SecretKey &&
key.getClass().getName().equals(SUNPKCS11_GENERIC_SECRET_CLASSNAME) &&
SUNPKCS11_GENERIC_SECRET_ALGNAME.equals(key.getAlgorithm());
public static boolean isGenericSecret(Key key) {
if (!(key instanceof SecretKey)) {
return false;
} else if (key.getClass().getName().equals(SUNPKCS11_GENERIC_SECRET_CLASSNAME) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit:

We tend to prefer pre-emptive returns in their own statement to help with readability, so putting the instanceof check in its own if/return is a little nicer than chaining else if logic. For example, I might rewrite this as:

public static isGenericSecret(Key key) {

    if (!(key instanceof SecretKey) {
        return false;
    }

    String algName = Assert.hasText(key.getAlgorithm(), "Key algorithm cannot be null or empty.");
    return algName.startsWith(GENERIC_PREFIX) && algName.endsWith(SECRET_SUFFIX);
}

SUNPKCS11_GENERIC_SECRET_ALGNAME.equals(key.getAlgorithm())) {
return true;
} else {
return GENERIC_SECRET_ALGNAME.equals(key.getAlgorithm());
}
}

/**
Expand Down
26 changes: 26 additions & 0 deletions impl/src/test/groovy/io/jsonwebtoken/impl/security/DefaultMacAlgorithmTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import io.jsonwebtoken.impl.io.Streams
import io.jsonwebtoken.security.*
import org.junit.Test

import javax.crypto.SecretKey
import javax.crypto.spec.SecretKeySpec
import java.nio.charset.StandardCharsets
import java.security.Key
Expand Down Expand Up @@ -232,4 +233,29 @@ class DefaultMacAlgorithmTest {
assertSame mac, DefaultMacAlgorithm.findByKey(oidKey)
}
}

/**
* Asserts that generic secrets are accepted
*/
@Test
void testValidateKeyAcceptsGenericSecret() {
def genericSecret = new SecretKey() {
@Override
String getAlgorithm() {
return 'GenericSecret'
}

@Override
String getFormat() {
return "RAW"
}

@Override
byte[] getEncoded() {
return Randoms.secureRandom().nextBytes(new byte[32])
}
}

newAlg().validateKey(genericSecret, true)
}
}
46 changes: 46 additions & 0 deletions impl/src/test/groovy/io/jsonwebtoken/impl/security/KeysBridgeTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,13 @@ package io.jsonwebtoken.impl.security

import org.junit.Test

import javax.crypto.SecretKey
import java.security.Key
import java.security.PrivateKey

import static org.junit.Assert.assertEquals
import static org.junit.Assert.assertFalse
import static org.junit.Assert.assertTrue

class KeysBridgeTest {

Expand Down Expand Up @@ -56,4 +60,46 @@ class KeysBridgeTest {
void testToStringPassword() {
testFormattedOutput(new PasswordSpec("foo".toCharArray()))
}

@Test
void testIsGenericSecret() {
def genericSecret = new SecretKey() {
@Override
String getAlgorithm() {
return "GenericSecret" ;
Copy link
Contributor

Choose a reason for hiding this comment

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

We'd probably want another test/tests with a space between 'Generic' and 'Secret' if we go with the .startsWith/.endsWith approach.

}

@Override
String getFormat() {
return null
}

@Override
byte[] getEncoded() {
return null;
}
};

def genericPrivateKey = new PrivateKey() {
@Override
String getAlgorithm() {
return "GenericSecret";
}

@Override
String getFormat() {
return null
}

@Override
byte[] getEncoded() {
return new byte[0]
}
}

assertTrue KeysBridge.isGenericSecret(genericSecret)
assertFalse KeysBridge.isGenericSecret(TestKeys.HS256)
assertFalse KeysBridge.isGenericSecret(TestKeys.A256GCM)
assertFalse KeysBridge.isGenericSecret(genericPrivateKey)
}
}