| Message ID | 7d250759ff7d01d2aec5f8f48ed51afb7fcfb17c.1424190993.git.mprivozn@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Feb 17, 2015 at 05:40:45PM +0100, Michal Privoznik wrote: > So, imagine you've started a guest with ticketing enabled. You've set > some password to access your SPICE/VNC session. However, later you > want to give the access to somebody else's and therefore disable the > ticketing. Come on, be imaginative! Currently, there's no way how to > achieve this. And while there are two possible ways to fulfill the > goal: 1) invent new monitor command to disable ticketing, or 2) let > @password argument to 'set_password' monitor command be optional, I'm > choosing the latter. It's easier to implement, after all. > > The idea behind, how this will work, is: if user issues the command > without the password field, it means they want to disable the > ticketing. Any subsequent call to the call with password field filled > in, will enable the ticketing again. When password auth is enabled with VNC, the use of a NULL / empty string password is explicitly intended to block access to the VNC server, by causing the password auth to always return failure. Overloading the 'set_password' command such that a missing password changes the auth scheme in use is a really surprising and bad side effect. If we want to have the ability to change the authentication protocol used for VNC/SPICE, then lets add a proper command for this. ie create a 'set_graphics_auth' command to change auth protocol. This is really better for VNC anyway, as there are far more possible auth schemes than just password or no-password, and overloading the 'set_password' command can't handle that. Regards, Daniel
On 02/17/2015 09:53 AM, Daniel P. Berrange wrote: > On Tue, Feb 17, 2015 at 05:40:45PM +0100, Michal Privoznik wrote: >> So, imagine you've started a guest with ticketing enabled. You've set >> some password to access your SPICE/VNC session. However, later you >> want to give the access to somebody else's and therefore disable the >> ticketing. Come on, be imaginative! Currently, there's no way how to >> achieve this. And while there are two possible ways to fulfill the >> goal: 1) invent new monitor command to disable ticketing, or 2) let >> @password argument to 'set_password' monitor command be optional, I'm >> choosing the latter. It's easier to implement, after all. >> >> The idea behind, how this will work, is: if user issues the command >> without the password field, it means they want to disable the >> ticketing. Any subsequent call to the call with password field filled >> in, will enable the ticketing again. > > When password auth is enabled with VNC, the use of a NULL / empty string > password is explicitly intended to block access to the VNC server, by > causing the password auth to always return failure. Overloading the > 'set_password' command such that a missing password changes the auth > scheme in use is a really surprising and bad side effect. > > If we want to have the ability to change the authentication protocol > used for VNC/SPICE, then lets add a proper command for this. ie > create a 'set_graphics_auth' command to change auth protocol. This > is really better for VNC anyway, as there are far more possible auth > schemes than just password or no-password, and overloading the > 'set_password' command can't handle that. Agreed about the need for a new command; another rationale is that making an argument optional is NOT discoverable without introspection or painful probing, but adding a new command IS easily discovered via the existing query commands that list all commands.
diff --git a/hmp-commands.hx b/hmp-commands.hx index e37bc8b..91dffb2 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1576,7 +1576,7 @@ ETEXI { .name = "set_password", - .args_type = "protocol:s,password:s,connected:s?", + .args_type = "protocol:s,password:s?,connected:s?", .params = "protocol password action-if-connected", .help = "set spice/vnc password", .mhandler.cmd = hmp_set_password, diff --git a/hmp.c b/hmp.c index b47f331..ce0d19e 100644 --- a/hmp.c +++ b/hmp.c @@ -1147,7 +1147,8 @@ void hmp_set_password(Monitor *mon, const QDict *qdict) const char *connected = qdict_get_try_str(qdict, "connected"); Error *err = NULL; - qmp_set_password(protocol, password, !!connected, connected, &err); + qmp_set_password(protocol, !!password, password, + !!connected, connected, &err); hmp_handle_error(mon, &err); } diff --git a/qapi-schema.json b/qapi-schema.json index e16f8eb..a0d9b38 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1561,12 +1561,14 @@ ## # @set_password: # -# Sets the password of a remote display session. +# Sets (and enables) the password of a remote display session. +# Or, optionally since 2.2.1, if no @password is passed, disables +# password for remote display. # # @protocol: `vnc' to modify the VNC server password # `spice' to modify the Spice server password # -# @password: the new password +# @password: #optional the new password # # @connected: #optional how to handle existing clients when changing the # password. If nothing is specified, defaults to `keep' @@ -1580,7 +1582,7 @@ # Since: 0.14.0 ## { 'command': 'set_password', - 'data': {'protocol': 'str', 'password': 'str', '*connected': 'str'} } + 'data': {'protocol': 'str', '*password': 'str', '*connected': 'str'} } ## # @expire_password: diff --git a/qmp-commands.hx b/qmp-commands.hx index a85d847..e926a4e 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1753,7 +1753,7 @@ EQMP { .name = "set_password", - .args_type = "protocol:s,password:s,connected:s?", + .args_type = "protocol:s,password:s?,connected:s?", .mhandler.cmd_new = qmp_marshal_input_set_password, }, diff --git a/qmp.c b/qmp.c index 6b2c4be..4f741f9 100644 --- a/qmp.c +++ b/qmp.c @@ -276,13 +276,17 @@ out: return 0; } -void qmp_set_password(const char *protocol, const char *password, +void qmp_set_password(const char *protocol, + bool has_password, const char *password, bool has_connected, const char *connected, Error **errp) { int disconnect_if_connected = 0; int fail_if_connected = 0; int rc; + if (!has_password) + password = NULL; + if (has_connected) { if (strcmp(connected, "fail") == 0) { fail_if_connected = 1;
So, imagine you've started a guest with ticketing enabled. You've set some password to access your SPICE/VNC session. However, later you want to give the access to somebody else's and therefore disable the ticketing. Come on, be imaginative! Currently, there's no way how to achieve this. And while there are two possible ways to fulfill the goal: 1) invent new monitor command to disable ticketing, or 2) let @password argument to 'set_password' monitor command be optional, I'm choosing the latter. It's easier to implement, after all. The idea behind, how this will work, is: if user issues the command without the password field, it means they want to disable the ticketing. Any subsequent call to the call with password field filled in, will enable the ticketing again. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- hmp-commands.hx | 2 +- hmp.c | 3 ++- qapi-schema.json | 8 +++++--- qmp-commands.hx | 2 +- qmp.c | 6 +++++- 5 files changed, 14 insertions(+), 7 deletions(-)