CRYPTO: Remove support for ex_data fields when building the FIPS module#10837
CRYPTO: Remove support for ex_data fields when building the FIPS module#10837levitte wants to merge 2 commits intoopenssl:masterfrom
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
| 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
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 Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Shane Lontis <[email protected]> (Merged from #10837)
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Shane Lontis <[email protected]> (Merged from #10837)
5 participants
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