Skip to content

Conversation

@paragonie-security
Copy link

This wraps keys into a struct, which has a header flag. This flag is checked at runtime.

KeyHeader is an enum. The least significant bit holds purpose (Local = 0, Public = 1); the remaining are reserved for the version. This results in the following pattern:

  • V1_LOCAL = 2
  • V1_PUBLIC = 3
  • V2_LOCAL = 4
  • V2_PUBLIC = 5
  • V3_LOCAL = 6
  • V3_PUBLIC = 7
  • V4_LOCAL = 8
  • V4_PUBLIC = 9
  • ...

See #7

This wraps keys into a struct, which has a header flag. This flag is checked at runtime.

KeyHeader is an enum. The least significant bit holds purpose (Local = 0, Public = 1); the remaining are reserved for the version. This results in the following pattern:

- V1_LOCAL = 2
- V1_PUBLIC = 3
- V2_LOCAL = 4
- V2_PUBLIC = 5
- V3_LOCAL = 6
- V3_PUBLIC = 7
- V4_LOCAL = 8
- V4_PUBLIC = 9
- ...
@paragonie-security
Copy link
Author

(This is a draft until we're confident that we didn't screw anything up.)

@minus7
Copy link
Member

minus7 commented Aug 6, 2021

Thanks for the PR. Here's some feedback:

  • The key structs should be prefixed with paseto_ additionally and typedefs shoud be avoided (they save the user typing the word "struct" at the cost of some clarity).
  • The KeyHeader enum should also be prefixed (type and its values). Preferably it should also use snake_case.
  • The key struct should be passed as a pointer everywhere, especially important for key loading

key.header = V2_PUBLIC;
return key_load_base64(key.key_bytes, paseto_v2_PUBLIC_SECRETKEYBYTES, key_base64);
struct paseto_v2_public_sk tmp;
*key = &tmp;
Copy link
Member

Choose a reason for hiding this comment

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

There's no point in doing this, since tmp does not get initialized in C, you can just drop it. Also the &tmp is plain wrong, you can't the address of a variable on the stack and returns it; aside from the fact that this doesn't even compile. Please test your patches before you send them, the readme contains the few simple instructions necessary.


enum paseto_key_header {
V2_LOCAL = 4,
V2_PUBLIC = 5
Copy link
Member

Choose a reason for hiding this comment

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

Everything in the header should be namespaced/prefixed so it doesn't potentially collide with other libraries or code. Enums aren't namespaced in C(++).

Copy link
Author

Choose a reason for hiding this comment

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

Understood. We don't normally do C development and was hoping the CI pipeline would catch these bugs.

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.

2 participants