[M] Change in openvpn[master]: oob: Add client PROBE_REPLY parser

Attention is currently required from: flichtenheld, plaisthos.

Hello flichtenheld, plaisthos, 

I'd like you to reexamine a change. Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1745?usp=email

to look at the new patch set (#12).


Change subject: oob: Add client PROBE_REPLY parser
......................................................................

oob: Add client PROBE_REPLY parser

Add oob_client_reply_read(), the client-side counterpart of
oob_server_probe_read(): it scans the TLV payload of a received PROBE_REPLY
for the probe_reply TLV, skipping other/future TLV types that are marked
optional. Both readers use
the shared ctrl_msg_find_tlv() from control_msg.h for the scan.

Exercised by unit tests; the client probe path calls it in a follow-up.

Change-Id: If04ce09d4c353f5384c0c48f48f63e05869a373f
Signed-off-by: Lev Stipakov <[email protected]>
---
M src/openvpn/oob.c
M src/openvpn/oob.h
M tests/unit_tests/openvpn/test_oob.c
3 files changed, 138 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/45/1745/12

diff --git a/src/openvpn/oob.c b/src/openvpn/oob.c
index c841d59..bc0e7a3 100644
--- a/src/openvpn/oob.c
+++ b/src/openvpn/oob.c
@@ -112,6 +112,23 @@
 }
 
 bool
