qdeconinck / mp-quic

Please read https://multipath-quic.org/2017/12/09/artifacts-available.html to figure out how to setup the code.
MIT License
176 stars 71 forks source link

Unable to set CreatePaths in server Config #35

Closed jmwample closed 2 years ago

jmwample commented 2 years ago

While the intention of the populateServerConfig function seems to be to enable path creation by default (the comments explicitly say that we want to "Grant this ability by default for a server"), and the implementation sets the CreatePaths value to true for the passed *Config... https://github.com/qdeconinck/mp-quic/blob/7a91883871b64bd01e991bcb732dc0b9918354d6/server.go#L166-L171

... the value of the passed config.CreatePaths is never propagated to the returned *Config meaning that it takes the default bool value (false) whenever populateServerConfig is called. https://github.com/qdeconinck/mp-quic/blob/7a91883871b64bd01e991bcb732dc0b9918354d6/server.go#L200-L209

populateServerConfig is in the call chain for all of the Listen* functions which means that unless you are creating the internal server struct with a config that has CreatePaths set, it is impossible to set the value to true.

Further down the call path, when the value of session.createPaths is false, the pathManager will never call createPaths in run which means that the pathManagers on the server never get a chance to call advertiseAddresses.

Impact:

This bug means that server pathManagers will never call createPaths -- preventing them from advertising local addresses to clients, and client pathManagers are currently prevented from advertising paths. I believe this means that configurations where the client has multiple addresses and the server has a single address will work as expected as the client will create connections from each suitable local address to the singular remote it knows about. The server will integrate these connections into the session properly using createPathFromRemote. However, in configurations where the server has multiple available addresses, the client will not be able discover the additional paths.

Fix:

Propagate the value of config.CreatePaths in populateServerConfig.

// populateServerConfig populates fields in the quic.Config with their default values, if none are set
// it may be called with nil
func populateServerConfig(config *Config) *Config {
...
    return &Config{
        Versions:                              versions,
...
        MaxReceiveStreamFlowControlWindow:     maxReceiveStreamFlowControlWindow,
        MaxReceiveConnectionFlowControlWindow: maxReceiveConnectionFlowControlWindow,
+       CreatePaths:                   config.CreatePaths,
    }
}

TLDR: The value of CreatePaths in the Config is not forwarded in populateServerConfig meaning that the current implementation does not allow the server to enable path advertisement no matter what value is passed in via config in the Listen* functions.