Initial rewrite of X509 STORE to replicate openssl behavior #8087
Conversation
|
Retest this please. |
JacobBarthelmeh
dismissed
SparkiDev’s stale review
Comments were added to the section of code
We do for direct usage of the CM, but I dont think anything for X509 STORE. |
| } | ||
| } | ||
|
|
||
| return ret == WOLFSSL_SUCCESS ? WOLFSSL_SUCCESS : WOLFSSL_FAILURE; |
There was a problem hiding this comment.
Can you confirm you only want to return success or failure and not any other possible return codes? Old logic was >= 0 success else failure.
@douzzer does this need to be WC_NO_ERR_TRACE(WOLFSSL_FAILURE), etc?
There was a problem hiding this comment.
Talked with douzzer, he said this should be OK.
There was a problem hiding this comment.
@ColtonWilley the first half of the question still stands -- do we really want to be masking the true return code here?
There was a problem hiding this comment.
Yes actually I believe we do. I believe openssl only returns 0 or 1, and uses X509_STORE_CTX_get_error for error reporting.
There was a problem hiding this comment.
OpenSSL returns -1 on an error, 0 on verification failure, and 1 on verification success. This rework conforms to that API.
src/x509_str.c
Outdated
| while (cnt > 0 && i > 0) { | ||
| /* The inner X509 is owned by somebody else, NULL out the reference */ | ||
| obj = wolfSSL_sk_X509_OBJECT_value(objs, i); | ||
| if (obj != NULL) { | ||
| obj->type = 0; | ||
| obj->data.x509 = NULL; | ||
| } | ||
| cnt--; | ||
| i--; | ||
| } |
There was a problem hiding this comment.
This could also be a crl so maybe better to use obj->data.ptr. Same effect just showing that we don't want to free anything the WOLFSSL_X509_OBJECT holds.
There was a problem hiding this comment.
In the future it would be nice to up ref what gets put into the object list so that we can just call pop_free but that is not necessary on this PR.
There was a problem hiding this comment.
Great catch on the CRLs. Agreed this whole thing could use a second pass at some point to take a close look at better using refcounts, but I think its large enough as is.
| * CA=TRUE */ | ||
| if (wolfSSL_X509_NAME_cmp(&x509->issuer, &x509->subject) == 0) { | ||
| result = X509StoreAddCa(store, x509, WOLFSSL_USER_CA); | ||
| #if !defined(WOLFSSL_SIGNER_DER_CERT) |
There was a problem hiding this comment.
Why !WOLFSSL_SIGNER_DER_CERT here?
There was a problem hiding this comment.
If !WOLFSSL_SIGNER_DER_CERT, CM internals do not hold onto the full DER buffer of the cert. In this case we need to keep a copy of it ourselves to meet requirements to return the chain. If WOLFSSL_SIGNER_DER_CERT, is defined, no need to keep a separate trusted stack as we can retrieve DER buffer from the underlying CM.
douzzer
force-pushed
the
x509_store_rewrite
branch
from
b02f145
to
a17ca40
Compare
…nal names, get rid of all lines over 80 chars
…ory leak for SSL case with proper flow
…layer X509_V_FLAG_PARTIAL_CHAIN; in src/x509_str.c, fix several C++ "invalid conversion" errors in X509StoreFreeObjList() and wolfSSL_X509_STORE_get0_objects().
douzzer
force-pushed
the
x509_store_rewrite
branch
from
a17ca40
to
cab20fb
Compare
|
retest this please |
Description
Rewrite the X509 STORE internals to replicate openssl behavior.
Fixes zd 18689
Testing
Added 9 additional test cases to api.c
Checklist