Allow using GenericSecret for HmacSHA* #935
Conversation
mnylen
force-pushed
the
allow-generic-secret-with-hmac
branch
3 times, most recently
from
9039f6c
to
74c90ab
Compare
| @@ -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"; | |||
There was a problem hiding this comment.
@mnylen Can you add a comment here about where this string comes from.
You have a great description in the conversation, but adding it inline in the code will help future viewers 😄
@lhazlewood any idea if this should be made more generic somehow? I'm not sure how often we would come across this type of problem. (but I'm guessing it was a PITA to troubleshoot?)
There was a problem hiding this comment.
I can add the comment. I also agree that it'd be great to be able to have this support for generic secrets a bit more generic, but not sure how to do that. As far as I'm aware, there's no standard for these HSM-backed JCE providers to follow.
One option of course would be to wrap the key with some kind of additional metadata (such as algorithm), and then unwrap it in the algorithms before passing it to the provider. There's existing ProviderKey already, but that is unwrapped way before the key hits the algorithm implementations.
Or perhaps add a mechanism for registering the secret class names that should not be treated as strictly in validations.
There was a problem hiding this comment.
Awesome! Thanks for the quick update!
There was a problem hiding this comment.
@bdemers @mnylen I'm wondering if this should be made to handle both names, something along the lines of:
if (algName.contains("Generic") && algName.contains("Secret")) ....or
if (algName.startsWith("Generic") && algName.endsWith("Secret"))...whichever is faster. (I'm mostly sure the .startsWith/.endsWith approach is faster)
There was a problem hiding this comment.
I don't mind doing the change. Wonder if just startsWith("Generic") would be enough though?
There was a problem hiding this comment.
Hrm, maybe? 🤔
I tend to be a little tighter on restrictions like this, but I think checking for just Generic makes sense because we're already checking for instanceof SecretKey before this step.
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.
mnylen
force-pushed
the
allow-generic-secret-with-hmac
branch
from
74c90ab
to
eb703c3
Compare
| public static boolean isGenericSecret(Key key) { | ||
| if (!(key instanceof SecretKey)) { | ||
| return false; | ||
| } else if (key.getClass().getName().equals(SUNPKCS11_GENERIC_SECRET_CLASSNAME) && |
There was a problem hiding this comment.
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);
}| def genericSecret = new SecretKey() { | ||
| @Override | ||
| String getAlgorithm() { | ||
| return "GenericSecret" ; |
There was a problem hiding this comment.
We'd probably want another test/tests with a space between 'Generic' and 'Secret' if we go with the .startsWith/.endsWith approach.
|
Made the requested changes. Been a bit busy, so it's somewhat slow going. :) |
|
Excellent stuff @mnylen, thanks! Once CI passes, we can merge 😄 |
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.
Relates to discussion #934
Let me know if more tests are needed. I didn't find a test that would've tested the existing Sun PKCS#11 generic secret case, and not sure how to test that when the class doesn't exist in the classpath.