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

[Marc-André Lureau]

Hi

On Wed, Feb 21, 2018 at 10:24 AM, Frediano Ziglio <fziglio at redhat.com> wrote:
>> 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 'port' or 'tls-port' query string parameter.
>>
>> 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..459e5f1 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: 'spice+tls' is available since v0.35, you have to use the
>> +spice:// query string with the 'tls-port' parameter before that.
>> +
>> +=head1 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 e6db424..d2aa5e7 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;
>>
>
> bool/gboolean?
> Fine for me, I like bool is C style and uses less bits.
>
>>      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);
>
> maybe this can be confusing here?
> Not a big deal I think.
>

not a big deal, it's mostly a debugging message (not exposed to user -
we would need to raise the error somewhere). Feel free to propose
improvements (use GInitable to fail at creation time? make the
property read-only and expose a setter instead?).

>>          goto fail;
>>      }
>> -    authority = uri + strlen(URI_SCHEME_SPICE);
>>
>>      tmp = strchr(authority, '@');
>>      if (tmp) {
>> @@ -531,6 +537,11 @@ static int spice_parse_uri(SpiceSession *session, const
>> char *original_uri)
>>          if (*query)
>>              query++;
>>
>> +        if (tls_scheme && (g_str_equal(key, "port") || g_str_equal(key,
>> "tls-port"))) {
>> +            g_warning(URI_SCHEME_SPICE_TLS " scheme doesn't accept '%s'",
>> key);
>> +            continue;
>> +        }
>> +
>>          target_key = NULL;
>>          if (g_str_equal(key, "port")) {
>>              target_key = &port;
>> @@ -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;
>
> Otherwise,
>
> Acked-by: Frediano Ziglio <fziglio at redhat.com>
>
> OT: there are a lot of mix between of "if (ptr)" and "if (ptr != NULL)" in
> these patches and the current code too, no much preference.

spice-gtk code is happily using both style. Some people prefer an
explicit != NULL. I don't mind.

-- 
Marc-André Lureau