-
Notifications
You must be signed in to change notification settings - Fork 701
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
Parsing a PKCS#8 ECDSA signing keypair is confusing and inefficient #1889
Comments
So if I understood that correctly, to summarize
So far that sounds reasonable to me. The issues caused by pairing up the curve + algorithm are:
The idea proposed here is (roughly)
You also mentioned splitting them, I assume like:
The disadvantage to this approach is that it's implicit in accepting combinations, there's no difference between using a FIPS compliant combination vs a non-compliant legacy combination, but the latter must still be supported due to existing uses. The proposed solution seems smart to me (from my very outside view). My only concern is I wonder how much information a user needs in order to deserialize key pairs. If a default algorithm set could be used that's fine, but if they need to know more about how the key will be eventually used by a library in order to deserialize it it leaks library implementation details. I think the pattern of "user gets pem, needs to deserialize it, then pass it to a library" is common. Right now rustls gets around it by just taking a pem (string?) and doing the deserialization itself, but if the cert needs to be reused it presents a less type safe interface since it pushes you to pass around untyped bytes (are those bytes from a pem? der? pcks8? sec1? etc). Another option might be to have |
It isn't necessarily about safe vs. unsafe, but "allowed" vs. "not allowed," depending on which rules one is required to follow. In particular, when one is required to follow NIST/FIPS rules then certain combinations are not allowed for whatever reason.
We do not provide things that would be like the
Close. When parsing, one would supply what {curve,digest algorithm} combinations are allowed. When signing, ring would verify that the digest algorithm used was one of the ones that were mentioned being allowed at parsing time.
In your library, you can maintain this "default algorithm set" yourself within your library, if you don't want the user to have to configure which algorithms are allowed. But usually if you are dealing with things like PKCS#8 you will need to have some user configuration about which algorithms are allowed, because different users have different policies they need to conform to. |
Ah okay, makes sense. Thanks! |
First, in general people find the API confusing. We have not done a good job providing insight into the design of the API, and also the API is admittedly needs to be improved.
It is common for a user to want to support multiple curves and multiple digest algorithms when parsing a PKCS#8 document. The current API only allows the user to specify a single curve and a single digest algorithm when asking ring to parse a PKCS#8 document.
Another issue: To support multiple digest algorithms, some users, Rustls most notably, have resorted to awkward ways of using
EcdsaKeyPair
so that a single key pair can be used with multiple digest algorithms. They should be able to parse anEcdsaSigningKey
once and then use it with multiple digest algorithms (but see below).Accordingly, instead of only allowing the user to provide a single
EcdsaSigningAlgorithm
to indicate which algorithm combination is to be supported, we should let them provide multipleEcdsaSigningAlgorithm
s. And instead of Fixed vs. ASN1 inEcdsaSigningAlgorithm
, we should instead allow the user to choose the format at signing time.Further, when multiple provided
EcdsaSigningAlgorithm
s have the same curve identifier, but different digest algorithms, a singleEcdsaKeyPair
should be returned, and thatEcdsaKeyPair
should store all the digest algorithms that are allowed to be used. I suggest we add a field toEcdsaKeyPair
that is a bitmap, where nth bit is set iff theAlgorithmID
of the digest algorithm has value n. At signing time, the user will pass in a digest algorithm (and the format Fixed vs ASN1) and theEcdsaKeyPair
will refuse to sign with the given digest algorithm if it wasn't whitelisted at the time theEcdsaKeyPair
was constructed.As to the question of why
EcdsaSigningAlgorithm
should continue to identify both the curve and the digest algorithm, instead of having separateCurve
anddigest::Algorithm
parameters: We need to be able to enforce the requirement "A hash function that provides a lower security strength than is associated with the bit length of n shall not be used" from FIPS-186-5. Note that ring may provide combinations of algorithms that do not meet FIPS/NIST requirements like this one, for compatibility with existing systems; these combinations should ideally be clearly-named as legacy-only.For example, we need a way for the user to "I want to use P-256 with SHA-256, SHA-384, or SHA-512/256, and I want to use P-384 with SHA-384 but not SHA-256, but no other combinations." If the PKCS#8 document contains a P-256 key then the resulting
EcdasKeyPair
would allowsign()
to be called with&digest::SHA256
or&digest::SHA384
, but if the PKCS#8 document contains a P-384 key then callingsign
with&digest::SHA256
should fail with an error.In summary, we should:
ECDSA_{x}_{y}_FIXED_SIGNING
withECDSA_{x}_{y}_SIGNING
, removing the signature format (Fixed VS ASN1). This is a breaking change.EcdsaKeyPair::sign
that takes the digest algorithm and signature format as parameters. This one variant should verify that the digest algorithm was allowed at the time the key was constructed.EcdsaSigningAlgorithm
s to be provided as explained above.This is a Semver-breaking change.
The text was updated successfully, but these errors were encountered: