1452075: print only readable SSL error to console #1643
Conversation
jirihnidek
changed the title
jirihnidek
force-pushed
the
jhnidek/1452075
branch
from
3e62082
to
8cbe4df
Compare
| @@ -74,7 +75,14 @@ def format_bad_ca_cert_exception(self, bad_ca_cert_error, message_template): | |||
| return message_template % bad_ca_cert_error.cert_path | |||
|
|
|||
| def format_ssl_error(self, ssl_error, message_template): | |||
| return message_template % str(ssl_error) | |||
| # SSL error has fixed form: [SSL/BIO/...: ERRORO_ID] readable string (file.c:#line) | |||
There was a problem hiding this comment.
Please revert the change to this method. It's okay to show the user the full error message, as long as we give them a human-readable message too.
There was a problem hiding this comment.
We write more details to rhsm.log in handle_exception(). I vote for don't reverting this part :-). I thing main concern of bug reporter was style of error message.
There was a problem hiding this comment.
This is hacky, imagine what happens if OpenSSL re-formats its error messages, for example, if they were to emit something like [SSL: SSLV3_ALERT_CERTIFICATE_UNKNOWN] sslv3 alert certificate unknown [subject=CN=example.com] (_ssl.c:579), then the regex produces the empty string. We can't really predict how SSL handling might change. Already we are at the mercy of two libraries: python standard library ssl and m2crypto. Let's not couple ourselves to any particular SSL implementation anymore than we have to.
There was a problem hiding this comment.
You are right. Regular expressions are hungry. It can be solved using modification of regular expression to something like this: output_error = re.sub('^\[[^\]]*\]|\([^\)]*\)$', '', str(ssl_error)), but when I see it, I start to be little bit scared of this :-). Golden rule: do not use regular expression, when it becomes too complicated. Thus I will revert it.
BTW: OpenSSL is very conservative in changing error messages.
| # debugging this use case (mapped message is misleading in this use case) | ||
| handle_exception("Unregister failed: %s", e, map_message=False) | ||
| # are different. | ||
| handle_exception("Unregister failed: %s", e, map_message=True) |
There was a problem hiding this comment.
With this change, there are no instances of handle_exception called with map_message=False, so please remove the map_message logic introduced in 217c386.
|
When I will do both changes you requested, then there will not left anything in this PR :-). Shouldn't we close BZ report as not a bug then? |
|
@jirihnidek, There will be a change, it will just a few lines though:
and then verify that the result looks sane. |
|
Let me, know if the last commit is OK. I will squash commits, when there will be no request on change. |
| @@ -74,7 +75,14 @@ def format_bad_ca_cert_exception(self, bad_ca_cert_error, message_template): | |||
| return message_template % bad_ca_cert_error.cert_path | |||
|
|
|||
| def format_ssl_error(self, ssl_error, message_template): | |||
| return message_template % str(ssl_error) | |||
| # SSL error has fixed form: [SSL/BIO/...: ERRORO_ID] readable string (file.c:#line) | |||
There was a problem hiding this comment.
This is hacky, imagine what happens if OpenSSL re-formats its error messages, for example, if they were to emit something like [SSL: SSLV3_ALERT_CERTIFICATE_UNKNOWN] sslv3 alert certificate unknown [subject=CN=example.com] (_ssl.c:579), then the regex produces the empty string. We can't really predict how SSL handling might change. Already we are at the mercy of two libraries: python standard library ssl and m2crypto. Let's not couple ourselves to any particular SSL implementation anymore than we have to.
|
@kahowell I reverted polishing of OpenSSL error. Let me know if the PR is OK. I will squash commits then. |
|
@jirihnidek looks good. Please squash. |
* Bug fix: https://bugzilla.redhat.com/show_bug.cgi?id=1452075 * More decriptive error message * Removed map_message argument from handle_exception()
jirihnidek
force-pushed
the
jhnidek/1452075
branch
from
036c7a8
to
24a3a91
Compare
|
Squashed. |
map_message to have nice error message.