Fix C2S VPN in parallel to S2S VPN by GutoVeronezi · Pull Request #6907 · apache/cloudstack

GutoVeronezi · 2022-11-17T14:34:49Z

Description

PR #5375, introduced in version 4.15.2.0, removed parameter %any of VPNs client-to-site (C2S) IPSec secrets:

structure before PR vr: ipsec/l2tp vpn secret with no ID selectors #5375:
<IP> %any : PSK "<PSK>"
structure after PR vr: ipsec/l2tp vpn secret with no ID selectors #5375:
<IP> : PSK "<PSK>"

Because of that, when a VPN site-so-site (S2S) is created in parallel to a VPN C2S in the same network, the C2S will not handle any IP (%any) anymore and, as the network is being tunneled to the other VPN, the connection will be handled by the final peer. This way, when a VPN S2S is created in parallel to a VPN C2S in the same network, it is only possible to connect to the C2S with the S2S PSK.

As ACS is only able to implement a single C2S per network (ACS allows setting more than one IP of the network as VPN, however, only the first will be implemented) and every S2S has its own secret file, the secrets structure of C2S was changed to contain only the PSK:

: PSK "<PSK>"

By doing that, StrongSwan will handle correctly C2S connections from any IP and still will use the correct PSK for S2S.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Bug Severity

How Has This Been Tested?

I created 3 networks (A, B, and C) and a VM in each network. In networks B and C I enabled the VPN. From VM in network A, I configured both VPNs and successfully connected to them with their respective PSK. Then, from B to C I established a S2S VPN. Without this change, when I was trying to connect again to C2S B with B PSK, the connection was refused; however, when tried to connect to C2S B with C PSK, the connection was successful. After the changes, I tried to connect again to C2S B with B PSK and the connection was successful.

codecov · 2022-11-17T16:49:16Z

Codecov Report

Merging #6907 (4beb910) into 4.17 (6c436ec) will increase coverage by 0.53%.
The diff coverage is 38.17%.

❗ Current head 4beb910 differs from pull request most recent head 23b16c2. Consider uploading reports for the commit 23b16c2 to get more accurate results

@@             Coverage Diff              @@
##               4.17    #6907      +/-   ##
============================================
+ Coverage     10.34%   10.87%   +0.53%     
- Complexity     6609     7117     +508     
============================================
  Files          2451     2485      +34     
  Lines        242340   245507    +3167     
  Branches      37924    38334     +410     
============================================
+ Hits          25061    26699    +1638     
- Misses       214192   215538    +1346     
- Partials       3087     3270     +183

Impacted Files	Coverage Δ
...estration/service/NetworkOrchestrationService.java	`100.00% <ø> (ø)`
...pi/src/main/java/com/cloud/alert/AlertManager.java	`100.00% <ø> (ø)`
...n/java/com/cloud/vm/VirtualMachineProfileImpl.java	`36.44% <0.00%> (-0.70%)`	⬇️
...n/java/com/cloud/vm/VirtualMachineManagerImpl.java	`0.00% <0.00%> (ø)`
...tack/engine/orchestration/NetworkOrchestrator.java	`0.00% <0.00%> (ø)`
...stack/engine/orchestration/VolumeOrchestrator.java	`0.00% <0.00%> (ø)`
...n/java/com/cloud/capacity/dao/CapacityDaoImpl.java	`3.24% <0.00%> (-0.13%)`	⬇️
...ema/src/main/java/com/cloud/network/vpc/VpcVO.java	`14.86% <0.00%> (-1.81%)`	⬇️
.../network/vpc/dao/VpcOfferingServiceMapDaoImpl.java	`0.00% <0.00%> (ø)`
...ma/src/main/java/com/cloud/projects/ProjectVO.java	`8.33% <0.00%> (+0.43%)`	⬆️
... and 245 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

yadvr · 2022-11-21T07:37:55Z

        secret = CsFile(vpnsecretfilte)
        secret.empty()
-        secret.addeq("%s : PSK \"%s\"" % (left, psk))
+        secret.addeq(": PSK \"%s\"" % (psk))


@GutoVeronezi LGTM, but should we revert to old ? I don't know what the syntax really means

secret.addeq("%s %%any : PSK \"%s\"" % (left, psk))

Update, from #4281 (comment) it seems the user's fix correlates with the syntax used.

yadvr

LGTM based on test results shared; this would need syntactic review from a strongswan/ipsec/vpn expert

