Skip to content

detecting IPSEC SPI unchanged #34037

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
Merged
merged 2 commits into from
Oct 2, 2024
Merged

detecting IPSEC SPI unchanged #34037

merged 2 commits into from
Oct 2, 2024

Conversation

smagnani96
Copy link
Contributor

@smagnani96 smagnani96 commented Jul 26, 2024 •
edited by pchaigno
Loading

Adding logic (and unit test) to detect unchanged SPI while creating a new IPSEC key.

Fixes: #14873

@pchaigno @julianwiedmann 😃

In case of an IPsec key rotation, error if the user forgot to increment the SPI per the documentation.

@smagnani96 smagnani96 requested a review from a team as a code owner July 26, 2024 14:51
@smagnani96 smagnani96 requested a review from rgo3 July 26, 2024 14:51
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 26, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jul 26, 2024
rgo3
rgo3 requested changes Jul 26, 2024
View reviewed changes
Copy link
Contributor

@rgo3 rgo3 left a comment

Choose a reason for hiding this comment

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

When I was assigned to the issue that's being addressed here, the originally proposed solution involved checking the key and SPI from an existing XFRM state and comparing it to the key and SPI we read in LoadIPSecKeys. Overall I prefer the simplicity of the approach here but LoadIPSecKeys not behaving the same if it's called on the same key file even if underlying kernel state has not changed worries me a bit.

@smagnani96 smagnani96 force-pushed the enhancement/ipsec_key_parse_robustness branch from 2990185 to d3f0fed Compare July 30, 2024 18:12
@smagnani96
Copy link
Contributor Author

smagnani96 commented Jul 30, 2024 •
edited
Loading

When I was assigned to the issue that's being addressed here, the originally proposed solution involved checking the key and SPI from an existing XFRM state and comparing it to the key and SPI we read in LoadIPSecKeys. Overall I prefer the simplicity of the approach here but LoadIPSecKeys not behaving the same if it's called on the same key file even if underlying kernel state has not changed worries me a bit.

I modified the code to:

  1. allow providing two identical keys. Though, now the ipSecKeysRemovalTime[oldKey.Spi] = time.Now() is executed only when needed;
  2. throw error when different keys with same spi.
  3. changed the tests accordingly. In particular, the SPI of all the keys variables declared for the legit TestLoadKeys, since now different keys with same SPI is not allowed.

My last doubt are:

  1. we could even load and compare the spi from the XFRM state, but AFAIU the latest spi and key should correspond to the last valid parsed key in this scan loop. If that is correct, it might not be needed to retrieve the XFRM state.
  2. why would someone have more than 1 key (duplicate or even different ones) in the secret file? Is it for backward compatibility?

@smagnani96 smagnani96 requested a review from rgo3 August 8, 2024 07:55
@julianwiedmann julianwiedmann added area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. feature/ipsec Relates to Cilium's IPsec feature release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Aug 9, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 9, 2024
@pchaigno pchaigno self-requested a review August 12, 2024 14:11
pchaigno
pchaigno reviewed Aug 14, 2024
View reviewed changes
@smagnani96 smagnani96 mentioned this pull request Aug 14, 2024
Closed
3 tasks
@smagnani96 smagnani96 marked this pull request as draft September 2, 2024 17:05
@smagnani96 smagnani96 force-pushed the enhancement/ipsec_key_parse_robustness branch from d3f0fed to d4cb5c4 Compare September 3, 2024 00:12
@smagnani96 smagnani96 marked this pull request as ready for review September 3, 2024 11:36
@smagnani96
Copy link
Contributor Author

This PRs has been rebased based on #34652 (1st commit 4e74c4a).

pchaigno
pchaigno approved these changes Sep 3, 2024
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

I think this is good to go without the first commit.

I'd remove it so we can merge because I suspect the other PR will take a bit longer to get ready.

@smagnani96 smagnani96 force-pushed the enhancement/ipsec_key_parse_robustness branch 2 times, most recently from c81b1e0 to f9f3549 Compare September 11, 2024 09:01
@smagnani96
Copy link
Contributor Author

/test

@smagnani96 smagnani96 force-pushed the enhancement/ipsec_key_parse_robustness branch from f9f3549 to 0bedf7e Compare September 12, 2024 11:38
@smagnani96 smagnani96 requested a review from a team as a code owner September 12, 2024 11:38
@smagnani96 smagnani96 requested a review from jibi September 12, 2024 11:38
@pchaigno pchaigno self-requested a review September 12, 2024 12:09
@smagnani96
Copy link
Contributor Author

