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

Env: HTTPS Support #53959

Open
wants to merge 18 commits into
base: trunk
Choose a base branch
from
Open

Env: HTTPS Support #53959

wants to merge 18 commits into from

Conversation

bgoewert
Copy link

What?

Provides HTTPS support for wp-env. Also allows to define the ports used for HTTPS.

Why?

Addresses #8211

For another use case, I'm currently developing a plugin for CSP management, and Chromium browsers seemingly require HTTPS for the report-to directive. I'm sure there are other scenarios that require the use of HTTPS.

This will allow you to provide a certificate and key file that you create to enable HTTPS. There are many methods of creating an SSL, everyone has there preferences, and each system is different. By providing a means of passing a certificate and key, it allows the individual to use their own method.

How?

Creates Docker configurable ports for development (8883:443) and/or tests (8884:443). Copies the given certificate and key files into the container. Enables Apache mod_ssl. Modifies the default-ssl.conf to use the given certificate and key, and the nenables the default-ssl site config.

Testing Instructions

  1. Create a certificate and key if none exist. There are multiple methods to do this.

  2. Create a .wp-env.override.json file and add a path to valid certificate and key files.

    {
        "ssl": {
            "cert": "/path/to/crt",
            "key": "/path/to/key"
        }
    }
  3. Optionally set the WP_ENV_SSL_PORT and WP_ENV_SSL_TESTS_PORT environment variables.

  4. Start wp-env

  5. Navigate to https://localhost:8883 (or whatever you set the development/tests port to)

Additional Comments

I'm a new contributor to Gutenberg and fairly new to Node.js in general, not to mention Jest. I was not able to figure out what was required to add to the tests. Any help there would be great. If there are any other issues, let me know.

@github-actions
Copy link

github-actions bot commented Aug 25, 2023

Warning: Type of PR label mismatch

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

  • Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
  • Labels found: First-time Contributor, [Package] Env.

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

@github-actions
Copy link

Warning: Type of PR label error

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

  • Type-related labels to choose from: [Type] Accessibility (a11y), [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
  • Labels found: .

Read more about Type labels in Gutenberg.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Aug 25, 2023
@github-actions
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @bgoewert! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@ObliviousHarmony
Copy link
Contributor

I have a few concerns about this feature:

  • SSL certificates for local websites are inherently problematic because you really shouldn't be registering a domain that maps to a local IP.
  • Requiring the user to generate and pass an SSL certificate is a power user feature. This isn't a common task for most developers and they're going to have to follow platform-specific guides to generate and trust a self-signed certificate.
  • Supporting https:// URLs is important for some use-cases but I don't think those are very common.

Given the above, I personally don't think this specific feature belongs in wp-env. It seems pretty project-specific and should likely be implemented accordingly. A lot of the features in this pull request are already provided. You can map the SSL certificates using the mappings option and you can use lifecycleScripts.afterStart to update the web server config with wp-env run commands.

The real problem is that we aren't exposing the port necessary for this to work. Even if the web server is configured correctly, you can't access it from the host. What if, instead of this PR, we added a feature to provide custom port mappings to the generated docker-compose.yml file? That would support a more general use-case while letting you implement this in your project. For instance, it would let people easily support mapping a different XDebug port to avoid collisions. I would also suggest supporting the remapping of the MySQL port for the same reason. I'm not totally sure what that configuration might look like because the mysql container is a separate service though.

With the features mentioned above you would only need to run wp-env start and it would set everything up correctly.

@bgoewert
Copy link
Author

use lifecycleScripts.afterStart to update the web server config with wp-env run commands

I was unaware of this. Looking back I should have reviewed the readme.md further to see what exactly was possible already.

The real problem is that we aren't exposing the port necessary for this to work. What if, instead of this PR, we added a feature to provide custom port mappings to the generated docker-compose.yml file?

I agree, allowing for custom Docker port mappings would be great.

Before closing this PR to work on Docker port mappings...

Requiring the user to generate and pass an SSL certificate is a power user feature. This isn't a common task for most developers and they're going to have to follow platform-specific guides to generate and trust a self-signed certificate.

If we were to add functionality to create an SSL for non power users that simply want to enable HTTPS, would this be considered a value adding feature?

By potentially using something like devcert, we could generate a certificate, key, and a CA. There are security concerns with creating a CA as well. All that would be needed is setting "https": true before running wp-env start. The certificate and key could maybe be saved under ~/.wp-env/{env}/ssl. If a CA is generated, unsure where this could be safely saved for different systems, it could be used to sign new key pairs.

Any thoughts on this @ObliviousHarmony? Or should we just close this in favor of custom Docker port mappings?

@ObliviousHarmony
Copy link
Contributor

By potentially using something like devcert, we could generate a certificate, key, and a CA.

This is a super cool package! If we went this route I think we would want to make opting-in a bit more explicit though. Adding "https": true should auto-generate the certificates and set up the mapping. For trusting the certificate though I think it makes sense to have a separate command that they explicitly run and it can warn them of any implications. This seems pretty standard with these kinds of tools that heavy-handedly trust a local CA.

Given the more general use-case of port mapping, I think I'd prefer if we did that and revisited the need for including an SSL certificate in the future. Regardless of what we do in terms of SSL I think there's a benefit to the port mapping. It would be great to get some second opinions though such as from @noahtallen!

@noahtallen
Copy link
Member

noahtallen commented Aug 30, 2023

I definitely like the autogenerated SSL concept, which is more user-friendly! Mapping more ports is also a good idea; I think people have wanted a stable mysql port as an example. I'm not sure how to get that working though

@ObliviousHarmony
Copy link
Contributor

ObliviousHarmony commented Sep 1, 2023

I think people have wanted a stable mysql port as an example. I'm not sure how to get that working though

My suggestion @noahtallen would be that we add a new root-only (ports, portMapping, exposedPorts, ???) config option:

{
  "portMapping": {
    "database": {
      "3303": 33003
    },
    "development": {
      "4621": 44621
    },
    "tests": {
      "4621": 44622
    }
  }
}

One of the nice things about this approach is that, internally, we could actually just have port, testsPort, and environment.{env}.port map into the port map during config loading. Then when we build the Compose config we can just pass in the port map to each container rather than generating it there. This simplifies the code there and lets folks expose any ports they want to any other port they want.

We would just need validation to prevent any reserved ports from being mapped since they're configurable already. If we did an HTTPS feature for instance we might have an httpsPort option that should reserve 443.

To be honest this actually doesn't seem like very much work!

@noahtallen
Copy link
Member

What's the purpose of the mapping -- could we just say "xdebug: 12345" and handle the internal port internally?

@ObliviousHarmony
Copy link
Contributor

What's the purpose of the mapping -- could we just say "xdebug: 12345" and handle the internal port internally?

I suppose you're right 😄 Assuming that we move forward with an HTTP feature, perhaps some kind of config for mapping the ports of these individual services would be good?

{
  "ports": {
    "http": 80,
    "https": 443,
    "database": 33006
    "xdebug": 12345
  }
}

This would be an environment-specific config. I would say it gives meaningful control over the ports used by the services without supporting or encouraging custom ports. My thinking was that people might want to run commands to install other services within the environment. In this case it would be valuable to support custom ports. If that's something we'd want to support maybe we can take a hybrid approach? Support both keywords and a container port? Just food for thought!

@bgoewert
Copy link
Author

bgoewert commented Sep 11, 2023

If we were to have custom port mappings like this, and if at some point we support an "easy" HTTPS feature, would we just use the HTTP ports for HTTPS?

{
  "development": {
    "https": true,
    "ports": {
      "http": 8888
    }
  }
}

Or would that be too confusing and should we just allow both?

{
  "development": {
    "https": true,
    "ports": {
      "http": 8888,
      "https": 8443
    }
  }
}

Or if the root port is defined.

{
  "port": 8888,
  "development": {
    "https": true
  }
}

Also, the env http port should override the root port.

@ObliviousHarmony
Copy link
Contributor

Or would that be too confusing and should we just allow both?

I would say supporting both is ideal. As for naming, I'd probably suggest httpsPort or something of the sort.

@skorasaurus skorasaurus added the [Package] Env /packages/env label Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Package] Env /packages/env
4 participants