randombit / botan

Cryptography Toolkit
https://botan.randombit.net
BSD 2-Clause "Simplified" License
2.6k stars 570 forks source link

Remove internal annotation from pcurves modules #4279

Closed randombit closed 4 months ago

randombit commented 4 months ago

This prevents a user from using the entirely sensible configuration of

--minimized --enable-modules=pcurves_secp256r1,sha2_32

reneme commented 4 months ago

We did have this discussion before, but --enable-modules and --disable-modules currently aren't consistent. The latter allows to manipulate internal modules, the former does not. Perhaps, instead we could just resolve this inconsistency.

Nevertheless: I think we should pick up the discussion about the definition of "internal module" and (if needed) update it in configure.rst:

https://github.com/randombit/botan/blob/03d8b16bbe4c007e879f2061938d2466247e18ed/doc/dev_ref/configure.rst?plain=1#L211-L212

I was of the impression that the pcurves modules don't expose any public API, hence they should be internal. Or am I missing something?

coveralls commented 4 months ago

Coverage Status

coverage: 91.636% (-0.002%) from 91.638% when pulling be1f32efb82780742525655083d4037896b63666 on jack/pcurves-not-internal into 40cf78055997a2ffc9f20b8ae15becf0bff14e64 on master.

randombit commented 4 months ago

I was of the impression that the pcurves modules don't expose any public API, hence they should be internal. Or am I missing something?

You are correct that there is no public API. And yet their availability or lack thereof is visible to the application developer via the side channels of performance and code size.

reneme commented 4 months ago

And yet their availability or lack thereof is visible to the application developer via the side channels of performance and code size.

😏 Sure, but that's likely true for any internal module. Things like aes_ni probably fall in the same category: no public interface, not strictly required for the functionality. Yet desirable.

No objection to make these modules public at all. I just feel I don't have a good mental model for what "internal" means then.

randombit commented 4 months ago

Things like aes_ni

But aes_ni is not an internal module :)

Looking at the other uses of Internal (kyber_common, pqcrystals, etc) I think the defining characteristic is that it's an implementation detail that not only has no public API but also has no effect or callers if some other module, which hard depends on the internal one, is not included. With that view, being able to disable the modules makes sense -- the user is saying they do not want any of that code. While being able to enable it doesn't, because the code that is built is dead without some other caller within the codebase, so the user should enable the thing that actually does what they want (say kyber_90s)

Compare

reneme commented 4 months ago

Sounds reasonable to me. Also the 'disableability' argument. Thanks.