Skip to content
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

doc: Fix documentation of EVP_EncryptUpdate(). #12435

Closed
wants to merge 2 commits into from

Conversation

@paulidale
Copy link
Contributor

@paulidale paulidale commented Jul 13, 2020

The documentation was off by one for the length this function could return.
The wrap functions could exceed the limit by one.

  • documentation is added or updated
The documentation was off by one for the length this function could return.
@kroeckx
Copy link
Member

@kroeckx kroeckx commented Jul 14, 2020

Do you think it's useful to have the documentation say it's different for wrap ciphers?

@t8m
Copy link
Member

@t8m t8m commented Jul 14, 2020

Do you think it's useful to have the documentation say it's different for wrap ciphers?

Yeah, unless we can come up with some plausible case for other ciphers where it would happen as well?

Because otherwise we are basically making most existing uses of EVP_EncryptUpdate non-conforming to the documentation. Which is kind-of API break. Inevitable for wrap ciphers, yes, but not for the others.

@kroeckx
Copy link
Member

@kroeckx kroeckx commented Jul 14, 2020

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Jul 15, 2020

I guess the other exception is the stream ciphers. Maybe it's also useful to document if you pass it a multiple of the block size, you get exactly that amount back.

Is this true for the wrap ciphers?? The memmove(out + 8, in, inlen) in CRYPTO_128_wrap() seems to indicate otherwise.

@t8m
Copy link
Member

@t8m t8m commented Jul 15, 2020

I think Kurt meant regular block cipher modes.

So I believe we should document all the possible cases if we really want to make the documentation truly helpful and the API contract reasonable. It would be pretty bad if we required for all EVP_EncryptUpdate() calls to reserve inlen rounded up to blocksize + blocksize. Even if the underlying cipher mode is stream one and the extra blocksize is needed only for the wrap modes.

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Jul 16, 2020

I've added statements for most ciphers, wrapping and stream ciphers.

Ideally, we should document the behaviour for each cipher mode individually.

@t8m
t8m approved these changes Jul 16, 2020
Copy link
Member

@t8m t8m left a comment

LGTM

openssl-machine pushed a commit that referenced this pull request Jul 17, 2020
The documentation was off by one for the length this function could return.

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #12435)
openssl-machine pushed a commit that referenced this pull request Jul 17, 2020
The documentation was off by one for the length this function could return.

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #12435)

(cherry picked from commit 3fc164e)
@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Jul 17, 2020

Merged to 1.1.1 and master. Thanks for the feedback and suggestions.

@paulidale paulidale closed this Jul 17, 2020
@paulidale paulidale deleted the paulidale:evp-encryptupdate branch Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants