-
Notifications
You must be signed in to change notification settings - Fork 914
Fix qt jenkins nightly test failure #9584
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: master
Are you sure you want to change the base?
Conversation
|
🛟 Devin Lifeguard found 3 likely issues in this PR
@miyazakh |
|
retest this please
|
| ret = ASN_BEFORE_DATE_E; | ||
| } | ||
| } | ||
| #if defined(OPENSSL_ALL) || defined(WOLFSSL_QT) |
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.
We are moving away from port specific code in wolfSSL. WOLFSSL_QT already enables OPENSSL_ALL.
| #if defined(OPENSSL_ALL) || defined(WOLFSSL_QT) | |
| #if defined(OPENSSL_ALL) |
| #endif | ||
| } | ||
|
|
||
| #if !defined(NO_ASN_TIME) && (defined(OPENSSL_ALL) || defined(WOLFSSL_QT)) |
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(NO_ASN_TIME) && (defined(OPENSSL_ALL) || defined(WOLFSSL_QT)) | |
| #if !defined(NO_ASN_TIME) && defined(OPENSSL_ALL) |
| #if !defined(NO_ASN_TIME) && (defined(OPENSSL_ALL) || defined(WOLFSSL_QT)) | ||
| if (ret != WC_NO_ERR_TRACE(ASN_BEFORE_DATE_E) && | ||
| ret != WC_NO_ERR_TRACE(ASN_AFTER_DATE_E)) { | ||
| /* With Qt and OpenSSL, we need to check the certificate's date |
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.
| /* With Qt and OpenSSL, we need to check the certificate's date | |
| /* With OpenSSL, we need to check the certificate's date |
| ret = X509StoreVerifyCertDate(ctx, ret); | ||
| SetupStoreCtxError(ctx, ret); | ||
| if (ctx->store->verify_cb) | ||
| ret = ctx->store->verify_cb(ret >= 0 ? 1 : 0, |
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.
Shouldn't only ret == 0 be success?
| ret = ctx->store->verify_cb(ret >= 0 ? 1 : 0, | ||
| ctx) == 1 ? WOLFSSL_SUCCESS : -1; |
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.
This is much more readable.
| ret = ctx->store->verify_cb(ret >= 0 ? 1 : 0, | |
| ctx) == 1 ? WOLFSSL_SUCCESS : -1; | |
| ret = ret == 0 ? 1 : 0 | |
| if (ctx->store->verify_cb(ret, ctx) == 1) | |
| ret = WOLFSSL_SUCCESS; | |
| else | |
| ret = -1; |
| static int X509Callback(int ok, X509_STORE_CTX *ctx) | ||
| { | ||
|
|
||
| if (!ok) { | ||
| last_errcode[err_index] = X509_STORE_CTX_get_error(ctx); | ||
| last_errdepth[err_index++] = X509_STORE_CTX_get_error_depth(ctx); | ||
| } | ||
| /* Always return OK to allow verification to continue.*/ | ||
| return 1; | ||
| } |
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.
Add some bounds check to err_index.
| static int X509Callback(int ok, X509_STORE_CTX *ctx) | ||
| { | ||
|
|
||
| if (!ok) { | ||
| last_errcode[err_index] = X509_STORE_CTX_get_error(ctx); | ||
| last_errdepth[err_index++] = X509_STORE_CTX_get_error_depth(ctx); | ||
| } | ||
| /* Always return OK to allow verification to continue.*/ | ||
| return 1; | ||
| } |
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.
Write a new callback and place it next to relevant tests. Keep X509Callback as is for test_X509_STORE_InvalidCa and write a new callback function for test_wolfSSL_X509_STORE_check_time. This will make it easier to manage.
| #include <tests/api/api.h> | ||
| #include <tests/api/test_ossl_x509_str.h> | ||
|
|
||
| #if (defined(OPENSSL_ALL) || defined(WOLFSSL_QT)) && \ |
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_ALL) || defined(WOLFSSL_QT)) && \ | |
| #if defined(OPENSSL_ALL) && \ |
| wolfSSL_X509_free(cert); | ||
| cert = NULL; | ||
|
|
||
| #if (defined(OPENSSL_ALL) || defined(WOLFSSL_QT)) && \ |
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_ALL) || defined(WOLFSSL_QT)) && \ | |
| #if defined(OPENSSL_ALL) && \ |
Description
Qt nightly Jenkins tests started failing after PR9522 was merged. OpenSSL performs date validation even when there are other verification issues. This PR reverts the removed logic and now uses X509StoreVerifyCertDate().
Testing
Add unit test
Run qt unit test
Checklist