Send connman mailing list submissions to
[email protected]
To subscribe or unsubscribe via the World Wide Web, visit
https://lists.01.org/mailman/listinfo/connman
or, via email, send a message with subject or body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of connman digest..."
Today's Topics:
1. Re: [PATCH 4/5] session: add source ip rule (Lukasz Nowak)
----------------------------------------------------------------------
Message: 1
Date: Wed, 14 Dec 2016 17:56:40 +0000
From: Lukasz Nowak <[email protected]>
To: Daniel Wagner <[email protected]>, [email protected]
Subject: Re: [PATCH 4/5] session: add source ip rule
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252
On 14/12/16 14:26, Daniel Wagner wrote:
>
>
> On 12/12/2016 07:35 PM, [email protected] wrote:
>> From: Lukasz Nowak <[email protected]>
>>
>> Implement an option for a session to enable packet filtering
>> based on interfce source ip address. This allows an application
>
> type: interface
>
>> to create a session, and direct traffic to a specific network
>> interface, on which the session is connected.
>>
>> Applications can use bind before connect on a socket to specify
>> the source ip address.
>>
>> This mechanism re-uses the routing table created by the session,
>> iproute fwmark rule, and adds a new iptables source ip rule.
>> ---
>> include/session.h | 1 +
>> src/connman.h | 3 ++-
>> src/firewall-iptables.c | 12 ++++++++-
>> src/firewall-nftables.c | 5 ++--
>> src/session.c | 65
>> ++++++++++++++++++++++++++++++++++++++++++++++---
>> 5 files changed, 78 insertions(+), 8 deletions(-)
>
> I think you need to split up this patch a bit. There are too many things
> going on in one go. At least you should try to split out the firewall-*
> changes.
>
>> diff --git a/include/session.h b/include/session.h
>> index e8d7e93..48f1510 100644
>> --- a/include/session.h
>> +++ b/include/session.h
>> @@ -66,6 +66,7 @@ struct connman_session_config {
>> bool ecall;
>> GSList *allowed_bearers;
>> char *allowed_interface;
>> + bool source_ip_rule;
>
> Could you write a small document for doc/ which explains how everything is
> supposed to work. For example I am not sure why you want this additional
> boolean. It isn't clear to me when you define allowed_interface (from patch
> #2) that you need to enable it via this bool?
I will write a doc and update man pages.
I decided to have the AllowedInterface and SrcIpRule as independent config
options. You can have either one of them enabled without the other.
We could group them into one (e.g. setting AllowedInterface automatically
enables creation of the source ip rule in iptables). Do you have a preference?
>
>> };
>>
>> typedef int (* connman_session_config_func_t) (struct connman_session
>> *session,
>> diff --git a/src/connman.h b/src/connman.h
>> index 577c808..51c3c03 100644
>> --- a/src/connman.h
>> +++ b/src/connman.h
>> @@ -1013,7 +1013,8 @@ int __connman_firewall_enable_snat(struct
>> firewall_context *ctx,
>> int __connman_firewall_disable_snat(struct firewall_context *ctx);
>> int __connman_firewall_enable_marking(struct firewall_context *ctx,
>> enum connman_session_id_type id_type,
>> - char *id, uint32_t mark);
>> + char *id, const char *src_ip_address,
>
> src_ip_address is a bit long, src_ip should be enough.
>
>> + uint32_t mark);
>> int __connman_firewall_disable_marking(struct firewall_context *ctx);
>>
>> int __connman_firewall_init(void);
>> diff --git a/src/firewall-iptables.c b/src/firewall-iptables.c
>> index c58efc1..e7a966d 100644
>> --- a/src/firewall-iptables.c
>> +++ b/src/firewall-iptables.c
>> @@ -495,7 +495,8 @@ static void firewall_disable_connmark(void)
>>
>> int __connman_firewall_enable_marking(struct firewall_context *ctx,
>> enum connman_session_id_type id_type,
>> - char *id, uint32_t mark)
>> + char *id, const char *src_ip_address,
>> + uint32_t mark)
>> {
>> int err;
>>
>> @@ -514,11 +515,20 @@ int __connman_firewall_enable_marking(struct
>> firewall_context *ctx,
>> "-m owner --gid-owner %s -j MARK --set-mark %d",
>> id, mark);
>> break;
>> + case CONNMAN_SESSION_ID_TYPE_UNKNOWN:
>> + break;
>> case CONNMAN_SESSION_ID_TYPE_LSM:
>> default:
>> return -EINVAL;
>> }
>>
>> + if (src_ip_address)
>> + {
>
> The braket goes on the line above.
>
>> + firewall_add_rule(ctx, "mangle", "OUTPUT",
>> + "-s %s -j MARK --set-mark %d",
>> + src_ip_address, mark);
>> + }
>> +
>> return firewall_enable_rules(ctx);
>> }
>>
>> diff --git a/src/firewall-nftables.c b/src/firewall-nftables.c
>> index 4d47f20..dc4c934 100644
>> --- a/src/firewall-nftables.c
>> +++ b/src/firewall-nftables.c
>> @@ -800,7 +800,8 @@ err:
>>
>> int __connman_firewall_enable_marking(struct firewall_context *ctx,
>> enum connman_session_id_type id_type,
>> - char *id, uint32_t mark)
>> + char *id, const char *src_ip_address,
>> + uint32_t mark)
>> {
>> struct nftnl_rule *rule;
>> struct mnl_socket *nl;
>> @@ -811,7 +812,7 @@ int __connman_firewall_enable_marking(struct
>> firewall_context *ctx,
>> DBG("");
>>
>>
>> - if (id_type != CONNMAN_SESSION_ID_TYPE_UID)
>> + if (id_type != CONNMAN_SESSION_ID_TYPE_UID || src_ip_address)
>> return -ENOTSUP;
>
> Obviously, you need to add this nftables implementation as well.
I can commit to that, although I would need to set up nftables in our system,
and learn how to use it.
Realistically it would be mid-January before I can provide that implementation.
Would it be acceptable to provide it as a separate patch in a few weeks time?
>
>>
>> pw = getpwnam(id);
>> diff --git a/src/session.c b/src/session.c
>> index 2dfd428..28a4f77 100644
>> --- a/src/session.c
>> +++ b/src/session.c
>> @@ -273,8 +273,11 @@ static int init_firewall_session(struct connman_session
>> *session)
>> {
>> struct firewall_context *fw;
>> int err;
>> + struct connman_ipconfig *ipconfig;
>> + const char *addr = NULL;
>>
>> - if (session->policy_config->id_type == CONNMAN_SESSION_ID_TYPE_UNKNOWN)
>> + if (session->policy_config->id_type == CONNMAN_SESSION_ID_TYPE_UNKNOWN
>> &&
>> + !session->info->config.source_ip_rule)
>> return 0;
>
> So you don't need to identify your application? The test is here because the
> firewall didn't make to start if there is no rule to install. I was tempted
> to suggest to define a new CONNMAN_SESSION_ID_TYPE for this use case but I
> think it would be good to stay as generic as possible and still allow
> application identification and source routing.
I have added the source ip rule independent of the policy. Technically it could
go straight into iproute rules, but I thought that re-using the fwmark and
firewall would make the rules easier to see from outside of ConnMan. But this
choice meant that the firewall could become enabled without any policy being
active.
Would you do it in some other way?
>
> At least I would say you should add a small helper for test which tells us
> what is tested.
>
> static bool firewall_needed() {}
>
> or a something with a better name.
>
>
> BTW, I think now I get why you want to expose the interface name via D-Bus.
> The application use this information to bind to the interface, no?
Yes, the application binds each session to one of the required interfaces via
the D-Bus config.
>
>
>>
>> DBG("");
>> @@ -283,10 +286,15 @@ static int init_firewall_session(struct
>> connman_session *session)
>> if (!fw)
>> return -ENOMEM;
>>
>> + ipconfig = __connman_service_get_ip4config(session->service);
>> + if (session->info->config.source_ip_rule && ipconfig) {
>> + addr = __connman_ipconfig_get_local(ipconfig);
>> + }
>> +
>> err =__connman_firewall_enable_marking(fw,
>> session->policy_config->id_type,
>> session->policy_config->id,
>> - session->mark);
>> + addr, session->mark);
>> if (err < 0) {
>> __connman_firewall_destroy(fw);
>> return err;
>> @@ -313,7 +321,8 @@ static int init_routing_table(struct connman_session
>> *session)
>> {
>> int err;
>>
>> - if (session->policy_config->id_type == CONNMAN_SESSION_ID_TYPE_UNKNOWN)
>> + if (session->policy_config->id_type == CONNMAN_SESSION_ID_TYPE_UNKNOWN
>> &&
>> + !session->info->config.source_ip_rule)
>> return 0;
>>
>> DBG("");
>> @@ -520,6 +529,7 @@ struct creation_data {
>> enum connman_session_type type;
>> GSList *allowed_bearers;
>> char *allowed_interface;
>> + bool source_ip_rule
>> };
>>
>> static void cleanup_creation_data(struct creation_data *creation_data)
>> @@ -897,6 +907,17 @@ static void append_notify(DBusMessageIter *dict,
>> info_last->config.allowed_interface =
>> info->config.allowed_interface;
>> }
>>
>> + if (session->append_all ||
>> + info->config.source_ip_rule !=
>> info_last->config.source_ip_rule) {
>> + dbus_bool_t source_ip_rule = FALSE;
>> + if (info->config.source_ip_rule)
>> + source_ip_rule = TRUE;
>> + connman_dbus_dict_append_basic(dict, "SourceIPRule",
>> + DBUS_TYPE_BOOLEAN,
>> + &source_ip_rule);
>> + info_last->config.source_ip_rule = info->config.source_ip_rule;
>> + }
>> +
>> session->append_all = false;
>> }
>>
>> @@ -917,7 +938,8 @@ static bool compute_notifiable_changes(struct
>> connman_session *session)
>>
>> if (info->config.allowed_bearers != info_last->config.allowed_bearers ||
>> info->config.type != info_last->config.type ||
>> - info->config.allowed_interface !=
>> info_last->config.allowed_interface)
>> + info->config.allowed_interface !=
>> info_last->config.allowed_interface ||
>> + info->config.source_ip_rule != info_last->config.source_ip_rule)
>> return true;
>>
>> return false;
>> @@ -1165,6 +1187,27 @@ static DBusMessage *change_session(DBusConnection
>> *conn,
>> goto err;
>> }
>> break;
>> + case DBUS_TYPE_BOOLEAN:
>> + if (g_str_equal(name, "SourceIPRule")) {
>> + dbus_bool_t source_ip_rule;
>> + dbus_message_iter_get_basic(&value, &source_ip_rule);
>> +
>> + cleanup_firewall_session(session);
>> + cleanup_routing_table(session);
>> +
>> + info->config.source_ip_rule = source_ip_rule;
>> + update_session_state(session);
>> +
>> + init_routing_table(session);
>
> check the return type
>
> I don't think you handle on-off-on toggling for routing initializtion and the
> firewall session setup.
I do (if I understand the scenario you are asking about, correctly).
On every change of this boolean, firewall will first be cleaned up by
cleanup_firewall_session(session), but then it will only be re-created if
SourceIPRule is true. Otherwise init_routing_table() returns without doing
anything (unless there is a policy in operation, but then it will not create
the source ip firewall rule).
Do you have any specific combinations of calls/events which you think are not
handled correctly?
>
> Thanks,
> Daniel
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 14, Issue 22
***************************************
