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

Static configuration of webserver and dbserver localhost ports, bind only to localhost, fixes #1491, fixes #941, #642 too #1502

Merged
merged 26 commits into from Mar 28, 2019

Conversation

Projects
None yet
2 participants
@rfay
Copy link
Member

rfay commented Mar 20, 2019

The Problem/Issue/Bug:

fixes #1491, fixes #941

OP #1491: For security reasons, host binding of the dbserver should only be localhost (127.0.0.1) not 0.0.0.0 (the default, all interfaces), because this allows other people on the network to possibly probe for the database connection, depending on firewall configuration.

OP #941: Keep a stable port number for the localhost ephemeral ports so that people can use standard techniques to access them.

#642: We've long talked about keeping a catalog of metadata even for projects that aren't running. This PR introduces that.

How this PR Solves The Problem:

  • add host_db_port and host_webserver_portto config.yaml, use those values in the docker-compose.yaml.
  • When a new host_db_port is written, add it to ~/.ddev/global_config.yaml so it's blacklisted from being used again for another project.
  • Change the generated docker-compose.yaml to use explicit binding to 127.0.0.1 instead of the implicit 0.0.0.0

This is actually an amazing step forward for us. For the first time it provides a global database of projects, as requested in #642 . It's now trivial to add additional project information in the global_config.yaml.

Manual Testing Instructions:

  • Experiment with host_db_port and host_webserver port, both generated and explicitly configured.
  • Verify that one project can't poach another project's ports.
  • Verify that used ports show up in ~/.ddev/global_config.yaml
  • Verify that db and web ports can only be accessed on the localhost interface, not via other interfaces on the machine. (You can use ifconfig on macOS and Linux to view other interfaces). You can test with telnet localhost 33434 for port 33434, telnet 172.28.99.33 33434 for a denied access to the same port on the 172.28.99.33 IP address (you must use the one that pertains to your system). Alternately, check from another machine on your network, but this requires the firewall to be turned off for correctness.
  • Verify maintenance of ~/.ddev/global_config.yaml when projects are removed and rm --omit-data (which should completely remove the host)

Automated Testing Overview:

  • Test for util.GetFreePort()
  • Test for access to used ports. (The existing router tests have some code that should help).
  • Test to make sure that one project's ports can be reused deliberately or accidentally by another one.
  • Explicitly test to make sure localhost mysql and http port can be connected to correctly. This was only caught by accident on TestWebserverType (Docker Toolbox)

Release/Deployment notes:

We'll probably want to tell people to do a ddev config on existing projects, which will add the new items in config.yaml.

@rfay rfay added this to the v1.6.1 milestone Mar 20, 2019

@rfay rfay self-assigned this Mar 20, 2019

@rfay rfay changed the title Static configuration of webserver and dbserber localhost ports, bind only to localhost, fixes #1491, fixes #941 Static configuration of webserver and dbserver localhost ports, bind only to localhost, fixes #1491, fixes #941 Mar 20, 2019

@rfay

This comment has been minimized.

Copy link
Member Author

rfay commented Mar 20, 2019

Artifacts for this round are at https://circleci.com/gh/drud/ddev/10976#artifacts/containers/0 and testing would be fantastic. @tmotyl and @t3easy

@rfay rfay force-pushed the rfay:20190320_bind_to_localhost branch from d4e31e8 to a0c8451 Mar 21, 2019

@rfay rfay changed the title Static configuration of webserver and dbserver localhost ports, bind only to localhost, fixes #1491, fixes #941 Static configuration of webserver and dbserver localhost ports, bind only to localhost, fixes #1491, fixes #941, #642 too Mar 24, 2019

@rfay rfay requested a review from andrewfrench Mar 25, 2019

@rfay

This comment has been minimized.

Copy link
Member Author

rfay commented Mar 25, 2019

@andrewfrench this one requires a significant effort both with the code review and manual testing and your review of the basic approach. I think it's good, but sure look forward to your opinion.

I guess the biggest basic design question is whether the host database should be kept in global_config.yaml (as currently) or split into a different file. We'll see what you think.

I also took a few minutes to think about docs and decided it didn't need extra, but interested in your opinion. The new items for the host db and web ports are documented in the text at the bottom of config.yaml.

@rfay rfay force-pushed the rfay:20190320_bind_to_localhost branch from fff68fb to 971778f Mar 25, 2019

@rfay rfay removed the needs docs label Mar 25, 2019

@andrewfrench

This comment has been minimized.

Copy link
Member

andrewfrench commented Mar 26, 2019

Hey @rfay, I'm working through review of this now and it adds a huge amount of both immediate utility and future potential for expanding on the local database of projects idea.

Managing port claims in global_config.ddev works for me, but I was able to spot some issues regarding the way project ports are claimed in global_config.ddev: after a project is configured and its ports are claimed under the project's name, the normal methods of changing the project name cause failed starts, like editing the "name" field in .ddev/config.yaml or ddev config and proving another name option. A user can bypass this by editing global_config.yaml, but we don't have any messaging to indicate this, only that the port is claimed by the project's previous name.

I can imagine a few solutions to this:

  • Creating a primary key that's not editable by the user (like a UUID, for example) to create a consistent and unchanging handle that can be used to manage a project
  • Handling the project name rewrite inside ddev config [--project-name xyz] and asking users to use that command to change a project name
  • Keep the logic as is, but add more verbose error messaging when a project fails to start telling the user what to do to solve the problem
@rfay

This comment has been minimized.

Copy link
Member Author

rfay commented Mar 26, 2019

Wow @andrewfrench I think you just saved a lot of people a lot of pain. Thanks for checking that carefully.

