Skip to content

Enhance the log messages for authentication failures#1241

Open
byshen wants to merge 4 commits into
cherokee:masterfrom
byshen:master
Open

Enhance the log messages for authentication failures#1241
byshen wants to merge 4 commits into
cherokee:masterfrom
byshen:master

Conversation

@byshen

@byshen byshen commented May 8, 2020

Copy link
Copy Markdown
Contributor

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

skinkie commented May 8, 2020

Copy link
Copy Markdown
Member

This looks indeed interesting and benifitial for others +1

@byshen

byshen commented Jul 17, 2020

Copy link
Copy Markdown
Contributor Author

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

skinkie commented Jul 17, 2020

Copy link
Copy Markdown
Member

@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

byshen commented Jul 17, 2020

Copy link
Copy Markdown
Contributor Author

@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

byshen commented Aug 31, 2020

Copy link
Copy Markdown
Contributor Author

@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

skinkie commented Sep 9, 2020

Copy link
Copy Markdown
Member

@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

byshen commented Sep 9, 2020

Copy link
Copy Markdown
Contributor Author

@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

skinkie commented Sep 10, 2020

Copy link
Copy Markdown
Member

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

@byshen

byshen commented Sep 10, 2020

Copy link
Copy Markdown
Contributor Author

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