[strongSwan-dev] Patch proposals

[Tobias Brunner]

Hi Emeric,

Thanks for the feedback.

>>> patch-0000-ha-flush-segment -> Add a command to flush all the SA of a HA segment
>>
>> What exactly is the use case here?
> 
> We do not use the heartbeat mechanism to detect failures since we have another means for that (corosync).
> If the active charon instance crashes and restarts immediately, we use this in order to flush everything on the passive instance.
> We have another patch that sends a flush message to the remote charon if we start as an active instance (not sure you're interested by this one)

Hm, isn't the HA extension all about having a synchronized view of all
the SAs and providing pretty much uninterrupted service for the clients?
 I don't really get how flushing the state of another node fits into
that.  In particular, shouldn't the restarting node get the state again
from the passive node when it restarts and resyncs?  Afterwards you
could move the segments to the single "active" host again if you want
that, all without any interruption of the existing SAs.

>>> patch-0001-alert-initial-contact -> Add an alert to listen to INITIAL CONTACT events
>>
>> Same question, what is the use case for this?  And can't you just use
> 
> Actually, the alert mechanism is quite useful to get notified from significant events. Therefore we made a custom alert plugin in order to trap
> interesting alerts and externally log them.

Something similar to the error-notify plugin I guess?  I'm not sure how
significant such an INTIAL_CONTACT alert is, what could be deduced from
that?  What do you use it for?  Just a counter?

>> the message hook to check for INITIAL_CONTACT notifies?  I'm mainly
>> concerned with polluting the alert space with new events that are rarely
>> used but increase traffic on the bus.
> 
> I am not sure to understand the message hook thing. You would get all the messages thanks to the hook and parse
> the decrypted payloads in order to intercept the INITIAL CONTACT notifies?
> Would the IKE SA be set in the current thread context? (it contains valuable information to be exported)

The IKE_SA is a actually an argument of the message() callback.  And you
can use the flags (incoming, plain) and perhaps get_exchange_type() on
message_t to select messages to check, then you could use the
get_notify() method of message_t to search for an INITIAL_CONTACT notify.

>>> patch-0004-ha-sync-delay -> Already in a branch on github. The goal is to request a sync only once the configuration has been completely reloaded.
>>
>> Can't say I like this.  It is specific to stroke, adds additional
>> alerts, won't work if multiple config backends are loaded etc.  Perhaps
>> Martin can chime in and comment on why he created the branch.
> 
> I do agree this is not an ideal fix. However, we have a real problem here and I did not find any better solution.

As confirmed by Martin, I don't think we can merge this in its current
state.  Perhaps we can find a better solution in the future.

>>> patch-0005-disable-backtrace-use -> Condition "backtrace" since we do not have backtrace available on our systems
>>
>> As far as I can tell, if backtrace() isn't available and no alternatives
>> are used (like libunwind) backtrace_t.log() is basically a noop that
>> will just log "no support for capturing backtraces".  Is that a problem?
> 
> Not sure about this one since I did not make it. I will try to see what is wrong with it and give you more information.

OK, let me know if you have any news on this.

>>> patch-0006-fix-compile-warnings -> Fix some of the warnings we got on our toolchain
>>
>> Applied most of them to master.  I don't get the cast in
>> openssl_asn1_obj2chunk(), though.  Why is that triple cast needed to
>> simply cast the const qualifier away (the `data` member seems to be a
>> const u_char*)?
> 
> I admit this is quite odd, I tried again:
> 
> return chunk_create((u_char*)asn1->data, asn1->length);
> ->
> openssl_util.c:151: warning: passing argument 1 of 'chunk_create' discards qualifiers from pointer target type
> 
> u_char* data = (u_char*)asn1->data;
> return chunk_create(data, asn1->length);
> ->
> OK

Interesting, might be a compiler bug (should basically be the same
thing).  What compiler/version do you use?

>> I haven't applied the cas_ptr() casts, as these are only required when
>> no atomic built-ins are available (we could probably define the fallback
>> with void* as first argument to avoid the warnings, however, in the
>> apidoc void** clarifies that a pointer to a pointer has to be passed).
> 
> Indeed we added this in order to build without warning on ARM.

Yes, I get them too when building for Android.

Regards,
Tobias