CRYPTO: Remove support for ex_data fields when building the FIPS module #10837
Conversation
These fields are purely application data, and applications don't reach into the bowels of the FIPS module, so these fields are never used there. Fixes openssl#10835
levitte
added
branch: master
| @@ -308,8 +308,10 @@ struct rand_drbg_st { | |||
| size_t seedlen; | |||
| DRBG_STATUS state; | |||
|
|
|||
| #ifndef FIPS_MODE | |||
| /* Application data, mainly used in the KATs. */ | |||
There was a problem hiding this comment.
Doesn't this comment cause alarm?
There was a problem hiding this comment.
I was wondering about that, but having looked around, I only saw that used in test programs (i.e applications), so.
They can always be re-enabled selectively later on, if actually needed.
|
I'm wondering why? Why not leave them in the FIPS provider albeit unused? |
|
It created havoc when implementing key copying in the dsa keymgmt, because suddenly I was required to pass a OPENSSL_CTX for no real reason. This would have meant some pretty heavy refactoring of the dsa keymgmt. Incidently, I noticed that EC has already disabled ex_data when building the fips module, so it seems someone had already been thinking the same thing before. |
There was a problem hiding this comment.
I think this needs a discussion..
Not sure what the side effects of this will be...
libctx is being passed around for a few things in the fips module currently.
ex_data is one of them. BN_CTX and RAND also need it.
@mattcaswell ?
Yes? But does that mean that the keys need to have an ex_data field? |
The only place I can see that being affected, specifically for keymgmt, is with key generation, and if you look in #10289, the key generation functions receive a |
|
So, uhmmm, ping ? |
I see no problem with this. |
mattcaswell
added
approval: done
|
To this non-member, it looks like you're kinda rushing things. I hope folks half-a-world away feel their concerns are being addressed. |
Not sure what concerns you mean. This all seems fairly innocuous to me. |
|
Well, @slontis has the 24hour period to raise issues. But seems like two folks in the same region wrote code, pinged, answered a query, approved all while he slept :) Shrug. |
That's the whole point of the 24hr period... |
|
FYI, there was video call this morning (European time), involving @mattcaswell, @paulidale, @slontis, @romen and myself, where this PR was discussed among other things. |
levitte
added
approval: ready to merge
Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Shane Lontis <shane.lontis@oracle.com> (Merged from #10837)
These fields are purely application data, and applications don't reach
into the bowels of the FIPS module, so these fields are never used
there.
Fixes #10835