Initial support for explicit EC #245
Conversation
|
I know I created curve name and pub key attributes for convenience, but I think it is just too much to create that many attributes that are used only in the export function. I would rather revert and remove those convenience attributes and explicitly decode CKA_EC_PARAMS where needed rather than extending their use this way. |
|
OK, I understand. I'll try to refactor without using the convenience attributes. |
manison
force-pushed
the
feature/explicit-ec
branch
from
947c0c1 to
9611123
Compare
|
I have rewritten the export function to not use the pseudo attributes. Now it decodes the CKA_EC_PARAMS as needed. Can you take a look whether it is the right direction? |
src/objects.c
Outdated
| if (p == NULL) { | ||
| return NULL; | ||
| } | ||
| export_ctx->allocs[export_ctx->ialloc++] = p; |
There was a problem hiding this comment.
Please increment on the next line in cases like this.
I am more interested in readability than compactness of the code.
There was a problem hiding this comment.
Completely removed and refactored.
|
Sorry for late review, I was on vacation. |
|
Thanks for the review. No need to be sorry, hope you enjoyed your vacation :) I'll walk through your comments and prepare updated revision of the PR. |
manison
force-pushed
the
feature/explicit-ec
branch
from
9611123 to
26bade5
Compare
|
I pushed another update, can you please take a look? Thanks. |
src/encoder.c
Outdated
| union { | ||
| ASN1_OBJECT *object; | ||
| ASN1_STRING *sequence; | ||
| void *p; |
There was a problem hiding this comment.
why do you need p?
you can always just set k->curve.object = NULL when you need to clear this.
|
Looks mostly good, but I do a full re-review in the next few days. |
|
Done. Should I prepare the final PR? I suppose I will squash everything into a single commit (both code changes and tests). Are you ok with that? |
I think we are good. |
Signed-off-by: manison <manison@users.noreply.github.com>
Signed-off-by: manison <manison@users.noreply.github.com>
manison
force-pushed
the
feature/explicit-ec
branch
from
82b9dab to
cf834be
Compare
|
The PR is ready, I ended up making it two commits. The SEGV from LSAN might be bug in LSAN itself (see e.g. skupperproject/skupper-router#206, google/sanitizers#1322, https://bugzilla.mozilla.org/show_bug.cgi?id=1635327 and lot more). I cannot reproduce locally and in my fork it fails rather randomly. |
|
I am rerunning the test, let's see if it is a flake or if more analysis is needed |
|
Well, what do you know, it is flakey, And thanks a lot for your patience, I really believe the code was improved 10 folds from first submission and really like the result! |
I'm happy for your review and I like the result better too. Thanks for providing your feedback and merging my contribution. |
This is initial PoC for #205 for review purposes.