Skip to content

Add ConnectionClosedCallback #244

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

Open
SimoneDutto opened this issue Jan 17, 2025 · 3 comments
Open

Add ConnectionClosedCallback #244

SimoneDutto opened this issue Jan 17, 2025 · 3 comments

Comments

@SimoneDutto
Copy link

Hello,
I am trying to implement a MaxConcurrentConnections for the ssh server.
I've notice the library already offers a ConnCallback and ConnectionFailedCallback.
However that seems not to be enough to create a guard around the number of maximum connection because we are missing the ConnectionClosedCallback.

With something like ConnectionClosedCallback I could simply implement my rate limiter by doing:

ssh.Server{
  ConnCallback: func(ctx ssh.Context, conn net.Conn) net.Conn {
	mux.Lock()
        defer mux.Unlock()
        if n > maxConnections {
		conn.Close()
		return conn
	}
	n ++
        return conn
  },
  ConnectionFailedCallback: func(conn net.Conn, err error) {
        mux.Lock()
        defer mux.Unlock()
  	n--
  },
  ConnectionClosedCallback: func(){
        mux.Lock()
        defer mux.Unlock()
  	n--
  }
}

I'll put a PR up just to show you how I've tried it to implement the ConnectionClosedCallback, but I am of course open to suggestions!

@SimoneDutto SimoneDutto changed the title Add CloseConnectionCallBack Add ClosedConnectionCallBack Jan 17, 2025
@SimoneDutto SimoneDutto changed the title Add ClosedConnectionCallBack Add ClosedConnectionCallback Jan 17, 2025
@SimoneDutto SimoneDutto changed the title Add ClosedConnectionCallback Add ConnectionClosedCallback Jan 17, 2025
@wolveix
Copy link

wolveix commented Mar 15, 2025

@SimoneDutto you can get around this by listening to the session context. Just spin off a goroutine in ConnCallback and wait for <- ctx.Done(), then do what you need to do.

E.g.:

ConnCallback: func(ctx ssh.Context, conn net.Conn) net.Conn {
	go func() {
		<-ctx.Done()
		fmt.Println("Connection concluded")
	}()

	return conn
},

@SimoneDutto
Copy link
Author

@wolveix i didn't know that, thank you so much!
I have another question:
From my test ctx.Done() channel is filled even in case of errors returned by one of the Handlers. Can you confirm that?

@wolveix
Copy link

wolveix commented Mar 18, 2025

@SimoneDutto you're very welcome :)

Yeah, context should always conclude. Looking at this library's source code, they don't set a timeout for the context itself; however, they initialize it with a cancel function and always call it. So unless a panic occurs within the library, the context will conclude :)

jujubot added a commit to juju/juju that referenced this issue Mar 26, 2025
…onnections

#19236

In this PR we hook the max concurrent connections into the ssh server. It works by checking for the number of connections via an atomic.Int in the connectionCallback.
It was originally suggested in a gliderlab ssh issue: gliderlabs/ssh#244

The error we write down the conn is not displayed by ssh clients, because they expect a ssh handshake. However if you telnet to a maxed ssh server to debug it, you could see the `too many connections` error, which is helpful.

## Checklist

<!-- If an item is not applicable, use `~strikethrough~`. -->

- [x] Code style: imports ordered, good names, simple structure, etc
- [x] Comments saying why design decisions were made
- [x] [Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing

## QA steps
```bash
juju bootstrap lxd test-max-conns --build-agent
juju controller-config ssh-max-concurrent-connections=10 #make it easier to qa
juju show-controller #to get controller ip
```

```console
$ ssh -J admin@<controller_ip>:17022 bob@e12e3493-d2f7-42e2-8049-8cb624dd0a4e
Your final destination is: e12e3493-d2f7-42e2-8049-8cb624dd0a4e as user: bob
Connection to e12e3493-d2f7-42e2-8049-8cb624dd0a4e closed.
```

Send multiple concurrent ssh connections
```console
$ seq 16 | parallel -j 16 "ssh -J admin@10.229.36.38:17022 -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null bob@e12e3493-d2f7-42e2-8049-8cb624dd0a4e"
kex_exchange_identification: read: Connection reset by peer
Connection reset by 10.229.36.38 port 17022
Connection closed by UNKNOWN port 65535
Connection closed by 10.229.36.38 port 17022
Connection closed by UNKNOWN port 65535
Connection closed by 10.229.36.38 port 17022
Connection closed by UNKNOWN port 65535
Connection closed by 10.229.36.38 port 17022
Connection closed by UNKNOWN port 65535
Connection closed by 10.229.36.38 port 17022
Connection closed by UNKNOWN port 65535
Connection closed by 10.229.36.38 port 17022
Connection closed by UNKNOWN port 65535
Your final destination is: e12e3493-d2f7-42e2-8049-8cb624dd0a4e as user: bob
Warning: Permanently added 'e12e3493-d2f7-42e2-8049-8cb624dd0a4e' (RSA) to the list of known hosts.
Your final destination is: e12e3493-d2f7-42e2-8049-8cb624dd0a4e as user: bob
Warning: Permanently added 'e12e3493-d2f7-42e2-8049-8cb624dd0a4e' (RSA) to the list of known hosts.
Your final destination is: e12e3493-d2f7-42e2-8049-8cb624dd0a4e as user: bob
Warning: Permanently added 'e12e3493-d2f7-42e2-8049-8cb624dd0a4e' (RSA) to the list of known hosts.
Your final destination is: e12e3493-d2f7-42e2-8049-8cb624dd0a4e as user: bob
Warning: Permanently added 'e12e3493-d2f7-42e2-8049-8cb624dd0a4e' (RSA) to the list of known hosts.
Your final destination is: e12e3493-d2f7-42e2-8049-8cb624dd0a4e as user: bob
Warning: Permanently added 'e12e3493-d2f7-42e2-8049-8cb624dd0a4e' (RSA) to the list of known hosts.
Your final destination is: e12e3493-d2f7-42e2-8049-8cb624dd0a4e as user: bob
Warning: Permanently added 'e12e3493-d2f7-42e2-8049-8cb624dd0a4e' (RSA) to the list of known hosts.
Your final destination is: e12e3493-d2f7-42e2-8049-8cb624dd0a4e as user: bob
Warning: Permanently added 'e12e3493-d2f7-42e2-8049-8cb624dd0a4e' (RSA) to the list of known hosts.
Your final destination is: e12e3493-d2f7-42e2-8049-8cb624dd0a4e as user: bob
Warning: Permanently added 'e12e3493-d2f7-42e2-8049-8cb624dd0a4e' (RSA) to the list of known hosts.
Your final destination is: e12e3493-d2f7-42e2-8049-8cb624dd0a4e as user: bob
Warning: Permanently added 'e12e3493-d2f7-42e2-8049-8cb624dd0a4e' (RSA) to the list of known hosts.
Your final destination is: e12e3493-d2f7-42e2-8049-8cb624dd0a4e as user: bob
Warning: Permanently added 'e12e3493-d2f7-42e2-8049-8cb624dd0a4e' (RSA) to the list of known hosts.

```
(this output can vary depending on how many requests you actually managed to send concurrently)


You should see as many "your final destination ..." as your max concurrent connections.

To see the metric being exposed by the inspection worker you can:
```bash
juju ssh 0
sudo curl --unix-socket /var/lib/juju/agents/machine-0/introspection.socket http://localhost/depengine
```
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

No branches or pull requests

2 participants