Skip to content
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

Enhance the log messages for authentication failures #1241

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

byshen
Copy link
Contributor

@byshen byshen commented May 8, 2020

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 skinkie self-requested a review May 8, 2020 06:13
@skinkie skinkie self-assigned this May 8, 2020
@skinkie
Copy link
Member

skinkie commented May 8, 2020

This looks indeed interesting and benifitial for others +1

@byshen
Copy link
Contributor Author

byshen commented Jul 17, 2020

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
Copy link
Member

skinkie commented Jul 17, 2020

@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
Copy link
Contributor Author

byshen commented Jul 17, 2020

@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
Copy link
Contributor Author

byshen commented Aug 31, 2020

@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
Copy link
Member

skinkie commented Sep 9, 2020

@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
Copy link
Contributor Author

byshen commented Sep 9, 2020

@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
Copy link
Member

skinkie commented Sep 10, 2020

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

@byshen
Copy link
Contributor Author

byshen commented Sep 10, 2020

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants