Wire SHAKE to EVP #4137
Wire SHAKE to EVP #4137
Conversation
dot-asm
commented
Aug 10, 2017
dot-asm
commented
- documentation is added or updated
- tests are added or updated
crypto/evp/digest.c
Outdated
| { | ||
| int ret; | ||
|
|
||
| if (ctx->digest->flags & EVP_MD_FLAG_XOF && size <= INT_MAX && |
There was a problem hiding this comment.
I would prefer the if test on three lines, each one starting with the &&
crypto/evp/digest.c
Outdated
| OPENSSL_cleanse(ctx->md_data, ctx->digest->ctx_size); | ||
|
|
||
| return ret; | ||
| } else { |
There was a problem hiding this comment.
don't like the else after return her. esp because we often do 'goto err'
so this has the additional argument that it's not like common use.
There was a problem hiding this comment.
Note that style guide doesn't dictate use of 'goto err': "The goto statement comes in handy when a function exits from multiple locations." In other words goto is [customarily] considered as sign of desperation, but you do what you have to do, but only when you have to do it. This is not case here, so I'd argue against goto. Function is short and simple, no multiple exits, no need for 'goto'. Now, one can argue in favour of omission of else. I wouldn't add it if not for EVPerr. I find it appropriate to explicitly denote that it's outcome of failed condition. I mean without 'else' UNSPECIFIED_XOF_ERROR would probably be more appropriate with EVPerr. Wouldn't you agree?
There was a problem hiding this comment.
I disagree but it's style and taste, so okay.
There was a problem hiding this comment.
How about updated version?
crypto/evp/evp_err.c
Outdated
| @@ -179,6 +180,8 @@ static const ERR_STRING_DATA EVP_str_reasons[] = { | |||
| {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_METHOD_NOT_SUPPORTED), | |||
| "method not supported"}, | |||
| {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_MISSING_PARAMETERS), "missing parameters"}, | |||
| {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_NOT_XOF_OR_INVALID_LENGTH), | |||
| "not xof or invalid length"}, | |||
There was a problem hiding this comment.
should XOF be uppercase, because it's an abbreviation-sortof?
There was a problem hiding this comment.
This is auto-generated. Is it OK to fix it up manually? In other words question is if it will get overwritten afterwards?
There was a problem hiding this comment.
you have to edit the txt file in crypto/err.
|
Any plans to make this available in the apps? |
Next on the list is to figure out how does it play along with ASN.1 and thinking about what can be done about shake-len OIDs. As for apps/*, what do you have in mind? dgst actually works with default 128/256 lengths as it is. I mean you can 'dgst -shake256' in this request, but you can't specify output length. [Should one add additional command-line argument? Say -xoflen?] You can also 'speed -evp shake128'. |
Just in case, keep in mind that assembly modules are not actually engaged yet. |
include/openssl/evp.h
Outdated
| @@ -109,6 +109,9 @@ int (*EVP_MD_meth_get_ctrl(const EVP_MD *md))(EVP_MD_CTX *ctx, int cmd, | |||
| /* digest can only handle a single block */ | |||
| # define EVP_MD_FLAG_ONESHOT 0x0001 | |||
|
|
|||
| /* digest is extensible-output finction, XOF */ | |||
There was a problem hiding this comment.
Missed 1cm to the left. Thanks! Fixed. Since it's pure commentary update I consider that approval still holds...
Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #4137)
Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #4137)
