Add ECDSA support (named curve only, requires OpenSSL 1.1.0) #11
Conversation
This commit adds ECDSA support. Currently, only named curve is supported. OpenSSL 1.1.0, which is not released yet, added necessary functions to support EC_KEY with custom engine, and prior versions have no workaround over these missing functions, we have to depend on OpenSSL 1.1.0. More specifically, we require OpenSSL 1.1.0 pre2.
|
I've added new commit to compile with OpenSSL 1.1.0pre5. It contains massive changes, and it broke RSA code in neverbleed. 1.1.0 makes many OpenSSL object struct opaque, and all codes to access field directly break. This commit fixes this issue. |
|
Since OpenSSL 1.1.0 is around the corner (the OpenSSL dev said that they will release it in August 25, 2016), it is a good time for us to seriously consider to review this PR. |
|
Merged master branch, resolved conflict. |
|
Now we only consider openssl 1.1.0 final version. I also fixed compiler warnings. |
| break; | ||
| #endif | ||
| default: | ||
| if ((errstr = expbuf_shift_str(&buf)) == NULL) { |
There was a problem hiding this comment.
I assume we should just return an error to the peer without trying to parse arguments calling by expbuf_shift_xxx, considering the fact that we cannot determine the number and types of the arguments unless the type is TYPE_RSA or TYPE_ECDSA.
There was a problem hiding this comment.
I'm ok with that. But we are sure that after key_index, error message string follows. Can we just use it to return the error message to the client?
|
Thank you for the updates. I like how the RSA-related APIs are backported? for 1.0.2. LGTM aside from my comments i-line (including the issues I found when trying to compile the file using OpenSSL 1.0.2). |
Reduce scope of some variables which are only used for RSA or ECDSA so that they are defined where they are used.
|
|
||
| Respond: | ||
| expbuf_dispose(buf); | ||
| expbuf_push_num(buf, rsa != NULL); | ||
| expbuf_push_num(buf, type != NEVERBLEED_TYPE_NONE); | ||
| expbuf_push_num(buf, type); |
There was a problem hiding this comment.
Would the code become more concise if NEVERBLEED_TYPE_NONE could be used as an indication of an error, instead of sending a boolean value indicating success and the type value?
(and if we are to use the none type as error indication it might make more sense to rename it the error type)
There was a problem hiding this comment.
If I understand it correctly, you are suggesting that remove boolean (type != NEVERBLEED_TYPE_NONE), and rename neverbleed_type as neverbleed_error_type. Is that correct?
There was a problem hiding this comment.
Perhaps, renaming NEVERBLEED_TYPE_NONE as NEVERBLEED_TYPE_ERROR ?
There was a problem hiding this comment.
Correct.
At the moment, the arguments used for load_key are: ret, type, key_index, …
However, considering the fact that ret is a boolean that just indicates either success or failure and that we have NEVERBLEED_TYPE_NONE, it seems to me that the argument should better be: type, key_index, ..., merging the type and ret into a single argument.
So to rephrase my suggestion is:
- change
NEVERBLEED_TYPE_NONEtoNEVERBLEED_TYPE_ERROR - stop passing
retas the first value ofload_key, but usetype != NEVERBLEED_TYPE_ERRORto see if the call has succeeded
There was a problem hiding this comment.
Thank you for clarification. Will do.
|
Thank you for the changes. The code looks great, however I left a comment since we might be possible to make it even better. |
We renamed NEVERBLEED_TYPE_NONE as NEVERBLEED_TYPE_ERROR, and instead of returning boolean value to indicate success/failure, we use type for its purpose. Now boolean value was removed.
Add ECDSA support (named curve only, requires OpenSSL 1.1.0)
|
Thank you for the update! Merged to master. |
This commit adds ECDSA support. Currently, only named curve is
supported. OpenSSL 1.1.0, which is not released yet, added necessary
functions to support EC_KEY with custom engine, and prior versions
have no workaround over these missing functions, we have to depend on
OpenSSL 1.1.0. More specifically, we require OpenSSL 1.1.0 pre2.