-
Notifications
You must be signed in to change notification settings - Fork 162
openssl-ca command implementation for self-sign certificates #2937
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 98. Check the log or trigger a new build to see more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 88. Check the log or trigger a new build to see more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 72. Check the log or trigger a new build to see more.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2937 +/- ##
==========================================
- Coverage 78.21% 78.07% -0.15%
==========================================
Files 690 685 -5
Lines 118750 119914 +1164
Branches 16681 16948 +267
==========================================
+ Hits 92885 93623 +738
- Misses 24976 25400 +424
- Partials 889 891 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 62. Check the log or trigger a new build to see more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 42. Check the log or trigger a new build to see more.
e543f78 to
4921f38
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 32. Check the log or trigger a new build to see more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 17. Check the log or trigger a new build to see more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
54b7abb to
b3f75e6
Compare
| #endif | ||
|
|
||
| // Windows compatibility layer | ||
| #ifdef _WIN32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#if defined(OPENSSL_WINDOWS)
| using ossl_uint8_ptr = std::unique_ptr<uint8_t, ossl_free>; | ||
| using ossl_char_ptr = std::unique_ptr<char, ossl_free>; | ||
|
|
||
| #ifdef _WIN32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#if defined(OPENSSL_WINDOWS)
| #include <cstring> | ||
| #include <memory> | ||
|
|
||
| // TODO: figure out windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment still needed?
| } | ||
| } | ||
| k = 0; | ||
| i -= again; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there a i -= again? It's possible that the loop on lines 90-96 had stopped prior to reaching the '\' character, so why decrease i based upon a character that exists potentially beyond the current position of i?
| } | ||
| i /= 2; | ||
| if (num + i > slen) { | ||
| sp = OPENSSL_realloc(s, num + i * 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NP: Allocating num + i * 2 bytes is unnecessary, since we only need num + i bytes to be allocated. This leads to 1/2 fewer allocations, but it also means we always over-allocate beyond what's needed.
| if (!OPENSSL_fromxdigit(&hex, bufp[k + n])) { | ||
| OPENSSL_PUT_ERROR(ASN1, ASN1_R_NON_HEX_CHARACTERS); | ||
| goto err; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NP: This can (should?) never fail b/c of the loop on lines 90-96 truncates on any character that might fail. It's fine to leave, but I wonder where we should rethink the loop above.
| #include <openssl/bio.h> | ||
| #include <openssl/err.h> | ||
|
|
||
| int a2i_ASN1_INTEGER(BIO *bp, ASN1_INTEGER *bs, char *buf, int size) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment above this function documenting what it expects from the input (and how it handles variance from expectations) would be helpful.
Description of changes:
Adds a fairly limited
openssl cacommand that is targeted towards supporting the specific efs-utils use case for generating and self-signing a certificate.Call-outs:
openssl cahas a series of "database" files that it record and tracks metadata in. Some of the behaviors like revocation checking is retained even though generation of revocations is not supported.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.