Skip to content

Conversation

@benironside
Copy link
Contributor

@benironside benironside commented Oct 12, 2022

Fixes #2458 by updating the Session view doc with information about Terminal output view.

AWP team, please review for accuracy and completeness. Thanks!

Preview: Session view

@github-actions
Copy link

Documentation previews:

@opauloh
Copy link

opauloh commented Oct 12, 2022

Hi @benironside

Here is some additional info regarding the TTY, if you find it relevant to add:

  • Hovering the markers displays the process name and clicking on the marker also navigates to the process output.
  • Playback controls: Click anywhere in the progress bar to navigate

Also, saw some things regarding the session view that changed in 8.5 (which can be addressed in a separate PR as well):

  • Session View is no longer Beta, but Terminal Output is. Should we move the Beta warning at the beginning to the Terminal Output section?
  • Should we include that Session View is part of a subscription (Enterprise) plan? (Workload session auditing)

Copy link

@mitodrummer mitodrummer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, some notes, but also, we probably need a section to describe some of the advanced configuration options that are available.

image
image
image

These settings are found at the bottom of the edit integration policy page.

max_kilobytes_per_process basically tells endpoint when to stop recording output for an instance of a process.

max_kilobytes_per_event controls how big an event gets before it gets sent to elastic and a new event continues recording for the process. basically our "batch size".

