Skip to content

Conversation

@remibergsma
Copy link
Contributor

This is work done together with @jayapalu on fixing the site2site VPN. The first part was done in PR #690 by @jayapalu. On top of that, some other fixes were needed and those are added in this PR. It made sense to make a new PR which includes all fixes so we can actually test it.

The original PR #690 is already merged into this one, so can be closed. Since the commit ids are kept the same, merging this will close both.

I closely compared the 4.4/4.5 implementation with the new 4.6 one. I did not only make it work, but also added some security improvements (some of which were also in 4.4/4.5). I noticed the pre shared key was being logged, so removed that as well.

This is how I tested and verified it:
https://github.com/schubergphilis/MCT-shared/tree/master/helper_scripts/cloudstack/vpn_tests
When I have some time available, I'll write a Marvin test for it that we can include in the repo.

It now works(tm) with one manual step due to CLOUDSTACK-8685:
We need a default gateway before site-to-site VPN will actually work. It will connect, but not forward packets. The reason for this, is due to the iptables setup. VM1 has router1 as gateway, but router1 does not know the route to VM2 so it will give up. With a default gateway, the packets are about to be forwarded to the default gateway but when they reach eth1 the public nic, iptables kicks in, does some magic and forwards it through the ipsec tunnel. So, you need a default gw set to upstream.

Workaround for now is setting the route manually:
route add default gw 1.2.3.4 or ip route add default via 1.2.3.4

In other words, we need to fix CLOUDSTACK-8685 soon, too.

Thanks to @snuf @jayapalu!

@jayapalu @snuf could you please review this?

Jayapal and others added 5 commits August 13, 2015 14:07
For site2site VPN to work, we need a default gateway to be set.
See CLOUDSTACK-8685
It was like this in 4.4 and 4.5
Logging before:
2015-08-12 16:30:07,126 Searching for 192.168.23.6  and replacing with 192.168.23.6 192.168.23.5: PSK "preSharedKey"

Logging after:
2015-08-12 16:30:07,126 Searching for 192.168.23.6  and replacing with 192.168.23.6 192.168.23.5: PSK "****"
CLOUDSTACK-8710: Fixed applying iptables rules for s2s vpn
@remibergsma @wilderrodrigues
Moved applying iptables rules apply after vpn configuration so that vpn specific rules also get applied

* pr/690:
  CLOUDSTACK-8710: Fixed applying iptables rules for s2s vpn

This closes apache#690

Signed-off-by: Remi Bergsma <github@remi.nl>
Copy link
Contributor

Choose a reason for hiding this comment

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

"Searching for %s and replacing the value following it with %s" would be closer to the truth

@DaanHoogland
Copy link
Contributor

no furhter comments, LGTM

@asfbot
Copy link

asfbot commented Aug 14, 2015

cloudstack-pull-rats #295 SUCCESS
This pull request looks good

@jayapal
Copy link

jayapal commented Aug 14, 2015

Guys wrong tag.

Myuser name is jayapal and actual commiter was jayapalu

On Fri, Aug 14, 2015 at 1:47 PM, Remi Bergsma notifications@github.com
wrote:

This is work done together with @jayapal https://github.com/Jayapal on
fixing the site2site VPN. The first part was done in PR #690
#690 by @jayapal
https://github.com/Jayapal. On top of that, some other fixes were
needed and those are added in this PR. It made sense to make a new PR which
includes all fixes so we can actually test it.

The original PR #690 #690 is
already merged into this one, so can be closed. Since the commit ids are
kept the same, merging this will close both.

I closely compared the 4.4/4.5 implementation with the new 4.6 one. I did
not only make it work, but also added some security improvements (some of
which were also in 4.4/4.5). I noticed the pre shared key was being logged,
so removed that as well.

This is how I tested and verified it:

https://github.com/schubergphilis/MCT-shared/tree/master/helper_scripts/cloudstack/vpn_tests
When I have some time available, I'll write a Marvin test for it that we
can include in the repo.

