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

Initial review feedback #2

Open
13 of 18 tasks
sosthene-nitrokey opened this issue Mar 15, 2023 · 2 comments
Open
13 of 18 tasks

Initial review feedback #2

sosthene-nitrokey opened this issue Mar 15, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@sosthene-nitrokey
Copy link
Contributor

sosthene-nitrokey commented Mar 15, 2023

  • Why is everything in a lib directory and not directly in src?
  • Please run cargo clippy
  • use standard Rust idioms. For example the ERROR_ID) enum should be called Error, with PascalCase variants, and no need for the ERR prefix in each variant.
  • The default PIN should probably be 123456 and not 1234 for consistency with other apps
  • Why re-export heapless and heapless-bytes?
  • You import serde_cbor but don't use it
  • Not a fan of adding the peek method to ctaphid_dispatch just for that, but I'm not sure how we could do differently. It should also IMHO be documented why exactly it is there. The dispatch crates already support multiple apps, so the commit names are not clear. Thinking about it, could this be made as a form of "middleware" or a wrapper around the FIDO app? That way ctaphid_dispatch stays "clean"? -> We will talk about it in the trussed steering call
  • __MAX_SIZE: You can use #[doc(hidden)] instead of prefixing with __
  • Please use pub use transport::Webcrypt. Type alias don't show up properly in doc
  • Is the dependency on git-version really necessary? Can't we just use env!("CARGO_PKG_VERSION")?
  • You can get rid of the min helper by using value1.min(value2)
  • Please move the cbor helper to the helper module
  • Is having a send_input_to_output really necessary if it's only used for one (debug) command?
  • Please use constants to document "magic values" (for example why 3 in send_input_to_output?)
  • It might be good to have a WebcryptClient trait that unites all the trait bounds, which are a bit verbose.
  • Have you tested the OpenPGP import functionality? unsafe_inject_shared_key Doesn't work with anything other than symmetric keys.
  • Wouldn't we be better off reusing opcard somehow for the OpenPGP stuff? I think users would expect to be able to use their "native" openpgp keys with wecrypt. If no I don't see why the OpenPGP compatibility can't be built into the JS side.
  • In command.rs, a comment mention ChaCha20 but ChaCha8 is used

Edit: remaking this ticket into a task list

@szszszsz szszszsz changed the title Initial feedback: Initial review feedback Mar 22, 2023
@szszszsz szszszsz added the bug Something isn't working label Mar 22, 2023
@szszszsz
Copy link
Member

szszszsz commented Jun 7, 2023

Some of the listed points are fixed in #3
Regarding the questions:

  1. Instead of peek, parsing the Credential's key type could work better, at a potential compatibility problems cost (browser validating and rejecting the requests).
  2. git-version is good for the development stage, so you do not need to bump version by hand constantly. Why do you think this is a problem? For the actual release this would not be required of course.
  3. OpenPGP stuff was all working at the time - you can check test reports.
  4. Passing traffic to opcard-rs would be cool to have. It was not ready back when this was worked on. Though right now it cannot be done by design, is it not that right? Also, I would be vary opening it to the Web applications. As for the actual keys, that would have to be decided by the user (separate set, or the same, or even connecting that with multiple identities feature).

For the context of some of the listed points, this implementation was a haste transcription from C, having as much same elements as possible to keep the compatibility with the related applications.

@sosthene-nitrokey
Copy link
Contributor Author

I do not understand what you mean by "parsing the credential's key type could work better".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants