fix(strongswan-connections): check fails with AEAD algorithms#1386
Merged
Conversation
The vici interface omits several (integ-alg, encr-alg, encr-keysize, prf-alg, dh-group) keys when using AEAD algorithms or connections are in connecting state.
markuslf
approved these changes
Jul 17, 2026
markuslf
left a comment
markuslf
left a comment
Member
There was a problem hiding this comment.
Thanks for the follow-up on #806. Merging this because it fixes a real crash: the plugin raises KeyError whenever the IKE SA carries no proposal yet, and also on fixed-key ciphers (3DES/DES/NULL), where vici omits encr-keysize.
I will polish a few things in a follow-up commit rather than block this PR on them:
if row['established'] is not Nonenever fires, because'None'is assigned as a string a few lines above. The result is invalid perfdata ('conn_established'=Nones;;;0,'conn_rekey-time'=n/as;;;0). Perfdata values have to match[-0-9.], so these need a realNonesentinel and a skip.- The
continuein the loop is dead code (it already was in #806):data[key]is assigned before it, so thesetdefault()calls never take effect for a present-but-empty value. - On the rationale: with AEAD, vici only omits
integ-alg/integ-keysize(vici_query.c:622), whileencr-alg,prf-alganddh-groupare still emitted. What your patch actually fixes is theif (proposal)guard atvici_query.c:612(SA not negotiated yet) and theif (ks)guard atvici_query.c:617(fixed-key ciphers). Same forestablished/rekey-time, which are gated onIKE_ESTABLISHEDatvici_query.c:642and therefore also missing in REKEYING/DELETING, not just CONNECTING.
markuslf
enabled auto-merge (squash)
July 17, 2026 12:21
markuslf
added a commit
that referenced
this pull request
Jul 17, 2026
…established SA #1386 stopped the KeyError on a connection whose IKE SA has no negotiated proposal yet, but left three problems behind: * The `established` guard never fired, because the string 'None' was assigned instead of None, and `rekey-time` had no guard at all. Both rendered non-numeric performance data ('conn_established'=Nones, 'conn_rekey-time'=n/as), which breaks ingestion. * The empty-value `continue` was dead code, as it already was in #806: the value is assigned before it, so the setdefault() calls never applied to a key that is present but empty. * `rekey-time-hr` was left unset whenever `rekey-time` was absent. Also drop the dangling dash that a fixed-key cipher such as 3DES rendered ("3DES-/...") on both the IKE and the CHILD SA, and correct the comment claiming `reauth-time`/`rekey-time` are a 5.7-vs-5.9 difference: vici_query.c has emitted both unchanged since 5.2.0, and their presence depends on the peer configuration and on the SA being established, not on the version. Add fixtures for a connecting SA and for a fixed-key cipher.
markuslf
added a commit
that referenced
this pull request
Jul 17, 2026
…zero Two problems predating #1386, both found while checking the previous commit against the vici_query.c source: * `format_child_data()` defaulted an absent `install-time`, `life-time` or `rekey-time` to 0. VICI only sends the timers of an installed CHILD SA, and sends the two lifetimes only if the SA expires at all (vici_query.c:412-423), so an absent timer is unknown, not zero. A CHILD SA without a configured lifetime graphed a life-time of 0 and rendered "Expires: <now>", which reads as if the tunnel were about to drop. Report them as None and skip their performance data instead, as the IKE SA timers already do. * `rekey-time` and `life-time` are `t - now` and go negative once the deadline has passed, but their performance data declared `_min=0`, so an RRD backend could reject exactly the overdue values worth alerting on. Upstream formats both with PRId64 while the byte counters use PRIu64, which is the giveaway. Drop the bogus minimum; `established` and `install-time` are `now - t` and keep theirs. Add a fixture for a CHILD SA that never expires.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The vici interface omits several (integ-alg, encr-alg, encr-keysize, prf-alg, dh-group) keys when using AEAD algorithms or connections are in connecting state.