Project

General

Profile

Issue #2663

keylife vs liftime mix up

Added by Jafar Al-Gharaibeh over 3 years ago. Updated over 3 years ago.

Status:
Closed
Priority:
Normal
Category:
starter
Affected version:
5.6.2
Resolution:
Won't fix

Description

I got into a strange situation where changes to "liftime" in a connection config in ipsec.conf file seems to be ignored. ipsec statusall shows child SAs with 8 hours rekey value no matter what I do. I discovered later that in the %default connection (included from another file) I had "keylife" configured to 8 hours. The "keylife" 8 hours configuration in %default was overriding "lifetime" in the actual connection configuration.

The documentation says lifetime is just an alias for keylife. That seems to be true only if the same keyword is used across all inherited configuration. The parser doesn't seem to handle the situation where the two keywords are used. Of course that is probably uncommon, but it does happen when you add a new connection configuration a few months after you initially created the original configuration. The documentation does not say you cannot use both, it just says it is an alias.

Quick grep in the sources show that both keywords are assigned the same "KW_KEYLIFE" value. grepping those words under src, didn't turn up hits that I can easily/quickly trace.

Associated revisions

Revision e6d17d56 (diff)
Added by Tobias Brunner over 3 years ago

man: Remove keylife/rekeymargin from ipsec.conf man page

We continue to parse them but remove the documentation because mixing the two
sets of keywords in the same config might result in unexpected behavior.

References #2663.

History

#1 Updated by Tobias Brunner over 3 years ago

  • Category set to starter
  • Status changed from New to Feedback
  • Resolution set to Won't fix

The underlying parser's output are grouped key-value pairs (strings) that are partially related to each other (depending on the use of also and %default). When producing actual config objects from these key-value pairs they are enumerated for each conn/ca section, first those in that section then those in parent sections (linearized, see source:src/starter/parser/conf_parser.c#L492), with %default last. Any key that has already been seen is ignored in any of the parent sections, however, aliases are not considered there (the code that enumerates the key-value pairs does that mapping). So in this particular case the setting in %default overrides the setting in the actual conn section. Since this is a legacy interface and it doesn't really make sense to mix different aliases in the same config I don't think it's worth fixing.

#2 Updated by Jafar Al-Gharaibeh over 3 years ago

Tobias, I agree with you that it's not worth fixing. Mixing the two aliases is uncommon, but as I said, it might happen when you have multiple files/connections to maintain. The side effect of mixing is nasty and hard to track down at first. I suggest to just update the documentation and warn users that while aliases are OK, mixing them is not OK. This is not reflected in the current documentation.

#3 Updated by Jafar Al-Gharaibeh over 3 years ago

I updated the wiki page to look like this:

keylife
synonym for lifetime. Be consistent and use one or the other across all connection configurations. Mixing the use of the two keywords might lead to unexpected behavior.

I also added the same text to rekeymargin/margintime. There are other places that I didn't update.

#4 Updated by Tobias Brunner over 3 years ago

These aliases only exist for compatibility with very old configs (before 4.3.5 where we added support for packet/byte-based limits and the new keywords were introduced to make configuration more consistent). So we could also just remove them from the documentation, or at least discourage their use.

#5 Updated by Jafar Al-Gharaibeh over 3 years ago

Tobias Brunner wrote:

So we could also just remove them from the documentation, or at least discourage their use.

I like this idea. When it comes to configuration, I never liked having multiple ways do the same the same thing. More things to maintain, more things to remember, and more ways for things to go wrong like in the example above. The code can continue to parse these synonyms for backward compatibility, but without mentioning them in the documentation. or at least the can be moved the deprecated section.

#6 Updated by Tobias Brunner over 3 years ago

  • Status changed from Feedback to Closed
  • Assignee set to Tobias Brunner

I've removed them from the documentation.

Also available in: Atom PDF