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

Add log message when the IP is not allowed #1264

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

byshen
Copy link
Contributor

@byshen byshen commented May 2, 2021

Before return access-denied, leave a message in the server log about the specific reason.

@skinkie
Copy link
Member

skinkie commented May 2, 2021

Your pull requests add logging, but at this moment I wonder, does it add any information? What can you do with seeing for example this message?

@byshen
Copy link
Contributor Author

byshen commented May 2, 2021

Currently, it first checks whether the connection's IP is allowed. If not, it will simply return an access-denied 403 error code. (If there is any other information, please let me know :D )

However, there are multiple checks in the phase_setup_connection phase. It would be hard to know why the connection fails with a 403. With the new information, it clearly points the admins to the IP related configs.

@skinkie
Copy link
Member

skinkie commented May 2, 2021

But wouldn't it be much more informative to have such information as part of the normal http logging? So we directly know which IP was not allowed? Maybe I am missing something.

@byshen
Copy link
Contributor Author

byshen commented May 12, 2021

Sorry for the late reply, I was swamped by other stuff last week ..

But wouldn't it be much more informative to have such information as part of the normal http logging?

The normal logging in cherokee access log already logs the information like IP and status, but it does not have other information in the error log. From the 403 status we know it is denied, but we don't know if it's because of the IP. If the error log has hints like an error message, people would clearly know that.

So we directly know which IP was not allowed?

I can also add the IP address to the log if that's preferred.

Thanks!

@skinkie
Copy link
Member

skinkie commented May 12, 2021

I can also add the IP address to the log if that's preferred.

My main question was why should we do this in two places. Both access log and error log. I do want to prevent that the system becomes bloated because of that, because it could become noisy. Now if I review the apache error logs, I notice that a log level defines what and when goes through the error log, by a format one can add all the information to make the error logs useful. Something we only have when tracing. I think we should have a chat on what to do with these specific errors, in my perspective they are not defects of the regular webserver processing.

@byshen
Copy link
Contributor Author

byshen commented May 12, 2021

they are not defects of the regular webserver processing.

I agree, these errors are expected based on the current webserver configuration, but it could be difficult to troubleshoot if the configuration is not intended, i.e., we want to understand why it is denied.

a log level defines what and when goes through the error log, by a format one can add all the information to make the error logs useful. Something we only have when tracing

Ah I got it. I add it as error level log because in Apache httpd, these configuration errors are logged at ERROR level. For example, one example here. I don't feel that will make the log bloated, because most requests do not result in a 403 status and will not be logged.

In my perspective, TRACE level logs are also helpful, but they require a re-compile and re-install process, also people may not know there are relevant information at trace level.

If you feel log at TRACE is more appropriate, I can submit another patch at trace level :D

@skinkie
Copy link
Member

skinkie commented May 12, 2021

If you feel log at TRACE is more appropriate, I can submit another patch at trace level :D

No, I don't. I think that the regular logs should have an option to tell why the error is present. So we should explore that, unless you have another idea.

@byshen
Copy link
Contributor Author

byshen commented May 13, 2021

the regular logs should have an option to tell why the error is present

Currently, the access logs are mostly used in the Apache format, as recommended here. There is no field for us to place the error msg in it. The other two access log formats also does not support it.

I think the access log does not record the reason of error, because the access logs are produced in the end of handling requests (e.g., purge_connection in cherokee), which is usually far away from where the error happens.

If we want to record that in the access log, we need to add one additional field in the cherokee_connection_t to record this error msg when the error happens, and log the msg when close the connection.

Another approach is to correlate the access log and error log with some log entry id for each connection. For example, %L can be enabled in apache log to correlate the entry in access log and error log, as specified here. In this way, we can just add some options for error log to control the verbosity of the error log. People can correlate the regular log and error log too.

I am happy to help implement this (may add a proposal in the issue). Let me know your thoughts, thanks 😊

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

Successfully merging this pull request may close these issues.

2 participants