add basic support for dgst hmac in tool #1755
Conversation
5d8c8e4 to
cd43457
Compare
cd43457 to
2e560b9
Compare
2e560b9 to
bebd895
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1755 +/- ##
==========================================
- Coverage 78.46% 78.32% -0.14%
==========================================
Files 580 582 +2
Lines 96779 96906 +127
Branches 13865 13884 +19
==========================================
- Hits 75936 75902 -34
- Misses 20226 20381 +155
- Partials 617 623 +6 ☔ View full report in Codecov by Sentry. |
tool-openssl/dgst_test.cc
Outdated
| awslc_hash = trim(awslc_hash); | ||
| openssl_hash = trim(openssl_hash); |
There was a problem hiding this comment.
What's with the trims? Do we have differing whitespace?
There was a problem hiding this comment.
Also how does this work if there are two files getting hashed? Is this only comparing the output of one of them?
There was a problem hiding this comment.
This was due to different OpenSSL versions having different outputs, but now that you mention it I don't think it applies here. Will change.
Also a reminder that I forgot to add these tests to the test script.
tool-openssl/dgst.cc
Outdated
| for (;;) { | ||
| size_t n; | ||
| if (!ReadFromFD(fd, &n, buf.get(), kBufSize)) { | ||
| fprintf(stderr, "%s: No such file or directory\n", filename.c_str()); |
There was a problem hiding this comment.
NP: There was an error reading from the file, which doesn't necessarily mean that there's "no such file". (Could you even read from a directory?)
There was a problem hiding this comment.
Oh this was to align with OpenSSL's error messaging, I don't really enjoy it either.. I'm conflicted on compatibility everywhere or better error messages in general.
I can change this to my original error message which also prints the errno. It's not really a 1 way door and we can change to align if we get complaints.
| } | ||
|
|
||
| // Print HMAC output. | ||
| fprintf(stdout, "HMAC-%s(%s)= ", EVP_MD_get0_name(digest), |
There was a problem hiding this comment.
NP: This would display the full path. Is that expected?
There was a problem hiding this comment.
Yes that's expected. OpenSSL's output example:
aws-lc on main [?] via △ v3.20.0 via v1.21.3
❯ openssl dgst $PWD/../bio_test
SHA256(/Users/sachiang/Documents/repo/aws-lc/../bio_test)= 28176a1f8de724adeb381febacf90595715b55a15294c14c24a3e5f96ced04dc
| fprintf(stderr, "Unknown option '%s'.\n", option.c_str()); | ||
| return false; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
I think this else block is only hit when arg.empty() -- I'm not sure the error message below this applies.
There was a problem hiding this comment.
The intention was to hit this if arg[0] != '-'. But true, that it doesn't really apply if arg.empty(). Will reconfigure this.
b4bc8a8 to
a156ce3
Compare
| @@ -39,10 +39,7 @@ declare -A openssl_branches=( | |||
| export AWSLC_TOOL_PATH="${BUILD_ROOT}/tool-openssl/openssl" | |||
| for branch in "${!openssl_branches[@]}"; do | |||
| export OPENSSL_TOOL_PATH="${install_dir}/openssl-${branch}/bin/openssl" | |||
| LD_LIBRARY_PATH="${install_dir}/openssl-${branch}/${openssl_branches[$branch]}" | |||
| for test in X509ComparisonTest RSAComparisonTest MD5ComparisonTest; do | |||
There was a problem hiding this comment.
Nice, this is much simpler to add more tests in the future automatically.
OpenSSL's hmac command line tool uses `dgst` with a `-hmac` option. Supporting the entire functionality of `dgst` will take a bit more work, but we can support `-hmac` with the default `SHA256` for the time being. 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 9178c62)
OpenSSL's hmac command line tool uses `dgst` with a `-hmac` option. Supporting the entire functionality of `dgst` will take a bit more work, but we can support `-hmac` with the default `SHA256` for the time being. 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 9178c62)
Issues:
Resolves
CryptoAlg-2602Description of changes:
OpenSSL's hmac command line tool uses
dgstwith a-hmacoption. Supporting the entire functionality ofdgstwill take a bit more work, but we can support-hmacwith the defaultSHA256for the time being.Call-outs:
Rearranged some of the header file function calls to make things cleaner
Testing:
Basic functionality tests against single and multiple /tmp 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.