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

Add support for local certificates #251

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andrewnicols
Copy link
Contributor

@andrewnicols andrewnicols commented Feb 14, 2023

This commit:

  • ssl configuration for webserver and exttests if a certificate authority exists
  • installs the CA server on the webserver, exttests
  • provides an SSL Certificate generator for Linux/MacOS (sorry Windows... not sure how to do that)

@scara
Copy link
Contributor

scara commented Feb 14, 2023

Hi @andrewnicols,

(sorry Windows... not sure how to do that)

You can still use openssl provided by Git (Bash) for Windows given that you'll use winpty as "a prefix" to each command.
Ref: Some native console programs don't work when run from Git Bash. How to fix it?.
Example: https://gist.github.com/vickramravichandran/c1190efbf9f1841234fcef624ef65956.

An issue could be if the user has installed Git for Windows even in the PATH or not e.g. you should check (EXIST) for the expected path:

>where /R "C:\Program Files\Git" winpty.exe
C:\Program Files\Git\usr\bin\winpty.exe

Otherwise, we should play with powershell via New-SelfSignedCertificate which is IMHO not an option here, to keep the things similar to the other OSes.

Trusting the CA could be done via powershell using Import-Certificate:

Import-Certificate -FilePath "C:\CA-PublicKey.PEM" -CertStoreLocation Cert:\CurrentUser\Root

Never tried by myself: I can offer some spare time but in these days.

HTH,
Matteo

@mattporritt
Copy link
Contributor

Hi @andrewnicols
I've given this a review and some testing.
I started the containers without ssl certs and everything was fine. As expected the site ran without ssl.
When running the setup with SSL enabled, I did need to modify config.php with regard to host and port. I didn’t debug it fully but it’s this code section: https://github.com/moodlehq/moodle-docker/blob/master/config.docker-template.php#L16-L29
Various setting env vars for port and host didn’t help. What fixed it for me was just doing: $CFG->wwwroot = "https://webserver/"; (and having webserver in my hosts file).

In regard to this not working with windows systems yet, I’d be inclined to split that into a separate issue. It means linux and osx based devs get the benefit now and most of the work is done for windows devs.

Cheers,
Matt P

@stronk7
Copy link
Member

stronk7 commented Apr 8, 2023

Hi,

can I ask which is the need for this? Right now it's not clear for me at all. Sure there is a (many) reasons, but it would be great to know about them.

Ciao :-)

@mattporritt
Copy link
Contributor

Hi @stronk7
The main reason is testing and developing for Oauth2. Moodle won't let you connect to an IDP that isn't https, and most IDPs wont' allow non HTTPS clients. So having local certs and a trusted local CA makes this much easier.
The main use case at the moment is MoodleNet, but upcoming Dev around SSO and Matrix needs it too.
It's also good to be able to test and dev using HTTPs as that's how the majority of our clients run their sites.
Finally, more and more services we want to integrate with won't accept no HTTPS connections. So better to solve this issue now before it becomes a blocker.

Cheers,
Matt P

@stronk7
Copy link
Member

stronk7 commented Jun 15, 2023

FYI, I've created #264 about to try to bring feature parity under Windows, assuming that this issue will remain unix-only.

Ciao :-)

@stronk7
Copy link
Member

stronk7 commented Jul 2, 2023

I've been re-reading this and looking to the proposed patch and, given that we have already created #264 , it's looking like we are almost there.

Only points that I can imagine are:

  1. Relatively important: The comment from @mattporritt above above "When running the setup with SSL enabled, I did need to modify config.php with regard to host and port". Maybe we have to do this (by raising some env flag/var) that config.php is able to read?
  2. Interesting: Maybe add one more job (I think one is enough) to the GHA tests to ensure that the whole thing (certs creation, container instantiation, access...) is working as expected?
  3. Luxury detail: This patch also adds support for custom entry points, that is something that, maybe we should document, pretty much like the ones that we have within the php images themselves.

Ciao :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants