Generic ECDSA implementation #181
Conversation
|
This is just PoC for now, because I would like to get some feedback if this in general would be something, that is interesting for the upstream project, because it probably can't be made backwards compatible. At least not, if removing the standard curves should also be supported. Also I am not sure about the performance impact by using a first-class module. My expectation would be, that it's negligible, but who knows. |
|
Thanks. I'm on vacation at the moment. An immediate question would be:
Would you mind to run a benchmark? Regarding this PR, it would make a big difference, if it is 10x slower or not. |
| let brainpoolP384r1 = brainpool_base <| 11 | ||
| let brainpoolP384t1 = brainpool_base <| 12 | ||
| let brainpoolP512r1 = brainpool_base <| 13 | ||
| let brainpoolP512t1 = brainpool_base <| 14 |
There was a problem hiding this comment.
these additions look unrelated to the scope of this PR.
There was a problem hiding this comment.
I actually exported them in the Dsa_curves.OIDs module as a convenience for registering the curve implementations.
There was a problem hiding this comment.
It's fine to export them, but I'd still prefer to have them added in a separate PR.
Sure. Based on the Old: new generic interface: |
|
Thanks for the benchmark. Would this approach scale to not only ECDSA, but as well other PK algorithms (Ed25519, RSA)? I suspect we'd need a generic module signature for all these, but that may be worth doing (to provide an X509 package which doesn't depend on libgmp...). I'll think a bit more about it when I have time, but this PR is for sure in the right direction. |
| let curves, add_curve = | ||
| let curves = ref [] in | ||
| (fun () -> !curves), | ||
| (fun c -> curves := c :: !curves) |
There was a problem hiding this comment.
this uses a list, which is ever growing. I wonder what about registration of the same curve multiple times? Since the lookup in the list is done by name, shouldn't we use a map?
| val pp : t Fmt.t | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
there's documentation lacking. For Dsa_curves purpose, and how to use it.
| module type Dsa = Mirage_crypto_ec.Dsa | ||
| module type S = Dsa_curves.S | ||
|
|
||
| type t = (module S) |
There was a problem hiding this comment.
are module type S and type t needed to be exposed?
| end | ||
|
|
||
| val register : | ||
| string -> Asn.oid -> (module Dsa) -> t |
There was a problem hiding this comment.
why return a t from register? shouldn't it just be a unit? or what is it that you like to do with t? (also, taking the other comment into account, should it possible to return an error to avoid multiple registrations of the same curve?)
This is a generic ECDSA implementation, that allows to register only the specific ECDSA implementation packages, that are actually required. This allows to remove unused curve implementations from the code base, but also to extend X509 with unusual curves, that are usually not available.