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

Implement support for the PROXY protocol #108

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Implement support for the PROXY protocol #108

wants to merge 4 commits into from

Conversation

tsipinakis
Copy link
Member

Please describe the change you are making

Implement support for the PROXY protocol. Sadly this cannot easily be tested as there are no user-visible changes if this is implemented correctly. (Header is transparently accepted/rejected)

Are you the owner of the code you are sending in, or do you have permission of the owner?

Sent with permission of the owner

The code will be published under the MIT-0 license. Have you read and understood this license?

Yes

@@ -15,6 +15,7 @@ import (
messageCodes "github.com/containerssh/libcontainerssh/message"
"github.com/containerssh/libcontainerssh/service"
"golang.org/x/crypto/ssh"
"github.com/pires/go-proxyproto"
Copy link

Choose a reason for hiding this comment

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

This is a third party library and should be abstracted away with its own package, including tests so it can be replaced if need be.

netListener, err := listenConfig.Listen(lifecycle.Context(), "tcp", s.cfg.Listen)
if err != nil {
s.lock.Unlock()
return messageCodes.Wrap(err, messageCodes.ESSHStartFailed, "failed to start SSH server on %s", s.cfg.Listen)
}
if useProxy {
Copy link

Choose a reason for hiding this comment

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

It seems to me that PROXY is a wrapper protocol like many others (e.g. websocket). We should consider creating a generic abstraction for wrapper protocols.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't foresee having any other wrapping protocols in-front of the ssh server since, we can consider that if it's necessary further down the line.

internal/sshserver/serverImpl.go Outdated Show resolved Hide resolved
@tsipinakis
Copy link
Member Author

@janosdebugs This one is ready for another review as well

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.

1 participant