Skip to content
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

feat: add eckey_t #93

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

gocarlos
Copy link
Member

@gocarlos gocarlos commented Apr 22, 2020

what

why

  • by creating the configure.h at configuration time, we have the right defines in the header file instead of relying on the compiler flags from library consumers...
  • namespace is important to not collide with other macros,

@coveralls
Copy link

coveralls commented Apr 22, 2020

Coverage Status

Coverage decreased (-1.07%) to 83.52% when pulling daba796 on gocarlos:feat--add-eckey_t into 9223f01 on cose-wg:master.

@gocarlos
Copy link
Member Author

@wgtdkp @jimsch please review this

@gocarlos gocarlos marked this pull request as ready for review April 22, 2020 11:39
src/Sign1.c Outdated Show resolved Hide resolved
src/Sign1.c Outdated Show resolved Hide resolved
src/bcrypt.c Outdated Show resolved Hide resolved
Copy link
Contributor

@jimsch jimsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long term this is not my preferred method of addressing this issue. Moving to a COSE Key structure and replacing the current CBOR key passed in allows for similar ways of dealing with the passed in keys and also allows for keeping the internal keys created around for a period of time if they are used multiple times, for example doing a signature verification against the same key multiple times.

I can understand doing this short term but it might mean another change later down the line

typedef struct eckey_t {
struct ec_key_st *key;
int group;
} eckey_t;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure where this came from. I would expect it to be an EVP_KEY*

include/cose/cose_configure.h.in Show resolved Hide resolved
include/cose/cose_configure.h.in Outdated Show resolved Hide resolved
@wgtdkp
Copy link
Contributor

wgtdkp commented Apr 23, 2020

@jimsch

Moving to a COSE Key structure and replacing the current CBOR key passed in

Replacing the CBOR key will break compatibility, is that acceptable? If true, I agree we should replace CBOR key with a COSE key.

@jimsch
Copy link
Contributor

jimsch commented Apr 23, 2020

It would be possible to provide a thin wrapper that converts a CBOR key to a COSE key for a while and mark those functions as depreciated. Although I think every compiler does that differently.

@wgtdkp
Copy link
Contributor

wgtdkp commented Apr 23, 2020

@jimsch

It would be possible to provide a thin wrapper that converts a CBOR key to a COSE key for a while and mark those functions as depreciated. Although I think every compiler does that differently.

Thanks! It makes sense to me.

@gocarlos
Copy link
Member Author

gocarlos commented Apr 23, 2020

Can we make this in a next mr? I wanted to make sure that ot-commissioner works with upstream cose-c. And it seems to work ok now... i’m for merging, adding a test and change the interfaces...

@jimsch what do you think?

@gocarlos
Copy link
Member Author

Although I think every compiler does that differently.

Yeah, if the code would be c++, then this would be part of the language ;)

@jimsch
Copy link
Contributor

jimsch commented Apr 23, 2020

A new PR WFM - I chose C rather than C++ because of the environment that I was shooting for. In retrospect I think I could have gone with C++ but that is life.

@gocarlos
Copy link
Member Author

A new PR WFM - I chose C rather than C++ because of the environment that I was shooting for. In retrospect I think I could have gone with C++ but that is life.

I meant, that i think is better to merge this now (no side effects for existing users) then to improve it...

@gocarlos gocarlos mentioned this pull request Apr 24, 2020
@gocarlos gocarlos requested a review from jimsch April 24, 2020 16:45
int group;
} eckey_t;
#endif // COSE_C_USE_MBEDTLS

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted in the other pull request, this is not the way that I think this should go in the end. It would be better to declare a COSE_Key structure and have a method to build that. This can this be changed internal to the library without issues as well as adding some other types of keys. If you are doing this, please just make this specific to MBEDTLS as passing in the structure is not how one would do this for OpenSSL, instead you would pass in an EVP_KEY *.

I don't know what the current support is in MBEDTLS for TEMs at the moment. I would expect that at some point in the future this is going to be added if not currently present and that would require yet a different possible internal change for that support.

@jimsch
Copy link
Contributor

jimsch commented May 6, 2020

I am tossing together a framework for this right now.

@wgtdkp
Copy link
Contributor

wgtdkp commented May 7, 2020

I am tossing together a framework for this right now.

thanks @jimsch !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants