Add ssl command to download mkcert and generate ssl certificates #465

shadyvb · 2022-04-22T06:02:12Z

ref https://github.com/humanmade/product-dev/issues/987

roborourke

Nice start, pretty cool that it works like that. There's a few potential gotchas around the platform installation I've commented on and some linter errors to fix up (some dumb ones but hey).

I think next steps are:

Add support for reading TLDs to generate certs for from config, default to altis.dev and maybe local and localhost for now
Update docs, plus FAQ as noted in inline comments
Run the install and generate subcommands on server start (maybe an option flag or config option to not do this would be useful?)

I'm also thinking in this instance it might be worth punting the code to another class, and making the subcommands into class methods. The same can be said for more of the other commands too so we can maybe refactor at some point. Don't do anything for now, I'm just thinking about code reuse or some of the checks & output.

inc/composer/class-command.php

roborourke · 2022-04-25T15:12:54Z

inc/composer/class-command.php

+						break;
+					# Windows
+					case 'Windows':
+						$binary_arch = 'windows-amd64.exe';


Will need to check what the situation with this and WSL is. If you can run .exe from WSL, or global commands from there we might be ok. @missjwo might be able to assist in testing.

Yes, testing cross-platform was supposed to be the next step. I can do that in a VM.

WSL won’t replicate accurately within a VM so far as I know, since it’s a VM itself.

For WSL though, the arch will be Linux because it’s literally a Linux VM you’re running inside of.

If composer is running within WSL, the certificate would be automatically installed to Windows store itself, which is a problem. We'll need to add docs on manually accepting the root CA.

Also, does this mean we do not support Windows itself but rather WSL ? ( I have not used WSL nor Docker on Windows )

Yeah there was a PR to support windows natively but I think there were some limitations to that. We do have a way to check it’s a WSL environment at least.

You can run .exe files from WSL https://docs.microsoft.com/en-us/windows/wsl/filesystems

Happy to test stuff - just let me know when the PR is ready to be tested and what i should be looking out for.

@shadyvb Correct, only WSL is supported, per https://docs.altis-dxp.com/local-server/windows/

You can execute exe files, yeah; WSL will transparently map execution of those, and execute them. Note though, php_uname( 's' ) will return Linux because it is Linux. We need to use the Command::is_wsl() function to detect WSL, not uname.

Also happy to test with my WSL env, and screenshare as needed.

Happy to test on my windows machine. Let me know when the PR is ready to be tested.

inc/composer/class-command.php

shadyvb · 2022-04-25T16:51:44Z

@roborourke the domain detection and programmatic generation has been added to #466 , does not factor localhost and local though, unsure how would those be used, but should be easy enough to add.

roborourke · 2022-04-25T17:02:15Z

@shadyvb yeah sorry - I was just looking there and realised that. Also verified what I was thinking about localhost and local - I was incorrect. I've linked to an avahi-daemon container that we may be able to use alongside the traefik proxy.

My thinking is if we can support .localhost and maybe .local OOTB and document how to add hosts entries for anything else that might be enough.

shadyvb · 2022-04-25T17:04:23Z

We can support ANY TLD when it comes to SSL certificates, if .local / .localhost would alleviate the need for DNS resolution then let's do that, unsure about that though. Either way, outside of the scope here.

roborourke

Something just occurred to me re. the DNS resolution etc... right now anything .altis.dev resolves to 127.0.0.1, including sub sub domains like foo.bar.altis.dev.

So - we can keep using altis.dev as the default TLD for local dev, but, we can now generate a certificate for it locally rather than having to purchase or update a real one that only works for *.altis.dev. Not only can we generate a local cert for *.altis.dev, but also for *.<project>.altis.dev. Boom, sub sub domains with https, no need to worry about avahi/zeroconf etc etc...

inc/composer/class-command.php

Co-authored-by: Robert O'Rourke <[email protected]>

docker/conf/traefik.toml

roborourke · 2022-04-28T10:20:14Z

inc/composer/class-command.php

