[Spice-devel] [PATCH spice-gtk 2/4] uri: learn to parse spice+tls:// form

[Frediano Ziglio]

> 
> From: Marc-André Lureau <marcandre.lureau at redhat.com>
> 
> spice:// has a weird scheme encoding, where it can accept both plain
> and tls ports with URI query parameters. However, it's not very
> convenient nor very common to use (who really want to mix plain & tls
> channels?).
> 
> Instead, let's introduce the more readable form spice+tls://host:port
> 
> This form will not accept query string, thus mixing plain and tls is
> not possible (it would be confusing to have ?port= for plain), nor
> passing password and such.
> 

OT: is this schema extensible? Now we have "spice", "spice+unix"
and "spice+tls", surely we don't want "spice+unix+tls" but maybe
somebody could this of a "spice+tls+kerberos" ?

> Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> ---
>  man/spice-client.pod | 29 +++++++++++++++++------------
>  src/spice-session.c  | 23 +++++++++++++++++++----
>  2 files changed, 36 insertions(+), 16 deletions(-)
> 
> diff --git a/man/spice-client.pod b/man/spice-client.pod
> index 7288b84..4b23c7d 100644
> --- a/man/spice-client.pod
> +++ b/man/spice-client.pod
> @@ -12,23 +12,24 @@ can be used to tweak some SPICE-specific option.
>  
>  =head1 URI
>  
> -The most basic SPICE URI which can be used is in the form
> +To initiate a plain SPICE connection (the connection will be
> +unencrypted) to hostname.example.com and port 5900, use the following
> +URI:
> +
>    spice://hostname.example.com:5900
>  
> -This will try to initiate a SPICE connection to hostname.example.com
> -to port 5900. This connection will be unencrypted. This URI is
> -equivalent to
> -  spice://hostname.example.com?port=5900
> +In order to start a TLS connection, one would use:
>  
> -In order to start a TLS connection, one would use
> -  spice://hostname.example.com?tls-port=5900
> +  spice+tls://hostname.example.com:5900
>  
> -Other valid URI parameters are 'username' and 'password'. Be careful that
> -passing a password through a SPICE URI might cause the password to be
> -visible by any local user through 'ps'.
> +Note: this form is available since v0.35, you have to use the spice://
> +query string with the tls-port parameter before that.
> +
> +=head1 spice:// URI query string
> +
> +spice:// URI accepts query string. Several parameters can be specified
> +at once if they are separated by & or ;
>  
> -Several parameters can be specified at once if they are separated
> -by & or ;
>    spice://hostname.example.com?port=5900;tls-port=5901
>  
>  When using 'tls-port', it's recommended to not specify any non-TLS port.
> @@ -39,6 +40,10 @@ then try to use the TLS port. This means a
> man-in-the-middle could force
>  the whole SPICE session to go in clear text regardless of the TLS settings
>  of the SPICE server.
>  
> +Other valid URI parameters are 'username' and 'password'. Be careful that
> +passing a password through a SPICE URI might cause the password to be
> +visible by any local user through 'ps'.
> +
>  =head1 OPTIONS
>  
>  The following options are accepted when running a SPICE client which
> diff --git a/src/spice-session.c b/src/spice-session.c
> index 2aabf58..7218449 100644
> --- a/src/spice-session.c
> +++ b/src/spice-session.c
> @@ -389,6 +389,7 @@ spice_session_finalize(GObject *gobject)
>  
>  #define URI_SCHEME_SPICE "spice://"
>  #define URI_SCHEME_SPICE_UNIX "spice+unix://"
> +#define URI_SCHEME_SPICE_TLS "spice+tls://"
>  #define URI_QUERY_START ";?"
>  #define URI_QUERY_SEP   ";&"
>  
> @@ -425,6 +426,7 @@ static int spice_parse_uri(SpiceSession *session, const
> char *original_uri)
>      gchar *authority = NULL;
>      gchar *query = NULL;
>      gchar *tmp = NULL;
> +    bool tls_scheme = false;
>  
>      g_return_val_if_fail(original_uri != NULL, -1);
>  
> @@ -438,12 +440,16 @@ static int spice_parse_uri(SpiceSession *session, const
> char *original_uri)
>      /* Break up the URI into its various parts, scheme, authority,
>       * path (ignored) and query
>       */
> -    if (!g_str_has_prefix(uri, URI_SCHEME_SPICE)) {
> +    if (g_str_has_prefix(uri, URI_SCHEME_SPICE)) {
> +        authority = uri + strlen(URI_SCHEME_SPICE);
> +    } else if (g_str_has_prefix(uri, URI_SCHEME_SPICE_TLS)) {
> +        authority = uri + strlen(URI_SCHEME_SPICE_TLS);
> +        tls_scheme = true;
> +    } else {
>          g_warning("Expected a URI scheme of '%s' in URI '%s'",
>                    URI_SCHEME_SPICE, uri);

I think here we should extent like

         g_warning("Expected a URI scheme of '%s' or '%s' in URI '%s'",
                   URI_SCHEME_SPICE, URI_SCHEME_SPICE_TLS, uri);

>          goto fail;
>      }
> -    authority = uri + strlen(URI_SCHEME_SPICE);
>  
>      tmp = strchr(authority, '@');
>      if (tmp) {
> @@ -502,6 +508,11 @@ static int spice_parse_uri(SpiceSession *session, const
> char *original_uri)
>      }
>      path = NULL;
>  
> +    if (tls_scheme && query[0] != '\0') {
> +        g_warning(URI_SCHEME_SPICE_TLS " scheme doesn't support query
> string");
> +        goto fail;
> +    }
> +

I would personally allow additional (currently not present) parameters,
I would just check that spice+tls and tls_port are not both specified.

>      while (query && query[0] != '\0') {
>          gchar key[32], value[128];
>          gchar **target_key;
> @@ -568,8 +579,12 @@ end:
>      s->unix_path = g_strdup(path);
>      g_free(uri);
>      s->host = host;
> -    s->port = port;
> -    s->tls_port = tls_port;
> +    if (tls_scheme) {
> +        s->tls_port = port;
> +    } else {
> +        s->port = port;
> +        s->tls_port = tls_port;
> +    }
>      s->username = username;
>      s->password = password;
>      return 0;

Frediano