You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2022/11/17 14:34:51 UTC
[GitHub] [cloudstack] GutoVeronezi opened a new pull request, #6907: Fix C2S VPN in parallel to S2S VPN
GutoVeronezi opened a new pull request, #6907:
URL: https://github.com/apache/cloudstack/pull/6907
### Description
PR #5375, introduced in version 4.15.2.0, removed parameter `%any` of VPNs client-to-site (C2S) IPSec secrets:
- previous structure:
`<IP> %any : PSK "<PSK>"`
- structure after the change:
`<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)
- [x] 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
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [x] Minor
- [ ] Trivial
### 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on a diff in pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
rohityadavcloud commented on code in PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#discussion_r1027664079
##########
systemvm/debian/opt/cloud/bin/configure.py:
##########
@@ -1002,7 +1002,7 @@ def configure_l2tpIpsec(self, left, obj):
secret = CsFile(vpnsecretfilte)
secret.empty()
- secret.addeq("%s : PSK \"%s\"" % (left, psk))
+ secret.addeq(": PSK \"%s\"" % (psk))
Review Comment:
Update, from https://github.com/apache/cloudstack/issues/4281#issue-684586236 it seems the user's fix correlates with the syntax used.
##########
systemvm/debian/opt/cloud/bin/configure.py:
##########
@@ -1002,7 +1002,7 @@ def configure_l2tpIpsec(self, left, obj):
secret = CsFile(vpnsecretfilte)
secret.empty()
- secret.addeq("%s : PSK \"%s\"" % (left, psk))
+ secret.addeq(": PSK \"%s\"" % (psk))
Review Comment:
@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))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#issuecomment-1330866560
Packaging result: :heavy_check_mark: el7 :heavy_multiplication_x: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 4681
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
rohityadavcloud commented on PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#issuecomment-1330467147
@GutoVeronezi can you cherry-pick or rebase your PR to 4.17 branch and edit PR base branch to 4.17?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#issuecomment-1330826441
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] codecov[bot] commented on pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#issuecomment-1318919293
# [Codecov](https://codecov.io/gh/apache/cloudstack/pull/6907?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#6907](https://codecov.io/gh/apache/cloudstack/pull/6907?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4beb910) into [main](https://codecov.io/gh/apache/cloudstack/commit/c5e657ddd80faa93585aef1381e30b7e2d9a9eaa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c5e657d) will **not change** coverage.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## main #6907 +/- ##
=========================================
Coverage 10.87% 10.87%
Complexity 7117 7117
=========================================
Files 2485 2485
Lines 245507 245507
Branches 38334 38334
=========================================
Hits 26699 26699
Misses 215538 215538
Partials 3270 3270
```
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#issuecomment-1331824661
<b>Trillian test result (tid-5295)</b>
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#issuecomment-1324232179
> @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)](https://github.com/apache/cloudstack/issues/4281#issue-684586236)) 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 #6907 and #4281 are fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
rohityadavcloud commented on PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#issuecomment-1330826058
Thanks @GutoVeronezi I’ll kick tests
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
rohityadavcloud commented on PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#issuecomment-1331330020
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#issuecomment-1321855896
@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 #5375 ?
- Since public IP is removed from the configuration in this PR, might it be potential security issue ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#issuecomment-1331331233
@rohityadavcloud a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
rohityadavcloud commented on PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#issuecomment-1330468644
Ping @weizhouapache @harikrishna-patnala can you help review this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on a diff in pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
rohityadavcloud commented on code in PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#discussion_r1027662449
##########
systemvm/debian/opt/cloud/bin/configure.py:
##########
@@ -1002,7 +1002,7 @@ def configure_l2tpIpsec(self, left, obj):
secret = CsFile(vpnsecretfilte)
secret.empty()
- secret.addeq("%s : PSK \"%s\"" % (left, psk))
+ secret.addeq(": PSK \"%s\"" % (psk))
Review Comment:
@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))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#issuecomment-1324194754
@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 (https://github.com/apache/cloudstack/issues/4281#issue-684586236) 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
rohityadavcloud commented on PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#issuecomment-1325071639
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
rohityadavcloud commented on PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#issuecomment-1321696241
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 4624
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#issuecomment-1330539174
> @GutoVeronezi can you cherry-pick or rebase your PR to 4.17 branch and edit PR base branch to 4.17?
@rohityadavcloud, done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#issuecomment-1330742082
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#issuecomment-1330576260
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud merged pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
rohityadavcloud merged PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] harikrishna-patnala commented on pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#issuecomment-1330612271
@GutoVeronezi I'm just asking about the structure mentioned in the description.
structure after the change:
<IP> : PSK "<PSK>"
I think now it is only
: PSK "<PSK>"
right!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] harikrishna-patnala commented on pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#issuecomment-1331706467
> @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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
rohityadavcloud commented on PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#issuecomment-1321698804
(manually fired smoketest)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #6907: Fix C2S VPN in parallel to S2S VPN
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6907:
URL: https://github.com/apache/cloudstack/pull/6907#issuecomment-1325072468
@rohityadavcloud a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org