proposal: x/crypto/ssh: add callbacks to dynamically change server and client configuration

[drakkan]

I propose to add the following callbacks.

// ServerConfig holds server specific configuration data.
type ServerConfig struct {
        ...
        // ConfigForClientCallback, if not nil, is called after receiving the
	// version from the client. It may return a non-nil ServerConfig in order to
	// change the ServerConfig that will be used to handle this connection. If
	// the returned ServerConfig is nil, the original ServerConfig will be used.
	// The ServerConfig returned by this callback may not be subsequently
	// modified. If an error is returned the handshake will fail.
	ConfigForClientCallback func(conn ConnMetadata) (*ServerConfig, error)
}

// A ClientConfig structure is used to configure a Client. It must not be
// modified after having been passed to an SSH function.
type ClientConfig struct {
        ...
        // ConfigForServerCallback, if not nil, is called after receiving the
	// version from the server. It may return a non-nil ClientConfig in order to
	// change the ClientConfig that will be used to handle this connection. If
	// the returned ClientConfig is nil, the original ClientConfig will be used.
	// The ClientConfig returned by this callback may not be subsequently
	// modified. If an error is returned the handshake will fail.
	ConfigForServerCallback func(conn ConnMetadata) (*ClientConfig, error)

These additions enable many advanced use cases, for example we can enable/disable algorithms, authentication methods, banners etc. depending on the client or server version.

cc @golang/security

[ianlancetaylor]

A style note: we tend to avoid a leading Get on function/method names.

[drakkan]

A style note: we tend to avoid a leading Get on function/method names.

proposal updated, thank you!

[gopherbot]

Change https://go.dev/cl/519096 mentions this issue: ssh: add callbacks to dynamically change server and client configuration

[neild]

On naming: The equivalent in crypto/tls is Config.GetConfigForClient. There are a number of other tls.Config callbacks with a Get prefix.

ssh.ClientConfig and ssh.ServerConfig contain a number of existing fields named ...Callback.

I think that package-internal consistency is more important than global consistency, so the names in the current proposal of ConfigForClientCallback and ConfigForServerCallback seems good to me.

(The general idea of the proposal also seems reasonable to me, but someone more familiar with crypto/ssh would be a better judge of that.)

[hanwen]

is this really necessary? I think you could wrap the connection into something that gobbles the client version strings, creates a Config and then replays the client version string.

Or maybe we can further decompose NewServerConn into something that takes the Client version as well as the net.Conn.

[hanwen]

Adding a callback in the Config that returns a Config is a very confusing API, as it's not obvious in which step the config gets swapped out for something else.

[neil-ca]

El sáb., 29 de jul. de 2023 8:59 AM, Nicola Murino ***@***.***> escribió:

…

I propose to add the following callbacks. // ServerConfig holds server specific configuration data. type ServerConfig struct { ... // GetConfigForClientCallback, if not nil, is called after receiving the // version from the client. It may return a non-nil ServerConfig in order to // change the ServerConfig that will be used to handle this connection. If // the returned ServerConfig is nil, the original ServerConfig will be used. // The ServerConfig returned by this callback may not be subsequently // modified. If an error is returned the handshake will fail. GetConfigForClientCallback func(conn ConnMetadata) (*ServerConfig, error) } // A ClientConfig structure is used to configure a Client. It must not be // modified after having been passed to an SSH function. type ClientConfig struct { ... // GetConfigForServerCallback, if not nil, is called after receiving the // version from the server. It may return a non-nil ClientConfig in order to // change the ClientConfig that will be used to handle this connection. If // the returned ClientConfig is nil, the original ClientConfig will be used. // The ClientConfig returned by this callback may not be subsequently // modified. If an error is returned the handshake will fail. GetConfigForServerCallback func(conn ConnMetadata) (*ClientConfig, error) These additions enable many advanced use cases, for example we can enable/disable algorithms, authentication methods, banners etc. depending on the client or server version. cc @golang/security <https://github.com/orgs/golang/teams/security> — Reply to this email directly, view it on GitHub <#61650>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJLWG3HKANZUDQJYHYXAYY3XSUCK3ANCNFSM6AAAAAA24PVFKI> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[hanwen]

See https://go-review.git.corp.google.com/c/crypto/+/538555 for what I mean.

Also note (as documented in the change description) that there is really no need to have anything in the ClientConfig. Since the client picks the algorithms to use, the client can just specify all algorithms in preference order and still be both secure (for newer servers) and interoperable with older servers.

[drakkan]

@hanwen, thank you. So an alternative proposal is this one

// ServerConfig holds server specific configuration data.
type ServerConfig struct {
   ...
   // ClientVersion is the client version to assume. If set, the
   // version handshake is skipped. In this case, callers must
   // explicitly call ExchangeVersions to obtain the remote version.
   ClientVersion string
}

maybe add the same to ClientConfig.

// A ClientConfig structure is used to configure a Client. It must not be
// modified after having been passed to an SSH function.
type ClientConfig struct {
   ...
   // ServerVersion is the server version to assume. If set, the
   // version handshake is skipped. In this case, callers must
   // explicitly call ExchangeVersions to obtain the remote version.
   ServerVersion string 
}

make the currently private exchangeVersions function public

[hanwen]

I should add: 1) I understand what this issue is asking for, but 2) I find it hard to coherently explain what security benefit it creates.

As discussed, the client doesn't have to adapt to the server to begin with.

But what use is it for the server to adapt to the client? If the server just supports all algorithms (including the weak ones), a client with a new binary will still get strong algorithms, and an old client will get the weak algorithms.

So, what this feature would achieve is that the server looks like it only supports strong algorithms during cursory inspection (eg. probing the server), but it actually is less secure because clients can fallback to something insecure if they say the right version number.

The way to increase security is to get clients to upgrade their binaries. Refusing to serve them is the typical way to force this to happen, but if we expose negotiated algorithms (#58523), you could also use a banner message to get that message across.