Verify signature_or_mac_3 #19
Conversation
+ Fix the broken example scripts + Use real X.509 certificates in the examples (authentication keys are Ed25519) --> remove pickled dictionary with legacy credential store + remote_cred_cb takes a Callable that returns the remote credentials + the return value of remote_cred_cb should go through _parse_credentials to parse/verify the RPK/certificate and extract the remote public authentication key + Add a custom CBOR encoder to properly encode COSE header maps with COSE header attributes + remove dependency on asn1crypto (everything can be done with the X509 module of the cryptography package)
|
Procedurally, it may also make sense to merge the fixes first (leaving out the last commit) and for me to merge that later with the checks on signature_or_mac2. |
|
Now extended to also verify mac_or_signature2; it'll probably need a round of refactoring to pass the tests again. [edit: envisioned refactoring steps are
] |
This will allow the verifier to rebuild a MAC in the next step.
The remote callback used to be evaluated lazily whenever one of the properties is first accessed; now it is evaluated as soon as all its input data are set (ie. when the ID_CRED_remote is set). That laziness currently *would* saved some calculation, but these savings go away as things are verified completely. The way this is done may later need to be changed again to accommodate asynchronous operation, either by making some parts async, or by altering how the library is used. Not evaluating the callback result also led to some usage patterns being untested. Resulting from these tests, *all* callback outputs are fed through _parse_credentials; this was previously done in only one code path.
|
Changes (including a consistent view on id_cred_remote) are regrouped as planned now, with a single "verify" commit on top. Tests start breaking at "cred / authkey: Evaluate callback eagerly, deduplicating code"; I'd need a bit of help understanding what fails here: if you check what's happening at tests/test_initiator.py line 26, in some cases cred_idr has not been set yet at all, so the initiator.remote_authkey would not have contained anything sensible before (None rather than what it now does in raising an AttributeError, which IMO is the better behavior for "not having been set"). How can that have yielded any sensible data before? (Along with that, I think this is now ready for review). |
The lambda wrapper represents a change already done earlier in "remote_cred_cb takes a Callable that returns the remote credentials" (just never hit the test cases). The cred_id[ri] setting is not so much done to have that available (it's not even stored currently), but to ensure that this step (that usually happens when parsing an incoming message) takes its side effects (of evaluating the credentials callback and storing remote cred and authkey) before these are used in the test evaluations.
Arguments may be had about whether this should be expanded even earlier, but at least now it's consistently accessible.
|
Tests are now all fixed, most of them in a manner I'd consider compatible (in the sense that the fixes are really to the test system, either because parts of it were buggy and just didn't trigger, or because they represent a usage pattern that is unobtainable outside test settings). What was not fixed that cleanly was testing of the mac_or_signature, as the auth keys for cred_type 0 (what is that? don't find it in the documented parts) are simply absent (None). In these cases, a warning is issued and the tests are aborted prematurely. With this, the test suite passes, I think this is good to review and merge. |
|
Thanks a lot for this update. I've went over it quickly (will do another more thorough pass later today) and it looks really good. The idea behind the |
| uhdr={headers.Algorithm: self.cipher_suite.sign_alg}, | ||
| payload=mac_3, | ||
| external_aad=external_aad) | ||
| # FIXME peeking into internals (probably best resolved at pycose level) |
There was a problem hiding this comment.
Yes I agree, using the pycose library here in edhoc has uncovered some design flaws in the API and its usage.
|
Merging in the changes. CI will fail because of the newer pycose package. |
There are some minor fixes, but primarily this introduces a check for the signature / MAC on message 3. (This is based on #17 and #18, so please ignore these first commits; this may be easier to process if they are merged first).
@TimothyClaeys please check whether the basic operation fits in your concepts for py-edhoc.
This can be tested by checking out the edhoc branch of aiocoap and running the
edhocserver.pyscript andedhocclient.py localhost, possibly with a--static-staticoption.