Skip to content

This commit adds the ability to use SSL with MySQL. The docker-entry…#99

Closed
icsy7867 wants to merge 2 commits intoYOURLS:mainfrom
icsy7867:main
Closed

This commit adds the ability to use SSL with MySQL. The docker-entry…#99
icsy7867 wants to merge 2 commits intoYOURLS:mainfrom
icsy7867:main

Conversation

@icsy7867
Copy link
Copy Markdown

@icsy7867 icsy7867 commented Jul 22, 2021

…point.sh script has been modified to use mysqli and real_connect instead of the mysqli function itself. This was done so that we can add support for adding a path /etc/ssl/certs/db-ca.crt. The PHP uses a file_exists function to verify it exists before adding it into the code. This cert needs to be mounted to the docker container using a volume mount:

-v /path/to/cert/ca.crt:/etc/ssl/certs/db-ca.crt

Additionally, the yourls code needs an additional piece to tell the PDO driver to use the same cert for verification. This file was included as db.php and must also be mounted with a volume mount:

    -v /path/to/db.php:/var/www/html/user/db.php

…oint.sh script has been modified to use mysqli and real_connect instead of the mysqli function itself. This was done so that we can add support for adding a path /etc/ssl/certs/db-ca.crt. The PHP uses a file_exists function to verify it exists before adding it into the code. This cert needs to be mounted to the docker container using a volume mount:

	-v /path/to/cert/ca.crt:/etc/ssl/certs/db-ca.crt

Additionally, the yourls code needs an additional piece to tell the PDO driver to use the same cert for verification.  This file was included as db.php and must also be mounted with a volume mount:

   -v /path/to/db.php:/var/www/html/user/db.php
@icsy7867 icsy7867 mentioned this pull request Jul 22, 2021
5 tasks
@LeoColomb LeoColomb linked an issue Jul 22, 2021 that may be closed by this pull request
5 tasks
@icsy7867
Copy link
Copy Markdown
Author

Also created a pull request on the main YOURLS code.
YOURLS/YOURLS#3005

If this is implemented, then the db.php file and settings can safely be ignored. (Unless the option to verify a SQL SSL Connection with a CA cert is still desired)

Copy link
Copy Markdown
Member

@LeoColomb LeoColomb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pull request @icsy7867!
That there is few points to review first to establish the value of these additions.
Don't forget that the main priority is to keep things simple and not enforce any custom settings, especially on the operating side (MySQL, behaviors, etc.).

@@ -0,0 +1,22 @@
<?php
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth adding this whole file for all docker deployments?
What is the value compared to a volume mount?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More for reference. The forums had some examples but nothing in detail. You could just as.easily add a blurb on the docker hub page. I don't think the actual file will be pulled into the docker container as it's not referenced in the Dockerfile.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, good.
If not inside the Docker image, it should not live inside this repository.
Probably better to suggest a documentation update if so, indeed.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you like for me to make a stab at a documentation addition? If so, how would you like to receive that?

100% your project! Just glad to share my specific use case

Co-authored-by: Léo Colombaro <git@colombaro.fr>
@LeoColomb
Copy link
Copy Markdown
Member

Ok, I reviewed the situation here and I have updated the entrypoint script to remove the mysql part (see #101).
It resolves the case you raised, and does not add clear regression (the database is expected to be created before).

The only thing left to cover 100% of your report is the db.php file. As this is always customized for the specific project deployed, I think the initial discussion contains and document its usage properly (YOURLS/YOURLS#3003).

For these reasons, I'm closing this pull request.
Thanks again for submitting, really appreciated anyway!

@LeoColomb LeoColomb closed this Jul 25, 2021
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.

Mysqli with SSL is broken

2 participants