linuxserver / docker-bookstack

A Docker container for the BookStack documentation wiki
GNU General Public License v3.0
747 stars 108 forks source link

This will fix bug for APP_PROXIES environment + added Bookstack Time Zone environment in README.md #148

Closed calvin-li-developer closed 1 year ago

calvin-li-developer commented 1 year ago

linuxserver.io



Description:

This will fix bug for APP_PROXIES environment. USE CASE: fail2ban.

Benefits of this PR and context:

anyone that uses APP_PROXIES and wants to enable logging with LOG_FAILED_LOGIN_MESSAGE="Failed login for %u" can see real IP at /config/log/nginx/error.log

How Has This Been Tested?

Source / References:

https://www.bookstackapp.com/docs/admin/security/

https://github.com/BookStackApp/BookStack/issues/3619

https://github.com/BookStackApp/BookStack/blob/development/.env.example.complete#L38

LinuxServer-CI commented 1 year ago

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.11-pkg-9a84e426-pr-148/index.html https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.11-pkg-9a84e426-pr-148/shellcheck-result.xml

LinuxServer-CI commented 1 year ago

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.11-pkg-9a84e426-pr-148/index.html https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.11-pkg-9a84e426-pr-148/shellcheck-result.xml

github-actions[bot] commented 1 year ago

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

nemchik commented 1 year ago

https://github.com/linuxserver/docker-bookstack/blob/2e6c9dfff8bae307ae0e17074fcc61589f5bd848/README.md?plain=1#L1-L2

It would be helpful if you could cite a source (upstream documentation) about the use of the variable.

The nginx changes are likely not to be accepted. A properly configured proxy should hand the correct IP address to the application (bookstack).

calvin-li-developer commented 1 year ago

The nginx changes are likely not to be accepted. A properly configured proxy should hand the correct IP address to the application (bookstack).

Through the documentation it will give proper IP IF you are logged in and accessing it through audit logs. it does not give proper it through nginx error logs from what IP. I know its a separate app but this nginx change is also made for nextcloud. I have tested it you can try yourself. as for the environment variables I will update my source/reference documentation.

nemchik commented 1 year ago

The nginx changes are likely not to be accepted. A properly configured proxy should hand the correct IP address to the application (bookstack).

Through the documentation it will give proper IP IF you are logged in and accessing it through audit logs. it does not give proper it through nginx error logs from what IP. I know its a separate app but this nginx change is also made for nextcloud. I have tested it you can try yourself. as for the environment variables I will update my source/reference documentation.

I've done a quick bit of extra reading on the nginx changes, and I actually think we can include something in our nginx base (which is used by this container) to handle this more broadly and generically. I'll do some testing this week and link this PR as a reference if changes land in the base.

As for the readme changes about the variable, please also read our contributing guidelines, mentioned in the code link in my last comment.

nemchik commented 1 year ago

After a bit of research and internal discussion on the Real IP topic, the team has agreed that it would not be ideal to roll out configurations to existing users (via the base) without some kind of opt-in and understanding of what the settings are doing.

The best recommendation we have for the moment is to use https://github.com/linuxserver/docker-mods/tree/swag-cloudflare-real-ip#mod-usage-instructions and to apply it to every nginx based container that is being proxied by swag. This mod sets up configuration for the detected container network interface and should work even without cloudflare.

modem7 commented 1 year ago

The best recommendation we have for the moment is to use https://github.com/linuxserver/docker-mods/tree/swag-cloudflare-real-ip#mod-usage-instructions and to apply it to every nginx based container that is being proxied by swag. This mod sets up configuration for the detected container network interface and should work even without cloudflare.

Would this also work with non-swag RP's such as traefik based upon the following: https://doc.traefik.io/traefik/getting-started/faq/#what-are-the-forwarded-headers-when-proxying-http-requests?

nemchik commented 1 year ago

Yes, it should work with any proxy. If the proxy is passing the XFF header to this container (bookstack) and you have the docker-mod setup it should correctly detect the IP.

modem7 commented 1 year ago

@nemchik

Worked great, thank you!

Exactly what I required for Crowdsec via Traefik.