-
Notifications
You must be signed in to change notification settings - Fork 52
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
add a session_locking option in tlog-rec-session.conf #328
base: main
Are you sure you want to change the base?
Conversation
Nice! Shouldn't this be the default since log loss is going to be the default in many (if not most) non-trivial cases? |
I'm incined to say yes it should be and happy to change it if the maintainers agree, but I appreciate it could be a major and unexpected change of behavior for some, so I didn't change the defaults. Maybe that could happen at a later time and with more consideration. I'm happy with just having the option and changing tlog-rec-session.conf in our config management. I sed the LOCK_SESS bigflag to be zero before building tlog at present and will continue to do so, but it seems like something that should be an option. |
Hi, this seems reasonable. On busy systems with locking disabled are you seeing an excess of duplicate recordings? |
I guess I look at it the other way. Would you rather lose logs or have to ramp it back if things get overwhelming? |
Hi, No, it hasn't resulted in excessive amounts of logs or any duplication for us, just that multiplexed sessions get logged. We have tlog on bastions which are restricted to just SSH and a few other commands, and on hosts which aren't logged into often. It's fine in both of these cases. I guess there could be duplication on a desktop / development host, if things are running your shell as defined by NSS rather than $SHELL. But we're not interested in tlog on such hosts. Thanks, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please name the option something like session_locking
which defaults to enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, Please use session_locking
instead of enable_locking
, The man page description should indicate this enables locking by audit session ID. Please squash into a single commit.
@ajf8 Would you like to write a test for this in https://github.com/Scribery/tlog/blob/master/lib/tlitest/test_tlog_rec_session.py ? You'll need to add this new option Line 185 in 3897015
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much for the tests. Could you please squash you commits. @spoore1 Would you mind reviewing?
Hi @justin-stephenson no problem, but I haven't quite got the tests working yet. I'll try to figure it out and fix them soon. |
defaults to true, preserving previous behaviour, only one tlog session will be recorded at a time per session ID. setting to false disables this locking, multiple recordings can be made simultaneously.
this adds a option to disable locking by session id.
locking may be desirable for some users to avoid potential duplication, but for others it's a bit of a liability, in particular not recording multiplexed SSH sessions which share the same session ID.