yadvr · 2022-11-21T08:38:35Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 4624

yadvr · 2022-11-21T08:40:48Z

(manually fired smoketest)

weizhouapache · 2022-11-21T10:45:15Z

@GutoVeronezi
as you said, PR #5375 changed the structure from
<IP> %any : PSK "<PSK>"
to
<IP> : PSK "<PSK>"
With this PR, the structure is changed to
: PSK "<PSK>"

I have few questions

Have you ever tested the result by reverting vr: ipsec/l2tp vpn secret with no ID selectors #5375 ?
Since public IP is removed from the configuration in this PR, might it be potential security issue ?

GutoVeronezi · 2022-11-22T20:19:44Z

@weizhouapache, yes, I have tested reverting #5375, and it works as well. The problem was that, by removing %any and enabling S2S, the request would be redirected to the final peer because the source (right) was not being handled by the VPN C2S. Removing the destination (left) makes the C2S handle the connection with the PSK, independent of the source.

Since only a single VPN C2S is configured for each network/VPC, I do not see how it could be a security issue.

@rohityadavcloud, and @weizhouapache, since the user's problem (#4281 (comment)) was observed in 4.14.0, with another version of StrongSwan, and I could not reproduce it, the change was made in order to honor their comment. However, if we can confirm that it was only a problem with the StrongSwan version and #5375 change was not necessary, I think we could revert #5375.

weizhouapache · 2022-11-22T20:59:51Z

@weizhouapache, yes, I have tested reverting #5375, and it works as well. The problem was that, by removing %any and enabling S2S, the request would be redirected to the final peer because the source (right) was not being handled by the VPN C2S. Removing the destination (left) makes the C2S handle the connection with the PSK, independent of the source.

Since only a single VPN C2S is configured for each network/VPC, I do not see how it could be a security issue.

@rohityadavcloud, and @weizhouapache, since the user's problem (#4281 (comment)) was observed in 4.14.0, with another version of StrongSwan, and I could not reproduce it, the change was made in order to honor their comment. However, if we can confirm that it was only a problem with the StrongSwan version and #5375 change was not necessary, I think we could revert #5375.

@GutoVeronezi
Thanks for your explanation.
I am ok with this pr or reverting #5375. We need to make sure both #6903 and #4281 are fixed.

yadvr · 2022-11-23T13:29:19Z

@blueorangutan test

blueorangutan · 2022-11-23T13:30:03Z

@rohityadavcloud a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

DaanHoogland

not tested but looks good. btween reverting and applying this, this seems to be the simpler configuration which has my preference.

yadvr · 2022-11-29T11:14:00Z

@GutoVeronezi can you cherry-pick or rebase your PR to 4.17 branch and edit PR base branch to 4.17?

yadvr · 2022-11-29T11:15:27Z

Ping @weizhouapache @harikrishna-patnala can you help review this?

GutoVeronezi · 2022-11-29T12:15:20Z

@GutoVeronezi can you cherry-pick or rebase your PR to 4.17 branch and edit PR base branch to 4.17?

@rohityadavcloud, done.

weizhouapache

code lgtm

not tested it yet

harikrishna-patnala

Code LGTM,

@GutoVeronezi can you update the structure in the description to reflect the latest change. Thanks

GutoVeronezi · 2022-11-29T12:36:42Z

Sorry, @harikrishna-patnala, I did not catch what you meant. There were no changes in the code, the branch was only rebased to 4.17.

harikrishna-patnala · 2022-11-29T12:56:27Z

@GutoVeronezi I'm just asking about the structure mentioned in the description.

I think now it is only
" : PSK """

right!

GutoVeronezi · 2022-11-29T14:34:11Z

@harikrishna-patnala, that part of the description is an introduction to the problem, explaining why it was happening. The changes I am doing in this PR is in the third paragraph. I made a little change to make it more explicit, thanks.

yadvr · 2022-11-29T15:27:47Z

Thanks @GutoVeronezi I’ll kick tests
@blueorangutan package

blueorangutan · 2022-11-29T15:28:03Z

@rohityadavcloud a Jenkins job has been kicked to build packages. It will be bundled with SystemVM template(s). I'll keep you posted as I make progress.

blueorangutan · 2022-11-29T15:56:01Z

Packaging result: ✔️ el7 ✖️ el8 ✔️ debian ✔️ suse15. SL-JID 4681

