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

Config+Auth: Add flags to log unauthorized requests #167

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

Conversation

networkException
Copy link
Contributor

@networkException networkException commented Sep 11, 2021

What is the purpose of this change? What does it change?

Currently it is not possible to integrate rest-server into a service like fail2ban
which prevents brute force login attempts.

This pull request adds new command line flags in order to support logging of
unauthorized requests to the server. The flag --log-auth-failure enables
the logging and uses the remote address of the request as the default for
the logged ip. If the server is used behind a reverse proxy for, --header-for-ip
can be used to specify a header like "X-Forwarded-For" to be used for logging
the ip.

Was the change discussed in an issue or in the forum before?

There was a forum question about fail2ban without an actual solution: https://forum.restic.net/t/rest-server-and-fail2ban/2569

Checklist

  • I have enabled maintainer edits for this PR
  • I have added tests for all changes in this PR: Unsure where to implement tests
  • I have added documentation for the changes (in the manual): Documentation has been added to the readme
  • There's a new file in changelog/unreleased/ that describes the changes for our users (template here)
  • I have run gofmt on the code in all commits
  • All commit messages are formatted in the same style as the other commits in the repo: Unsure, commit style inconsistent
  • I'm done, this Pull Request is ready for review

@rawtaz
Copy link
Contributor

rawtaz commented Sep 11, 2021

Thanks for your contribution. I'm however thinking that instead of having a separate flag to turn this feature on, it's better with sane defaults and if needed a way to specify different verbosity in the regular log. The latter could also be used for other things, and we already use that type of thing in restic (the --verbose option). In other words, instead of adding flags for every thing we want loggable, better to put them in the right verbosity level and use that.

It's unfortunate that we're using -v for --version, it would be better if --version would be its own command instead, like in restic, as opposed to an option. This can be changed though, it's not like a ton of people are depending on the -v option..

I'm also pondering if the --header-for-ip shouldn't be renamed --ip-header or possibly --header-ip.

@networkException
Copy link
Contributor Author

I'm not familiar enough with the codebase to refactor the whole logging system to respond to verbosity levels I fear.

As for the flag --ip-header sounds alright, I will change that

This patch adds new command line flags in order to support logging of
unauthorized requests to the server. The flag `--log-auth-failure` enables
the logging and uses the remote address of the request as the default for
the logged ip. If the server is used behind a reverse proxy for, `--ip-header`
can be used to specify a header like "X-Forwarded-For" to be used for logging
the ip.
@mozlima
Copy link

mozlima commented Sep 13, 2024

By default, log at least the '401 Unauthorized' error so that we can take automatic action on the server. Currently, there is no way to do this, and it poses a security risk.

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.

3 participants