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.

Sign up for GitHub

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

Jump to bottom

Clear web terminal when session ends #8850

Merged
merged 12 commits into from
Dec 2, 2021
Merged

Clear web terminal when session ends #8850

merged 12 commits into from
Dec 2, 2021

Conversation

atburke
Copy link
Contributor

@atburke atburke commented Nov 3, 2021

Only applies when Teleport is compiled for FedRAMP compliance. Closes #6107.

@atburke atburke self-assigned this Nov 3, 2021
zmb3
zmb3 approved these changes Nov 4, 2021
View reviewed changes
lib/client/terminal/terminal_unix.go Outdated
@@ -152,6 +152,15 @@ func (t *Terminal) Stderr() io.Writer {
return t.stderr
}

// Clear clears the terminal, including scrollback.
func (t *Terminal) Clear() error {
const resetPattern = "\x1b[3J\x1b\x63\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe leave a comment linking to the source of this escape code so we know where to look if we need to change it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good plan. @gzdunek can you add this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think some of it came from here: https://superuser.com/questions/122911/what-commands-can-i-use-to-reset-and-clear-my-terminal

But the above looks different, @gzdunek how did you arrive at that escape sequence?

Copy link
Contributor

Choose a reason for hiding this comment

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

/x1b[3J - clears scrollback (https://newbedev.com/how-do-i-reset-the-scrollback-in-the-terminal-via-a-shell-command https://apple.stackexchange.com/a/187773) - it is needed at least for the Mac terminal

If n is 3, clear entire screen and delete all lines saved in the scrollback buffer (this feature was added for xterm and is supported by other terminal applications)."

\x1b\x63 - clears current screen - same as '\0033\0143' linked above

Add it as a comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Added

lib/client/terminal/terminal_windows.go Outdated
@@ -272,6 +272,10 @@ func (t *Terminal) Stderr() io.Writer {
return t.stderr
}

func (t *Terminal) Clear() error {
return nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to work on Windows too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#6107 only asks for it to work in the web terminal.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually don't know the answer to this, but would this windows terminal not show up in the web UI as well? Is there any reason we shouldn't make it work on Windows?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should support all terminals. The Web UI was requested in the ticket, but after talking with the customer, it's a FedRAMP requirement for all terminals.

If the escape code approach works for the Web UI (as in xterm.js understands it) we should just make the changes in lib/client and let all terminals automatically pick it up.

Copy link
Contributor

Choose a reason for hiding this comment

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

xtermjs does understand it

Copy link
Contributor

Choose a reason for hiding this comment

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

@timothyb89 What does tsh.exe support? Also, any hints on how @atburke could handle this for Windows?

Copy link
Contributor

Choose a reason for hiding this comment

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

tsh does enable ANSI processing on Windows, so assuming Terminal.Close() hasn't yet been called (which appears to be the case) I'd expect the usual escape sequence to just work. Microsoft recommends the same: https://docs.microsoft.com/en-us/windows/console/clearing-the-screen

There are other fallback methods involving old Win32 APIs if we really have to, but hopefully we can get the ANSI sequence working.

(Slight possible caveat that the ANSI sequences might not work on very early versions of Windows 10, as in v1609 if not earlier. Hopefully we don't need to worry about them w.r.t compliance issues?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, we should not fall back to Win32 or support very early version of Windows 10.

@atburke What version of Windows were you using?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was on Windows 10 version 1809.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just copied the impl over from terminal_unix.go to terminal_windows.go and it cleared my cmd.exe just fine (on Win10 20H2)

@atburke
Copy link
Contributor Author

atburke commented Nov 19, 2021

@timothyb89 Would you mind giving this a review when you have a moment?

timothyb89
timothyb89 approved these changes Dec 2, 2021
View reviewed changes
Copy link
Contributor

@timothyb89 timothyb89 left a comment

Choose a reason for hiding this comment

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

Ah, sorry about that, looks good to me!

@atburke atburke enabled auto-merge (squash) December 2, 2021 00:44
@atburke atburke merged commit 699f588 into master Dec 2, 2021
@atburke atburke deleted the atburke/clear-terminal-on-session-termination branch December 2, 2021 20:48
atburke added a commit that referenced this pull request Dec 6, 2021
@atburke @gzdunek @russjones
Clear web terminal when session ends (#8850)
3634281 
This change clears the screen when an ssh session ends (only in FIPS mode). Note: This doesn't currently do anything in `tsh` on Windows since BoringCrypto isn't supported, but once it is supported, the behavior will match Unix and web.

Co-authored-by: Grzegorz <grzegorz.zdunek@goteleport.com>
Co-authored-by: Russell Jones <russjones@users.noreply.github.com>
@atburke atburke mentioned this pull request Dec 6, 2021
Closed
atburke added a commit that referenced this pull request Dec 9, 2021
@atburke @gzdunek @russjones
Clear web terminal when session ends (#8850)
fb20de0 
This change clears the screen when an ssh session ends (only in FIPS mode). Note: This doesn't currently do anything in `tsh` on Windows since BoringCrypto isn't supported, but once it is supported, the behavior will match Unix and web.

Co-authored-by: Grzegorz <grzegorz.zdunek@goteleport.com>
Co-authored-by: Russell Jones <russjones@users.noreply.github.com>
@atburke atburke mentioned this pull request Dec 9, 2021
Merged
atburke added a commit that referenced this pull request Dec 9, 2021
@atburke @gzdunek @russjones
Clear web terminal when session ends (#8850)
404fdc4 
This change clears the screen when an ssh session ends (only in FIPS mode). Note: This doesn't currently do anything in `tsh` on Windows since BoringCrypto isn't supported, but once it is supported, the behavior will match Unix and web.

Co-authored-by: Grzegorz <grzegorz.zdunek@goteleport.com>
Co-authored-by: Russell Jones <russjones@users.noreply.github.com>
@atburke atburke mentioned this pull request Dec 9, 2021
Closed
russjones added a commit that referenced this pull request Dec 10, 2021
@atburke @gzdunek @russjones
Clear web terminal when session ends (#8850)
980b203 
This change clears the screen when an ssh session ends (only in FIPS mode). Note: This doesn't currently do anything in `tsh` on Windows since BoringCrypto isn't supported, but once it is supported, the behavior will match Unix and web.

Co-authored-by: Grzegorz <grzegorz.zdunek@goteleport.com>
Co-authored-by: Russell Jones <russjones@users.noreply.github.com>
russjones added a commit that referenced this pull request Dec 10, 2021
@atburke @gzdunek @russjones
Clear web terminal when session ends (#8850)
8eb3878 
This change clears the screen when an ssh session ends (only in FIPS mode). Note: This doesn't currently do anything in `tsh` on Windows since BoringCrypto isn't supported, but once it is supported, the behavior will match Unix and web.

Co-authored-by: Grzegorz <grzegorz.zdunek@goteleport.com>
Co-authored-by: Russell Jones <russjones@users.noreply.github.com>
russjones added a commit that referenced this pull request Dec 15, 2021
@atburke @gzdunek @russjones
Clear web terminal when session ends (#8850)
f699ebb 
This change clears the screen when an ssh session ends (only in FIPS mode). Note: This doesn't currently do anything in `tsh` on Windows since BoringCrypto isn't supported, but once it is supported, the behavior will match Unix and web.

Co-authored-by: Grzegorz <grzegorz.zdunek@goteleport.com>
Co-authored-by: Russell Jones <russjones@users.noreply.github.com>
russjones added a commit that referenced this pull request Dec 15, 2021
@atburke @gzdunek @russjones
Clear web terminal when session ends (#8850)
c15dd32 
This change clears the screen when an ssh session ends (only in FIPS mode). Note: This doesn't currently do anything in `tsh` on Windows since BoringCrypto isn't supported, but once it is supported, the behavior will match Unix and web.

Co-authored-by: Grzegorz <grzegorz.zdunek@goteleport.com>
Co-authored-by: Russell Jones <russjones@users.noreply.github.com>
@russjones russjones mentioned this pull request Jan 13, 2022
Closed
@atburke atburke mentioned this pull request Feb 1, 2022
Merged
@webvictim webvictim mentioned this pull request Mar 4, 2022
Closed
@alexatcanva alexatcanva mentioned this pull request Oct 13, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alex-kovoy alex-kovoy alex-kovoy left review comments

@gzdunek gzdunek gzdunek left review comments

@timothyb89 timothyb89 timothyb89 approved these changes

@russjones russjones russjones approved these changes

@zmb3 zmb3 zmb3 approved these changes

@nklaassen nklaassen Awaiting requested review from nklaassen

@fspmarshall fspmarshall Awaiting requested review from fspmarshall

@klizhentas klizhentas Awaiting requested review from klizhentas

@r0mant r0mant Awaiting requested review from r0mant

Assignees

@atburke atburke

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Clear web terminal after session idle timeout.
7 participants
@atburke @timothyb89 @russjones @zmb3 @alex-kovoy @nklaassen @gzdunek