search

aws / aws-lc

AWS-LC is a general-purpose cryptographic library maintained by the AWS Cryptography team for AWS and their customers. It іs based on code from the Google BoringSSL project and the OpenSSL project.
Other
354 stars 111 forks source link

refactor md5 tool with dgst and fix stdin behavior #1766

Closed samuel40791765 closed 1 month ago

samuel40791765 commented 1 month ago

Issues:

Addresses CryptoAlg-2602

Description 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.

ubuntu in 🌐 ip-172-31-63-232 in aws-lc on  fix-md5-tool [=$+?] via △ v3.16.3 via 🐹 v1.20.4 
❯ echo "test" | openssl md5 
(stdin)= d8e8fca2dc0f896fd7cb4cb0031ba249

ubuntu in 🌐 ip-172-31-63-232 in aws-lc on  fix-md5-tool [=$+?] via △ v3.16.3 via 🐹 v1.20.4 
❯ 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. 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:

ubuntu in 🌐 ip-172-31-63-232 in aws-lc on  fix-md5-tool [$?] via △ v3.16.3 via 🐹 v1.20.4 
❯ echo "test_fix" | openssl md5
(stdin)= 1312e69e9dc6ec31b51af24fc42ebdea

ubuntu in 🌐 ip-172-31-63-232 in aws-lc on  fix-md5-tool [$?] via △ v3.16.3 via 🐹 v1.20.4 
❯ echo "test_fix" | ./test_build_dir/tool-openssl/openssl md5
(stdin)= 1312e69e9dc6ec31b51af24fc42ebdea

Call-outs:

Deleted the md5 files since most logic was moved under dgst.

Testing:

  1. New tests for hmac stdin
  2. New tests for md5 stdin and md5 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.

codecov-commenter commented 1 month ago

Codecov Report

Attention: Patch coverage is 2.96296% with 131 lines in your changes missing coverage. Please review.

Project coverage is 78.31%. Comparing base (0c52b12) to head (93ad99f).

Files Patch % Lines
tool-openssl/dgst.cc 0.00% 73 Missing :warning:
tool-openssl/dgst_test.cc 6.45% 58 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.