Libguestfs - Sep 2019 - Re: [PATCH libnbd 2/2] api: New API for reading NBD protocol.

When LIBNBD_TLS_ALLOW is used we don't have a way to find out if TLS
was really negotiated.  This adds a flag and a way to read it back.

Unfortunately there is no test yet, because LIBNBD_TLS_ALLOW is not
tested -- it really should be but requires quite a complicated set of
tests because ideally we'd like to find out whether it falls back
correctly for all supported servers.
---
 TODO                                     |  3 +++
 generator/generator                      | 34 +++++++++++++++++++++---
 generator/states-newstyle-opt-starttls.c |  1 +
 lib/crypto.c                             |  6 +++++
 lib/internal.h                           |  3 +++
 5 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/TODO b/TODO
index 642d39f..21feb2f 100644
--- a/TODO
+++ b/TODO
@@ -17,6 +17,9 @@ NBD_INFO_BLOCK_SIZE.
 
 TLS should properly shut down the session (calling gnutls_bye).
 
+LIBNBD_TLS_ALLOW is not tested.  Related to this,
+nbd_get_tls_negotiated is not tested.
+
 Implement nbd_connect + systemd socket activation.
 
 Improve function trace output so that:
diff --git a/generator/generator b/generator/generator
index 87a8cdf..28248ed 100755
--- a/generator/generator
+++ b/generator/generator
@@ -1132,17 +1132,42 @@ TLS are not handled automatically.  Setting the level
higher than
 zero will fail if libnbd was not compiled against gnutls; you can
 test whether this is the case with L<nbd_supports_tls(3)>.";
     example = Some "examples/encryption.c";
-    see_also = ["L<libnbd(3)/ENCRYPTION AND AUTHENTICATION>"];
+    see_also = ["L<libnbd(3)/ENCRYPTION AND AUTHENTICATION>";
+                "L<nbd_get_tls(3)>";
"L<nbd_get_tls_negotiated(3)>"];
   };
 
   "get_tls", {
     default_call with
     args = []; ret = RInt;
     may_set_error = false;
-    shortdesc = "get the current TLS setting";
+    shortdesc = "get the TLS request setting";
     longdesc = "\
-Get the current TLS setting.";
-    see_also = ["L<nbd_set_tls(3)>"];
+Get the TLS request setting.
+
+B<Note:> If you want to find out if TLS was actually negotiated
+on a particular connection use L<nbd_get_tls_negotiated(3)>
instead.";
+    see_also = ["L<nbd_set_tls(3)>";
"L<nbd_get_tls_negotiated(3)>"];
+  };
+
+  "get_tls_negotiated", {
+    default_call with
+    args = []; ret = RBool;
+    permitted_states = [ Connected; Closed ];
+    shortdesc = "find out if TLS was negotiated on a connection";
+    longdesc = "\
+After connecting you may call this to find out if the
+connection is using TLS.
+
+This is only really useful if you set the TLS request mode
+to C<LIBNBD_TLS_ALLOW> (see L<nbd_set_tls(3)>), because in this
+mode we try to use TLS but fall back to unencrypted if it was
+not available.  This function will tell you if TLS was
+negotiated or not.
+
+In C<LIBNBD_TLS_REQUIRE> mode (the most secure) the connection
+would have failed if TLS could not be negotiated, and in
+C<LIBNBD_TLS_DISABLE> mode TLS is not tried.";
+    see_also = ["L<nbd_set_tls(3)>";
"L<nbd_get_tls(3)>"];
   };
 
   "set_tls_certificates", {
@@ -2527,6 +2552,7 @@ let first_version = [
   "can_fast_zero", (1, 2);
   "set_request_structured_replies", (1, 2);
   "get_request_structured_replies", (1, 2);
+  "get_tls_negotiated", (1, 2);
 
   (* These calls are proposed for a future version of libnbd, but
    * have not been added to any released version so far.
diff --git a/generator/states-newstyle-opt-starttls.c
b/generator/states-newstyle-opt-starttls.c
index 0a18db0..a35e10b 100644
--- a/generator/states-newstyle-opt-starttls.c
+++ b/generator/states-newstyle-opt-starttls.c
@@ -116,6 +116,7 @@
   }
   if (r == 0) {
     /* Finished handshake. */
+    h->tls_negotiated = true;
     nbd_internal_crypto_debug_tls_enabled (h);
 
     /* Continue with option negotiation. */
diff --git a/lib/crypto.c b/lib/crypto.c
index c0a57d7..3274954 100644
--- a/lib/crypto.c
+++ b/lib/crypto.c
@@ -57,6 +57,12 @@ nbd_unlocked_get_tls (struct nbd_handle *h)
   return h->tls;
 }
 
+int
+nbd_unlocked_get_tls_negotiated (struct nbd_handle *h)
+{
+  return h->tls_negotiated;
+}
+
 int
 nbd_unlocked_set_tls_certificates (struct nbd_handle *h, const char *dir)
 {
diff --git a/lib/internal.h b/lib/internal.h
index ccaca32..eb76ac1 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -87,6 +87,9 @@ struct nbd_handle {
   uint64_t exportsize;
   uint16_t eflags;
 
+  /* Flag set by the state machine to tell whether TLS was negotiated. */
+  bool tls_negotiated;
+
   int64_t unique;               /* Used for generating cookie numbers. */
 
   /* For debugging. */
-- 
2.23.0

This commit adds a new API which can be used from the connected to
state to read back which NBD protocol (eg. oldstyle, newstyle-fixed)
we are using.

It was helpful to add a new state in newstyle negotiation
(%NEWSTYLE.FINISHED) so we can route all successful option
negotiations through a single path before moving to the %READY state,
allowing us to set h->protocol in one place.
---
 generator/generator                         | 25 +++++++++++++++++++++
 generator/states-newstyle-opt-export-name.c |  2 +-
 generator/states-newstyle-opt-go.c          |  2 +-
 generator/states-newstyle.c                 |  9 ++++++++
 generator/states-oldstyle.c                 |  2 ++
 lib/handle.c                                | 12 ++++++++++
 lib/internal.h                              |  5 ++++-
 tests/get-size.c                            | 13 ++++++++++-
 tests/oldstyle.c                            |  9 ++++++++
 9 files changed, 75 insertions(+), 4 deletions(-)

diff --git a/generator/generator b/generator/generator
index 28248ed..21aae3b 100755
--- a/generator/generator
+++ b/generator/generator
@@ -357,6 +357,16 @@ and newstyle_state_machine = [
   Group ("OPT_SET_META_CONTEXT",
newstyle_opt_set_meta_context_state_machine);
   Group ("OPT_GO", newstyle_opt_go_state_machine);
   Group ("OPT_EXPORT_NAME", newstyle_opt_export_name_state_machine);
+
+  (* When option parsing has successfully finished negotiation
+   * it will jump to this state for final steps before moving to
+   * the %READY state.
+   *)
+  State {
+    default_state with
+    name = "FINISHED";
+    comment = "Finish off newstyle negotiation";
+  };
 ]
 
 (* Fixed newstyle NBD_OPT_STARTTLS option. *)
@@ -1574,6 +1584,20 @@ are free to pass in other contexts."
                 "L<nbd_block_status(3)>";
"L<nbd_aio_block_status(3)>"];
   };
 
+  "get_protocol", {
+    default_call with
+    args = []; ret = RStaticString;
+    permitted_states = [ Connected; Closed ];
+    shortdesc = "return the NBD protocol variant";
+    longdesc = "\
+Return the NBD protocol variant in use on the connection.  At
+the moment this returns one of the strings C<\"oldstyle\">,
+C<\"newstyle\"> or C<\"newstyle-fixed\">. 
Other strings might
+be returned in future.  Most modern NBD servers use
C<\"newstyle-fixed\">.
+"
+^ non_blocking_test_call_description
+  };
+
   "get_size", {
     default_call with
     args = []; ret = RInt64;
@@ -2553,6 +2577,7 @@ let first_version = [
   "set_request_structured_replies", (1, 2);
   "get_request_structured_replies", (1, 2);
   "get_tls_negotiated", (1, 2);
+  "get_protocol", (1, 2);
 
   (* These calls are proposed for a future version of libnbd, but
    * have not been added to any released version so far.
diff --git a/generator/states-newstyle-opt-export-name.c
b/generator/states-newstyle-opt-export-name.c
index ec73136..1c6b443 100644
--- a/generator/states-newstyle-opt-export-name.c
+++ b/generator/states-newstyle-opt-export-name.c
@@ -68,7 +68,7 @@
     SET_NEXT_STATE (%.DEAD);
     return 0;
   }
-  SET_NEXT_STATE (%.READY);
+  SET_NEXT_STATE (%^FINISHED);
   return 0;
 
 } /* END STATE MACHINE */
diff --git a/generator/states-newstyle-opt-go.c
b/generator/states-newstyle-opt-go.c
index 49875a5..d0d2123 100644
--- a/generator/states-newstyle-opt-go.c
+++ b/generator/states-newstyle-opt-go.c
@@ -108,7 +108,7 @@
 
   switch (reply) {
   case NBD_REP_ACK:
-    SET_NEXT_STATE (%.READY);
+    SET_NEXT_STATE (%^FINISHED);
     return 0;
   case NBD_REP_INFO:
     if (len > maxpayload /* see RECV_NEWSTYLE_OPT_GO_REPLY */)
diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c
index c8f817e..7742ea3 100644
--- a/generator/states-newstyle.c
+++ b/generator/states-newstyle.c
@@ -155,4 +155,13 @@ handle_reply_error (struct nbd_handle *h)
   }
   return 0;
 
+ NEWSTYLE.FINISHED:
+  if ((h->gflags & NBD_FLAG_FIXED_NEWSTYLE) == 0)
+    h->protocol = "newstyle";
+  else
+    h->protocol = "newstyle-fixed";
+
+  SET_NEXT_STATE (%.READY);
+  return 0;
+
 } /* END STATE MACHINE */
diff --git a/generator/states-oldstyle.c b/generator/states-oldstyle.c
index 1aff185..cb4f0da 100644
--- a/generator/states-oldstyle.c
+++ b/generator/states-oldstyle.c
@@ -64,6 +64,8 @@
     return 0;
   }
 
+  h->protocol = "oldstyle";
+
   SET_NEXT_STATE (%.READY);
 
   return 0;
diff --git a/lib/handle.c b/lib/handle.c
index bc4206c..85d10cd 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -315,3 +315,15 @@ nbd_unlocked_supports_uri (struct nbd_handle *h)
   return 0;
 #endif
 }
