refactor md5 tool with dgst and fix stdin behavior #1766
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
justsmth
reviewed
Aug 14, 2024
samuel40791765
force-pushed
the
fix-md5-tool
branch
from
f37c4d3 to
f4d4970
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1766 +/- ##
=========================================
+ Coverage 0 78.31% +78.31%
=========================================
Files 0 580 +580
Lines 0 96981 +96981
Branches 0 13901 +13901
=========================================
+ Hits 0 75946 +75946
- Misses 0 20413 +20413
- Partials 0 622 +622 ☔ View full report in Codecov by Sentry. |
justsmth
previously approved these changes
Aug 14, 2024
andrewhop
previously approved these changes
Aug 14, 2024
samuel40791765
dismissed stale reviews from andrewhop and justsmth
via
93ad99f
justsmth
approved these changes
Aug 14, 2024
andrewhop
approved these changes
Aug 14, 2024
samuel40791765
added a commit
to samuel40791765/aws-lc
that referenced
this pull request
Aug 14, 2024
Our original md5 stdin behavior was acting weird. The issue seems to be that we weren't actually parsing all of the input from `stdin`. ``` ❯ echo "test" | openssl md5 (stdin)= d8e8fca2dc0f896fd7cb4cb0031ba249 ❯ echo "test" | ./test_build_dir/tool-openssl/openssl md5 (stdin)= 098f6bcd4621d373cade4e832627b4f6 ``` The original implementation used `getline` which only reads one line. This worked with the original test since the file contents were sent into `stdin` with `<`, but this doesn't work correctly with other `stdin` behaviors such as `echo test | openssl md5`. The correct and more useful way to handle `stdin` is to read it from [the `0` file descriptor.](https://en.wikipedia.org/wiki/File_descriptor) This is how OpenSSL/BoringSSL reads from files. 1. This change fixes the behavior by rebuilding `md5` onto our `dgst` implementation. This is identical to how OpenSSL factors the specific hashing tool names. `md5` now uses `dgst` under the hood and inherits the file hashing behavior we implemented for `dgst hmac`. 2. `stdin` was missing from `dgst`, so that was implemented as well. `stdin` now works for both `openssl dgst -hmac ...` and `openssl md5`. Corresponding tests have also been added to verify. 3. Code was structured so that we can easily extend new digest support for the openssl tool with just a few lines. I considered adding it along with this PR, but didn't want to muddle it with the new logic. FIxed version: ``` ❯ echo "test_fix" | openssl md5 (stdin)= 1312e69e9dc6ec31b51af24fc42ebdea ❯ echo "test_fix" | ./test_build_dir/tool-openssl/openssl md5 (stdin)= 1312e69e9dc6ec31b51af24fc42ebdea ``` By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license. (cherry picked from commit 52d1651)
samuel40791765
added a commit
that referenced
this pull request
Aug 15, 2024
Our original md5 stdin behavior was acting weird. The issue seems to be that we weren't actually parsing all of the input from `stdin`. ``` ❯ echo "test" | openssl md5 (stdin)= d8e8fca2dc0f896fd7cb4cb0031ba249 ❯ echo "test" | ./test_build_dir/tool-openssl/openssl md5 (stdin)= 098f6bcd4621d373cade4e832627b4f6 ``` The original implementation used `getline` which only reads one line. This worked with the original test since the file contents were sent into `stdin` with `<`, but this doesn't work correctly with other `stdin` behaviors such as `echo test | openssl md5`. The correct and more useful way to handle `stdin` is to read it from [the `0` file descriptor.](https://en.wikipedia.org/wiki/File_descriptor) This is how OpenSSL/BoringSSL reads from files. 1. This change fixes the behavior by rebuilding `md5` onto our `dgst` implementation. This is identical to how OpenSSL factors the specific hashing tool names. `md5` now uses `dgst` under the hood and inherits the file hashing behavior we implemented for `dgst hmac`. 2. `stdin` was missing from `dgst`, so that was implemented as well. `stdin` now works for both `openssl dgst -hmac ...` and `openssl md5`. Corresponding tests have also been added to verify. 3. Code was structured so that we can easily extend new digest support for the openssl tool with just a few lines. I considered adding it along with this PR, but didn't want to muddle it with the new logic. FIxed version: ``` ❯ echo "test_fix" | openssl md5 (stdin)= 1312e69e9dc6ec31b51af24fc42ebdea ❯ echo "test_fix" | ./test_build_dir/tool-openssl/openssl md5 (stdin)= 1312e69e9dc6ec31b51af24fc42ebdea ``` By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license. (cherry picked from commit 52d1651)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issues:
Addresses
CryptoAlg-2602Description of changes:
Our original md5 stdin behavior was acting weird. The issue seems to be that we weren't actually parsing all of the input from
stdin.The original implementation used
getlinewhich only reads one line. This worked with the original test since the file contents were sent intostdinwith<, but this doesn't work correctly with otherstdinbehaviors such asecho test | openssl md5.The correct and more useful way to handle
stdinis to read it from the0file descriptor. This is how OpenSSL/BoringSSL reads from files.md5onto ourdgstimplementation. This is identical to how OpenSSL factors the specific hashing tool names.md5now usesdgstunder the hood and inherits the file hashing behavior we implemented fordgst hmac.stdinwas missing fromdgst, so that was implemented as well.stdinnow works for bothopenssl dgst -hmac ...andopenssl md5. Corresponding tests have also been added to verify.FIxed version:
Call-outs:
Deleted the md5 files since most logic was moved under
dgst.Testing:
hmac stdinmd5 stdinandmd5 files.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.