Vesta: Function get_web_config_lines() can screw up apache2.conf user files

[dpeca]

... because it counts vhost size (count of lines) by looking at template file.

$1 is path to /usr/local/vesta/data/templates/web/apache2//basedir.tpl

    tpl_lines=$(egrep -ni "name %domain_idn%" $1 |grep -w %domain_idn%)
    tpl_lines=$(echo "$tpl_lines" |cut -f 1 -d :)
    tpl_last_line=$(wc -l $1 |cut -f 1 -d ' ')

And if you add only one line in user's /home/username/conf/web/apache2.conf - it will cut vhost block on wrong line - and Apache goes down when script trigger apache restart - whole server goes down.

Function get_web_config_lines() is triggered by /bin/v-delete-web-domain command - so server can go down if admin modified user's apache2.conf (for example to add additional php_admin_value) and when the user also in the middle of the night deletes some domain in his account.

[Skamasle]

I think there is no reason to modify /home/username/conf/web/apache2.conf

Because in any vhost rebuild this file was changed,

So, people need use the include file for that or create your own template

This is old bug, I see it a lot of times, but I never think is a bug, because this file /home/username/conf/web/apache2.conf was rebuilded and any manual change was deleted.

So, custom modificatiosn need be in new template or include file, I think is a correct way to do it.

[dpeca]

But when it's rebuilded?
I have custom modified apache2.conf that is untouched almost two years...

And even if I stop to do it - many other users will not be aware that it's risky to modify apache2.conf .

I think that get_web_config_lines() function should search for </VirtualHost> in actual config file, not to measure length of template file...

[dpeca]

... and even if i'm wrong - it's still a bug, because, in that case, script should first rebuild apache2.conf - in order to ensure that vhost block has correct size....

[Skamasle]

Its rebuilded when you run rebuild vhost from gui or cli, so in this case you lost your custom changes

If there is option to lost custom changes I think is not correct to do manually modifications to files

[dpeca]

On forum you can find tons of posts where is suggested modification of apache2.conf.

On other hand, is it somewhere documented that it should not be modified?
Is it documented that it can brake server because Apache can fall down?

At the end - when we build a software - as programers we should put ''checking if something goes wrong'' everywhere where we can - and if apache2.conf should not be modified - OK - but let then make additional check in case it was modified - to avoid Apache falling.

[Skamasle]

I agree with you is not documented and yes a lot off people modify it but I
think is preferred custom template for it

And yes there is better way to do it and fix this problem but in my opinion
this is not critical

In other topic not remember when I recommend Vesta team improve this
function when it breaker fastcgi template

I think the best option is create one file per vhost / domain and not just
one file with all domains
Why ?
Easy to find
Easy to edit
You can add some line than say #no rebuild and when you rebuild all your
vhost without some option like --force script not touch it

In my opinion is better way to solve it and work with it

But I don't know is just opinion

El 8 jul. 2017 8:19 p. m., "dpeca" [e-mail] escribió:

On forum you can find tons of posts where is suggested modification of
apache2.conf.

On other hand, is it somewhere documented that it should not be modified?
Is it documented that it can brake server because Apache can fall down?

At the end - when we build a software - as a programers we should put
''checking if something goes wroing'' everywhere where we can - and if
apache2.conf should not be modified - OK - but let then make additional
check in case it was modified - to avoid Apache falling.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/serghey-rodin/vesta/issues/1226#issuecomment-313872763,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AIlXdfrFy0sakK_H6G7OIeYV_hNMMVUKks5sL8gpgaJpZM4OQ1mI
.

[dpeca]

I will try to make ''soft fallback'' in case script can find </VirtualHost>.
So, in case script can find </VirtualHost> it will use length of it, instead of using length of template file.
That way nothing can goes wrong, just will be avoided disaster situation in case user shrinked VirtualHost block.

[dpeca]

Since @serghey-rodin told me not to change a "core" functionality directly to master branch, but to push it as a push request - here it is - https://github.com/serghey-rodin/vesta/pull/1248

This code will find </VirtualHost> instead of measuring template length.
That way VirtualHost block can be stretched as long as administrator wants it - without a fear that code will cut it on wrong number.

I will roll-out this patch to all my 15 production servers, just to avoid potential disasters.
Code is tested very well, I did 20 different types of testing, everything passed.

[dpeca]

And I'm sure I didn't change a ''core functionality'', actually I only added a code that will safely find VirtualHost borders - but anyway - let it goes as a push request - and let @serghey-rodin review and allow this patch.

[dpeca]

Patch is rolled-out to my 16 production servers, so if something is wrong I'll find out it very soon.

[cmstew]

This seems like a non-issue to me and for the record,

If you opened that apache2.conf file and looked at the vhost for one of the domains, assuming the domain is mydomain.com, you will find something like this:

IncludeOptional /home/$username/conf/web/apache2.mydomain.com.conf*

So if you create the file "/home/$username/conf/web/apache2.mydomain.com.conf_use_https" and then add your custom entries in that file, they will be loaded the next time apache is restarted. Plus, your changes will not be overwritten if you rebuild v-host.

This is hardly a "bug" if you're making custom modifications to the default server files. It seems to me like you're causing the "bug".

[dpeca]

You are right, but if you search a forum you'll find a tons of users with broken apache2.conf, which leads us to conclusion that I'm not only one that did it on wrong way.
And if few lines of code can detect and prevent disasters - personally I'll not be against that code.
Anyway, @serghey-rodin will review and decide.

Code did not cause any error on my 16 production servers - just prevented disaster - becauase I added a few php_admin_value to many domains.

[cmstew]

Normally I would still disagree with you but considering that these files are user accessable, I can see where you're coming from.

Here's a possible suggestion though:

What if we change the add domain script a bit so that when we add a new domain, we make the conf files read only to the user. Or better yet, move them out of the reach of the user. And then add an example README for in their conf folder explaining how to add v-host values.

Right now, this is just in theory but I feel like it might benefit your idea. Perhaps even complement it.

[dpeca]

I'm pretty sure it's already read-only for user.
Anyway...
It's not about who is right.... let's say I'm not right - because we both will agree that apache2.conf files should not be changed.
I made mistake when I changed apache2.conf files - I didn't know it can cause apache falling when conf file became badly truncated.

This fix is generally fine to me - because I will avoid searching for modified apache2.conf files - avoid cut and paste added lines to apache2.domain.conf* files - and avoid disaster if someone in the middle of the night delete domain that has shriinked virtualhost block.

And either if this push request will be rejected - admins that made the same mistake as me can use this patch - to avoid mentioned difficulties.

And I'm ok with it if this will be rejected.

Patch will be still used on my servers.
Because it does not change anything, just avoid possible disasters - caused by ignorance.