@@ -211,6 +220,8 @@ protected function get_env() : array {
 	protected function start( InputInterface $input, OutputInterface $output ) {
 		$output->writeln( '<info>Starting...</>' );

+		$this->check_ssl_certificate( $input, $output );


I don't think we want to do this if the module config has secure set to false.

Agreed 👍

Actually, Traefik will still expect the SSL certificate to exist, so it's safer to just generate the file either way.

Ah right. Hmm. That's based on the current static traefik.toml file right. I was more conscious of this code running on codespaces, not sure if it'll work or not there.

roborourke · 2022-04-28T10:27:27Z

inc/composer/class-command.php

+				break;
+			case 'generate':
+				// TODO figure out how to programmatically detect the domains to use.
+				$domains = $input->getArgument( 'options' )[1] ?? '*.altis.dev';


I think the default behaviour here should be the following:

Always generate certs for <project>.<tld> and *.<project>.<tld> (no harm in them being there by default, even if someone calls the function with other domains, its better to avoid surprises here IMO than assume someone knows it might be a deleterious action if we don't explicitly call that out)

Add support for a list of hosts in the config and merge those in by default too, we can document that custom ones must be pointed to localhost

Agreed 👍

Traefik will need to know about the extra domains too 🤔

Added the extra domains support to nginx ⬆️

Also added the configured domain(s) to the domains within generate() command as discussed, also altis.dev *.altis.dev.

inc/composer/class-command.php

shadyvb · 2022-05-03T06:22:16Z

@roborourke there is a dilemma here, when the site name is changed in composer.json, the naming convention for containers changes, and that brings new containers instead of reusing old ones, and leaves the old ones running.

Related to the custom domain configuration rather than SSL.

shadyvb · 2022-05-03T06:23:24Z

Related to the above ^, we can instruct people to destroy containers before changing domains, so we don't have to programmatically do that ( we do have a reference to the previous domain, but don't think it's worth the effort ).

shadyvb · 2022-05-03T07:08:49Z

Ah, another issue, since we're generating Traefik's certificate based on the project parameters ( name, tld, extra domains ), the resulting certificate won't really work for other projects (other local-server instances running on the same host), so it'll stop working there IF they're not using the default altis.dev ( which is included in certificate generation now ).

shadyvb · 2022-05-04T02:49:34Z

Labels on the proxy? Not sure I follow completely, but definitely have a play around!

Not on the proxy itself, but on containers, i.e. you put the domains as labels on web containers, and then query them while regenerating the certificate via something like docker ps -q --filter 'label=traefik.port' | xargs -I{} docker inspect {} --format='{{index .Config.Labels "traefik.port"}}' (substituting traefik.port with our custom key e.g. traefik.domain).

shadyvb · 2022-05-04T02:53:49Z

.. same can be done if we decide to manage the hosts entry automagically as well, we'd have the list of domains that needs to be added.

shadyvb · 2022-05-04T03:56:35Z

S3, Tachyon, and Analytics services are now working as expected after 81ab150 and 90ce957

shadyvb · 2022-05-04T04:17:32Z

Added a notice about missing hosts entries with a full line to copy/paste into /etc/hosts.

Figure out complexity of managing custom domains across all running instances of Altis local-server > Should be straight-forward by labeling containers with domains and collecting those labels via the command above to generate the certificate, will defer that to a new issue though so we can ship this change here sooner.
Fix tests
Document the changes

roborourke

One very minor suggestion to just return the exit codes versus using exit( 1 ); but it's up to you, the result is exactly the same.

This is really thorough, well thought out and makes this feature about as helpful for devs as it can possibly be. Great work bud

inc/composer/class-docker-compose-generator.php

inc/composer/class-command.php

shadyvb · 2022-05-05T12:38:43Z

Further actions:

Documentation
Add a new issue to support custom domains across different running instances through labels > Support custom domains across all running instances of Altis local-server #475

roborourke · 2022-05-05T14:01:06Z

docs/README.md

 You may find that file sharing performance or server response times are slower than you would like on Windows or MacOS. Local Server provides an experimental integration with [Mutagen](https://mutagen.io/) to resolve this.

 See the [Mutagen set up guide for detailed instructions on how to install and run it](./mutagen-file-sharing.md).

+#### Subdomain and Custom domains in multisites


Do we want this under experimental features? Custom domains / codespaces perhaps but I think the SSL stuff itself doesn't need to be considered experimental as we need it to work for the default TLD at the very least.

It's experimental in the sense that it is tied to custom domains which is experimental 🤔

Ah, and we might want to remove it altogether in favor of Traefik managing it on its own, given the centralization needed. I'm writing the issue around that that would explain my suggested approach.

I think it’s fine to not list it as experimental personally, codespaces definitely will be to start with

@roborourke Updated docs in 2f11c98 , should be ready for a final review.

- humanmade/altis-local-server#465 - humanmade/altis-local-server#479

Add ssl command to download mkcert and generate ssl certificates

480f504

shadyvb requested a review from roborourke April 22, 2022 06:02

shadyvb mentioned this pull request Apr 23, 2022

Codespaces support, custom domain support, ssl cert support, what else? #466

Closed

roborourke reviewed Apr 25, 2022

View reviewed changes

shadyvb added 10 commits April 25, 2022 16:55

Add link to install mkcert after errors installing it

3b1dd4f

Remove redundant return statement

824746e

Improve logging around errors with ssl

901b3b9

Remove redundant output

afad00a

Removed duplicate handling of get_mkcert_binary()

f2786f9

Fix docblock

9441243

💅 Fix CS

26494bd

Programmatically create SSL certificate and use with Traefik

b0bb233

Allow custom domain name/tld from #341

1be8adf

Fix function args

60baaf5

roborourke reviewed Apr 27, 2022

View reviewed changes

inc/composer/class-command.php Outdated Show resolved Hide resolved

shadyvb and others added 4 commits April 27, 2022 10:10

Fix an error with the run command

aa942fc

Skip verifying https for S3 requests

5c025de

Try to fix s3 bucket/path mapping

97e4ce9

Better detect WSL environment

cfec957

Co-authored-by: Robert O'Rourke <[email protected]>

roborourke reviewed Apr 28, 2022

View reviewed changes

shadyvb added 4 commits May 3, 2022 09:12

Restart the proxy container after generating a certificate

9827897

Add support for extra custom domains

7f36778

Stop starting if domain has changed to avoid orphan containers

af3b6c0

Only attempt to restart the proxy container if it is running

859114c

shadyvb added 2 commits May 4, 2022 05:50

Add aux service URLs to generated certificate

81ab150

Fix S3 and Tachyon issues with bucket path

90ce957

Add a warning for missing hosts entries

4e6fd76

shadyvb added 9 commits May 4, 2022 06:23

Fix indentation

7c0a020

Remove relative path use in traefik config

8a79a58

Fix generation of hosts entries note

2634b85

Fix generation of cert around default domain

0d6430e

Less testing output

6c187a1

Do not generate SSL for altis.dev

bd8eabc

More efficient domain selection

37518b7

Correct typo

b164550

Add traefik.domain label for future multi-instance SSL generation

72c6318

shadyvb requested a review from roborourke May 4, 2022 12:19

roborourke approved these changes May 5, 2022

View reviewed changes

inc/composer/class-docker-compose-generator.php Show resolved Hide resolved

inc/composer/class-command.php Outdated Show resolved Hide resolved

inc/composer/class-command.php Outdated Show resolved Hide resolved

Return instead of exit

b008194

Document custom domains and SSL command features

4e26c8f

roborourke reviewed May 5, 2022

View reviewed changes

Move SSL generation out of experimental section

2f11c98

roborourke approved these changes May 6, 2022

View reviewed changes

shadyvb merged commit 93d9fe0 into master May 6, 2022

shadyvb deleted the product-dev-987/ssl-command branch May 6, 2022 14:52

roborourke mentioned this pull request Aug 2, 2022

SSL certificates can cause problems humanmade/local-vip#16

Open

alwaysblank added a commit to humanmade/local-vip that referenced this pull request Aug 3, 2022

Initial cherry-picking of altis mkcert feature

628bb13

- humanmade/altis-local-server#465 - humanmade/altis-local-server#479

alwaysblank mentioned this pull request Aug 3, 2022

Improve SSL certificate handling humanmade/local-vip#18

Merged

rmccue mentioned this pull request Sep 2, 2023

COMPOSE_PROJECT_NAME is set incorrectly in shell #627

Open

4 tasks

Add ssl command to download mkcert and generate ssl certificates #465

Add ssl command to download mkcert and generate ssl certificates #465

Conversation

shadyvb commented Apr 22, 2022

roborourke left a comment • edited by shadyvb Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shadyvb commented Apr 25, 2022

roborourke commented Apr 25, 2022

shadyvb commented Apr 25, 2022

roborourke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shadyvb commented May 3, 2022

shadyvb commented May 3, 2022

shadyvb commented May 3, 2022 • edited Loading

shadyvb commented May 4, 2022

shadyvb commented May 4, 2022

shadyvb commented May 4, 2022

shadyvb commented May 4, 2022 • edited Loading

roborourke left a comment

Choose a reason for hiding this comment

shadyvb commented May 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roborourke left a comment •

edited by shadyvb

Loading

shadyvb commented May 3, 2022 •

edited

Loading

shadyvb commented May 4, 2022 •

edited

Loading

shadyvb commented May 5, 2022 •

edited

Loading