Skip to content

Conversation

@JonBendtsen
Copy link
Contributor

I combined my 2 previous "security" PR and also removed the autoindex module

This PR should replace these 2:
#46
#45

@cibero42
Copy link
Contributor

cibero42 commented Mar 22, 2025

Hey @JonBendtsen ,

Why are you doing rm -rf /etc/apache2/mods-enabled/dir.conf?

Actually, the dir module adds the "trailing slash" to URLs - can't not having it break Dolibarr at some random point of the future?

Also, can't we disable the auth_basic_module? (Or even all auth modules)

@JonBendtsen
Copy link
Contributor Author

Hey @JonBendtsen ,

Why are you doing rm -rf /etc/apache2/mods-enabled/dir.conf?

Because disabling the module caused a fail, and I did not want to leave the configuration file in place because I was worried about possible load order and ending up with an DirectoryIndex we did not want.

It is configured in the docker-php.conf file

root@v21pod:/etc/apache2/conf-enabled# cat docker-php.conf 
<FilesMatch \.php$>
	SetHandler application/x-httpd-php
</FilesMatch>

DirectoryIndex disabled
DirectoryIndex index.php index.html

<Directory /var/www/>
	Options -Indexes
	AllowOverride All
</Directory>

that I expect comes from when PHP releases their docker image.

Actually, the dir module adds the "trailing slash" to URLs - can't not having it break Dolibarr at some random point of the future?

Also, can't we disable the auth_basic_module? (Or even all auth modules)

I tried, but I would have to force remove it :-( and in some cases ended up with a container that would not start.

@JonBendtsen
Copy link
Contributor Author

QUESTION

So far I removed only loaded apache modules, but there is a whole bunch of available modules that I frankly just don't see we would never use inside a Dolibarr container. Example, all the proxy modules:

root@v21pod:/usr/lib/apache2/modules# du -shc *proxy*
164K	mod_proxy.so
64K	mod_proxy_ajp.so
68K	mod_proxy_balancer.so
20K	mod_proxy_connect.so
16K	mod_proxy_express.so
36K	mod_proxy_fcgi.so
16K	mod_proxy_fdpass.so
44K	mod_proxy_ftp.so
36K	mod_proxy_hcheck.so
40K	mod_proxy_html.so
48K	mod_proxy_http.so
68K	mod_proxy_http2.so
24K	mod_proxy_scgi.so
24K	mod_proxy_uwsgi.so
28K	mod_proxy_wstunnel.so
696K	total

Not a lot of space saving, but maybe there are other places where we can trim the image.

@cibero42
Copy link
Contributor

Because disabling the module caused a fail, and I did not want to leave the configuration file in place because I was worried about possible load order and ending up with an DirectoryIndex we did not want.

I'll take a look on yhat later today

I tried, but I would have to force remove it :-( and in some cases ended up with a container that would not start.

Well, better to keep it then

@cibero42
Copy link
Contributor

Example, all the proxy modules:

Well, I'd say that it's better to remove the proxy modules, as I can't imagine an use case for them...

@creekorful, thoughts?

@cibero42
Copy link
Contributor

@creekorful , should we also disable the proxy modules? Or better to merge these commits first?

@creekorful creekorful self-assigned this Mar 28, 2025
@creekorful
Copy link
Member

I think

@creekorful , should we also disable the proxy modules? Or better to merge these commits first?

It's already a step forward so let's move on with this :-)

@cibero42
Copy link
Contributor

It's already a step forward so let's move on with this :-)

@JonBendtsen can we move on with disabling all proxy modules then?

@creekorful
Copy link
Member

It's already a step forward so let's move on with this :-)

@JonBendtsen can we move on with disabling all proxy modules then?

I meant the opposite actually. Let's keep that PR simple so that it is merged fast. We'll see afterwards for the other modules

Cheers

@cibero42
Copy link
Contributor

@creekorful

Ah, ok, sorry.

So... we are just waiting for the PHP issue to be solved, right?

Appart from that rm, the rest seems ok for me :)

@creekorful creekorful added question Further information is requested and removed waiting eternal feedback labels Apr 3, 2025
@JonBendtsen
Copy link
Contributor Author

It's already a step forward so let's move on with this :-)

@JonBendtsen can we move on with disabling all proxy modules then?

I think those modules are already disabled, but not deleted.

@creekorful creekorful added enhancement New feature or request and removed question Further information is requested labels Apr 8, 2025
@creekorful
Copy link
Member

Looks good to me.

I will test the changes locally this week and if nothing is broken I will move on and merge this.

Thanks for your contribution!

@creekorful creekorful merged commit dd847c2 into Dolibarr:main Apr 8, 2025
2 checks passed
@creekorful
Copy link
Member

Thank you @JonBendtsen for your contribution and @cibero42 for the feedback!

Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants