Skip to content

Allow to encrypt database connection in installation [rebased]#12

Merged
richard67 merged 13 commits intorichard67:4.0-dev-tls-encrypt-db-connections-at-installfrom
andrepereiradasilva:patch-34
Nov 1, 2019
Merged

Allow to encrypt database connection in installation [rebased]#12
richard67 merged 13 commits intorichard67:4.0-dev-tls-encrypt-db-connections-at-installfrom
andrepereiradasilva:patch-34

Conversation

@andrepereiradasilva
Copy link

same as #8 with one addon;
now the fields only show if the database server field is different from "localhost"
showon="db_host!:localhost"

Probably this could also be used in global config and privacy dashboard.
There is no encrypting in socket connections

@richard67
Copy link
Owner

Thanks, I will keep your comments in mind for the stuff already merged into the CMS.

@richard67
Copy link
Owner

Regarding localhost: Shouldn't we check for 127.0.0.1 and ::1, too?

@andrepereiradasilva
Copy link
Author

127.0.0.1 and ::1 are TCP connections so they can be encrypted with TLS (as you can test), if it makes sense or not is other mather, but as far as i read about it, it could make sense if an attacker got low priviledge access to the operating system and then they could "peek" into the connections being made in the loopback tcp address (127.0.0.1/::1), so in high security contexts could make sense to encrypt 127.0.0.1/::1 connections - but i'm not an expert in that matter.

@richard67
Copy link
Owner

Will localhost not be resolved to 127.0.0.1 or ::1, depending on IP version 4 or 6 availability? Who or what decides if to make a socket connection or a TCP connection? Am just curious.

@andrepereiradasilva
Copy link
Author

andrepereiradasilva commented Oct 21, 2019

At least in Linux normally MySQL/mariadb and PostgreSQL bind localhost to a Unix socket. Why? Because unix sockets are faster since they dont have TCP protocol overhead.
But sockets ONLY work when the Database is in the same host as the APP.

@andrepereiradasilva
Copy link
Author

@richard67
Copy link
Owner

I see, thanks for the info. Not sure yet if I will use that change for what is already in staging. On weekend I will work on it and on this here, testing and if ok making PR for the CMS.

Again many thanks for advise and help and code.

@richard67
Copy link
Owner

I'll leave this open for a while in case if you wanna make modifications. Later I'll merge it to my branch when I've made a PR for the CMS.

@andrepereiradasilva
Copy link
Author

I dont see anythng more to do here.
I can see a future potential PR to the database framework to have a method to return the Database connection properties, i.e., if is socket (returns path of the uns socket) or tcp (returns host, IP and port), but thats another thing.

@richard67
Copy link
Owner

richard67 commented Oct 21, 2019

You know problem is this showon thing: It is not clever enough to clear fields before hiding them, so when someone enters a hostname, enters some bad stuff for encryption and then changes back to localhost and encryption options disappear ... that could be at least annoying. Connection test and better error handling which you've added at the end should keep one from saving such bad stuff ... but I am not really comfortable with that showon.

Another thing what I was thinking about was if we could add a field to the system config (maybe as an info text or hidden, if it does not fit to other enterable fields regarding consistent UX) for the info if the server supports it, same info as we show in system info, and make showon also dependent on this. Not sure if a good idea or not. Maybe we should check this parameter in code too and reset any encryption settings if server does not support it?

@andrepereiradasilva
Copy link
Author

You know problem is this showon thing: It is not clever enough to clear fields before hiding them, so when someone enters a hostname, enters some bad stuff for encryption and then changes back to localhost and encryption options disappear ... that could be at least annoying. Connection test and better error handling which you've added at the end should keep one from saving such bad stuff ... but I am not really comfortable with that showon.

You can do that in XML "onchange=" i can check it Larter

Another thing what I was thinking about was if we could add a field to the system config (maybe as an info text or hidden, if it does not fit to other enterable fields regarding consistent UX) for the info if the server supports it, same info as we show in system info, and make showon also dependent on this. Not sure if a good idea or not. Maybe we should check this parameter in code too and reset any encryption settings if server does not support it?

I though about it but you have to do it with Ajax js since you need to check it everytime the user chances without submitting the form. I particulary dont like sending sensível data with Ajax, unkess they are POST methods with HTTPS and the receiving Ajax method is very security hardened , also GET HTTP requests stays in the log files which woukd put logins and passwords there

@andrepereiradasilva
Copy link
Author

andrepereiradasilva commented Oct 26, 2019

showon stuff improved.
Now when you change (on real time typing) the db host de DB field, if you put localhost it will reset the db encryption fields.
Also put the postgresql conditions for capath and cipher fields