It now works(tm) with one manual step due to CLOUDSTACK-8685:
We need a default gateway before site-to-site VPN will actually work. It
will connect, but not forward packets. The reason for this, is due to the
iptables setup. VM1 has router1 as gateway, but router1 does not know the
route to VM2 so it will give up. With a default gateway, the packets are
about to be forwarded to the default gateway but when they reach eth1 the
public nic, iptables kicks in, does some magic and forwards it through the
ipsec tunnel. So, you need a default gw set to upstream.

Workaround for now is setting the route manually:
route add default gw 1.2.3.4 or ip route add default via 1.2.3.4

In other words, we need to fix CLOUDSTACK-8685 soon, too.

@jayapal https://github.com/Jayapal @snuf https://github.com/snuf

could you please review this?

You can view, comment on, or merge this pull request online at:

#693
Commit Summary

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#693.

@remibergsma
Copy link
Contributor Author

@jayapal Sorry for bothering you, just updated the tag. Thanks for letting me know!

@asfbot
Copy link

asfbot commented Aug 14, 2015

cloudstack-pull-requests #992 SUCCESS
This pull request looks good

@asfbot
Copy link

asfbot commented Aug 14, 2015

cloudstack-pull-analysis #228 SUCCESS
This pull request looks good

@jayapalu
Copy link
Contributor

The iptables rules and vpn config changes are looking good.
LGTM

@asfgit asfgit merged commit 7ddec66 into apache:master Aug 14, 2015
asfgit pushed a commit that referenced this pull request Aug 14, 2015
Fix site-to-site VPN featureThis is work done together with @jayapalu on fixing the site2site VPN. The first part was done in PR #690 by @jayapalu. On top of that, some other fixes were needed and those are added in this PR. It made sense to make a new PR which includes all fixes so we can actually test it.

The original PR #690 is already merged into this one, so can be closed. Since the commit ids are kept the same, merging this will close both.

I closely compared the 4.4/4.5 implementation with the new 4.6 one. I did not only make it work, but also added some security improvements (some of which were also in 4.4/4.5). I noticed the pre shared key was being logged, so removed that as well.

This is how I tested and verified it:
https://github.com/schubergphilis/MCT-shared/tree/master/helper_scripts/cloudstack/vpn_tests
When I have some time available, I'll write a Marvin test for it that we can include in the repo.

It now works(tm) with one manual step due to CLOUDSTACK-8685:
We need a default gateway before site-to-site VPN will actually work. It will connect, but not forward packets. The reason for this, is due to the iptables setup. VM1 has router1 as gateway, but router1 does not know the route to VM2 so it will give up. With a default gateway, the packets are about to be forwarded to the default gateway but when they reach eth1 the public nic, iptables kicks in, does some magic and forwards it through the ipsec tunnel. So, you need a default gw set to upstream.

Workaround for now is setting the route manually:
``route add default gw 1.2.3.4``  or  ``ip route add default via 1.2.3.4``

In other words, we need to fix CLOUDSTACK-8685 soon, too.

Thanks to @snuf @jayapalu!

@jayapalu @snuf could you please review this?

* pr/693:
  do not log sensitive site-to-site VPN PSK
  tighten security of site-to-site VPN
  CLOUDSTACK-8730: fix s2s iptables rules and ipsec config
  CLOUDSTACK-8710: Fixed applying iptables rules for s2s vpn

Signed-off-by: Remi Bergsma <github@remi.nl>
@snuf
Copy link

snuf commented Aug 14, 2015

LGTM

@jayapal
Copy link

jayapal commented Aug 14, 2015

No problem

Best,
Jayapal
Mobile, xcuse typoerror.
On 14 Aug 2015 18:29, "Funs" notifications@github.com wrote:

LGTM


Reply to this email directly or view it on GitHub
#693 (comment).

rohityadavcloud pushed a commit that referenced this pull request Jan 20, 2021
…693)

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
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.

7 participants