lkml.org 
[lkml]   [2022]   [Aug]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 08/31] net/tcp: Introduce TCP_AO setsockopt()s
    On 8/23/22 15:45, Leonard Crestez wrote:
    > On 8/18/22 19:59, Dmitry Safonov wrote:
    [..]
    >> +#define TCP_AO 38 /* (Add/Set MKT) */
    >> +#define TCP_AO_DEL 39 /* (Delete MKT) */
    >> +#define TCP_AO_MOD 40 /* (Modify MKT) */
    >
    > The TCP_AO_MOD sockopt doesn't actually modify and MKT, it only controls
    > per-socket properties. It is equivalent to my TCP_AUTHOPT sockopt while
    > TCP_AO is equivalent to TCP_AUTHOPT_KEY. My equivalent of TCP_AO_DEL
    > sockopt is a flag inside tcp_authopt_key.

    Fair point, the comment could be "Modify AO", rather than "Modify MKT".
    On the other side, this can later support more per-key changes than in
    the initial proposal: i.e., zero per-key counters. Password and rcv/snd
    ids can't change to follow RFC text, but non-essentials may.
    So, the comment to the command here is not really incorrect.

    >> +struct tcp_ao { /* setsockopt(TCP_AO) */
    >> + struct __kernel_sockaddr_storage tcpa_addr;
    >> + char tcpa_alg_name[64];
    >> + __u16 tcpa_flags;
    >
    > This field accept TCP_AO_CMDF_CURR and TCP_AO_CMDF_NEXT which means that
    > you are combining key addition with key selection. Not clear it
    > shouldn't just always be a separate sockopt?

    I don't see any downside. A user can add a key and start using it immediately
    with one syscall instead of two. It's not necessary, one can do it in
    2 setsockopt()s if they want.

    [..]
    > I also have two fields called "recv_keyid" and "recv_rnextkeyid" which
    > inform userspace about what the remote is sending, I'm not seeing an
    > equivalent on your side.

    Sounds like a good candidate for getsockopt() for logs/debugging.

    > The specification around send_keyid in the RFC is conflicting:
    > * User must be able to control it

    I don't see where you read it, care to point it out?
    I see choosing the current_key by marking the preferred key during
    an establishment of a connection, but I don't see any "MUST control
    current_key". We allow changing current_key, but that's actually
    not something required by RFC, the only thing required is to respect
    rnext_key that's asked by peer.

    > * Implementation must respect rnextkeyid in incoming packet
    >
    > I solved this apparent conflict by adding a
    > "TCP_AUTHOPT_FLAG_LOCK_KEYID" flag so that user can choose if it wants
    > to control the sending key or let it be controlled from the other side.

    That's exactly violating the above "Implementation must respect
    rnextkeyid in incoming packet". See RFC5925 (7.5.2.e).

    [..]
    > Only two algorithms are defined in RFC5926 and you have to treat one of
    > them as a special case. I remain convinced that generic support for
    > arbitrary algorithms is undesirable; it's better for the algorithm to be
    > specified as an enum.

    On contrary, I see that as a really big feature. RFC5926 was published in 2010,
    when sha1 was yet hard to break. These days sha1 is considered insecure.
    I.e., the first link from Google:

    > Starting with version 56, released this month, Google Chrome will mark all
    > SHA-1-signed HTTPS certificates as unsafe. Other major browser vendors
    > plan to do the same.
    > "Hopefully these new efforts of Google of making a real-world attack possible
    > will lead to vendors and infrastructure managers quickly removing SHA-1 from
    > their products and configurations as, despite it being a deprecated algorithm,
    > some vendors still sell products that do not support more modern hashing
    > algorithms or charge an extra cost to do so," [..]

    So, why limit a new TCP sign feature to already insecure algorithms?
    One can already use any crypto algorithms for example, in tunnels.
    And I don't see any benefit in defining new magic macros, only downside.

    I prefer UAPI that takes crypto algo name as a string, rather than new
    defined magic number from one of kernel headers.
    IOW,
    : strcpy(ao.tcpa_alg_name, "cmac(aes128)");
    : setsockopt(sk, IPPROTO_TCP, opt, &ao, sizeof(ao));
    is better than
    : ao.tcp_alg = TCP_AO_CMAC_MAGIC_DEFINE;
    : setsockopt(sk, IPPROTO_TCP, opt, &ao, sizeof(ao));

    Neither I see a point in more patches adding new
    #define TCP_AO_NEW_ALGO

    BTW, I had some patches to add testing in fcnal-test.sh and covered
    the following algorithms, that worked just fine (test changes not
    included in v1):
    hmac(sha1) cmac(aes128) hmac(rmd128) hmac(rmd160) hmac(sha512)
    hmac(sha384) hmac(sha256) hmac(md5) hmac(sha224) hmac(sha3-512)

    No point in artificially disabling them or introducing new magic #defines.

    Thanks,
    Dmitry

    \
     
     \ /
      Last update: 2022-08-31 20:48    [W:4.325 / U:0.116 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site