+
+const char *
+nbd_unlocked_get_protocol (struct nbd_handle *h)
+{
+  /* I believe that if we reach the Connected or Closed permitted
+   * states, then the state machine must have set h->protocol.  So if
+   * this assertion is hit then it indicates a bug in libnbd.
+   */
+  assert (h->protocol);
+
+  return h->protocol;
+}
diff --git a/lib/internal.h b/lib/internal.h
index eb76ac1..87b413d 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -87,7 +87,10 @@ struct nbd_handle {
   uint64_t exportsize;
   uint16_t eflags;
 
-  /* Flag set by the state machine to tell whether TLS was negotiated. */
+  /* Flags set by the state machine to tell what protocol and whether
+   * TLS was negotiated.
+   */
+  const char *protocol;
   bool tls_negotiated;
 
   int64_t unique;               /* Used for generating cookie numbers. */
diff --git a/tests/get-size.c b/tests/get-size.c
index f10597c..e6f44f7 100644
--- a/tests/get-size.c
+++ b/tests/get-size.c
@@ -26,6 +26,7 @@
 #include <stdlib.h>
 #include <stdint.h>
 #include <inttypes.h>
+#include <string.h>
 
 #include <libnbd.h>
 
@@ -38,8 +39,10 @@ main (int argc, char *argv[])
 {
   struct nbd_handle *nbd;
   int64_t r;
-  char *args[] = { "nbdkit", "-s",
"--exit-with-parent", "-v",
+  /* -n forces newstyle even if someone is still using nbdkit < 1.3 */
+  char *args[] = { "nbdkit", "-s",
"--exit-with-parent", "-n", "-v",
                    "null", "size=" STR(SIZE), NULL };
+  const char *s;
 
   nbd = nbd_create ();
   if (nbd == NULL) {
@@ -51,6 +54,14 @@ main (int argc, char *argv[])
     exit (EXIT_FAILURE);
   }
 
+  /* Even ancient versions of nbdkit only supported newstyle-fixed. */
+  s = nbd_get_protocol (nbd);
+  if (strcmp (s, "newstyle-fixed") != 0) {
+    fprintf (stderr,
+             "incorrect protocol \"%s\", expected
\"newstyle-fixed\"\n", s);
+    exit (EXIT_FAILURE);
+  }
+
   if ((r = nbd_get_size (nbd)) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
diff --git a/tests/oldstyle.c b/tests/oldstyle.c
index c179c45..b90b775 100644
--- a/tests/oldstyle.c
+++ b/tests/oldstyle.c
@@ -84,6 +84,7 @@ main (int argc, char *argv[])
   char *args[] = { "nbdkit", "-s", "-o",
"--exit-with-parent", "-v",
                    "memory", "size=" STR(SIZE), NULL };
   int calls = 0;
+  const char *s;
 
   progname = argv[0];
 
@@ -114,6 +115,14 @@ main (int argc, char *argv[])
     exit (EXIT_FAILURE);
   }
 
+  /* Protocol should be "oldstyle". */
+  s = nbd_get_protocol (nbd);
+  if (strcmp (s, "oldstyle") != 0) {
+    fprintf (stderr,
+             "incorrect protocol \"%s\", expected
\"oldstyle\"\n", s);
+    exit (EXIT_FAILURE);
+  }
+
   if ((r = nbd_get_size (nbd)) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
-- 
2.23.0

On 9/17/19 5:02 AM, Richard W.M. Jones wrote:
> When LIBNBD_TLS_ALLOW is used we don't have a way to find out if TLS
> was really negotiated.  This adds a flag and a way to read it back.
> 
> Unfortunately there is no test yet, because LIBNBD_TLS_ALLOW is not
> tested -- it really should be but requires quite a complicated set of
> tests because ideally we'd like to find out whether it falls back
> correctly for all supported servers.
qemu doesn't support fallback - if the server is configured for TLS, the
client must supply it.

nbdkit DOES support fallback, so yes, our existing tests/*tls.sh can be
a starting point for a setup where we test ALLOW.  But leaving it as a
separate patch via your TODO addition is fine for now.
>    "get_tls", {
>      default_call with
>      args = []; ret = RInt;
>      may_set_error = false;
> -    shortdesc = "get the current TLS setting";
> +    shortdesc = "get the TLS request setting";
>      longdesc = "\
> -Get the current TLS setting.";
> -    see_also = ["L<nbd_set_tls(3)>"];
> +Get the TLS request setting.
> +
> +B<Note:> If you want to find out if TLS was actually negotiated
> +on a particular connection use L<nbd_get_tls_negotiated(3)>
instead.";
> +    see_also = ["L<nbd_set_tls(3)>";
"L<nbd_get_tls_negotiated(3)>"];
> +  };
> +
> +  "get_tls_negotiated", {
> +    default_call with
> +    args = []; ret = RBool;
> +    permitted_states = [ Connected; Closed ];
> +    shortdesc = "find out if TLS was negotiated on a
connection";
> +    longdesc = "\
> +After connecting you may call this to find out if the
> +connection is using TLS.
Seeing this makes me think I should probably add
"get_structured_replies", since
"get_request_structured_replies" has the
same issue of reporting only what was requested, not what actually
happened.  I'll propose that on top of your patches.

ACK that we need this (for 1.1, but not backported to stable-1.0).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 9/17/19 5:02 AM, Richard W.M. Jones wrote:
> This commit adds a new API which can be used from the connected to
> state to read back which NBD protocol (eg. oldstyle, newstyle-fixed)
> we are using.
Somewhat of an overlap with my get_handshake_flags (as newstyle-fixed
corresponds to whether handshake_flags includes the FIXED_NEWSTYLE
flag), but I don't see the redundancy as an issue.
> 
> It was helpful to add a new state in newstyle negotiation
> (%NEWSTYLE.FINISHED) so we can route all successful option
> negotiations through a single path before moving to the %READY state,
> allowing us to set h->protocol in one place.
> ---
> +  "get_protocol", {
> +    default_call with
> +    args = []; ret = RStaticString;
> +    permitted_states = [ Connected; Closed ];
> +    shortdesc = "return the NBD protocol variant";
> +    longdesc = "\
> +Return the NBD protocol variant in use on the connection.  At
> +the moment this returns one of the strings
C<\"oldstyle\">,
> +C<\"newstyle\"> or C<\"newstyle-fixed\">. 
Other strings might
> +be returned in future.  Most modern NBD servers use
C<\"newstyle-fixed\">.
s/in/in the/

> +++ b/generator/states-newstyle.c
> @@ -155,4 +155,13 @@ handle_reply_error (struct nbd_handle *h)
>    }
>    return 0;
>  
> + NEWSTYLE.FINISHED:
> +  if ((h->gflags & NBD_FLAG_FIXED_NEWSTYLE) == 0)
> +    h->protocol = "newstyle";
> +  else
> +    h->protocol = "newstyle-fixed";
Should work whether this lands before or after my set_handshake_flags patch.

> +++ b/lib/handle.c
> @@ -315,3 +315,15 @@ nbd_unlocked_supports_uri (struct nbd_handle *h)
>    return 0;
>  #endif
>  }
> +
> +const char *
> +nbd_unlocked_get_protocol (struct nbd_handle *h)
> +{
> +  /* I believe that if we reach the Connected or Closed permitted
> +   * states, then the state machine must have set h->protocol.  So if
> +   * this assertion is hit then it indicates a bug in libnbd.
> +   */
> +  assert (h->protocol);
Sounds correct to me.

> @@ -51,6 +54,14 @@ main (int argc, char *argv[])
>      exit (EXIT_FAILURE);
>    }
>  
> +  /* Even ancient versions of nbdkit only supported newstyle-fixed. */
> +  s = nbd_get_protocol (nbd);
> +  if (strcmp (s, "newstyle-fixed") != 0) {
> +    fprintf (stderr,
> +             "incorrect protocol \"%s\", expected
\"newstyle-fixed\"\n", s);
> +    exit (EXIT_FAILURE);
> +  }
With nbdkit 1.16 --mask-handshake and/or my patch for
nbd_set_handshake_flags, we can also provoke a test of "newstyle" as
output; looks like it's probably better for your patch to go in and me
to rebase mine on top of yours (since I was still working on writing
interop tests for mine).

ACK.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org