search

raphaelrobert / privacypass

Privacy Pass implementation in Rust
MIT License
6 stars 7 forks source link

#[repr(C)] on Token/TokenChallenge? #34

Open bcspragu opened 2 weeks ago

bcspragu commented 2 weeks ago

Hi there!

I was looking at the Token definition, and noticed it doesn't have a #[repr(C)] attribute on it: https://github.com/raphaelrobert/privacypass/blob/2858da319ee37cdeead00fd9fa03ea53f7787038/src/auth/authorize.rs#L31-L33

Since the spec defines things in terms of C structs, I think you want repr(C) to guarantee Rust does the correct thing here.

The same is likely true of TokenChallenge and TokenInput: https://github.com/raphaelrobert/privacypass/blob/2858da319ee37cdeead00fd9fa03ea53f7787038/src/auth/authenticate.rs#L34-L36 https://github.com/raphaelrobert/privacypass/blob/2858da319ee37cdeead00fd9fa03ea53f7787038/src/lib.rs#L63-L65

raphaelrobert commented 2 weeks ago

Can you expand on what you mean by "the correct thing"? In my mind, the correct thing is to let Rust manage the memory layout. Note that the TLS serialization is implemented independently of the memory layout.

bcspragu commented 2 weeks ago

Ah indeed, I see the TLS ser/de, which I hadn't seen before, agreed this is definitely not an issue when using those functions. By "the correct thing", I mean that since these are public structs, clients could try ser/de on them themselves, which might create a binary representation incompatible with the spec if naively using this package in some FFI scenario

raphaelrobert commented 2 weeks ago

I see. Clients would have to use unsafe Rust to do that. Personally, I'd find it unintuitive to access a Rust struct like that and expect it to work. But you are right that it could be an issue in some scenarios. We should either have better documentation or an actual C API to prevent that, or both. I'll happily take PRs.

Using repr(C) wouldn't fix the problem btw, because it would only work for literal C structs, not the TLS representation used for e.g. BatchedTokenRequest.