Conversation
chrysn
left a comment
There was a problem hiding this comment.
A few comments inline.
From a final reviewing PoV, I'd hope to see the refactoring in shared and lib separated from the addition of an application, but that can be a split in the PR when it's going out of the draft stage (as keeping them split during development may incur excessive rebasing).
0e4b41f to
070686d
Compare
070686d to
bfdf6d6
Compare
a1adffd to
81a039a
Compare
f133afb to
0ae587d
Compare
handling of credentials is incomplete in this commit it will be refactored subsequently
0ae587d to
8bbd5c3
Compare
|
Edit: the mapping of tasks in this comment have been moved to the first comment. |
api: - instantiate initial Start state directly from within Initiator/Responder - make c_i/c_r optional: if not provided by the application, will be automatically generated - change order of some parameters cleanup: remove structs and functions from before the refactor
0aaa6e0 to
a80adeb
Compare
072cd9f to
fa8b65c
Compare
|
This looks to me like it's really independent of #170 (apart from commits such as 49cff1a that applies small fixes). I'm not sure how you do / want reviews done here, but to me this would be way easier to review if the splitting changes could happen in a branch of their own. Depends a bit on the level of review you want here -- if it's more a rough look you're after, that works here as well. If you prefer commit-by-commit review, that might be more productive on a dedicated branch. |
chrysn
left a comment
There was a problem hiding this comment.
Still mid-review, leaving some comments.
For context, the glass through which I'm looking at the code right now is git diff --ignore-all-space 5dd6c72^..geonnave/split-message-processing -- shared lib.
shared/src/lib.rs
Outdated
| pub x_or_y: BytesP256ElemLen, // ephemeral private key of myself | ||
| pub c_i: u8, // connection identifier chosen by the initiator | ||
| pub gy_or_gx: BytesP256ElemLen, // g_y or g_x, ephemeral public key of the peer | ||
| pub struct Start; |
There was a problem hiding this comment.
How long does generating a key take? (I have no clue, can be just-take-the-few-bytes-of-entropy, can be do-some-complex-GF2-computations). If it's not trivial, it may be a good idea to store the x_or_y already in the Start, as that allows a system to prepare a key while not in use.
(Could also spin that out to an own low-prio issue)
There was a problem hiding this comment.
I only have data for preparing message 1, which as a whole takes about 20 ms, given these conditions: crypto initialisation is stateful (see #94); using the cc310 backend. So it is fast, although software backends might take longer.
|
Thanks for the review so far. The type of review I am looking for is about the current state of the API and an overall rough look. |
|
I think the rough API is good from a high-level PoV. There are lots of details we can enhance, but we can do that progressively once this is in. One thing probably makes sense to do now because it's a pain to do later: look through the newly introduce type and function names. (Now this can be one commit with |
this includes moving the generation of ephemeral keys and selection of cipher suite to the `initiator/responder::new` functions
937eeca to
2e2e1cb
Compare
also fix examples
2e2e1cb to
b75d40a
Compare
On top of PR #170. Aims to provide the split message processing discussed in issue #99.
Main tasks:
update to only pass cred_x later on (instead of when callingEDIT: tracked in Pass CRED_X later on #179initiator/responder::newIn addition, as a result of the split there is now
IdCredandIdCredOwned. Thus,this should be made homogeneous.EDIT: tracked in Improve credential handling #178.Finally, with respect to the EAD:
lake-authzEAD implementationimplementEDIT: tracked in Ability to prepare and process more than one EAD field #177add_eadXfunctions