-
-
Notifications
You must be signed in to change notification settings - Fork 34.7k
gh-150101: Expose OpenSSL's error queue through SSLError.error_queue #150103
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?
Changes from all commits
5c0de5f
6900187
83591f2
28e1f40
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1909,6 +1909,7 @@ def test_lib_reason(self): | |
| self.assertEqual(cm.exception.library, 'PEM') | ||
| regex = "(NO_START_LINE|UNSUPPORTED_PUBLIC_KEY_TYPE)" | ||
| self.assertRegex(cm.exception.reason, regex) | ||
| self.assertTrue(len(cm.exception.error_queue) >= 1) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. assertGreaterEqual |
||
| s = str(cm.exception) | ||
| self.assertIn("NO_START_LINE", s) | ||
|
|
||
|
|
@@ -4688,6 +4689,7 @@ def cb_returning_alert(ssl_sock, server_name, initial_context): | |
| chatty=False, | ||
| sni_name='supermessage') | ||
| self.assertEqual(cm.exception.reason, 'TLSV1_ALERT_ACCESS_DENIED') | ||
| self.assertTrue(len(cm.exception.error_queue) >= 1) | ||
|
|
||
| def test_sni_callback_raising(self): | ||
| # Raising fails the connection with a TLS handshake failure alert. | ||
|
|
@@ -4708,6 +4710,7 @@ def cb_raising(ssl_sock, server_name, initial_context): | |
| "|SSLV3_ALERT_HANDSHAKE_FAILURE" | ||
| "|NO_PRIVATE_VALUE)") | ||
| self.assertRegex(cm.exception.reason, regex) | ||
| self.assertTrue(len(cm.exception.error_queue) >= 1) | ||
| self.assertEqual(catch.unraisable.exc_type, ZeroDivisionError) | ||
|
|
||
| def test_sni_callback_wrong_return_type(self): | ||
|
|
@@ -4727,6 +4730,7 @@ def cb_wrong_return_type(ssl_sock, server_name, initial_context): | |
|
|
||
|
|
||
| self.assertEqual(cm.exception.reason, 'TLSV1_ALERT_INTERNAL_ERROR') | ||
| self.assertTrue(len(cm.exception.error_queue) >= 1) | ||
| self.assertEqual(catch.unraisable.exc_type, TypeError) | ||
|
|
||
| def test_shared_ciphers(self): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Expose OpenSSL's error queue through SSLError.error_queue |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -513,6 +513,8 @@ fill_and_set_sslerror(_sslmodulestate *state, | |
| PyObject *verify_obj = NULL, *verify_code_obj = NULL; | ||
| PyObject *init_value, *msg, *key; | ||
| PyUnicodeWriter *writer = NULL; | ||
| PyObject *error_queue = PyList_New(0); | ||
| if (!error_queue) goto fail; | ||
|
|
||
| if (ssl_errno == PY_SSL_ERROR_EOF && sslsock != NULL) { | ||
| sslsock->got_eof_error = 1; | ||
|
|
@@ -542,6 +544,45 @@ fill_and_set_sslerror(_sslmodulestate *state, | |
| if (errstr == NULL) { | ||
| errstr = ERR_reason_error_string(errcode); | ||
| } | ||
|
|
||
| /* populate error_list from OpenSSL's error queue */ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. error_queue? |
||
| unsigned int q_pos = 0; /* Error queue position */ | ||
| const char *eq_filename, *eq_func, *eq_data; | ||
| char eq_msg[256]; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather we did not put large things on the stack. Have it be a pointer set to NULL here. allocate space within the while loop if null before we use it. free it unconditionally when not NULL later on the ways out of the function. |
||
| int eq_lineno; | ||
| int flags; /* Flags (discarded) */ | ||
| unsigned long openssl_errorcode; | ||
|
|
||
| /* Presumably, we no longer need the OpenSSL error queue after this, so | ||
| we can call ERR_get_error (destructive) instead of ERR_peek_error */ | ||
| while ((openssl_errorcode = ERR_get_error_all(&eq_filename, &eq_lineno, &eq_func, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is OpenSSL >= 3.x only - we also work with some 1.1.1-ish forks such as AWS-LC and can still build and link against 1.1.1 itself so conditional compilation is likely needed here. |
||
| &eq_data, &flags))) { | ||
| if (q_pos == 0) { | ||
| /* errcode should have come from a caller, and should have been | ||
| returned from ERR_peek_last_error() */ | ||
| assert(openssl_errorcode == errcode); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this correct? oldest first in one list vs value in errcode being the last one when there are multiple. |
||
| } | ||
|
|
||
| ERR_error_string_n(openssl_errorcode, eq_msg, 256); | ||
|
|
||
| // Follows [lib reason] error_string extra_data (OpenSSL file:func:line) | ||
| PyObject *current_eq_msg = NULL; | ||
| if (eq_data != NULL) { | ||
| current_eq_msg = PyUnicode_FromFormat( | ||
| "%s %s (%s:%s:%d)", | ||
| eq_msg, eq_data, eq_filename, eq_func, eq_lineno | ||
| ); | ||
| } else { | ||
| current_eq_msg = PyUnicode_FromFormat( | ||
| "%s (%s:%s:%d)", | ||
| eq_msg, eq_filename, eq_func, eq_lineno | ||
| ); | ||
| } | ||
| if (PyList_Append(error_queue, current_eq_msg) != 0) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. decref current_eq_msg after this (regardless of success or failure) to avoid a memory leak. |
||
| goto fail; | ||
| } | ||
| q_pos++; | ||
| } | ||
| } | ||
|
|
||
| /* verify code for cert validation error */ | ||
|
|
@@ -655,6 +696,12 @@ fill_and_set_sslerror(_sslmodulestate *state, | |
| goto fail; | ||
| } | ||
|
|
||
| /* Add the full OpenSSL error queue to exception */ | ||
| if (PyObject_SetAttr(err_value, state->str_error_queue, error_queue) != 0) { | ||
| goto fail; | ||
| } | ||
| Py_DECREF(error_queue); | ||
|
|
||
| PyErr_SetObject(type, err_value); | ||
| fail: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fail: needs to Py_XDECREF(error_queue); |
||
| Py_XDECREF(err_value); | ||
|
|
@@ -7359,6 +7406,10 @@ sslmodule_init_strings(PyObject *module) | |
| if (state->str_verify_code == NULL) { | ||
| return -1; | ||
| } | ||
| state->str_error_queue = PyUnicode_InternFromString("error_queue"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a relevant Py_CLEAR(state->...) for this. |
||
| if (state->str_error_queue == NULL) { | ||
| return -1; | ||
| } | ||
| return 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.
FYI we spell this "versionadded:: next" and the docs automation replaces it with the right version at the right time. (don't worry about it, 3.16 is correct)