Refactor to support multiple kinds of keys in the future #51
Conversation
rkeene
force-pushed
the
feature/base-refactor-multiple-kinds-of-keys
branch
from
15d4fb4
to
4192114
Compare
rkeene
force-pushed
the
feature/base-refactor-multiple-kinds-of-keys
branch
from
4192114
to
de9caa1
Compare
| ) { | ||
| gpg_err_code_t error = GPG_ERR_GENERAL; | ||
| gcry_sexp_t sexp = NULL; | ||
| keyinfo keyinfo; |
There was a problem hiding this comment.
Missing initializer
| keyinfo keyinfo; | |
| keyinfo keyinfo = NULL; |
| unsigned char *tag; | ||
| unsigned char *value; | ||
| void (*value_free)(void *); | ||
| void (*tag_free)(void *); |
There was a problem hiding this comment.
the free method should not be exposed, the allocator should know how to free the structure it allocated, if you need hints you should have a private member here, as I am looking on header first, I may not understand why it is so.
| keyinfo = malloc(sizeof(*keyinfo)); | ||
| if (keyinfo == NULL) { | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
memset(keyinfo, 0, sizeof(*keyinfo)) will do the trick in here and at init.
| return NULL; | ||
| } | ||
|
|
||
| keyinfo_init(keyinfo, KEYINFO_KEY_TYPE_UNKNOWN); |
There was a problem hiding this comment.
I am unsure what is the difference between unknown and invalid key types.
in what case do we call init twice and why? I would make sure that memset(0) set the structure to uninitialized state, while when applying the key probably via der or similar set the key type and the data, and never reuse the structure for different key.
There was a problem hiding this comment.
UNKNOWN indicates the object has been constructed but not assigned a type yet; INVALID is a sentinel value set before free() to mark it in memory as no longer initialized, to help with future use-after-free issues.
There was a problem hiding this comment.
I think you can manage using a single type and a memset(0) of fields in order to manage the object with less complexity, as long as it is not permitted to reuse.
| /** | ||
| * Initialize a KeyUtil KeyInfo Object | ||
| */ | ||
| void keyinfo_init(keyinfo keyinfo, keyinfo_key_type_t keytype) { |
| if (keyinfo->type == KEYINFO_KEY_TYPE_RSA || keyinfo->type == KEYINFO_KEY_TYPE_UNKNOWN) { | ||
| keyinfo->data.rsa.e = NULL; | ||
| keyinfo->data.rsa.n = NULL; | ||
| } |
There was a problem hiding this comment.
not sure why the new cannot zero the memory to make this redundant. we should not call init twice.
There was a problem hiding this comment.
In the future, initializing for a specific kind of key may not set all values to zero, but to key-kind specific initial values.
There was a problem hiding this comment.
you set type when you actually have a key, for example in from_der so there you can set whatever value you want, what important is that the key type of uninitialized will be 0 which is provided by memset(0) in simple manner.
| )) != GPG_ERR_NO_ERROR | ||
| ) { | ||
| goto cleanup; | ||
| gcry_sexp_t keyinfo_to_sexp(keyinfo keyinfo) { |
| unsigned char *e_hex = NULL; | ||
|
|
||
| if (keyinfo->type == KEYINFO_KEY_TYPE_INVALID) { | ||
| return NULL; |
There was a problem hiding this comment.
please avoid return and use goto cleanup if possible.
| return NULL; | ||
| } | ||
|
|
||
| if (keyinfo->type != KEYINFO_KEY_TYPE_UNKNOWN) { |
There was a problem hiding this comment.
I truly think you should drop one of the invalid and unknown, I see no benefit of having them both as there is no different logic.
There was a problem hiding this comment.
The INVALID case used to abort() because a use-after-free had been detected, but now it must always do the same thing as UNKNOWN
There was a problem hiding this comment.
no abort() allowed in code, a proper error should be raised and resource management should take that into account.
| e_item->tag = (unsigned char *) "e"; | ||
| e_item->value = e_hex; | ||
| e_item->value_free = gcry_free; | ||
| e_item->tag_free = NULL; |
There was a problem hiding this comment.
won't it be much simpler to just write free explicit logic per key type?
There was a problem hiding this comment.
In the future, the value may need to be free'd using a different free method.
There was a problem hiding this comment.
For the EcDSA support the only two values we ever supply to value_free are currently gcry_free or NULL (if the value does not need to be freed).
There was a problem hiding this comment.
for RSA with have one method of free and ecdsa we have another, why make it that generic?
There was a problem hiding this comment.
Keeping release near allocation increases the chance that it will get done by people copy/pasting code.
There was a problem hiding this comment.
I do not follow... the keyinfo is the allocator and also responsible for free, the user (aka copy/paste) does not do anything but asking the keyinfo to release the list. the release function calls the function per algo to perform the actual release, should be simple as any resource release management. what am I missing?
There was a problem hiding this comment.
In the case where the decision regarding which freeing mechanism (if any) to use is in a different function far from the allocation function, it may get done incorrectly.
Currently the freeing function (keyinfo_data_free()) won't need to change as any new key types are added. In the proposed change, both the freeing function (keyinfo_data_free()) and the allocating function (keyinfo_get_key_data()) will both need to change in sync. The default free action must be not to free
There was a problem hiding this comment.
I understand if we had a very complex application, and in this case we would have a private member of the structure to avoid exposing the internal information, more probably we would not expose a struct as we want to be backward compatible when/if format change and provide accessor methods.
This application is not complex enough or generic infrastructure to require this kind of flexibility.
There was a problem hiding this comment.
I've already fixed several bugs in the code due to these decisions being made far apart.
| char buffer[1024]; | ||
| skip = 0; | ||
| switch (keyinfo_get_type(keyinfo)) { | ||
| case KEYINFO_KEY_TYPE_RSA: |
There was a problem hiding this comment.
I would be happy if we can avoid any algo specific logic in command.c, this logic may also be similar to the list.
There was a problem hiding this comment.
The format of message the SCD sends back to the agent differs based on the algorithm in a way that's specific to this interaction so it'll be a bit disjointed to put SCD command responses somewhere else.
This changeset is a refactor to support multiple kinds of keys in the future, such as EcDSA.