Accept oid in assertValid #589
Conversation
| @@ -122,16 +122,18 @@ | |||
| private final boolean jdkStandard; | |||
| private final int digestLength; | |||
| private final int minKeyLength; | |||
| private final List<String> alternativeNames; | |||
There was a problem hiding this comment.
If anything, I'd want to introduce a single field oid and check that if the jcaName check fails. Because it is all private, we don't have any risk of refactoring this later to a collection if we ever find that there is more than one.
I suspect all the other signature algorithms have OIDs as well, and if so (we should check), they should have a non-null value as well.
Also, let's mark this field as @Deprecated with a comment pointing to https://bugs.openjdk.java.net/browse/JDK-8243551. Having deprecated fields in internal implementations are fine because users never see/experience them.
There was a problem hiding this comment.
There was a problem hiding this comment.
I have extended the test I used to demonstrate this error to check all signature-algorithms against the JCA and Bouncycastle-Keystores. Only HmacSha-Keys loaded from a JCA-pkcs12-Keystore have shown the mentioned behaviour, so I went with checking the OID only for the HmacSha SignatureAlgorithms for now.
The following table lists Jca Names for each (private) Key when loaded from each keystore:
| SignatureAlgorithm / Keystore | jca | pkcs12 | jks | jceks | bks | uber | bcpkcs12 |
|---|---|---|---|---|---|---|---|
| HS256 | HmacSHA256 | 1.2.840.113549.2.9 | HmacSHA256 | HmacSHA256 | HmacSHA256 | ||
| HS384 | HmacSHA384 | 1.2.840.113549.2.10 | HmacSHA384 | HmacSHA384 | HmacSHA384 | ||
| HS512 | HmacSHA512 | 1.2.840.113549.2.11 | HmacSHA512 | HmacSHA512 | HmacSHA512 | ||
| RS256 | RSA | RSA | RSA | RSA | RSA | RSA | RSA |
| RS384 | RSA | RSA | RSA | RSA | RSA | RSA | RSA |
| RS512 | RSA | RSA | RSA | RSA | RSA | RSA | RSA |
| ES256 | EC | EC | EC | EC | EC | EC | EC |
| ES384 | EC | EC | EC | EC | EC | EC | EC |
| ES512 | EC | EC | EC | EC | EC | EC | EC |
| PS256 | RSA | RSA | RSA | RSA | RSA | RSA | RSA |
| PS384 | RSA | RSA | RSA | RSA | RSA | RSA | RSA |
| PS512 | RSA | RSA | RSA | RSA | RSA | RSA | RSA |
|
|
||
| SignatureAlgorithm(String value, String description, String familyName, String jcaName, boolean jdkStandard, | ||
| int digestLength, int minKeyLength) { | ||
| this(value, description,familyName, jcaName, jdkStandard, digestLength, minKeyLength, null); |
There was a problem hiding this comment.
Instead of passing null, is it possible to use the OIDs listed in the OpenJDK link I provided for the other algorithms? Thoughts?
There was a problem hiding this comment.
Sorry, I took a while: #589 (comment)
Seems, that it's not necessary to have oids for every Signature-Algorithm. Should we have them anyways? Or could we leave them out, since the field is going to be removed anyways?
There was a problem hiding this comment.
Ah, I see! Instead of oid then, we should have a pkcs12Name field that captures the names represented in the table above. This guarantees that everything is non-null as well as ensures a parallel variable (jcaName and pkcs12Name make more sense now).
There was a problem hiding this comment.
Yes, I agree. I renamed the field and made it a copy of jcaName for non-HmacSHA algorithms.
|
Now the field pkcs12Name has always a value (except for |
|
Resolves #588 |
Hello,
This is a first draft for #588
I've added the OIDs for Hmac-Keys and extended the check in the assertValid method. There are multiple ways of doing this, I decided to extend the SignatureAlgorithm's constructor with a varargs parameter to avoid having to modify all enum values.
For the test, an actual pkcs12 KeyStore instance is used to store and load the key. I suppose, one could mock the key. Would that be necessary?
The coverage checks failed to upload in my cloned repository, I suppose they will successfully do so, when the pr is built.