[strongSwan-dev] Backward compatibility option for inbound SA/SP marking

[Christophe Gouault]

Hello strongSwan maintainers,

I am quite concerned by the following commit, that appeared in strongSwan
5.5.2:

067fd2c69c25 ("child-sa: Do not install mark on inbound kernel SA")

As the title states, this patch prevents charon from setting the mark of the
inbound IPsec SA, while still marking the SPs and the outbound SA. This
behavioral change was done as a workaround for route-based VPN to work, and
assumed that it did not break other uses of the mark.

The patch results in several issues, by increasing order of severity:

1/ this violates strongSwan's documentation:

mark_in = <value>[/<mask>]

sets an XFRM mark in the inbound IPsec SA and policy. If the mask is
missing then a default mask of 0xffffffff is assumed.

2/ this breaks the symmetry and coherency of SA and SP configuration.

The inbound SA is the only object that does not wear a mark. The mark is
precisely designed to convey information and to link objects together.

3/ this changes strongSwan's behavior with no backward compatibility option.

So I propose to write a patch to avoid changing the default behavior of
mark_in, while offering the ability to not mark the SA:

the most flexible solution would be to enable to split mark_in/mark_out into
mark_in_sa/mark_in_sp/mark_out_sa/mark_out_sp, just like "mark" may today be
split into "mark_in" and "mark_out".

That way, one would be free to finely mark SPs and/or SAs in conformance to
one's needs. The old behavior would be preserved by default.

Best Regards,
Christophe

[Tobias Brunner]

Hi Christophe,

Post by Christophe Gouault
As the title states, this patch prevents charon from setting the mark of the
inbound IPsec SA, while still marking the SPs and the outbound SA. This
behavioral change was done as a workaround for route-based VPN to work, and
assumed that it did not break other uses of the mark.

Could you provide an example where the old behavior is necessary? Or
are you just concerned with setups that already have firewall rules for
inbound traffic in place? (Which might not be a problem because if no
mark is set on the SA the kernel does not care if the packet has a mark
set.)
So? We update the docs.

Post by Christophe Gouault
2/ this breaks the symmetry and coherency of SA and SP configuration.

So?

Post by Christophe Gouault
The inbound SA is the only object that does not wear a mark. The mark is
precisely designed to convey information and to link objects together.

I don't really see it as a link (that's the reqid). But it makes
configuration easier as marking encrypted inbound traffic is tricky and
should not really be necessary (when do you have duplicate local SPIs?).

Post by Christophe Gouault
3/ this changes strongSwan's behavior with no backward compatibility option.

If that behavior is actually required.

Post by Christophe Gouault
the most flexible solution would be to enable to split mark_in/mark_out into
mark_in_sa/mark_in_sp/mark_out_sa/mark_out_sp, just like "mark" may today be
split into "mark_in" and "mark_out".

Go ahead, if you think that's really necessary. (But please only for
swanctl.conf.)

Regards,
Tobias

[Christophe Gouault]

Hi Tobias,

thanks for your answer,

Post by Tobias Brunner
Hi Christophe,

Post by Christophe Gouault
As the title states, this patch prevents charon from setting the mark of the
inbound IPsec SA, while still marking the SPs and the outbound SA. This
behavioral change was done as a workaround for route-based VPN to work, and
assumed that it did not break other uses of the mark.

Could you provide an example where the old behavior is necessary? Or
are you just concerned with setups that already have firewall rules for
inbound traffic in place? (Which might not be a problem because if no
mark is set on the SA the kernel does not care if the packet has a mark
set.)

I had in mind an existing vti use case with IPsec offloading. The
kernel does not need the inbound SA to be marked, but the IPsec
offloading solution benefits from the fact that SPs and SAs reference
their vti through the mark.

But I am aware it is not the general case, that's why I advocate a
configurable behavior.

A patch proposal follows, with a global boolean option in strongswan.conf.

I let the new behavior by default (no inbound SA marking) to avoid
causing trouble to people already exploiting the new behavior. However
note that the documentation and several tests in the testing/
directory still state and assume that the inbound SA is marked.

Best regards,
Christophe

[Tobias Brunner]

Hi Christophe,

Post by Christophe Gouault

Post by Tobias Brunner

Post by Christophe Gouault
As the title states, this patch prevents charon from setting the mark of the
inbound IPsec SA, while still marking the SPs and the outbound SA. This
behavioral change was done as a workaround for route-based VPN to work, and
assumed that it did not break other uses of the mark.

Could you provide an example where the old behavior is necessary? Or
are you just concerned with setups that already have firewall rules for
inbound traffic in place? (Which might not be a problem because if no
mark is set on the SA the kernel does not care if the packet has a mark
set.)

I had in mind an existing vti use case with IPsec offloading. The
kernel does not need the inbound SA to be marked, but the IPsec
offloading solution benefits from the fact that SPs and SAs reference
their vti through the mark.

I see. VTIs themselves don't seem to require a mark on the SA or the
packets (I guess they are just selected based on the IPs), the kernel
will even apply the key configured on the VTI interface as mark to the
packet before doing the inbound policy check (it removes it again, though).

I actually never liked the inbound handling of the marks on IPsec SAs,
I'd very much preferred it if the kernel, instead of using it as
selector when looking up an SA, would simply apply the mark to decrypted
packets.

Post by Christophe Gouault
But I am aware it is not the general case, that's why I advocate a
configurable behavior.
A patch proposal follows, with a global boolean option in strongswan.conf.

Thanks. But I'd actually prefer a connection specific config. I did
something like that in the mark-inbound-sa branch.

Post by Christophe Gouault
However
note that the documentation and several tests in the testing/
directory still state and assume that the inbound SA is marked.

I guess some test scenarios could be adapted (for several it's still
required to mark the packets before decryption, though, e.g. based on
UDP-encap ports). The documentation (what's in the repo) is updated in
the branch above the wiki is updated too.

Regards,
Tobias

[Christophe Gouault]

Post by Tobias Brunner

Post by Christophe Gouault
I had in mind an existing vti use case with IPsec offloading. The
kernel does not need the inbound SA to be marked, but the IPsec
offloading solution benefits from the fact that SPs and SAs reference
their vti through the mark.

I see. VTIs themselves don't seem to require a mark on the SA or the
packets (I guess they are just selected based on the IPs), the kernel
will even apply the key configured on the VTI interface as mark to the
packet before doing the inbound policy check (it removes it again, though).

Indeed, I confirm.

Post by Tobias Brunner
I actually never liked the inbound handling of the marks on IPsec SAs,
I'd very much preferred it if the kernel, instead of using it as
selector when looking up an SA, would simply apply the mark to decrypted
packets.

I find it tricky too. It's not a simple marker, it is really part of the SA ID.
The kernel even enables 2 SAs to have the same triple (@dst, proto,
SPI), provided their mark do not overlap...

Post by Tobias Brunner

Post by Christophe Gouault
But I am aware it is not the general case, that's why I advocate a
configurable behavior.
A patch proposal follows, with a global boolean option in strongswan.conf.

Thanks. But I'd actually prefer a connection specific config. I did
something like that in the mark-inbound-sa branch.

OK, thanks.
I had a look in the mark-inbound-sa branch, I think there are other
methods where the SA mark must be set: child_sa_t.update,
child_sa_t.destroy.

Best regards,
Christophe

[Christophe Gouault]

The first patch completes the marking of inbound kernel SAs.

The second enables to configure the mark_in_sa option in ipsec.conf
connection section. Since it is a compatibility option with an older behavior,
let us enable to configure it via this legacy API.

[Christophe Gouault]

---
src/libcharon/sa/child_sa.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/src/libcharon/sa/child_sa.c b/src/libcharon/sa/child_sa.c
index 7ed96910d7e7..fdd25e474f85 100644
--- a/src/libcharon/sa/child_sa.c
+++ b/src/libcharon/sa/child_sa.c
@@ -163,6 +163,12 @@ struct private_child_sa_t {
mark_t mark_out;

/**
+ * mark used for the inbound kernel SA
+ * (= mark_in if OPT_MARK_IN_SA, 0/0 else)
+ */
+ mark_t mark_sa_in;
+
+ /**
* absolute time when rekeying is scheduled
*/
time_t rekey_time;
@@ -754,7 +760,7 @@ static status_t install_internal(private_child_sa_t *this, chunk_t encr,
uint32_t tfc = 0;
host_t *src, *dst;
status_t status;
- mark_t mark = (mark_t){};
+ mark_t mark;
bool update = FALSE;

/* BEET requires the bound address from the traffic selectors */
@@ -778,10 +784,7 @@ static status_t install_internal(private_child_sa_t *this, chunk_t encr,
this->my_cpi = cpi;
dst_ts = my_ts;
src_ts = other_ts;
- if (this->config->has_option(this->config, OPT_MARK_IN_SA))
- {
- mark = this->mark_in;
- }
+ mark = this->mark_sa_in;
}
else
{
@@ -1481,6 +1484,7 @@ METHOD(child_sa_t, update, status_t,
.dst = this->my_addr,
.spi = this->my_spi,
.proto = proto_ike2ip(this->protocol),
+ .mark = this->mark_sa_in,
};
kernel_ipsec_update_sa_t sa = {
.cpi = this->ipcomp != IPCOMP_NONE ? this->my_cpi : 0,
@@ -1666,6 +1670,7 @@ METHOD(child_sa_t, destroy, void,
.dst = this->my_addr,
.spi = this->my_spi,
.proto = proto_ike2ip(this->protocol),
+ .mark = this->mark_sa_in,
};
kernel_ipsec_del_sa_t sa = {
.cpi = this->my_cpi,
@@ -1855,6 +1860,11 @@ child_sa_t * child_sa_create(host_t *me, host_t* other,
}
}

+ if (config->has_option(this->config, OPT_MARK_IN_SA))
+ {
+ this->mark_sa_in = this->mark_in;
+ }
+
if (!this->reqid)
{
/* reuse old reqid if we are rekeying an existing CHILD_SA. While the

[Christophe Gouault]

---
src/libcharon/plugins/stroke/stroke_config.c | 3 ++-
src/starter/args.c | 1 +
src/starter/confread.h | 1 +
src/starter/keywords.h | 1 +
src/starter/keywords.txt | 1 +
src/starter/starterstroke.c | 2 ++
src/stroke/stroke_msg.h | 1 +
7 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/libcharon/plugins/stroke/stroke_config.c b/src/libcharon/plugins/stroke/stroke_config.c
index ac01292104da..0d7cc88b7089 100644
--- a/src/libcharon/plugins/stroke/stroke_config.c
+++ b/src/libcharon/plugins/stroke/stroke_config.c
@@ -1090,7 +1090,8 @@ static child_cfg_t *build_child_cfg(private_stroke_config_t *this,
(msg->add_conn.ipcomp ? OPT_IPCOMP : 0) |
(msg->add_conn.me.hostaccess ? OPT_HOSTACCESS : 0) |
(msg->add_conn.install_policy ? 0 : OPT_NO_POLICIES) |
- (msg->add_conn.sha256_96 ? OPT_SHA256_96 : 0),
+ (msg->add_conn.sha256_96 ? OPT_SHA256_96 : 0) |
+ (msg->add_conn.mark_in_sa ? OPT_MARK_IN_SA : 0),
.tfc = msg->add_conn.tfc,
.inactivity = msg->add_conn.inactivity,
.dpd_action = map_action(msg->add_conn.dpd.action),
diff --git a/src/starter/args.c b/src/starter/args.c
index 477a52082d85..af2b55c65e1b 100644
--- a/src/starter/args.c
+++ b/src/starter/args.c
@@ -178,6 +178,7 @@ static const token_info_t token_info[] =
{ ARG_MISC, 0, NULL /* KW_MARK */ },
{ ARG_MISC, 0, NULL /* KW_MARK_IN */ },
{ ARG_MISC, 0, NULL /* KW_MARK_OUT */ },
+ { ARG_ENUM, offsetof(starter_conn_t, mark_in_sa), LST_bool },
{ ARG_MISC, 0, NULL /* KW_TFC */ },
{ ARG_MISC, 0, NULL /* KW_PFS_DEPRECATED */ },
{ ARG_MISC, 0, NULL /* KW_CONN_DEPRECATED */ },
diff --git a/src/starter/confread.h b/src/starter/confread.h
index 8ee730daa078..3e3f215a64f5 100644
--- a/src/starter/confread.h
+++ b/src/starter/confread.h
@@ -143,6 +143,7 @@ struct starter_conn {
uint32_t reqid;
mark_t mark_in;
mark_t mark_out;
+ bool mark_in_sa;
uint32_t replay_window;
uint32_t tfc;
bool install_policy;
diff --git a/src/starter/keywords.h b/src/starter/keywords.h
index 0cb46a7401c9..85f626c98875 100644
--- a/src/starter/keywords.h
+++ b/src/starter/keywords.h
@@ -77,6 +77,7 @@ enum kw_token_t {
KW_MARK,
KW_MARK_IN,
KW_MARK_OUT,
+ KW_MARK_IN_SA,
KW_TFC,
KW_PFS_DEPRECATED,
KW_CONN_DEPRECATED,
diff --git a/src/starter/keywords.txt b/src/starter/keywords.txt
index 3f92dc83f50f..7699245c1399 100644
--- a/src/starter/keywords.txt
+++ b/src/starter/keywords.txt
@@ -74,6 +74,7 @@ replay_window, KW_REPLAY_WINDOW
mark, KW_MARK
mark_in, KW_MARK_IN
mark_out, KW_MARK_OUT
+mark_in_sa, KW_MARK_IN_SA
tfc, KW_TFC
cacert, KW_CACERT
crluri, KW_CRLURI
diff --git a/src/starter/starterstroke.c b/src/starter/starterstroke.c
index 90af9372ac06..a43e3424076c 100644
--- a/src/starter/starterstroke.c
+++ b/src/starter/starterstroke.c
@@ -229,8 +229,10 @@ int starter_stroke_add_conn(starter_config_t *cfg, starter_conn_t *conn)
msg->add_conn.replay_window = conn->replay_window;
msg->add_conn.mark_in.value = conn->mark_in.value;
msg->add_conn.mark_in.mask = conn->mark_in.mask;
+ msg->add_conn.mark_in.mask = conn->mark_in.mask;
msg->add_conn.mark_out.value = conn->mark_out.value;
msg->add_conn.mark_out.mask = conn->mark_out.mask;
+ msg->add_conn.mark_in_sa = conn->mark_in_sa;
msg->add_conn.tfc = conn->tfc;

add_end(&msg, offsetof(stroke_msg_t, add_conn.me), &conn->left);
diff --git a/src/stroke/stroke_msg.h b/src/stroke/stroke_msg.h
index 60ea0028d8b9..d31a4cfe0765 100644
--- a/src/stroke/stroke_msg.h
+++ b/src/stroke/stroke_msg.h
@@ -300,6 +300,7 @@ struct stroke_msg_t {
uint32_t value;
uint32_t mask;
} mark_in, mark_out;
+ bool mark_in_sa;
stroke_end_t me, other;
uint32_t replay_window;
bool sha256_96;

[Simon Deziel]

Post by Christophe Gouault
diff --git a/src/starter/starterstroke.c b/src/starter/starterstroke.c
index 90af9372ac06..a43e3424076c 100644
--- a/src/starter/starterstroke.c
+++ b/src/starter/starterstroke.c
@@ -229,8 +229,10 @@ int starter_stroke_add_conn(starter_config_t *cfg, starter_conn_t *conn)
msg->add_conn.replay_window = conn->replay_window;
msg->add_conn.mark_in.value = conn->mark_in.value;
msg->add_conn.mark_in.mask = conn->mark_in.mask;
+ msg->add_conn.mark_in.mask = conn->mark_in.mask;

Probably added by accident...

[Christophe Gouault]

Hi Simon,

indeed, it's a temporary keyboard/chair interface problem ;-)

Thanks for noticing it.

Regards,
Christophe

Post by Simon Deziel

Post by Christophe Gouault
diff --git a/src/starter/starterstroke.c b/src/starter/starterstroke.c
index 90af9372ac06..a43e3424076c 100644
--- a/src/starter/starterstroke.c
+++ b/src/starter/starterstroke.c
@@ -229,8 +229,10 @@ int starter_stroke_add_conn(starter_config_t *cfg, starter_conn_t *conn)
msg->add_conn.replay_window = conn->replay_window;
msg->add_conn.mark_in.value = conn->mark_in.value;
msg->add_conn.mark_in.mask = conn->mark_in.mask;
+ msg->add_conn.mark_in.mask = conn->mark_in.mask;

Probably added by accident...

[Christophe Gouault]

---
src/libcharon/sa/child_sa.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/src/libcharon/sa/child_sa.c b/src/libcharon/sa/child_sa.c
index 7ed96910d7e7..fdd25e474f85 100644
--- a/src/libcharon/sa/child_sa.c
+++ b/src/libcharon/sa/child_sa.c
@@ -163,6 +163,12 @@ struct private_child_sa_t {
mark_t mark_out;

/**
+ * mark used for the inbound kernel SA
+ * (= mark_in if OPT_MARK_IN_SA, 0/0 else)
+ */
+ mark_t mark_sa_in;
+
+ /**
* absolute time when rekeying is scheduled
*/
time_t rekey_time;
@@ -754,7 +760,7 @@ static status_t install_internal(private_child_sa_t *this, chunk_t encr,
uint32_t tfc = 0;
host_t *src, *dst;
status_t status;
- mark_t mark = (mark_t){};
+ mark_t mark;
bool update = FALSE;

/* BEET requires the bound address from the traffic selectors */
@@ -778,10 +784,7 @@ static status_t install_internal(private_child_sa_t *this, chunk_t encr,
this->my_cpi = cpi;
dst_ts = my_ts;
src_ts = other_ts;
- if (this->config->has_option(this->config, OPT_MARK_IN_SA))
- {
- mark = this->mark_in;
- }
+ mark = this->mark_sa_in;
}
else
{
@@ -1481,6 +1484,7 @@ METHOD(child_sa_t, update, status_t,
.dst = this->my_addr,
.spi = this->my_spi,
.proto = proto_ike2ip(this->protocol),
+ .mark = this->mark_sa_in,
};
kernel_ipsec_update_sa_t sa = {
.cpi = this->ipcomp != IPCOMP_NONE ? this->my_cpi : 0,
@@ -1666,6 +1670,7 @@ METHOD(child_sa_t, destroy, void,
.dst = this->my_addr,
.spi = this->my_spi,
.proto = proto_ike2ip(this->protocol),
+ .mark = this->mark_sa_in,
};
kernel_ipsec_del_sa_t sa = {
.cpi = this->my_cpi,
@@ -1855,6 +1860,11 @@ child_sa_t * child_sa_create(host_t *me, host_t* other,
}
}

+ if (config->has_option(this->config, OPT_MARK_IN_SA))
+ {
+ this->mark_sa_in = this->mark_in;
+ }
+
if (!this->reqid)
{
/* reuse old reqid if we are rekeying an existing CHILD_SA. While the

[Christophe Gouault]

--
v2:
- remove accidental line duplication in starter_stroke_add_conn

---
src/libcharon/plugins/stroke/stroke_config.c | 3 ++-
src/starter/args.c | 1 +
src/starter/confread.h | 1 +
src/starter/keywords.h | 1 +
src/starter/keywords.txt | 1 +
src/starter/starterstroke.c | 1 +
src/stroke/stroke_msg.h | 1 +
7 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/libcharon/plugins/stroke/stroke_config.c b/src/libcharon/plugins/stroke/stroke_config.c
index ac01292104da..0d7cc88b7089 100644
--- a/src/libcharon/plugins/stroke/stroke_config.c
+++ b/src/libcharon/plugins/stroke/stroke_config.c
@@ -1090,7 +1090,8 @@ static child_cfg_t *build_child_cfg(private_stroke_config_t *this,
(msg->add_conn.ipcomp ? OPT_IPCOMP : 0) |
(msg->add_conn.me.hostaccess ? OPT_HOSTACCESS : 0) |
(msg->add_conn.install_policy ? 0 : OPT_NO_POLICIES) |
- (msg->add_conn.sha256_96 ? OPT_SHA256_96 : 0),
+ (msg->add_conn.sha256_96 ? OPT_SHA256_96 : 0) |
+ (msg->add_conn.mark_in_sa ? OPT_MARK_IN_SA : 0),
.tfc = msg->add_conn.tfc,
.inactivity = msg->add_conn.inactivity,
.dpd_action = map_action(msg->add_conn.dpd.action),
diff --git a/src/starter/args.c b/src/starter/args.c
index 477a52082d85..af2b55c65e1b 100644
--- a/src/starter/args.c
+++ b/src/starter/args.c
@@ -178,6 +178,7 @@ static const token_info_t token_info[] =
{ ARG_MISC, 0, NULL /* KW_MARK */ },
{ ARG_MISC, 0, NULL /* KW_MARK_IN */ },
{ ARG_MISC, 0, NULL /* KW_MARK_OUT */ },
+ { ARG_ENUM, offsetof(starter_conn_t, mark_in_sa), LST_bool },
{ ARG_MISC, 0, NULL /* KW_TFC */ },
{ ARG_MISC, 0, NULL /* KW_PFS_DEPRECATED */ },
{ ARG_MISC, 0, NULL /* KW_CONN_DEPRECATED */ },
diff --git a/src/starter/confread.h b/src/starter/confread.h
index 8ee730daa078..3e3f215a64f5 100644
--- a/src/starter/confread.h
+++ b/src/starter/confread.h
@@ -143,6 +143,7 @@ struct starter_conn {
uint32_t reqid;
mark_t mark_in;
mark_t mark_out;
+ bool mark_in_sa;
uint32_t replay_window;
uint32_t tfc;
bool install_policy;
diff --git a/src/starter/keywords.h b/src/starter/keywords.h
index 0cb46a7401c9..85f626c98875 100644
--- a/src/starter/keywords.h
+++ b/src/starter/keywords.h
@@ -77,6 +77,7 @@ enum kw_token_t {
KW_MARK,
KW_MARK_IN,
KW_MARK_OUT,
+ KW_MARK_IN_SA,
KW_TFC,
KW_PFS_DEPRECATED,
KW_CONN_DEPRECATED,
diff --git a/src/starter/keywords.txt b/src/starter/keywords.txt
index 3f92dc83f50f..7699245c1399 100644
--- a/src/starter/keywords.txt
+++ b/src/starter/keywords.txt
@@ -74,6 +74,7 @@ replay_window, KW_REPLAY_WINDOW
mark, KW_MARK
mark_in, KW_MARK_IN
mark_out, KW_MARK_OUT
+mark_in_sa, KW_MARK_IN_SA
tfc, KW_TFC
cacert, KW_CACERT
crluri, KW_CRLURI
diff --git a/src/starter/starterstroke.c b/src/starter/starterstroke.c
index 90af9372ac06..0258732a91ac 100644
--- a/src/starter/starterstroke.c
+++ b/src/starter/starterstroke.c
@@ -231,6 +231,7 @@ int starter_stroke_add_conn(starter_config_t *cfg, starter_conn_t *conn)
msg->add_conn.mark_in.mask = conn->mark_in.mask;
msg->add_conn.mark_out.value = conn->mark_out.value;
msg->add_conn.mark_out.mask = conn->mark_out.mask;
+ msg->add_conn.mark_in_sa = conn->mark_in_sa;
msg->add_conn.tfc = conn->tfc;

add_end(&msg, offsetof(stroke_msg_t, add_conn.me), &conn->left);
diff --git a/src/stroke/stroke_msg.h b/src/stroke/stroke_msg.h
index 60ea0028d8b9..d31a4cfe0765 100644
--- a/src/stroke/stroke_msg.h
+++ b/src/stroke/stroke_msg.h
@@ -300,6 +300,7 @@ struct stroke_msg_t {
uint32_t value;
uint32_t mask;
} mark_in, mark_out;
+ bool mark_in_sa;
stroke_end_t me, other;
uint32_t replay_window;
bool sha256_96;

[Tobias Brunner]

Hi Christophe,

Post by Christophe Gouault
I had a look in the mark-inbound-sa branch, I think there are other
methods where the SA mark must be set: child_sa_t.update,
child_sa_t.destroy.

You're right, thanks! (You missed the one in update_usebytes() btw.)

While I appreciate your creating that stroke patch, I probably won't
apply it. We need to stop adding new features to starter/stroke. Maybe
that will get people to abandon the legacy interface and switch to
swanctl/vici already.

Regards,
Tobias

[Christophe Gouault]

Post by Tobias Brunner
Hi Christophe,

Post by Christophe Gouault
I had a look in the mark-inbound-sa branch, I think there are other
methods where the SA mark must be set: child_sa_t.update,
child_sa_t.destroy.

You're right, thanks!

you're welcome

Post by Tobias Brunner
(You missed the one in update_usebytes() btw.)

Darn! :)

Post by Tobias Brunner
While I appreciate your creating that stroke patch, I probably won't
apply it. We need to stop adding new features to starter/stroke. Maybe
that will get people to abandon the legacy interface and switch to
swanctl/vici already.

I completely understand your will to get rid of the legacy
ipsec.conf/stroke API. However in this specific case, it is not
exactly a new feature. It is the restauration of the former (legacy)
behavior.

Then maybe the behavior when using the legacy API should be the old
behavior: if using stroke/ipsec.conf, the inbound SAs are marked as
they used to be (OPT_MARK_IN_SA). If using the new swanctl API, you
benefit from the new behavior by default (inbound SAs are not marked,
but you may alter this behavior via configuration). This would avoid
to add new features in stroke/ipsec.conf, while not breaking existing
deployments based on stroke.

Christophe

[Tobias Brunner]

Hi Christophe,

Post by Christophe Gouault
This would avoid
to add new features in stroke/ipsec.conf, while not breaking existing
deployments based on stroke.

Except for your special use case it should not have an effect on
existing deployments. With or without VTI devices marking packets
before decryption, as was necessary before, should still work the same
even if the SA has no mark set anymore (unless the SA is marked, the
kernel just ignores the marks on packets, so it doesn't match only
unmarked packets, to do so requires setting a mark of 0/0xffffffff
explicitly).

Therefore, I don't really see the need to change the default or make
this configurable via ipsec.conf.

Actually, for somebody to use a recent enough version to get bothered by
this change a switch to swanclt/vici should be in order anyway. And
since the behavior was changed with 5.5.2, which also brought
swanctl/vici basically on par with starter/stroke (see the changelog
[1]), there should really be no reason to prefer the old interface.

There are some control features, like `down-srcip`, `purgeike` or
`list|resetcounters`, that are not implemented (yet). But they may
still be used, if necessary (ideally users would notify us of features
they still need), when loading the stroke plugin even if the
configuration is done via swanctl/vici (and some can even be replicated
via vici).

Regards,
Tobias

[1] https://wiki.strongswan.org/versions/64

[Christophe Gouault]

Post by Tobias Brunner
Hi Christophe,

Post by Christophe Gouault
This would avoid
to add new features in stroke/ipsec.conf, while not breaking existing
deployments based on stroke.

Except for your special use case it should not have an effect on
existing deployments. With or without VTI devices marking packets
before decryption, as was necessary before, should still work the same
even if the SA has no mark set anymore (unless the SA is marked, the
kernel just ignores the marks on packets, so it doesn't match only
unmarked packets, to do so requires setting a mark of 0/0xffffffff
explicitly).
Therefore, I don't really see the need to change the default or make
this configurable via ipsec.conf.
Actually, for somebody to use a recent enough version to get bothered by
this change a switch to swanclt/vici should be in order anyway. And
since the behavior was changed with 5.5.2, which also brought
swanctl/vici basically on par with starter/stroke (see the changelog
[1]), there should really be no reason to prefer the old interface.

Hi Tobias,

OK, I surrender ;-)

Post by Tobias Brunner
There are some control features, like `down-srcip`, `purgeike` or
`list|resetcounters`, that are not implemented (yet). But they may
still be used, if necessary (ideally users would notify us of features
they still need),

I started using the vici API for monitoring and stats, and I must
admit it is far better suited than stroke to interface with an
external application.

I precisely missed the equivalent for `listcounters` in the vici
interface, so I take the opportunity to notify you officially that its
support would be appreciated :)

Regards,
Christophe

[Tobias Brunner]

Hi Christophe,

Post by Christophe Gouault

Post by Tobias Brunner
There are some control features, like `down-srcip`, `purgeike` or
`list|resetcounters`, that are not implemented (yet). But they may
still be used, if necessary (ideally users would notify us of features
they still need),

I started using the vici API for monitoring and stats, and I must
admit it is far better suited than stroke to interface with an
external application.
I precisely missed the equivalent for `listcounters` in the vici
interface, so I take the opportunity to notify you officially that its
support would be appreciated :)

Noted...and actually already implemented in the vici-counters branch :)

Since it's based on the same code, it has the same behavior and
limitations. For instance, counters are associated with an IKE_SA's
name not individual IKE_SAs (e.g. via their unique IDs). So they may
cover several IKE_SAs with the same name and they are not automatically
reset or removed once SAs are terminated. And since the name may change
on responders due to the identities or authentication settings the
connection-specific counters might not be exactly accurate (e.g. if the
first defined connection is configured for pubkey client authentication
but most clients will connect via EAP using an otherwise identical
second connection, too many inbound IKE_AUTHs will recorded for the
first connection's name and too few for the second). Let me know if you
see anything that could be improved (e.g. the naming of the counters).

Regards,
Tobias

[Christophe Gouault]

Post by Tobias Brunner
Hi Christophe,

Post by Christophe Gouault
I precisely missed the equivalent for `listcounters` in the vici
interface, so I take the opportunity to notify you officially that its
support would be appreciated :)

Noted...and actually already implemented in the vici-counters branch :)
[...]
Let me know if you
see anything that could be improved (e.g. the naming of the counters).

Hi Tobias,

Thank you for working on this porting so fast and for pointing its limitations.

The names seem good to me, they remind of the old camel case ones, in
a more vici fashion.

Regards,
Christophe