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

add a session_locking option in tlog-rec-session.conf #328

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

Conversation

ajf8
Copy link
Contributor

@ajf8 ajf8 commented Apr 7, 2021

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.

@coveralls
Copy link

coveralls commented Apr 7, 2021

Coverage Status

Coverage decreased (-0.04%) to 28.855% when pulling b4639f3 on ajf8:no_locking_option into 3897015 on Scribery:master.

@trevor-vaughan
Copy link
Contributor

Nice! Shouldn't this be the default since log loss is going to be the default in many (if not most) non-trivial cases?

@ajf8
Copy link
Contributor Author

ajf8 commented Apr 9, 2021

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.

@justin-stephenson
Copy link
Collaborator

Hi, this seems reasonable. On busy systems with locking disabled are you seeing an excess of duplicate recordings?

@trevor-vaughan
Copy link
Contributor

I guess I look at it the other way. Would you rather lose logs or have to ramp it back if things get overwhelming?

@ajf8
Copy link
Contributor Author

ajf8 commented Apr 13, 2021

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,

Copy link
Collaborator

@justin-stephenson justin-stephenson left a 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?

Copy link
Collaborator

@justin-stephenson justin-stephenson left a 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 ajf8 changed the title add a no-locking option in tlog-rec-session.conf add a session_locking option in tlog-rec-session.conf May 18, 2021
@justin-stephenson
Copy link
Collaborator

@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

class TlogRecSessionConfig(TlogRecConfig):

Copy link
Collaborator

@justin-stephenson justin-stephenson left a 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?

@ajf8
Copy link
Contributor Author

ajf8 commented Jun 16, 2021

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.
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.

4 participants