openssh unix dev - Oct 2022 - [PATCH] Use EVP_MAC interface for Poly1305 if supported.

On Tue, 25 Oct 2022 at 06:23, Chris Rapier <rapier at psc.edu> wrote:
> +#if OPENSSL_VERSION_NUMBER >= 0x30000000UL
As mentioned by Dmitry Belyavskiy upthread, since this depends on
EVP_MAC_fetch() this should probably be checked by configure instead
and put inside an ifdef HAVE_EVP_MAC_FETCH.  I'm also wondering if the
additional OpenSSL specific code belongs in the poly1305_auth function
in cipher-chachapoly-libcrypto.c.
> +       size_t poly_out_len;
> +#endif
Since poly_out_len is only ever used inside the  "if (!do_encrypt)"
block below, you could move this declaration inside the existing ifdef
inside that block and reduce this diff by one hunk.


--
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860  37F4 9357 ECEF 11EA A6FA (new)
    Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.

On 10/24/22 4:23 PM, Darren Tucker wrote:
> On Tue, 25 Oct 2022 at 06:23, Chris Rapier <rapier at psc.edu> wrote:
> 
>> +#if OPENSSL_VERSION_NUMBER >= 0x30000000UL
> 
> As mentioned by Dmitry Belyavskiy upthread, since this depends on
> EVP_MAC_fetch() this should probably be checked by configure instead
> and put inside an ifdef HAVE_EVP_MAC_FETCH.  I'm also wondering if the
> additional OpenSSL specific code belongs in the poly1305_auth function
> in cipher-chachapoly-libcrypto.c.
Okay, I think I'm understanding. We'd still have the #ifdefs in the code
but we'd be moving away from actually checking a specific version string 
to seeing if the function itself exists. I'll work on that tomorrow.

As for putting it in the poly_auth function itself. I'm concerned that 
making the parameters work would be difficult and possible confusing if 
we maintained the current ones for poly1305_auth(). As far as I can 
figure we'd need 5 parameters and to set ctx->poly_ctx explictly to null 
in is HAVE_EVP_MAC_FETCH is false. Thoughts?
>> +       size_t poly_out_len;
>> +#endif
> 
> Since poly_out_len is only ever used inside the  "if
(!do_encrypt)"
> block below, you could move this declaration inside the existing ifdef
> inside that block and reduce this diff by one hunk.
Good point. I've made that change. I'm going to think about a few more 
things and work out the configure before I submit a new patch.

Chris