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

Syslog audit #727

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Syslog audit #727

wants to merge 4 commits into from

Conversation

CrackerJackMack
Copy link
Contributor

Refactor auditing to use a single entry point, removes a lot duplicate lines of code. Also adds info logger of audit activities.

Move audit logging into a single function for easier
refactoring later.
@CrackerJackMack
Copy link
Contributor Author

In testing this, it seems that using info() is squashed by the default logger. Should the default log level be changed or auditing logged via syslog be elevated to warn()?
edit: words

@plajjan
Copy link
Member

plajjan commented Apr 6, 2015

Not everyone might want to see the audit log via syslog, so we need a knob to turn it off. We can't use the severity level since it should be possible to have debug level on NIPAP without seeing the audit log.

I'm pondering if this should go into a separate log than the rest of the messages, so perhaps we want a second logger with a different (and configurable?) facility?

@plajjan
Copy link
Member

plajjan commented Apr 6, 2015

My goal has been to move audit log functionality into the database instead of trying to deal with it in Python, see #368 for more details/thoughts on that. There are pros and cons to that and the main argument in favour of doing it in the database (using a stored procedure or similar) is simply database consistency. There are likely fewer errors and weird situations we can end up with if we do it in the database, we get closer to the data and so forth.

I'm not sure at all how well this plays with trying to send it over syslog. We can either not focus on moving it to the database at all or we can try to extract it from the database once it has been written. PostgreSQL has notification support so a subscriber can subscribe to the audit log table and when something is inserted, the subscriber is notified upon which the row can be retrieved.

@CrackerJackMack
Copy link
Contributor Author

My company is already dealing with auditing and compliance with syslog for tacacs+, sudo, rancid updates, etc. This makes searching through timelines and auditing much easier as adding each source to the log makes back tracking an issue much easier.

I'll update the PR with some nobs so it can be turned on and off.

@plajjan
Copy link
Member

plajjan commented Apr 8, 2015

Again, I never mentioned it in my first post but nice code! Naturally the single point of entry makes this much cleaner :)

The audit log in NIPAP is not meant merely as an audit log but the idea was to have an integrated history of changes so that users could look at a prefix or VRF and see that their colleague had changed something. This is why the log is in a SQL database and not sent, per default, to an external source. For an actual audit log it makes much more sense to send it (make sure it's over reliable transport!!) somewhere else so that it is less likely it can be tampered with. When we started coding on NIPAP I didn't want to launch the project without an audit log at all. I wanted history from day 1 and therefore we built this simple and somewhat hackish approach of just inserting text entries into a table. We never exposed the log over the XML-RPC API simply because our development effort was focused elsewhere.

Moving forward, I would like to have "undo" functionality so that users can revert modifications or parts of modifications, and this naturally requires a better format than what we currently have. #368 is all about improving that format.

From my perspective, these are two similar but yet distinct use cases. One is for undo-type functionality and providing a quick (and searchable) way of looking at changes from inside the NIPAP user interface, while the other is for strict auditing purposes. I find both to be valid use cases and something that we should try to support.

For improving the audit format I think that we will look into doing it in the database. That will complicate things for actually sending syslog audits but I guess we can take that hurdle once we get there.

@plajjan plajjan self-assigned this Apr 16, 2015
@plajjan
Copy link
Member

plajjan commented Apr 16, 2015

I missed that you updated this so sorry for delay. Is there a specific reason why you wish to introduce functionality to actually turn off the integrated DB log?

While the built-in log might not be as advanced as I would like it to be I'm not sure I would go to the length of calling it immature ;) It is working after all and records all changes. After the introduction of a more complete audit format, my hope was to parse the text entries in the ip_net_log table and convert them into the new format, thus making it possible to have a undo-history all the way back to when someone started using NIPAP. I'm not sure that it's possible but that's my hope.
Why would anyone want to turn off audit db log?

@plajjan
Copy link
Member

plajjan commented Apr 16, 2015

And as for audit log over syslog, I would like to have a second log handler so that it can be sent to a different facility so that it is easier to separate the audit log from the nipap "system" log.. does this make sense?

@CrackerJackMack
Copy link
Contributor Author

I used the word immature because it's not only subject to change but not currently part of the API and from the understand of our conversations is that you indeed plan to change it and the API around it. Honestly meant no offense by it.

As for the undo log, that is not someone of interest here. The comment field is more than efficient for my use case along with the use of the syslog. In fact, having an undo is the least of our concerns. Using the webui is only a temporary solution until the xml-rpc interface is more integrated with our other infrastructure.

Reach out to me on IRC tomorrow and we'll chat about what you are more looking for. Adding another handler didn't look straight forward so maybe it should also be augmented in config parser as well?

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

Successfully merging this pull request may close these issues.

3 participants