On Mon, Nov 7, 2022 at 9:02 PM Niels Möller <[email protected]> wrote:
> Norbert Pocs <[email protected]> writes: > > > The HPKE pull request was untouched for a long time, but I got back to > it. > > In short, the suggestions written by Niels at[0] were made. The rfc was > > finished, see [1]. > > Nice. > > > You are suggesting to work with a small subset of algorithms - I don't > know > > what exactly do you mean by that; Do you want to delete the other parts > of > > the code for now? > > It would be easier to review in smaller pieces. It might be a good idea > to take a step back and start with sorting out what the api should be > like. I take it that is hpke.h? It declares a few more function that are > included in the docs, and almost no comments. > My idea about the API was: set up a context, do encryption with the created context; as mentioned in the standard [2]. The documentation was updated to cover all the functions listed in hpke.h In short: - hpke_setup_* sets up the encryption context for sender or receiver in the given mode - hpke_seal: encryption - hpke_open: decryption - export_secret: derives a secret from the internal secret To narrow the scope of the review I suggest to start with the base mode and for the algorithms perhaps with one of the FIPS 140-3 approved ones, but otherwise I have not much preference in the algorithms. > > Also I see in the contribution doc that patches should be communicated > > through email. Should I resend the patch here? > > It's great that you send an update to the list, but no need to send the > full patches here. > > Some smaller comment of a very quick scroll through of the code: > > * In hpke-internal.h, it's right to have an underscore prefix on the > internal functions, but there's no need to have that underscore on the > function typedefs. Also the extern const structs need a prefix (with > and without underscore depending on whether or not they are intended > to be public; if they are public, their sizes will leak into the abi). > Underscore on typedefs fixed. I am a bit confused on the external structs. How I understood was that the users can use it for the context creation - which algorithms they prefer to use. Therefore it should be public so they can pass it to `hpke_ctx_init` or do I understand it wrong? > * In the cleanup code, there's no need to do "if (foo) free(foo);", by > the C standard, it's fine to call free with a NULL pointer (and I'm not > aware of any systems where it causes problems in practive either). > > Fixed. Is there a function which can clear memory space after a private key? I see `gmp_free`, but no comments to it. > * There's quite a lot in struct hpke_ctx. I think it would be good to > think about what parts are (i) algorithm parameterization, (ii) > related to key schedule, (iii) per message state? Can some parts be > moved from the state to inputs or outputs of relevant functions? > > I didn't want to have enormous function parameters everywhere, that's why I moved the variables there. If you are okay with bulkier function arguments I'll rework this. > * There's an RFCXXXX comment that I take it can be updated now? > Yes, thank you for the reminder! The standard decided to not use the rfc number, so no update needed on this. > > Regards, > /Niels > > -- > Niels Möller. PGP key CB4962D070D77D7FCB8BA36271D8F1FF368C6677. > Internet email is subject to wholesale government surveillance. > > [2] - https://datatracker.ietf.org/doc/html/rfc9180#section-5 Regards Norbert Pócs _______________________________________________ nettle-bugs mailing list -- [email protected] To unsubscribe send an email to [email protected]
