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

Logging via syslog #36

Closed
ancarda opened this issue Apr 6, 2018 · 7 comments · Fixed by #39
Closed

Logging via syslog #36

ancarda opened this issue Apr 6, 2018 · 7 comments · Fixed by #39

Comments

@ancarda
Copy link
Contributor

ancarda commented Apr 6, 2018

I'd like to be able to log behavior that happening on selfauth so there's some amount of visibility on my system around authentication. If login attempts are being made, I want to be able to get OSSEC alerts which can be picked up if selfauth writes to syslog.

Since not everyone needs this, I'm thinking a few new (completely optional, for backwards compatibility) constants:

  • LOG_SUCCESSFUL -- defaults to true, logs when the user authenticates successfully.
  • LOG_FAILED -- defaults to true, logs when an authentication attempt is rejected because the password is incorrect, or another problem occurred.
  • LOG_FAILED_PASSWORDS -- defaults to false, allows logging the password attempt when the login fails.

Logging would be enabled by default for existing installations and for new ones, although with passwords omitted. The setup script could emit these constants to allow people to configure logging should they wish to.

When LOG_SUCCESSFUL is true, the following is then written to syslog via the syslog() function:

IndieAuth login successful from {remote-ip}.

Should the User Agent be included?

Logs would write at the Notice level for a successful login and the Error level for a failed login.

When LOG_FAILED is true, the following is written to syslog. The terminology is intentionally designed to trip up OSSEC:

IndieAuth: login failure from {remote-ip}.

When LOG_FAILED_PASSWORDS is also true, then this is appended to the end of the previous syslog entry:

Password refused: {password}.

Please let me know what you think. If you're happy with this idea, I can start work on an implementation.

@sknebel
Copy link
Collaborator

sknebel commented Apr 24, 2018

Interesting idea.

Something I don't know and would need an answer for: Does this conflict with e.g. using selfauth on shared hosting, where the syslog functions are disabled? Are disabled functions safely skipped, or can we detect that and skip them?

@ancarda
Copy link
Contributor Author

ancarda commented Apr 24, 2018

Hi,

This shouldn't conflict with shared hosting. If syslog is disabled or otherwise unsupported on the host OS, syslog() might return false when unable to successfully write to syslog, so that return value could be ignored. In the future, perhaps another method like mail() could be tried.

If the syslog() function is disabled through the disable_functions INI setting, then AFAIK, the function won't exist at all, so wrapping it in function_exists() should be enough to detect that.

Checking the documentation, syslog() apparently never raises any PHP notices, warnings, or errors, doesn't throw any exceptions, and has been available since PHP 4.0, so should be available on a shared hosting platform -- although I can imagine the logs may not be accessible.

Perhaps during setup we could expose a small checkbox to control the default value so it may be completely disabled?

@Zegnat
Copy link
Collaborator

Zegnat commented Apr 25, 2018

This totally slipped me by, so here we go. I do like the idea of logging things, and syslog() is probably the best solution unless we want to pull in something like PSR-3. More thoughts:

  1. I would not turn any logging on by default. I do think logging IPs with authentication requests makes sense, and I would simply never want to log any IPs by default. Especially when people running this on shared hosts might be feeding it into logs they themselves cannot clear.
  2. LOG_FAILED_PASSWORDS sounds like a nice-to-have that needs massive disclaimers around it. We can’t work on the assumption that everyone is using a password manager. This means people are typing their passwords, and typos happen. This option sounds good, but if you over time fill logs with deviations of your real password, you better be making sure you are purging those logs real good. (Of course again with the problem that syslog() may be out of reach to the user who unwittingly turned this on.)

I can almost see us strategically dropping these into the source code, but commented. Anyone who understands syslog() and wants to use it to trip up other alarm bells on a server, will probably be OK uncommenting a couple of functions. Even if they aren’t well versed with PHP. This will at least keep it out of the hands of users who cannot see the possible side-effects.

Like the idea, just not sure how to execute it without giving users some flags in the config with huge warning disclaimers. And I don’t like warning disclaimers in what is supposed to be a simple single-purpose thing.

@aaronpk
Copy link
Contributor

aaronpk commented Apr 25, 2018

I like the idea of making logging opt-in by uncommenting the code. I'm struggling to think of a case where logging failed passwords is ever a good idea. It seems others would agree with this assessment as well. https://security.stackexchange.com/questions/16824/is-it-common-practice-to-log-rejected-passwords

@ancarda
Copy link
Contributor Author

ancarda commented Apr 26, 2018

Hi,

Good points being made here, I hadn’t thought about typos or shared hosting. That’s what years of using a password manager and dedicated hosting will do!

I like the idea of having some commented out code, although I wonder if that is going to be problematic every time there is an update and I git pull as there would be changes on the local filesystem.

I’d be happy to write this, likely on the weekend when I have more time. I’ll send a pull request when I can.

Perhaps longer term, there could be a small plugin system for self auth so any additional features that may need to carry warning labels can be developed outside self auth? I could then continue to develop it to have, e.g. Pushover support (which is quite a bit more complex as I’d probably need ext/curl support).

Would you be open to me coming up with a proposal for a plugin system in another issue?

@Zegnat
Copy link
Collaborator

Zegnat commented Apr 27, 2018

I like the idea of having some commented out code, although I wonder if that is going to be problematic every time there is an update

This is actually a really good point to raise. And something I forgot because I have been operating out of my own private branch and merging changes into it as we go anyway. Everyone is so used to their own setups that it is sometimes hard to think of other setups out there 😉

So maybe not hide it behind a comment but behind a constant anyway? And just not include the constant in the example config at all, so people can only discover it by looking at the code and seeing what it does?

if (defined('SYSLOG_FAILURES') && SYSLOG_FAILURES === 'I understand') {
    syslog(sprintf(
        'IndieAuth: login failure from %s for %s',
        $_SERVER['REMOTE_ADDR'],
        $me
    ));
}

@Zegnat
Copy link
Collaborator

Zegnat commented May 3, 2018

For future reference, logging passwords to system logs …

@TwitterSupport:

We recently found a bug that stored passwords unmasked in an internal log. We fixed the bug and have no indication of a breach or misuse by anyone. As a precaution, consider changing your password on all services where you’ve used this password.

Oopsie.

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 a pull request may close this issue.

4 participants