/test

@smagnani96 smagnani96 force-pushed the enhancement/ipsec_key_parse_robustness branch from 0bedf7e to e55ab8d Compare September 13, 2024 09:46
@smagnani96
Copy link
Contributor Author

/test

pchaigno
pchaigno reviewed Sep 13, 2024
View reviewed changes
@smagnani96 smagnani96 force-pushed the enhancement/ipsec_key_parse_robustness branch from e55ab8d to c986b49 Compare September 13, 2024 16:33
@smagnani96
Copy link
Contributor Author

/test

@jibi jibi removed their request for review September 17, 2024 14:48
@jibi
Copy link
Member

jibi commented Sep 17, 2024

removing myself as Paul has already approved on behalf of sig-datapath

rgo3
rgo3 approved these changes Sep 18, 2024
View reviewed changes
Copy link
Contributor

@rgo3 rgo3 left a comment

Choose a reason for hiding this comment

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

Left one comment with regards to cleaning state after individual tests, otherwise LGTM. Sorry for the long delay on my part.

@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 Sep 18, 2024
@rgo3 rgo3 added the dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. label Sep 18, 2024
@ldelossa ldelossa removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 18, 2024
@smagnani96
detecting IPSEC SPI unchanged
187f663 
This commit introduce a control during the `LoadIPSecKeys` to return
an error when providing a new key (e.g., during rotation) without
incrementing the SPI. In this fix, such case is intercepted and logged
with an error, while preserving status and policies referring to the
latest/previous valid key by not inserting the key in `ipSecKeysRemovalTime`.
Moreover, this commit adjusts the SPI fields in the test data so that they
can preserve their expected behaviour. Finally, a new test is introduced
to verify that `LoadIPSecKeys` effectively returns an error when the SPI
does not while loading a new IPSec key.

This "bug" has a user-visible impact during the IPSec key-rotation:
if performing the key rotation without changing the SPI, an error message
is logged. In this case, the oldKey is not marked to be removed and
is still in use.

Expected error in the daemons logs while reproducing after the fix:
`invalid SPI: changing IPSec keys requires incrementing the key id`.

Fixes: cilium#14873

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@smagnani96
ipsec: add UnsetTestIPSecKey API for testing purpose
609d010 
This commit introduces the `UnsetTestIPSecKey` API for testing purpose only.
This function is used to reset the current state of IPSec global variables
during testing. In particular, it helps to prevent loading two IPSec keys
with the same SPI in `node_linux_test`, which would cause the tests to fail
after the previous commit. With this function, it is easy to reset the internal
state of the IPSec-related variables either while executing a test or when
tearing down a test suite. This function is also used in the local
`ipsec_linux_test.go`, to expect a coherent behavior while tearing down
a test suite.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@smagnani96 smagnani96 force-pushed the enhancement/ipsec_key_parse_robustness branch from c986b49 to 609d010 Compare September 19, 2024 15:05
@smagnani96
Copy link
Contributor Author

/test

@rgo3 rgo3 added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. and removed dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. labels Sep 25, 2024
@julianwiedmann
Copy link
Member

@smagnani96 fyi there's an outstanding discussion here, once that is resolved the PR should auto-merge.

@smagnani96
Copy link
Contributor Author

@smagnani96 fyi there's an outstanding discussion here, once that is resolved the PR should auto-merge.

Yep, I wanted to wait for the last feedback from @pchaigno to make sure I got what he meant 👍

@julianwiedmann julianwiedmann added this pull request to the merge queue Oct 2, 2024
Merged via the queue into cilium:main with commit c8e0131 Oct 2, 2024
63 checks passed
@smagnani96 smagnani96 deleted the enhancement/ipsec_key_parse_robustness branch March 18, 2025 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchaigno pchaigno pchaigno approved these changes

@rgo3 rgo3 rgo3 approved these changes

Assignees
No one assigned
Labels
area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. feature/ipsec Relates to Cilium's IPsec feature kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Throw error if IPSec key changed but key ID didn't
6 participants
@smagnani96 @jibi @julianwiedmann @pchaigno @rgo3 @ldelossa