Implement ZeroizeOnDrop for the output of buffer_ct_variable

[kayabaNerve]

This allows blake2::Blake2b: ZeroizeOnDrop.

[newpavlov]

Can't we do something like this instead? It relies on buffer and "core" to implement zeroization on drop, so explicit Drop is not needed.

[kayabaNerve]

... huh. Since the fields impl ZeroizeOnDrop, I can see why we'd impl ZeroizeOnDrop even without Drop. Neat. Thanks for the suggestion :)

[kayabaNerve]

Updated the PR to add ZeroizeOnDrop with absolutely no concerns re: breaking changes and updated the description accordingly.

[newpavlov]

This looks like an unnecessary complication. Why can't you simply copy what is done in buffer_fixed!?

[kayabaNerve]

To keep the cfg(feature = "zeroize") local to digest and not in the generated code which assumes the caller will have a feature of the same name. In all my work, I don't have zeroize as optional and don't define such a feature, so I'd not have such codegen applied despite explicitly wanting it as shown by enabling digest/zeroize. The fact the other macro exhibited that pattern would be a hygiene issue and bug in it, AFAIK.

[newpavlov]

This introduces inconsistency across macros and makes the macros API even more complicated and confusing than it's already is.

The fact the other macro exhibited that pattern would be a hygiene issue and bug in it, AFAIK.

No, it's intentional. The macro is a helper to define a buffered struct, so the generated code should work with features defined by the downstream crate. If it does not define zeroize crate feature for some reason, then it should not implement ZeroizeOnDrop using the macro and instead do it with an explicit impl.

[kayabaNerve]

I don't actually believe this makes the macro API more complicated, as the user-facing API is identical. The only change is when digest/zeroize, it'll also derive ZeroizeOnDrop.

No, it's intentional. The macro is a helper to define a buffered struct, so the generated code should work with features defined by the downstream crate.

If I have to update this macro to be unhygienic and impose undocumented requirements on the caller's declared features (assumed to have the exact same presence and name as seen within RustCrypto) to get it merged, I'm fine doing so because I honestly can't be arsed to care, but please confirm if that's a prerequisite for this PR getting merged. Right now, I'm unclear if this is still a discussion or if I am being told this is a hard blocker, sorry.

If it does not define zeroize crate feature for some reason, then it should not implement ZeroizeOnDrop using the macro and instead do it with an explicit impl

I'm unsure if this is anything more than a footgun. Specifically, the digest/zeroize feature means that zeroize is a dependency and already present in-tree. I have no idea why, when zeroize is present, digest wouldn't derive traits from zeroize. If the goal is a caller who's using these macros but can opt-in/out of these features, then this macro should have an explicit variant with ZeroizeOnDrop/a way to exclude traits/shouldn't generate ZeroizeOnDrop at all IMO. This would imply I should PR this trait implementation directly to blake2, though I assume such a PR will then be kicked back as it should be part of the macro.

Also, manually unconditionally implementing ZeroizeOnDrop somewhat conflicts with a macro which conditionally implements it. I don't believe it'd ever actually come up, as it'd only be realized if that feature was added and enabled, but it truly seems like a poor solution to me.