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

Fix issue with helo behind nat #1810

Merged
merged 7 commits into from May 19, 2021

Conversation

jaapmarcus
Copy link
Member

  1. When the local ip has an different value then the public ip an error was show and publicip config was created causing potential issues with Nginx / Apache2
  2. When HELO was empty an new key was created
  3. On update of natted ip the local ip was updated in mailhelo.conf instead the public ip

@jaapmarcus jaapmarcus requested review from a user, Lupul and ScIT-Raphael May 17, 2021 20:54
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks good so far, @cmstew can you please review?

Comment on lines 65 to 75
#Create or update ip:helo pair in mailhelo.conf file
if [ ! -z "$helo" ]; then
if [ $(cat /etc/${MAIL_SYSTEM}/mailhelo.conf | grep "$ip") ]; then
sed -i "/^$ip:/c $ip:$helo" /etc/${MAIL_SYSTEM}/mailhelo.conf
if [ $(cat /etc/${MAIL_SYSTEM}/mailhelo.conf | grep "$natip") ]; then
sed -i "/^$natip:/c $natip:$helo" /etc/${MAIL_SYSTEM}/mailhelo.conf
else
echo $ip:$helo >> /etc/${MAIL_SYSTEM}/mailhelo.conf
echo $natip:$helo >> /etc/${MAIL_SYSTEM}/mailhelo.conf
fi
else
sed -i "/^$ip:/d" /etc/${MAIL_SYSTEM}/mailhelo.conf
sed -i "/^$natip:/d" /etc/${MAIL_SYSTEM}/mailhelo.conf
fi
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we want the real ip here?

Copy link
Member Author

Choose a reason for hiding this comment

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

We want the public ip to be visible or not? Local ip doesn't make any sense

https://serverfault.com/questions/305925/what-exactly-should-helo-say

Copy link
Member Author

Choose a reason for hiding this comment

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

# Update ip helo for exim
if [[ ! -z "$MAIL_SYSTEM" && "$helo" == "yes" ]]; then
pub_ip=$(curl --ipv4 -s https://ip.hestiacp.com/)
$BIN/v-change-sys-ip-helo $pub_ip $domain
fi

If we run v-change-sys-hostname dev.hestiacp.com the public ip pass trough will be 1.2.3.4

hestiacp/func/ip.sh

Lines 48 to 55 in 4a8b229

ip="$1"
helo="$2"
natip="$1"
# In case the IP is an NAT use the real ip address
if [ ! -f $HESTIA/data/ips/$ip ]; then
$ip=$(get_real_ip $ip);
fi

Currently this "$ip" value creates an new config file for "1.2.3.4"

"ip" pass trough in the system becomes both $ip and $natip and get overwritten for the local ip 192.168.1.10 for example

For Help we still need to announce the public ip so far I can understand if we use 192.168.1.10 the DNS records will not match the sending domain

Copy link
Contributor

Choose a reason for hiding this comment

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

We want the public ip to be visible or not? Local ip doesn't make any sense

https://serverfault.com/questions/305925/what-exactly-should-helo-say

Yes, we want the public IP.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm not following your changes because it says "In case the IP is an NAT use the real ip address". Okay.

So that check does, if the real ip file doesn't exist, get the real ip and put it in $ip variable. This I follow.

But then later you put the $natip variable into the mailhelo.conf file. Which might not be the public ip.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should save the HELO "value" for /usr/local/hestia/data/ips/local.ip other wise users are allowed to select the public ip and causing issues

For mailhelo.conf we have to use the public ip. And is set with the current changes.
When v-change-sys-ip-helo is called for any reason the value set will update in mailhelo.conf the public ip and for Hestia the local ip...

I think we are safe now

Copy link
Contributor

@cmstew cmstew May 19, 2021

Choose a reason for hiding this comment

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

Yes, I just misunderstood the roles of the variables. It looks good!

@ScIT-Raphael ScIT-Raphael merged commit a00adb6 into hestiacp:main May 19, 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.

None yet

3 participants