Add Audit Trail/Logging

[KN4CK3R]

Fixes #8

This PR adds logging of audit events to provide documentary evidence of the sequence of important activities.

Notes for reviewers:

• Contains some small refactorings to get the needed objects
• I extracted the rotating file logger from modules/log to reuse it for the audit logging

Admin panel:

Repo settings:

[delvh]

Here's my preliminary review after the easiest 10 files.
There are already quite a few flaws I've noticed.

[philip-peterson]

PR is looking good overall! This is an impressive amount of work. Lots of updates, I guess that's required for adding logging to nearly every action.

[KN4CK3R]

@lunny Please remove your block or specify what should be changed.

[techknowlogick]

I got back to this; thanks for your patience. I'm unsure about the details provided for some of the entries (ex. "UserX changed repo settings"), as it shows that settings have been updated, but not which settings nor what they were previously (some of the audit items do have this though). I also worry about the amount of entries being generated as it could lead to just a deluge of data, allowing some folks to possibly miss information. Some possible solutions for the second could be to have a new setting where you can select the entry types (ex INCLUDE_FILTER=user.*, repo.visibility....), or to have a dropdown on the audit page so that users can filter to just the entries they want to see (advanced implementation of this could be a dynamic filter where the admin can type out the filter they want, wildcards and all)

[kdumontnu]

I got back to this; thanks for your patience. I'm unsure about the details provided for some of the entries (ex. "UserX changed repo settings"), as it shows that settings have been updated, but not which settings nor what they were previously (some of the audit items do have this though). I also worry about the amount of entries being generated as it could lead to just a deluge of data, allowing some folks to possibly miss information. Some possible solutions for the second could be to have a new setting where you can select the entry types (ex INCLUDE_FILTER=user.*, repo.visibility....), or to have a dropdown on the audit page so that users can filter to just the entries they want to see (advanced implementation of this could be a dynamic filter where the admin can type out the filter they want, wildcards and all)

This sounds like a downstream feature, no? Usually, that is the policy of whatever system ingests that data (eg. a filter in Graphana). I think adding filters to the input could lead to a configuration nightmare.

[KN4CK3R]

(ex. "UserX changed repo settings"), as it shows that settings have been updated, but not which settings

You may be looking at an older version of this PR (the one you use in the enterprise Gitea) because there isn't a "repo updated" event anymore. All events have a narrow subject, there aren't any "something was changed" events anymore.

And I agree with @kdumontnu about the filters.

[delvh]

@techknowlogick what do you think about making the filters part of the clients consuming the audit log?
@lunny please specify exactly why this PR shouldn't be merged.

[6543]

Please resolve conflicts

[kdumontnu]

@lunny Please advise why you think this should not be merged ^

[techknowlogick]

For filtering in the UI I was thinking about how we already do it with labels in the issues and PR list pages where you can select multiple. Mix that in with hierarchy and then folks can bookmark their most used filters or quickly filter down to just the information they are looking for?

[kdumontnu]

For filtering in the UI I was thinking about how we already do it with labels in the issues and PR list pages where you can select multiple. Mix that in with hierarchy and then folks can bookmark their most used filters or quickly filter down to just the information they are looking for?

I think UI dropdowns/filters are a great idea, but is out of scope for this PR and can easily be added in the future. This PR has unfortunately already been the victim of substantial bikeshedding.

[techknowlogick]

@kdumontnu, fair enough. I'm not a blocking review on this PR, so I'll make a tracking ticket once it's merged so the filtering in the UI enhancement isn't lost. As mentioned in chat, this may have dropped off @lunny's radar due to dealing with things in his personal life, but I've pinged him in DMs, so hopefully, he can clarify what he means.

[lunny]

I have below concerns.

1. Should we record all these events? Although everything will be related to security, if we record everything, I think it will become another action table. I prefer only to record necessary events.
2. If we record everything, the records on the table will become more and more and it will also encounter the same performance problem as we did in action table.

[KN4CK3R]

1. Please define the "necessary events"
2. The first version did not record into the database, so there was no problem. But even with database logging we can have a clean up job with a configurable time. Could default to 30 days, so the table will keep a smaller size.

[lafriks]

I agree that it's not much of the problem that the audit table grows in size as it's not used much for selecting data only when occasionally needed and in these cases it's ok for it to be slower than daily operations. Also as mentioned audit data should be archived/deleted for records older than amount of days/weeks/months depending on regulatory requirements or enterprise policies

Anyway this can be improved later on and should not be blocking this PR

[kdumontnu]

@techknowlogick do you have requirements from Gitea enterprise users for audit logs? Can you describe them here? I'm not sure there is a clear consensus as to what the goal of this PR is. I will describe again from my perspective, and we should see if there is alignment.

Audit logs MUST include any transactions that affect:

• User settings
• System settings
  • External auth source changes
  • Config changes if they can be changed at runtime
  • Manual trigger of cron jobs
• Repo, team, org settings
• User data
  (see table of logged events here)

The logs must include:

• Event identifier
• Timestamp
• Actor
• Affected object
• (Description)
• (Severity?) - This is not currently added, but could be later

The goal here is for system admins to be able to understand when activity on their server is potentially compromised & in the event of a security incident to be able to track down the series of events. This is required for many audits (like SOC 2, ISO 27001), and also a requirement for many companies.

