Skip to content

API: Use u8 instead of hex str#101

Merged
geonnave merged 5 commits intolake-rs:mainfrom
chrysn-pull-requests:api-u8-instead-of-hex
Oct 5, 2023
Merged

API: Use u8 instead of hex str#101
geonnave merged 5 commits intolake-rs:mainfrom
chrysn-pull-requests:api-u8-instead-of-hex

Conversation

@chrysn
Copy link
Copy Markdown
Member

@chrysn chrysn commented Sep 29, 2023

This first commit only changes the i part of the EdhocInitiator for review.

Apart from the obvious API break, the weirdest part here is the classify loop. @geonnave, how would I best sequence this against your pending changes that remove the hacspec arrays? Is there a branch I can build this on, should I hold it off for a few days, or should I just wrap the classify loop in a one-shot compatibility wrapper that doesn't need to be pretty, is used for all the conversions where the un-hexification conveniently hid the classify before, and wait for it to be removed later?

Contributes-to: #99

@chrysn chrysn force-pushed the api-u8-instead-of-hex branch from f799b40 to 371d0aa Compare September 29, 2023 16:45
@geonnave
Copy link
Copy Markdown
Collaborator

geonnave commented Oct 2, 2023

I think the proposed change is beneficial, give me one more day and I will report back on the state of the hacspec cleaning.

@chrysn chrysn force-pushed the api-u8-instead-of-hex branch from 371d0aa to f484f68 Compare October 3, 2023 19:14
@chrysn
Copy link
Copy Markdown
Member Author

chrysn commented Oct 3, 2023

This is targeting remove-hacspec now, given that that's the way forward and much easier with this PR -- every change made against main would need manual rebasing after #106 is merged anyway. I'm having trouble building locally with the remove-hacspec branch so far (from psa-crypto-sys); until I've managed to resolve that, I may be a bit force-pushy while I iron out bugs CI finds.

@chrysn chrysn force-pushed the api-u8-instead-of-hex branch 4 times, most recently from 2bfcff9 to f6d703b Compare October 3, 2023 20:06
@chrysn
Copy link
Copy Markdown
Member Author

chrysn commented Oct 3, 2023

Applying this to all the remaining hex parts, I think I'm done, except that I have questions / notes:

  • A few of the arguments could take an array reference instead of a slice; this would move some checks from runtime to build time. I chose not to do that because I hope that on the long run there will be the option for algorithm agility, and then we need flexibility again.
  • ID_CRED_x have fixed lengths. That appears odd to me, but I won't change unrelated things in here.
  • The low-level functions such as r_process_message_3 take CRED_x as a reference into an array and an extra length instead just as a slice. Is there any good reason for this? If not I'd change it in here right away, and it's a justifiable change in this PR because there was at least a (bad) reason for doing it before, being that the hex string had to be turned into a stack allocated buffer, which went away. (It's a bad reason because even then a slice could have been passed in, but at least the allocation made sense, so it may have not been caught by an earlier refactoring).
  • There is trouble with f-star generation. Could you help me out here?

@chrysn chrysn force-pushed the api-u8-instead-of-hex branch 3 times, most recently from e863535 to 20495a3 Compare October 3, 2023 20:49
@geonnave
Copy link
Copy Markdown
Collaborator

geonnave commented Oct 3, 2023

Responding to some comments:

I'm having trouble building locally with the remove-hacspec branch so far (from psa-crypto-sys)

Just to make sure, this is how I call it as of today in the remove-hacspec branch: cargo test --package edhoc-rs --package edhoc-crypto --package edhoc-ead --no-default-features --features="cb-psa, ead-none" --no-fail-fast.
You could also try cb-hacspec instead.

There is trouble with f-star generation. Could you help me out here?

Don't worry about it, we will likely skip this test from CI until the hacspec side is adjusted.

@geonnave
Copy link
Copy Markdown
Collaborator

geonnave commented Oct 4, 2023

@chrysn remove-hacspec has been merged, targeting main should be fine now :)

chrysn added 3 commits October 4, 2023 11:05
This only changes the `i` part of the EdhocInitiator; follow-ups will
extend this once the style is final.

BREAKING CHANGE: This alters EdhocInitiator's argument style.
This applies the changes previously made only to `i` on all hex style
types.

BREAKING CHANGE: This alters EdhocInitiator's argument style.
@chrysn chrysn force-pushed the api-u8-instead-of-hex branch from 20495a3 to d2ca7a7 Compare October 4, 2023 09:05
The need for this to be a buffer has gone away when changing the API to
use &[u8] instead of hex strings.
@chrysn
Copy link
Copy Markdown
Member Author

chrysn commented Oct 4, 2023

Way ahead of y... oh, that comment came in 4 minutes before I rebased, so things all have their proper sequence :-)

I've also given converting the CRED_x buffers into slices a try; that leaves only comments with on likely need for change from my side. It works in the first tests, but the big question is whether there is anything hax doesn't like about it.

(And the build issues, but that's a combination of "I don't have the right PSA sources installed" and "some examples don't respect the crypto- features and pull in PSA crypto unconditionally", which I think I can address separately).

@geonnave
Copy link
Copy Markdown
Collaborator

geonnave commented Oct 4, 2023

Regarding the compilation for PSA, I've easily made it work on Ubuntu but not on MacOS. As a result, when I'm on the Mac, I cargo test it within a multipass Ubuntu VM.

@geonnave
Copy link
Copy Markdown
Collaborator

geonnave commented Oct 4, 2023

The changes look great! Some notes:

C example

I don't know if I got it right but should it be updated as follows?

Use

static const uint8_t ID_CRED_I[] = {0xa1, ... };

Instead of

static const uint8_t ID_CRED_I[] = "a104412b";

Impacts on hax

@W95Psp could you give a quick look? Here's one example of the kind of change we're looking at:

fn compute_mac_2(
    prk_3e2m: &BytesHashLen,
    id_cred_r: &BytesIdCred,
    cred_r: &[u8],  // <--- this type changed from `&BytesMaxBuffer` (note that `type BytesMaxBuffer = [u8; MAX_BUFFER_LEN]`)
    th_2: &BytesHashLen,
) -> BytesMac2 {

The hex string lines were assigned to `s` in a Python interpreter, and
replaced by the output of

>>> def f(s): return '{' + ", ".join("0x" + "".join(x) for x in itertools.batched(s.groups(1)[0], 2)) + '}'
>>> print(re.sub('"([a-fA-F0-9]+)"', f, s))
@chrysn
Copy link
Copy Markdown
Member Author

chrysn commented Oct 4, 2023

Good catch on the C side; fixed. (That's why I like statically typed languages :-D).

@geonnave
Copy link
Copy Markdown
Collaborator

geonnave commented Oct 5, 2023

Looks good to me! The only unknown is the use of &[u8] with hax, but since hax only targets lib/src/lib.rs, that means only cred_X are impacted, which would be easy to modify if needed.

@geonnave geonnave added the type:enhancement New feature or request label Oct 5, 2023
@geonnave geonnave merged commit 7853302 into lake-rs:main Oct 5, 2023
@chrysn chrysn deleted the api-u8-instead-of-hex branch October 5, 2023 08:23
@chrysn chrysn mentioned this pull request Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants