[strongSwan-dev] Patch proposals

[Emeric POUPON]

Hello,

>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.

Yes you're right.
We made that patch because we noticed some annoying situations (combined local/remote failures with no segment responsibility changes)
where the passive node gets more and more dead connections stored, making the troobleshooting even harder.
This is obviously not an ideal patch: we will try to report such situations in order to properly fix the underlying problems.

>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?

It gives a clue the remote node just showed up or has rebooted due to a failure.

>> 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.

Thanks for the information!

>>>> 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.

Ok, no problem with that.

>>>> 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.

Ok here is the thing. In the build environment, we have the libexecinfo available but we do not want for various reasons to embed it.
Therefore strongswan successfully detects and compiles with it but of course it won't run since the lib is missing.
That's why we did this patch.


>> 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?
This warning shows up with gcc 4.2.1


Regards,

Emeric