Re: [PATCH] tcp: md5: Fix overlap between vrf and non-vrf keys

[David Ahern <dsahern@gmail.com>]

On 10/8/21 9:51 AM, Leonard Crestez wrote:
> On 07.10.2021 21:27, David Ahern wrote:
>> On 10/7/21 12:41 AM, Leonard Crestez wrote:
>>> On 07.10.2021 04:14, David Ahern wrote:
>>>> On 10/6/21 11:48 AM, Leonard Crestez wrote:
>>>>> @@ -1103,11 +1116,11 @@ static struct tcp_md5sig_key
>>>>> *tcp_md5_do_lookup_exact(const struct sock *sk,
>>>>>    #endif
>>>>>        hlist_for_each_entry_rcu(key, &md5sig->head, node,
>>>>>                     lockdep_sock_is_held(sk)) {
>>>>>            if (key->family != family)
>>>>>                continue;
>>>>> -        if (key->l3index && key->l3index != l3index)
>>>>> +        if (key->l3index != l3index)
>>>>
>>>> That seems like the bug fix there. The L3 reference needs to match for
>>>> new key and existing key. I think the same change is needed in
>>>> __tcp_md5_do_lookup.
>>>
>>> Current behavior is that keys added without tcpm_ifindex will match
>>> connections both inside and outside VRFs. Changing this might break real
>>> applications, is it really OK to claim that this behavior was a bug all
>>> along?
>>
>> no.
>>
>> It's been a few years. I need to refresh on the logic and that is not
>> going to happen before this weekend.
> 
> It seems that always doing a strict key->l3index != l3index condition
> inside of __tcp_md5_do_lookup breaks the usecase of binding one listener
> to each VRF and not specifying the ifindex for each key.
> 
> This is a very valid usecase, maybe the most common way to use md5 with
> vrf.
> 
> Ways to fix this:
> * Make this comparison only take effect if TCP_MD5SIG_FLAG_IFINDEX is set.
> * Make this comparison only take effect if tcp_l3mdev_accept=1
> * Add a new flag?
> 
> Right now passing TCP_MD5SIG_FLAG_IFINDEX and ifindex == 0 results in an
> error but maybe it should be accepted to mean "key applies only for
> default VRF".
> 

I think I remember the history now: prior to the set
98c8147648fa..5cad8bce26e0 MD5 lookups for VRF and default VRF both
succeed on an address or prefix match because the L3 domain was not
checked. That set did not want to break the legacy behavior which is why
the change is based on db key having l3index set and matching the
ingress domain.

That means the limitation (hole depending on perspective) is a default
VRF and a VRF having overlap addresses with a key installed with the L3
index set (can't since default VRF does not have one) - which I believe
is your point.

So, yes, one option is to have a flag that indicates strict checking to
close the legacy path.