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.

Sign up for GitHub

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

Jump to bottom

Fix computation of IPsec max. sequence number #27656

Merged
merged 3 commits into from Aug 28, 2023
Merged

Fix computation of IPsec max. sequence number #27656

merged 3 commits into from Aug 28, 2023

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented Aug 23, 2023 •
edited

See commits for details. First commit refactors to allow for subsequent unit test; second fixes the bug. third adds a unit test.

Fix a bug that could cause an incorrect max. sequence number to be reported by `cilium encrypt status` when IPsec is enabled.

@pchaigno pchaigno added release-note/bug This PR fixes an issue in a previous release of Cilium. area/cli Impacts the command line interface of any command in the repository. area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. needs-backport/1.12 needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Aug 23, 2023
@pchaigno pchaigno force-pushed the fix-ipsec-max-seq-number-parsing branch from a129929 to 946e73e Compare August 23, 2023 12:42
@pchaigno pchaigno marked this pull request as ready for review August 23, 2023 14:24
@pchaigno pchaigno requested review from a team as code owners August 23, 2023 14:24
gentoo-root
gentoo-root approved these changes Aug 25, 2023
View reviewed changes
cilium/cmd/encrypt_status.go Show resolved Hide resolved
derailed
derailed requested changes Aug 25, 2023
View reviewed changes
Copy link
Contributor

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@pchaigno Great catch! Nice work. Just a few small picks...

cilium/cmd/encrypt_status.go Show resolved Hide resolved
cilium/cmd/encrypt_status.go Show resolved Hide resolved
cilium/cmd/encrypt_status_test.go Show resolved Hide resolved
@pchaigno pchaigno added release-blocker/1.12 This issue will prevent the release of the next version of Cilium. release-blocker/1.13 This issue will prevent the release of the next version of Cilium. release-blocker/1.14 This issue will prevent the release of the next version of Cilium. release-blocker/1.15 This issue will prevent the release of the next version of Cilium. and removed release-blocker/1.15 This issue will prevent the release of the next version of Cilium. labels Aug 25, 2023
@derailed derailed self-requested a review August 25, 2023 19:30
derailed
derailed approved these changes Aug 25, 2023
View reviewed changes
Copy link
Contributor

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@pchaigno Thanks Paul for the comments. LGTM

@pchaigno
cmd: Refactor maxSequenceNumber
8e2b132 
This refactoring moves the actual logic to extract the maximum sequence
number into a dedicated function. That will be useful to allow us to
test this logic in a following commit.

This commit has no functionnal changes.

As a reminder, we can't use netlink.XfrmStatesList here because it
doesn't have the sequence numbers. We can't use JSON format because the
ip xfrm commands don't support it.

Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
@pchaigno
cmd: Fix the computed IPsec max. sequence number
c6cb225 
maxSequenceNumber currently iterates over all XFRM states in the ip xfrm
state list output to find the largest sequence number. It however does
so while keeping the parsed sequence numbers as hexadecimal strings.
Hence, a number like "0xc1" is understood as being larger than e.g.
"0x1234".

This commit fixes it by parsing the sequence numbers into int64 before
comparing them.

We also need to adapt the regular expression slightly to avoid
considering the "0x" prefix as part of the number, given
strconv.ParseInt doesn't support it.

Fixes: 2842c49 ("cli: add helper functions for `cilium encrypt`")
Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
@pchaigno pchaigno force-pushed the fix-ipsec-max-seq-number-parsing branch from 946e73e to e642a9f Compare August 28, 2023 12:34
@pchaigno
cmd: Unit tests for extractMaxSequenceNumber
ae0ed2a 
This commit simply adds two small unit tests for the
extractMaxSequenceNumber function. The first test covers the bug fixed
in the previous commit.

Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
@pchaigno pchaigno force-pushed the fix-ipsec-max-seq-number-parsing branch from e642a9f to ae0ed2a Compare August 28, 2023 12:35
@pchaigno
Copy link
Member Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 28, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.2 Aug 28, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.13.7 Aug 28, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.12.14 Aug 28, 2023
@pchaigno pchaigno merged commit 21d7d0a into cilium:main Aug 28, 2023
59 of 60 checks passed
@pchaigno pchaigno deleted the fix-ipsec-max-seq-number-parsing branch August 28, 2023 13:50
@jibi jibi mentioned this pull request Sep 4, 2023
Merged
16 tasks
@jibi jibi added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Sep 4, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.14 in 1.14.2 Sep 4, 2023
@jibi jibi mentioned this pull request Sep 4, 2023
Merged
10 tasks
@jibi jibi added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Sep 4, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.13 in 1.13.7 Sep 4, 2023
@jibi jibi mentioned this pull request Sep 5, 2023
Merged
5 tasks
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.12 in 1.12.14 Sep 5, 2023
@jibi jibi added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Sep 7, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.14 to Backport done to v1.14 in 1.14.2 Sep 7, 2023
@michi-covalent michi-covalent mentioned this pull request Sep 9, 2023
Merged
@michi-covalent michi-covalent added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Sep 9, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.13 to Backport done to v1.13 in 1.13.7 Sep 9, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.13 to Backport done to v1.13 in 1.13.7 Sep 9, 2023
@michi-covalent michi-covalent mentioned this pull request Sep 9, 2023
Merged
@michi-covalent michi-covalent added backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed backport-pending/1.12 labels Sep 9, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.12 to Backport done to v1.12 in 1.12.14 Sep 9, 2023
@michi-covalent michi-covalent mentioned this pull request Sep 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gentoo-root gentoo-root gentoo-root approved these changes

@derailed derailed derailed approved these changes

Assignees
No one assigned
Labels
area/cli Impacts the command line interface of any command in the repository. area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-blocker/1.12 This issue will prevent the release of the next version of Cilium. release-blocker/1.13 This issue will prevent the release of the next version of Cilium. release-blocker/1.14 This issue will prevent the release of the next version of Cilium. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.12.14
Backport done to v1.12
1.13.7
Backport done to v1.13
1.14.2
Backport done to v1.14
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pchaigno @derailed @gentoo-root @jibi @michi-covalent