I think the fundamental error I've introduced is reserving the port for every single project, when only a few people have actually requested it. I'll experiment with only reserving globally when explicitly specified in project config.yaml. It was just wrong to have a second global reservation at this point (besides project name)

My initial response to the general issue is that we've always used project name as the unique systemwide identifier, but we haven't ever enforced it. As a result of this undocumented assumption, we have occasional failures like

  • Stolen databases (database is keyed by name) so if you start a new project with the same name it adopts (and may damage) another project's db.
  • Conflicting containers: Most of us have seen this occasionally, and we even have tests for it. And it's annoying.

I suspect we can solve both of those pretty easily as this functionality gets fleshed out. In the next sprint we can add the project directory and it will solve an enormous amount of this kind of trouble, and also allow ddev start of a rm'd project.

@rfay

This comment has been minimized.

Copy link
Member Author

rfay commented Mar 26, 2019

@andrewfrench I changed the logic so that the port is dynamic unless specified in the config.yaml, and not reserved in the global_config.yaml unless it's set as static in config.yaml. I think this is far more in keeping with the request, and should solve the problem you described.

// ReservePorts() adds the ProjectInfo if necessary and assigns the reserved ports
func ReservePorts(projectName string, ports []string) error {
// If the project doesn't exist, add it.
_, ok := DdevGlobalConfig.ProjectList[projectName]

This comment has been minimized.

Copy link
@andrewfrench

andrewfrench Mar 26, 2019

Member

After playing around with this post-update, I think there should be a check either here or perhaps in the caller that returns early if len(ports) == 0. As written, an empty ProjectInfo struct and usedhostports list are written to global_config.yaml, even if no ports are reserved -- not the end of the world, but adds some unneeded clutter to that config file.

This comment has been minimized.

Copy link
@rfay

rfay Mar 27, 2019

Author Member

This sounded like a great idea, and I started working on it... but since I was a good boy and used a struct instead of a map, I can't do it without removing the entire ProjectInfo, which would mess up future uses of the list.

@andrewfrench

This comment has been minimized.

Copy link
Member

andrewfrench commented Mar 26, 2019

I noticed that host_webserver_port and host_db_port aren't configurable through command-line flags, would you be open to adding those options (ddev config --host-db-port 1515 --host-webserver-port 5151)?

@andrewfrench
Copy link
Member

andrewfrench left a comment

Approving after confirming that this works quite well, especially after only reserving ports when requested. I've left a couple notes about non-blocking preferences, but this does a great job of:

  • Reserving a webserver/db port when requested in .ddev/config.yaml
  • Preventing other projects from reserving the ports already reserved by another project in global_config.yaml
  • Running containers on their requested ports
  • Removing port reservations from global_config.yaml on ddev rm --remove-data

@rfay rfay force-pushed the rfay:20190320_bind_to_localhost branch from f4cf414 to fe9e614 Mar 27, 2019

@rfay

This comment has been minimized.

Copy link
Member Author

rfay commented Mar 27, 2019

I added the config flags and some test coverage. Couldn't get rid of the empty used_host_ports in global_config unless you have a suggestion about how to do that @andrewfrench . Thanks for the careful review. Saved an enormous amount of future pain.

@@ -29,7 +29,7 @@ func init() {
}

type ProjectInfo struct {
UsedHostPorts []string `yaml:"used_host_ports,flow"`
UsedHostPorts []string `yaml:"used_host_ports,omitempty,flow"`

This comment has been minimized.

Copy link
@andrewfrench

andrewfrench Mar 27, 2019

Member

I confirmed that this will hide the used_host_ports list when none were requested, but also played around with the logic in CheckAndReserveHostPorts (ddevapp/config.go) that prevents the project info map record from being generated when no relevant port information was provided, I'll comment there as well.

return err
}
}
err := globalconfig.ReservePorts(app.Name, portsToReserve)

This comment has been minimized.

Copy link
@andrewfrench

andrewfrench Mar 27, 2019

Member

If this call to globalconfig.ReservePorts is moved up inside the if block right before it (if len(portsToReserve) > 0 {), the project info map won't be written to the global config file in the case where no ports are requested. There may be some unintended consequences there that I'm not considering, but it does seem a touch cleaner in the general case from what I can tell.

This comment has been minimized.

Copy link
@rfay

rfay Mar 27, 2019

Author Member

Yeah, but I want the project info map always, for future use. I'm expecting it to be there for every project by the next sprint, containing a minimum of the approot. Given that I want it in the projectlist, are you good with it?

This comment has been minimized.

Copy link
@andrewfrench

andrewfrench Mar 27, 2019

Member

Yep, in that case it's totally fine after your most recent commit.

@rfay rfay merged commit 9fb54a4 into drud:master Mar 28, 2019

9 checks passed

buildkite/ddev-containers-windows Build #1541 passed (28 minutes, 15 seconds)
Details
buildkite/ddev-macos Build #1916 passed (34 minutes, 59 seconds)
Details
buildkite/ddev-macos-use-nfsmount Build #147 passed (45 minutes, 25 seconds)
Details
buildkite/ddev-windows-apache-fpm Build #1108 passed (1 hour, 5 minutes, 30 seconds)
Details
buildkite/ddev-windows-dockerforwindows Build #2079 passed (58 minutes, 58 seconds)
Details
buildkite/ddev-windows-dockerforwindows-nfs Build #225 passed (1 hour, 2 minutes, 54 seconds)
Details
buildkite/ddev-windows-dockertoolbox Build #1685 passed (55 minutes, 16 seconds)
Details
buildkite/ddev-windows-dockertoolbox-nfs Build #221 passed (55 minutes, 5 seconds)
Details
license/cla Contributor License Agreement is signed.
Details

@rfay rfay deleted the rfay:20190320_bind_to_localhost branch Mar 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.