yadvr · 2022-11-29T21:24:09Z

@blueorangutan test

blueorangutan · 2022-11-29T21:25:04Z

@rohityadavcloud a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

harikrishna-patnala · 2022-11-30T06:39:04Z

@harikrishna-patnala, that part of the description is an introduction to the problem, explaining why it was happening. The changes I am doing in this PR is in the third paragraph. I made a little change to make it more explicit, thanks.

Thanks @GutoVeronezi for the change

blueorangutan · 2022-11-30T08:52:27Z

Trillian test result (tid-5295)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 40046 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6907-t5295-kvm-centos7.zip
Smoke tests completed. 100 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL	`Failure`	467.85	test_vpc_redundant.py

boring-cyborg Bot added component:networking component:virtual-router Python Warning... Python code Ahead! labels Nov 17, 2022

GutoVeronezi mentioned this pull request Nov 17, 2022

Remote VPN is not working if site-to-site VPN exists #6903

Closed

yadvr added this to the 4.18.0.0 milestone Nov 21, 2022

yadvr requested review from DaanHoogland, shwstppr and weizhouapache November 21, 2022 07:37

yadvr reviewed Nov 21, 2022

View reviewed changes

yadvr approved these changes Nov 21, 2022

View reviewed changes

shwstppr linked an issue Nov 21, 2022 that may be closed by this pull request

Remote VPN is not working if site-to-site VPN exists #6903

Closed

DaanHoogland approved these changes Nov 28, 2022

View reviewed changes

yadvr changed the base branch from main to 4.17 November 29, 2022 11:13

boring-cyborg Bot added component:agent component:api component:build labels Nov 29, 2022

yadvr changed the base branch from 4.17 to main November 29, 2022 11:13

yadvr modified the milestones: 4.18.0.0, 4.17.2.0 Nov 29, 2022

yadvr added the type:regression label Nov 29, 2022

Fix C2S VPN in parallel to S2S VPN

23b16c2

GutoVeronezi force-pushed the fix-c2s-vpn-in-parallel-to-s2s-vpn branch from 4beb910 to 23b16c2 Compare November 29, 2022 12:14

boring-cyborg Bot added component:compute component:orchestration component:UI component:XenServer labels Nov 29, 2022

GutoVeronezi changed the base branch from main to 4.17 November 29, 2022 12:14

weizhouapache approved these changes Nov 29, 2022

View reviewed changes

harikrishna-patnala approved these changes Nov 29, 2022

View reviewed changes

yadvr merged commit cf32f77 into apache:4.17 Nov 30, 2022

yadvr mentioned this pull request Nov 30, 2022

vmware: snapshot operation failure with VMs on datastorecluster #6894

Closed

Uh oh!

Conversation

GutoVeronezi commented Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Bug Severity

How Has This Been Tested?

Uh oh!

codecov Bot commented Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yadvr Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadvr Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Nov 21, 2022

Uh oh!

yadvr commented Nov 21, 2022

Uh oh!

weizhouapache commented Nov 21, 2022

Uh oh!

GutoVeronezi commented Nov 22, 2022

Uh oh!

weizhouapache commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yadvr commented Nov 23, 2022

Uh oh!

blueorangutan commented Nov 23, 2022

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Nov 29, 2022

Uh oh!

yadvr commented Nov 29, 2022

Uh oh!

GutoVeronezi commented Nov 29, 2022

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

harikrishna-patnala left a comment

Choose a reason for hiding this comment

Uh oh!

GutoVeronezi commented Nov 29, 2022

Uh oh!

harikrishna-patnala commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GutoVeronezi commented Nov 29, 2022

Uh oh!

yadvr commented Nov 29, 2022

Uh oh!

blueorangutan commented Nov 29, 2022

Uh oh!

blueorangutan commented Nov 29, 2022

Uh oh!

yadvr commented Nov 29, 2022

Uh oh!

blueorangutan commented Nov 29, 2022

Uh oh!

harikrishna-patnala commented Nov 30, 2022

Uh oh!

blueorangutan commented Nov 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

GutoVeronezi commented Nov 17, 2022 •

edited

Loading

codecov Bot commented Nov 17, 2022 •

edited

Loading

yadvr Nov 21, 2022 •

edited

Loading

weizhouapache commented Nov 22, 2022 •

edited

Loading

harikrishna-patnala commented Nov 29, 2022 •

edited

Loading