@richard67
Copy link
Owner

@andrepereiradasilva Yeah, just saw. Looks great. Will see what has also to go into the stuff which has already been merged into the CMS and make a PR for that. And this PR here will be a separate one, also created soon. I have time this weekend.

@andrepereiradasilva
Copy link
Author

andrepereiradasilva commented Oct 26, 2019

Will see what has also to go into the stuff which has already been merged into the CMS and make a PR for that.

i can make a new PR here for that. already working on that

@richard67
Copy link
Owner

Great.

@richard67
Copy link
Owner

@andrepereiradasilva I still get:
snap-15

at the start of installation.

@andrepereiradasilva
Copy link
Author

Do you have that error without this Patch?

@richard67
Copy link
Owner

richard67 commented Oct 27, 2019

Yes.

Update 2019-10-29: Sorry, have misread. I have this error ONLY with this patch, NOT without it. Do you use Windows or Linux as Joomla Host for testing? I use Linux. If it works on Windows but does not work on Linux, it could be some problem with file names having to fit to class or namespace names?

@richard67
Copy link
Owner

Update 2019-10-29: Sorry, have misread. I have this error ONLY with this patch, NOT without it. Do you use Windows or Linux as Joomla Host for testing? I use Linux. If it works on Windows but does not work on Linux, it could be some problem with file names having to fit to class or namespace names?

@richard67
Copy link
Owner

@andrepereiradasilva Now I've replaced file by file and tested if index.php of admin works. With file installation/src/Model/SetupModel.php the error comes.

@richard67
Copy link
Owner

@andrepereiradasilva Got it, see review.

Silly mistake ... Sorry richard

Co-Authored-By: Richard Fath <richard67@users.noreply.github.com>
@andrepereiradasilva
Copy link
Author

Solved, sorry richard, my mistake

@richard67
Copy link
Owner

I will test again before merging. Stay tuned.

@richard67 richard67 merged commit b3ec36f into richard67:4.0-dev-tls-encrypt-db-connections-at-install Nov 1, 2019
@richard67
Copy link
Owner

Thanks @andrepereiradasilva . I've decided to merge first, then make a draft PR to the CMS so I can test, and when test is ok I'll "undraft" it. I'll let you know if all ok or some issues.

@richard67
Copy link
Owner

@andrepereiradasilva Do you think you can provide the javascript part for PR joomla#26889?

@richard67
Copy link
Owner

@andrepereiradasilva I've just tested my PR joomla#26888 and found that when using the real host name and not localhost, and I adjust one-way encryption without certificate verification, like it works out of the box for me, then the installation finishs with succes, but then in backend encryption is set to default again. Can it be that there is missing something for saving the changed configuration during the installation?

@richard67
Copy link
Owner

@andrepereiradasilva P.S. to previous comment: Or can it be that the javascript clears the field somehow when continuing the installation after having selected the tls options and so on? I'll make a break and then test again without the JS.

@richard67
Copy link
Owner

@richard67
Copy link
Owner

Yeah, that's it. Have to take the options values instead of the defaults for off.

@andrepereiradasilva
Copy link
Author

:) yeah that was neede also, but since was already in Joomla main branch i didn't do it here.
Thanks Richard for all yor work!

@andrepereiradasilva andrepereiradasilva deleted the patch-34 branch November 1, 2019 18:00
@richard67
Copy link
Owner

@andrepereiradasilva No, thank you for your work. I only wish that sometimes you would trust my testing results a bit more ;-)

Do you think you can do the JS part of joomla#26889. If you don't have time I can try it myself.

@andrepereiradasilva
Copy link
Author

out from home this weekend, only have time Next weekend, if you have some time until them please go ahead.

@richard67
Copy link
Owner

Ok, will let you know if done or when problems.

@richard67
Copy link
Owner

@andrepereiradasilva Regarding the JS part of joomla#26889, i.e. using onchange event in Global Config for the hostname field to reset stuff in case of localhost: Could you give me a hint to which js the new function should be added? For the installations it was easy to understand for me, but in backend I haven't found a suitable place. It seems the onchange stuff isn't used in Global Config yet. I'll see if I can find it out today or tomorrow, but if you can give me a quick hint it would be very helpful.

@richard67
Copy link
Owner

@andrepereiradasilva I've found it out myself. Could you have a look on joomla#26889 if it is ok?

richard67 pushed a commit that referenced this pull request Dec 4, 2022
Replace alert to j message, remove duplication
richard67 pushed a commit that referenced this pull request Mar 16, 2025
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.

2 participants