+oob_client_reply_read(struct buffer *payload, struct oob_probe_reply *reply)
+{
+    if (!ctrl_msg_read_header(payload, OOB_MSG_PROBE_REPLY))
+    {
+        return false;
+    }
+
+    struct buffer value;
+    if (!ctrl_msg_find_tlv(payload, OOB_TLV_PROBE_REPLY, &value))
+    {
+        return false;
+    }
+
+    return oob_probe_reply_read(&value, reply);
+}
+
+bool
 oob_timestamp_in_window(uint64_t probe_ts, uint64_t now, uint64_t window_secs)
 {
     uint64_t diff = (now > probe_ts) ? (now - probe_ts) : (probe_ts - now);
diff --git a/src/openvpn/oob.h b/src/openvpn/oob.h
index 49d64e8..80775a4 100644
--- a/src/openvpn/oob.h
+++ b/src/openvpn/oob.h
@@ -132,6 +132,19 @@
 bool oob_client_reply_write(struct buffer *buf, const struct oob_probe_reply *reply);
 
 /**
+ * Read a received OOB PROBE_REPLY: verify its message-type header, then scan
+ * for the probe_reply TLV; the client-side counterpart of
+ * oob_server_probe_read(). TLV types other than probe_reply are skipped.
+ * payload is consumed as it is read.
+ *
+ * @param payload  buffer positioned at the start of the OOB message payload
+ * @param reply    filled with the parsed probe_reply on success
+ * @return true if the header matched and a well-formed probe_reply was found,
+ *         false otherwise
+ */
+bool oob_client_reply_read(struct buffer *payload, struct oob_probe_reply *reply);
+
+/**
  * Check whether a probe timestamp is within an acceptable window around the
  * current time. Used to cheaply drop replayed or implausibly-timed probes
  * before doing any further work (see the probe_parameter timestamp rationale
diff --git a/tests/unit_tests/openvpn/test_oob.c b/tests/unit_tests/openvpn/test_oob.c
index 3b29b52..183e107 100644
--- a/tests/unit_tests/openvpn/test_oob.c
+++ b/tests/unit_tests/openvpn/test_oob.c
@@ -463,6 +463,109 @@
     gc_free(&gc);
 }
 
+/* A PROBE_REPLY carrying a probe_reply is found by the client scan, with all
+ * fields surviving. */
+static void
+test_client_reply_read_finds_reply(void **state)
+{
+    struct gc_arena gc = gc_new();
+    struct buffer buf = alloc_buf_gc(128, &gc);
+
+    struct oob_probe_reply in = {
+        .priority = 5,
+        .weight = 50,
+        .connect_lifetime = 120,
+        .flags = 1,
+    };
+    memcpy(in.peer_session_id.id, "SRVREPLY", SID_SIZE);
+    assert_true(oob_client_reply_write(&buf, &in));
+
+    struct oob_probe_reply out = { 0 };
+    assert_true(oob_client_reply_read(&buf, &out));
+    assert_memory_equal(out.peer_session_id.id, in.peer_session_id.id, SID_SIZE);
+    assert_int_equal(out.priority, in.priority);
+    assert_int_equal(out.weight, in.weight);
+    assert_int_equal(out.connect_lifetime, in.connect_lifetime);
+    assert_int_equal(out.flags, in.flags);
+
+    gc_free(&gc);
+}
+
+/* TLVs other than probe_reply are skipped. */
+static void
+test_client_reply_read_skips_unknown(void **state)
+{
+    struct gc_arena gc = gc_new();
+    struct buffer buf = alloc_buf_gc(128, &gc);
+
+    assert_true(buf_write_u16(&buf, OOB_MSG_PROBE_REPLY));
+    assert_true(ctrl_msg_tlv_write_header(&buf, 0x7ff, true, 4));
+    assert_true(buf_write_u32(&buf, 0xabad1dea));
+    struct oob_probe_reply in = { .priority = 7 };
+    assert_true(oob_probe_reply_write(&buf, &in));
+
+    struct oob_probe_reply out = { 0 };
+    assert_true(oob_client_reply_read(&buf, &out));
+    assert_int_equal(out.priority, 7);
+
+    gc_free(&gc);
+}
+
+/* As on the probe side, a mandatory TLV we do not understand -- here trailing
+ * the probe_reply -- invalidates the reply. */
+static void
+test_client_reply_read_rejects_unknown_mandatory(void **state)
+{
+    struct gc_arena gc = gc_new();
+    struct buffer buf = alloc_buf_gc(128, &gc);
+
+    assert_true(buf_write_u16(&buf, OOB_MSG_PROBE_REPLY));
+    struct oob_probe_reply in = { .priority = 7 };
+    assert_true(oob_probe_reply_write(&buf, &in));
+    assert_true(ctrl_msg_tlv_write_header(&buf, 0x7ff, false, 4));
+    assert_true(buf_write_u32(&buf, 0xabad1dea));
+
+    struct oob_probe_reply out = { 0 };
+    assert_false(oob_client_reply_read(&buf, &out));
+
+    gc_free(&gc);
+}
+
+/* A payload with no probe_reply is rejected. */
+static void
+test_client_reply_read_missing(void **state)
+{
+    struct gc_arena gc = gc_new();
+    struct buffer buf = alloc_buf_gc(128, &gc);
+
+    assert_true(buf_write_u16(&buf, OOB_MSG_PROBE_REPLY));
+    assert_true(ctrl_msg_tlv_write_header(&buf, 0x7ff, true, 4));
+    assert_true(buf_write_u32(&buf, 0));
+
+    struct oob_probe_reply out = { 0 };
+    assert_false(oob_client_reply_read(&buf, &out));
+
+    gc_free(&gc);
+}
+
+/* Likewise, a PROBE_REPLY reader rejects a payload with the wrong message
+ * type even when a valid probe_reply TLV follows. */
+static void
+test_client_reply_read_wrong_msg_type(void **state)
+{
+    struct gc_arena gc = gc_new();
+    struct buffer buf = alloc_buf_gc(128, &gc);
+
+    assert_true(buf_write_u16(&buf, OOB_MSG_SERVER_PROBE));
+    struct oob_probe_reply in = { .priority = 7 };
+    assert_true(oob_probe_reply_write(&buf, &in));
+
+    struct oob_probe_reply out = { 0 };
+    assert_false(oob_client_reply_read(&buf, &out));
+
+    gc_free(&gc);
+}
+
 int
 main(void)
 {
@@ -485,6 +588,11 @@
         cmocka_unit_test(test_build_probe_reply_valid),
         cmocka_unit_test(test_build_probe_reply_stale),
         cmocka_unit_test(test_build_probe_reply_no_parameter),
+        cmocka_unit_test(test_client_reply_read_finds_reply),
+        cmocka_unit_test(test_client_reply_read_skips_unknown),
+        cmocka_unit_test(test_client_reply_read_rejects_unknown_mandatory),
+        cmocka_unit_test(test_client_reply_read_missing),
+        cmocka_unit_test(test_client_reply_read_wrong_msg_type),
     };
 
     return cmocka_run_group_tests_name("oob tests", tests, NULL, NULL);

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1745?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: If04ce09d4c353f5384c0c48f48f63e05869a373f
Gerrit-Change-Number: 1745
Gerrit-PatchSet: 12
Gerrit-Owner: stipa <[email protected]>
Gerrit-Reviewer: flichtenheld <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
Gerrit-Attention: flichtenheld <[email protected]>

_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel