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

feat: add ssh jump server #1517

Merged
merged 4 commits into from
Jan 13, 2025
Merged

Conversation

SimoneDutto
Copy link
Contributor

Description

This pr introduces the ssh jump server.

It is the basic struct to start, we still are missing:

  • handling host keys
  • resolving actual controllers

But this is the initial blueprint.

Engineering checklist

  • Documentation updated
  • Covered by unit tests
  • Covered by integration tests

Test instructions

@SimoneDutto SimoneDutto requested a review from a team as a code owner January 10, 2025 10:55
@SimoneDutto SimoneDutto force-pushed the JUJU-7352/add-ssh-jump branch 6 times, most recently from 1b6de70 to bbfd120 Compare January 10, 2025 11:47
go.mod Outdated Show resolved Hide resolved
internal/ssh/ssh.go Outdated Show resolved Hide resolved
internal/ssh/ssh.go Outdated Show resolved Hide resolved
internal/ssh/ssh_test.go Outdated Show resolved Hide resolved
internal/ssh/ssh_test.go Outdated Show resolved Hide resolved
internal/ssh/ssh_test.go Outdated Show resolved Hide resolved
internal/ssh/ssh_test.go Outdated Show resolved Hide resolved
internal/ssh/ssh_test.go Outdated Show resolved Hide resolved
internal/ssh/ssh_test.go Outdated Show resolved Hide resolved
@@ -21,3 +23,15 @@ func NewConversationID() string {
}
return hex.EncodeToString(buf)
}

// GetFreePort asks the kernel for a free open port that is ready to use.
func GetFreePort() (int, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nifty!

internal/ssh/export_test.go Show resolved Hide resolved
internal/ssh/ssh.go Outdated Show resolved Hide resolved
internal/ssh/ssh.go Outdated Show resolved Hide resolved
internal/ssh/ssh.go Show resolved Hide resolved
internal/ssh/ssh.go Show resolved Hide resolved
internal/ssh/ssh_test.go Show resolved Hide resolved

func (s *sshSuite) Init(c *qt.C) {
s.received = make(chan bool)
port, err := utils.GetFreePort()
Copy link
Collaborator

Choose a reason for hiding this comment

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

i don't think there's a guarantee that the returned port won't be taken by another process before it's used in the gliderssh.Server ListenAndServe..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, is not guaranteed. If we see these tests being flaky I will try to implement a locking mechanism or smt else.
Is it fine for now to leave it like this with a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the port is chosen randomly from the free ones, so you'd be very unlucky to have two times the same port

internal/ssh/ssh_test.go Outdated Show resolved Hide resolved
internal/ssh/ssh_test.go Outdated Show resolved Hide resolved
@SimoneDutto SimoneDutto force-pushed the JUJU-7352/add-ssh-jump branch from 9e4f2d4 to 1ac215f Compare January 13, 2025 08:00
@SimoneDutto SimoneDutto force-pushed the JUJU-7352/add-ssh-jump branch from 1ac215f to 33674e5 Compare January 13, 2025 08:21
internal/utils/utils.go Outdated Show resolved Hide resolved
@alesstimec alesstimec requested a review from ale8k January 13, 2025 08:30
internal/ssh/ssh.go Show resolved Hide resolved
}
}()
go func() {
defer srcDest.Close()
Copy link
Collaborator

Choose a reason for hiding this comment

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

won't we get a panic if we try to close the channel twice (in both goroutines)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// writeMu serializes calls to mux.conn.writePacket() and
// protects sentClose and packetPool. This mutex must be
// different from windowMu, as writePacket can block if there
// is a key exchange pending.
writeMu   sync.Mutex
sentClose bool

there is a mutex and a bool guarding the close msg

@SimoneDutto SimoneDutto merged commit eadc412 into canonical:v3 Jan 13, 2025
4 checks passed
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.

3 participants