New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cache constants for fetched EVP_cipher #10461
Conversation
much gain for few lines of code; nice! |
Is it safe enough to cache flags? I'm not familiar with the provider code so are the cached values cached per implementation? |
The values that are cached here are equivalent to values that were in the old const EVP_CIPHER structs.. i.e- they are constant default values that should not change. The flags should be treated in the same way as the key_length (i.e this is the constant default key length). |
I'm not sure about flags because some of them can be implementation-defined (e.g. |
The values have been stored into the EVP_cipher during the fetch operation as they need to exist before either the new_ctx method or the ctx_int have been called. e.g- it needs to handle the sequence |
Well that particular one is a dead flag left over from the old fips module. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the semantics so that all the ciphers have to support all these cached constant parameters. However IMO that is actually a good thing. We could even add to documentation that these cached constant functions cannot fail if we wanted to make life of application writers easier.
@t8m, I don't think it changes the semantics quite like that: a cipher that doesn't know/support one of the cached parameters won't return it from the get query and it will default to zero. True, the unsupported return code won't happen anymore. It would be an easy enough change to keep the unsupported return code, if that is preferable. I like the idea of all ciphers supporting all of these constants. I do wonder what the IV length for an ECB mode cipher should be. Unsupported seems more reasonable than zero. |
Why do you want to distinguish an unsupported IV length from zero? Is there a practical reason? |
It's the first example of a change that sprung to mind -- agreed, it isn't a good one 😴 This does represent a (small) change of behaviour which is the issue. |
Actually, this gets us back to pre-3.0 behavior, no? |
@levitte, did you intend to approve this? It is still missing an OMC approval.... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replacing @slontis (for review purposes).
I was too quick on the labeling... but since you approved, all's good. |
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Patrick Steuer <patrick.steuer@de.ibm.com> (Merged from #10461)
merged. master: 3c957bc |
Checklist