cherokee / webserver

Cherokee Web Server
GNU General Public License v2.0
568 stars 105 forks source link

Enhance the log messages for authentication failures #1241

Open byshen opened 4 years ago

byshen commented 4 years ago

I hope you are safe and well in this difficult times.

In the cherokee_connection_check_authentication() function, Cherokee will check the authorization info in the connection during the setup stage. When the authentication fails, there is little info about how it failed. (e.g. wrong authentication type, invalid user, or wrong password.) All the messages are simply 401 unauthorized errors.

I feel it would be beneficial to create some more hints for the sysadmins to know how the authentication fails when users were denied access. So I created some messages inside the authorization function, before it returns an unauthorized error.

Any thoughts are appreciated!

skinkie commented 4 years ago

This looks indeed interesting and benifitial for others +1

byshen commented 4 years ago

Hi Stefan,

I hope you are doing well!

Is it possible for you to help me review this patch? Any feedbacks are greatly appreciated! I also find several other log messages that may help with debugging the access-denied issues. I will submit more patches if that helps :)

Best, Bingyu

skinkie commented 4 years ago

@byshen there is one thing related to the authentication code that must be done first. But while fixing that I stumbled on something that I don't like.

byshen commented 4 years ago

@byshen there is one thing related to the authentication code that must be done first. But while fixing that I stumbled on something that I don't like.

Is there something that I can help with? Thank you.

Bingyu

byshen commented 4 years ago

@skinkie Hi, I added some more messages related to access control checks. Could you please help me review this patch? Any feedbacks are appreciated!

skinkie commented 4 years ago

@byshen this is what I get using fixed list authentication. I don't think this is what we want to confuse our users with. My suggestion; I think you should this approach this from a CHEROKEE_TRACE perspective.

{'type': "error", 'time': "09/09/2020 22:39:51.385", 'title': "Could not get authentication information from the header", 'code': "connection.c:2313", 'error': "188", 'description': "It looks like you've hit a bug in the server. Please, do not hesitate to report it at http://bugs.cherokee-project.com/ so  the developer team can fix it.", 'version': "1.2.104", 'compilation_date': "Sep  9 2020 22:35:07", 'configure_args': " '--prefix=/tmp/byshen' '--with-python=/usr/bin/python2' '--prefix=/tmp/byshen' '--with-python=/usr/bin/python2'", 'backtrace': ""}
byshen commented 4 years ago

@byshen this is what I get using fixed list authentication. I don't think this is what we want to confuse our users with. My suggestion; I think you should this approach this from a CHEROKEE_TRACE perspective.

Thanks for the update. I changed the descriptions to make log message more appropriate (obviously it is not a CODING_BUG, my apologies). 624fd0c

I was also thinking about using TRACE() to log this, but it seems that TRACE is usually not enabled in production, where the log can be more helpful to debug the authentication failures. As the cherokee_connection_check_authentication() fails rarely, it will not add much overhead.

If you feel TRACE() is a better option, I can switch the added logs to TRACE() with another PR. Thank you!

skinkie commented 4 years ago

In my perspective if you are debugging something it should not be enabled permanently.

byshen commented 4 years ago

Thanks for the feedback. I raised another PR #1246 to change the messages to TRACE(). I also changed the message for cherokee_mkdir_p_perm() a bit to make it more accurate about the permissions.

Please let me know if that is more appropriate. Thank you!