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

host-level ttys #1208

Merged
merged 5 commits into from
Apr 1, 2016
Merged

host-level ttys #1208

merged 5 commits into from
Apr 1, 2016

Conversation

2opremio
Copy link
Contributor

Fixes #1016

@2opremio 2opremio changed the title [WIP] host level ttys [WIP] host-level ttys Mar 24, 2016
@2opremio 2opremio force-pushed the 1016-host-level-ttys branch 5 times, most recently from 146e4a9 to a0d46e8 Compare March 25, 2016 15:37
@2opremio 2opremio changed the title [WIP] host-level ttys host-level ttys Mar 25, 2016
@2opremio 2opremio force-pushed the 1016-host-level-ttys branch 2 times, most recently from 25f5e48 to 1dd27f1 Compare March 25, 2016 16:16
@tomwilkie
Copy link
Contributor

@2opremio would you mind squashing into 2 changesets - one for the vendoring and one for the rest?

tomwilkie
tomwilkie reviewed Mar 29, 2016
View reviewed changes
probe/host/controls.go
// Controls handles controls for a hosts.
type Controls struct {
pipes controls.PipeClient
}

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@2opremio 2opremio force-pushed the 1016-host-level-ttys branch from 1dd27f1 to c1c40ad Compare March 29, 2016 16:05
@2opremio
Copy link
Contributor Author

would you mind squashing into 2 changesets - one for the vendoring and one for the rest?

Done + rebased

@2opremio 2opremio force-pushed the 1016-host-level-ttys branch from db42381 to 59af5d2 Compare March 29, 2016 17:31
@2opremio 2opremio mentioned this pull request Mar 30, 2016
Open
paulbellamy
paulbellamy reviewed Mar 31, 2016
View reviewed changes
probe/host/controls.go
log.Errorf("Error waiting on host shell: %v", err)
}
ptyPipe.Close()
pipe.Close()

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@paulbellamy
Copy link
Contributor

I'm really not convinced this is a good idea, for security reasons.

@2opremio
Copy link
Contributor Author

I'm really not convinced this is a good idea, for security reasons.

This doesn't degrade security in any way. To prove it, just open a terminal in the Scope container and type nsenter -t1 -m bash

Productizing this makes it more evident as @rade pointed out offline, but that's it.

@2opremio 2opremio assigned paulbellamy and unassigned tomwilkie Mar 31, 2016
@2opremio 2opremio force-pushed the 1016-host-level-ttys branch from f66fce7 to 4a49607 Compare April 1, 2016 12:08
@paulbellamy
Copy link
Contributor

Technically, looks fine to me. But I'm really not convinced it's a good idea.

@pidster
Copy link
Contributor

pidster commented Apr 1, 2016

LGTM
Security in Scope is an issue should be addressed separately, as there's lots more things to consider than just this.

@2opremio 2opremio merged commit 7643d7a into master Apr 1, 2016
@2opremio 2opremio deleted the 1016-host-level-ttys branch April 1, 2016 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@paulbellamy paulbellamy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@2opremio @tomwilkie @paulbellamy @pidster