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.

Sign up for GitHub

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

Jump to bottom

synchronous challange-response verification #4

Merged
merged 12 commits into from
Sep 9, 2022
Merged

synchronous challange-response verification #4

merged 12 commits into from
Sep 9, 2022

Conversation

thomas-fossati
Copy link
Contributor

@thomas-fossati thomas-fossati commented Sep 2, 2022 •
edited
Loading

Fixes #1

@thomas-fossati thomas-fossati linked an issue Sep 2, 2022 that may be closed by this pull request
implement challenge-response verification API (synchronous model) #1
Closed
ionut-arm
ionut-arm reviewed Sep 2, 2022
View reviewed changes
Copy link

@ionut-arm ionut-arm left a comment

Choose a reason for hiding this comment

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

Left a few comments for now, but the important bit would be to take out the ambiguity from ChallengeResponseConfig by making it impossible for it to exist in an "invalid" state.

@thomas-fossati
Copy link
Contributor Author

Left a few comments for now, but the important bit would be to take out the ambiguity from ChallengeResponseConfig by making it impossible for it to exist in an "invalid" state.

absolutely agree

@thomas-fossati
Copy link
Contributor Author

Left a few comments for now, but the important bit would be to take out the ambiguity from ChallengeResponseConfig by making it impossible for it to exist in an "invalid" state.

absolutely agree

To be clear: the two main suggestions I have taken in are:

  1. using the builder pattern to make sure the object is in a valid state before any methods is invoked
  2. make nonce a binary choice using the "valued"-enum, and passing it to the run() method rather than making it part of the configuration. This way the same config can be reused as many times as needed.

ionut-arm
ionut-arm reviewed Sep 5, 2022
View reviewed changes
Copy link

@ionut-arm ionut-arm left a comment

Choose a reason for hiding this comment

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

Some more comments ✍🏻

@thomas-fossati thomas-fossati force-pushed the challange-response-sync branch from 9f49676 to be6ebbb Compare September 5, 2022 14:55
ionut-arm
ionut-arm approved these changes Sep 5, 2022
View reviewed changes
Copy link

@ionut-arm ionut-arm left a comment

Choose a reason for hiding this comment

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

LGTM! Do you want to expand the CI checks in this PR, or leave that for a future version?

@thomas-fossati
Copy link
Contributor Author

LGTM! Do you want to expand the CI checks in this PR, or leave that for a future version?

My plan was: once I get an OK on the implementation & API from you, I'd add the necessary test coverage. For now I have only used the default "Rust CI configuration" that GitHub makes available via the UI. What kind of extra CI checks are you thinking of?

@ionut-arm
Copy link

Just some static checking, format checking... For example, you could just copy-paste these checks from our PKCS11 crate - from line 28 onwards.

@ionut-arm
Copy link

Also, worth putting this blob at the beginning of lib.rs, it will make clippy more annoying useful.

@thomas-fossati thomas-fossati force-pushed the challange-response-sync branch from c246850 to 464be85 Compare September 5, 2022 15:26
ionut-arm
ionut-arm reviewed Sep 5, 2022
View reviewed changes
@thomas-fossati
Copy link
Contributor Author

Just some static checking, format checking... For example, you could just copy-paste these checks from our PKCS11 crate - from line 28 onwards.

awesome! Done in 7a6733c

thomas-fossati and others added 7 commits September 5, 2022 17:03
@thomas-fossati
initial commit (with poor unit test coverage)
655cb30 
Signed-off-by: Thomas Fossati <[email protected]>
@thomas-fossati
a few lessons learned
ad4a4bb 
Signed-off-by: Thomas Fossati <[email protected]>
@thomas-fossati
Ionut's comments
424f1a1 
Signed-off-by: Thomas Fossati <[email protected]>
@thomas-fossati
change &Vec<T> to &[T] wherever possible.
e40bbcc 
Signed-off-by: Thomas Fossati <[email protected]>
@thomas-fossati
fix clippy errors
92e2874 
Signed-off-by: Thomas Fossati <[email protected]>
@thomas-fossati
add further CI checks
917208d 
Signed-off-by: Thomas Fossati <[email protected]>
@thomas-fossati
more clippy stuff (+ fix inversion spotted by Ionut)
a4c10db 
Signed-off-by: Thomas Fossati <[email protected]>
@thomas-fossati thomas-fossati force-pushed the challange-response-sync branch from 6a1fc4b to a4c10db Compare September 5, 2022 16:03
@ionut-arm
Copy link

Cool, looks good! 👍🏻

@thomas-fossati
Copy link
Contributor Author

Cool, looks good! 👍🏻

thanks for the super help!

@thomas-fossati
builder unit tests
0b37bce 
Signed-off-by: Thomas Fossati <[email protected]>
@thomas-fossati thomas-fossati force-pushed the challange-response-sync branch from 6cf001b to 0b37bce Compare September 5, 2022 20:21
@thomas-fossati thomas-fossati marked this pull request as ready for review September 9, 2022 09:35
ionut-arm
ionut-arm approved these changes Sep 9, 2022
View reviewed changes
Copy link

@ionut-arm ionut-arm left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple of nits I highlighted below

yogeshbdeshpande
yogeshbdeshpande approved these changes Sep 9, 2022
View reviewed changes
Copy link

@yogeshbdeshpande yogeshbdeshpande left a comment

Choose a reason for hiding this comment

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

LGTM, with my limited knowledge of rust!

@thomas-fossati thomas-fossati merged commit 1c2ebaa into main Sep 9, 2022
@thomas-fossati thomas-fossati deleted the challange-response-sync branch September 9, 2022 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yogeshbdeshpande yogeshbdeshpande yogeshbdeshpande approved these changes

@setrofim setrofim Awaiting requested review from setrofim

@ionut-arm ionut-arm Awaiting requested review from ionut-arm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

implement challenge-response verification API (synchronous model)
3 participants
@thomas-fossati @ionut-arm @yogeshbdeshpande