To be clear, this feature is NOT intended developers or for in-app features, like activity feed. Those features require different things (eg. stack-trace), which serve a different purpose. I understand the desire to combine these efforts, but doing so would make each solution half-baked and complicate development.

Gitea must work for many different users, so these logs should also be:

• Disabled (if the user requires very low resources, eg. for personal use)
• Configured with existing log settings (eg for rotation)
• KN4CK3R has pointed out that we can also add an option/cron truncate the database if there is a concern about the infinitely growing db. I don't think this is a major issue, because for small deployments this feature can be disabled altogether.

@lunny I have clearly explained the goals here this PR solves. Can you explain if there you disagree with any of them, and please be specific.

For context regarding Audit logs, here is a helpful article: https://www.datadoghq.com/knowledge-center/audit-logging/
Also consider Gitlab/Gitea documentation for audit logs.

[lafriks]

Transaction source (request IP address) is required for audit logs

[delvh]

So, any news on this?
My last state was that we waited for 1.22.0 to be released to have lots of time to test it before 1.23.
By now, we are already in the middle of the cycle for 1.23.
Do we postpone this PR for another release, or do we finally merge it?

I can currently see two fractions:
Fraction 1 wants to see this PR being merged as soon as possible, Fraction 2 is unsure of the use for this PR.
I can tell you that this is an absolute requirement for many companies, especially when there needs to be an airtight event log, i.e. in finance or insurance companies.
So I'm proposing to merge it now instead of later.

[kdumontnu]

@delvh I completely agree with you (which should be quite evident from my comments here).

Unfortunately, they have already merged a version of this code into the private gitea repo and are charging $$ for it. So Gitea Ltd. (owners group) has no incentive to review and merge this in an open source repo, even though the open source community has funded and developed it.

This is a cynical take, but I have raised these concerns in private channels and there has been no evidence to refute it. If Gitea Ltd. wants to focus its efforts on the private fork and to make money, that is fine with me, but unfortunately, they also decide what gets merged here, so there is a massive conflict of interest (cough open core). Since Lunny has blocked it and refuses to say why, it appears we are stuck.

I have asked them to commit publicly to not merging code in their private codebase that is still open for review upstream, but that hasn’t been well received, leaving me and some other members of the community pretty disheartened.

[techknowlogick]

So clarification here: you asked me about that, but I haven’t been able to respond to you due to my illnesses and I’m just getting back on my feet now. So it’s not that it hasn’t been well received, it’s that I’ve been physically unable to respond to you.

[kdumontnu]

So clarification here: you asked me about that, but I haven’t been able to respond to you due to my illnesses and I’m just getting back on my feet now. So it’s not that it hasn’t been well received, it’s that I’ve been physically unable to respond to you.

Understood, happy to work with everyone to find a path forward. Glad to know that you see the issue & hope you continue to feel better.

[techknowlogick]

@kdumontnu thanks. I'm working on a response to our conversation but sadly I just lost it due to a mix up between crtl-v and crtl-w (on dvorak w and v are next to each other), so I'll need to draft it up again.

A general response for everyone following this thread: coming from the perspective of the development of the application, we've run into performance regressions in several areas in the DB, the biggest being the activity table growing to no end, slowing down the application due to incorrectly indexed columns, and more. This has led to many bug reports and PRs needed, taking time away from other pressing matters. Until we have a way of running doctor tasks automatically (which we would do if the xormigrate PR progresses), if changes to indexes are needed, we would have to ask users who run into issues to run a CLI command manually. As we've seen before, asking users to run doctor commands is potentially a blocker for users, so they wait with an impacted experience until the next major upgrade. As the sole sysadmin of Gitea.com, any performance impacts show up with a bigger impact due to the scale, but even with things like the activity table, it affected instances of all sizes (and even still there are still optimizations that could be made to it).
I've had a PR that took over 2+ years to get merged due to similar performance reasons.
As for "Audit Logging" in "Gitea Enterprise" it doesn't utilize this code. It is a much more minimal implementation with a SQL insert for logins, package downloads, and git clones, but from my gauge of the conversation above, the more options, the better. This PR is also more fully fleshed out, so hopefully, it can be merged (and just generally, maintaining multiple codebases is a lot of extra work)

Could, at the very least, filters be added and configurable (likely in the config file is the best place, as then to change them you'd need shell access to do so) so that if there are performance impacts, then there are actionable workarounds that administrators can take to lessen the effect until it can be resolved because otherwise, the other option would be to ask them to disable it entirely, and if they are someone who needs this for a regulated reason, then disabling isn't an option, but perhaps less impactful events could then be temporarily disabled?

To clarify, I am not claiming there are known performance impacts with the DB changes from this PR, but I would like to be careful because I am sensitive to the issue due to other areas. The code in this PR is top-notch, as is with all of @KN4CK3R's PRs, and I'm thankful for all the work he's done for the project.

@KN4CK3R If it helps, since I know you've already put a ton of work into this, I can look into implementing those configurable filters myself and adding them to this PR and possibly the cron cleanup task, too. I can't promise any timelines, but I can promise that it will be my top priority.

[6543]

about performance issues, it is disabled by default ... so most users should just not notice it.

as for the other idea: what about to have the option to use a dedicated database for it as the logging destination?