max_event_interval_seconds how long to wait for the next tty_write call before sending the event to elastic. As an example, if you started a session, and typed nothing and then waited. The endpoint would wait 30 seconds (since bash hasn't exited yet), to send up the output generated so far (most likely a welcome message and an empty prompt). This setting could be reduced to say 5 seconds, if a user cares about seeing more frequent updates.

It's worth mentioning that we only record a single timestamp per batch/event, so if a user requires more granular timestamps they can reduce both max_kilobytes_per_event and max_event_interval_seconds to achieve this. Higher values are recommend to save on storage and network performance.

@benironside
Copy link
Contributor Author

Thank you @mitodrummer and @opauloh for the great feedback. Incorporated it.

Copy link
Contributor

@jmikell821 jmikell821 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benironside left some general suggestions for conciseness and consistency. In some places, you have "session view button" and others, "session view icon". I'm trying to find our UX ponderisms page, but based on our manage detection alerts topic, most of the table elements are listed as "buttons." Please make sure all those references are the same.

I'd like us all to review this again with fresh eyes once feedback is merged. Thanks!

benironside and others added 4 commits October 14, 2022 12:57
Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>
Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>
Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>
Copy link
Contributor

@joepeeples joepeeples left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good content, added some comments, thank you!

One general thing: noticed that "terminal" is both lowercase and uppercase in various spots throughout. I couldn't tell if there was a specific nuance behind that, but I'd opt for lowercase overall.

@benironside benironside mentioned this pull request Oct 17, 2022
28 tasks
Copy link

@mitodrummer mitodrummer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@m-sample
Copy link

pls pardon my tardiness - I will have completed a review on this today.

@benironside benironside added the readyforQA PRs that are ready for QA review. label Oct 18, 2022
Comment on lines +123 to +124
1. Search bar. Use to find and highlight search terms within the current session.
The left and right arrows allow you to navigate through search results.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Detail on why search may behave unexpectedly would help as well as emphasis that this is only search over terminal output held in the browser (ie it is not an Elasticsearch query - @mitodrummer pls correct me if I'm wrong). E.g.:

"Use to find and highlight search terms within the current session's terminal output held by the browser. Note that because terminal output may include terminal codes for colorizing the text and moving the cursor, some searches may not find a match. For example, if one is searching output for a command like "sudo" (echoed command input) and the user mis-entered "sudo" as "sdo", and moved the cursor back to insert the missing "u", the terminal output search for "sudo" will not match. This challenge can also be present in visual editors like vi and emacs where cursor movement for editing is commonplace"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct @m-sample, though, for the browser side "plain text" search, I strip out all ansi chars on a a line before matching the user's search against it. This is required so that I get an accurate string index position for each match, so that when I seek to this "line" in the tty player, it's able to highlight the correct match (if there are multiple on a single line). That being said, you are correct in that if a user mistypes a command and then hits backspace to correct it, the resulting output text will not be the "corrected" version.

e.g
User types, sdo
They hit backspace twice, then type udo to fix their typo.
The resulting output will look something simlilar to:
sdo\b\budo

The \b is an escape code for backspace, so you can see why searching for the corrected command will not match.

That being said, most of the formating ansi codes for color and text weight would be stripped out and should not interfere with most plain text searches. This is in contrast to using KQL or EQL to match on values in process.io.text as it will be the un altered version of the value that will potentially contain many escape sequences between words in the output text. Here is a link to a resource which describes these codes. https://gist.github.com/fnky/458719343aabd01cfb17a3a4f7296797

Comment on lines +119 to +120

[role="screenshot"]
Copy link

@m-sample m-sample Oct 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any docs on writing alerting rules based on matching on terminal output?

It is important to note that when writing Alerting Rules querying Elasticsearch for matching terminal output, that multi-line matching will not behave consistently because the output text may be stored in more than one output event. For example, Foo\nBar - that is, Foo on one line followed by Bar on the next line - will not behave consistently because Bar might be in output event after Foo. Elastic tries to ensure a given line of terminal output is contained in exactly one output event to ensure in-line matches work as expected (beware of terminal color codes). However, very long lines may be split across more than one output event.

Copy link
Contributor Author

@benironside benironside Oct 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not have any docs specifically about alerting rules based on terminal output, yet. But we could mention something here, or in our query rules docs, about the issue you outlined.

Any thoughts on how to work around that limitation? I guess one thing would be to increase the maximum size of output events to reduce the number of unexpected newlines, but that would still be inconsistent.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, increasing the max event buffering size and 30 second capture to be larger (60s?) create bigger events that are more likely to work for multi-line matches but there are still chances for inconsistency. Also, IIRC we flush the output immediately if the program writing to the terminal changes - e.g. I ctrl-Z a program that is producing output and the shell then echos a prompt (the shell is another program writing to the terminal, causing a flush), then put the original program back into the foreground with fg. THis will break up the output of the original program over at least 2 events.

If this important for some customers, the best way is to post process these output events to group all the output events of a given process, and while they're at it ideally scrub out terminal control codes for color, etc.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for rules that match against process.io.text, it is advised that all rules match with leading/trailing wildcards (*).

e.g
process.io.text: *hello world*

EQL equivalent:
process where process.io.text : "*hello world*"

This will ensure a less strict rule which will have a much better chance of firing. Without the * wildcards, it would try to match an event with exactly the string "hello world" which in most cases will not match on anything, since an output event can have multiple lines in it. This also opens up another potential bit of confusion for the end user. E.g if their event size is quite large. e.g 512kb (due to some long cat'd file), there will be a single alert for this document no matter how little the text we are matching against. This could make it difficult to figure out which part of the proces.io.text value actually triggered the alert. We may add some UX down the road to help with this. e.g showing alerts where event.action: 'text_output' inside the tty player itself (potentially highlighting the bit of text responsible for the alert). Reducing the timeout and batch size configs could help limit the amount of text per event, thus alleviating this issue.

@ghost
Copy link

ghost commented Oct 18, 2022

Hi @jmikell821

we have validated the above shared preview links are documentation are good to go.

Screen-Shot

image

@ghost ghost added the QA:Validated Issue has been Validated by QA Team label Oct 18, 2022
@benironside benironside merged commit a9f6722 into main Oct 19, 2022
mergify bot pushed a commit that referenced this pull request Oct 19, 2022
* Add Terminal output view information to Session view doc

* adjusts image sizes

* Update images

* Incorporates Karl's and Paulo's feedback

* Update docs/detections/session-view.asciidoc

* removes link to security-team repo

* fixes list

* Update info about session view data being off by default

* Changes references to "icon" to "button"

* Update docs/detections/session-view.asciidoc

Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>

* Update docs/detections/session-view.asciidoc

Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>

* Update docs/detections/session-view.asciidoc

Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>

* Update docs/detections/session-view.asciidoc

Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>

* Update docs/detections/session-view.asciidoc

Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>

* Update docs/detections/session-view.asciidoc

Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>

* Update docs/detections/session-view.asciidoc

Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>

* Update docs/detections/session-view.asciidoc

Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>

* Update docs/detections/session-view.asciidoc

Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>

* finish incorporating Janeen's feedback.

* Joe's feedback

* Incorporates Joe's feedback

* Replaces "TTY" with "terminal"

* Adds output nuances

* Adds a word

* Incorporates Karl's feedback

* minor tweaks

* fixes a link

* Update docs/detections/session-view.asciidoc

Co-authored-by: Joe Peeples <joe.peeples@elastic.co>

Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>
Co-authored-by: Joe Peeples <joe.peeples@elastic.co>
(cherry picked from commit a9f6722)
Copy link

@opauloh opauloh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

benironside added a commit that referenced this pull request Oct 19, 2022
… (#2607)

* Add Terminal output view information to Session view doc

* adjusts image sizes

* Update images

* Incorporates Karl's and Paulo's feedback

* Update docs/detections/session-view.asciidoc

* removes link to security-team repo

* fixes list

* Update info about session view data being off by default

* Changes references to "icon" to "button"

* Update docs/detections/session-view.asciidoc

Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>

* Update docs/detections/session-view.asciidoc

Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>

* Update docs/detections/session-view.asciidoc

Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>

* Update docs/detections/session-view.asciidoc

Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>

* Update docs/detections/session-view.asciidoc

Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>

* Update docs/detections/session-view.asciidoc

Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>

* Update docs/detections/session-view.asciidoc

Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>

* Update docs/detections/session-view.asciidoc

Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>

* Update docs/detections/session-view.asciidoc

Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>

* finish incorporating Janeen's feedback.

* Joe's feedback

* Incorporates Joe's feedback

* Replaces "TTY" with "terminal"

* Adds output nuances

* Adds a word

* Incorporates Karl's feedback

* minor tweaks

* fixes a link

* Update docs/detections/session-view.asciidoc

Co-authored-by: Joe Peeples <joe.peeples@elastic.co>

Co-authored-by: Janeen Mikell-Straughn <57149392+jmikell821@users.noreply.github.com>
Co-authored-by: Joe Peeples <joe.peeples@elastic.co>
(cherry picked from commit a9f6722)

Co-authored-by: Benjamin Ironside Goldstein <91905639+benironside@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature: Session View QA:Validated Issue has been Validated by QA Team readyforQA PRs that are ready for QA review. v8.5.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[DOCS] Terminal Output Review for Session View

8 participants