Add req to OpenSSL CLI tool #2284
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2284 +/- ##
==========================================
- Coverage 79.05% 78.76% -0.30%
==========================================
Files 614 616 +2
Lines 107042 107519 +477
Branches 15161 15283 +122
==========================================
+ Hits 84619 84683 +64
- Misses 21771 22180 +409
- Partials 652 656 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if (*subject_name_ptr == '\\' && *(subject_name_ptr + 1)) { | ||
| // Handle escaped character | ||
| subject_name_ptr++; | ||
| value += *subject_name_ptr++; |
There was a problem hiding this comment.
Isn't this just adding the escaped character as if it was not escaped? That is "\nhello" would parse as "nhello" not "hello"? What's expected?
There was a problem hiding this comment.
So if you wanted to escape \n, you would say "\\nhello", then we would skip over the first two "\" and provide "\nhello" as the name.
You can see an example of what escaping looks like here: https://github.com/aws/aws-lc/pull/2284/files#diff-d581491b3de79e020c39ed0c9e09ff4c8babd8a473966bf39103f1736418c5abR375
There was a problem hiding this comment.
NP: I was assuming the input was simply "\nhello".
This condition drops the first escape \ character but always keeps whatever the following character is regardless... unless it's at the end of the string, in which case it keeps the escape character.
Is that the intended behavior?
There was a problem hiding this comment.
Yes I believe this is intended.
If the input string contains a new line character (ASCII 10), then the parsing logic would not see a backslash and this condition would not be triggered.
But if you pass in \nhello as a string literal, then you must escape the provided backslash with another backslash to indicate "\n" is intended not just "n". Then this code would correctly parse it.
Also since this is for fields in the subject, users should technically not be passing in new line symbols.
| if (*subject_name_ptr == '\\' && *(subject_name_ptr + 1)) { | ||
| // Handle escaped character | ||
| subject_name_ptr++; | ||
| value += *subject_name_ptr++; |
There was a problem hiding this comment.
NP: I was assuming the input was simply "\nhello".
This condition drops the first escape \ character but always keeps whatever the following character is regardless... unless it's at the end of the string, in which case it keeps the escape character.
Is that the intended behavior?
tool-openssl/req.cc
Outdated
| const char* value = nullptr; | ||
| if (strncmp(buffer, ".", sizeof(buffer)) == 0) { | ||
| // Empty entry requested | ||
| value = ""; | ||
| } else if (len == 0 && field.default_value) { |
There was a problem hiding this comment.
Is it possible to leave every field empty? Do we need to do any input validation to avoid creating invalid requests?
There was a problem hiding this comment.
Ah good call out, OpenSSL did this validation in a deeper layer of their req tooling (they handle reading from conf/user in a different part of the application).
We should be verifying there is at least one subject field added should the user ask for every field to be empty. I have added this additional check.
Issues:
CryptoAlg-2992Description of changes:
Add the req tool. Only some options are supported. We don't support the default config file interface that OpenSSL does, default values are hardcoded where appropriate.
This PR also modified the createTempDirPath utility function. This func previously had a race condition causing intermittent CI failures.
Testing:
Unit test for parsing subject function. OpenSSL comparison tests for CSR and Cert generation with cross-checking of attributes.
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.