Re: [Qemu-devel] [PATCH 1/1] slirp: add SOCKS5 support

From: Laurent Vivier <laurent@vivier.eu>
To: Eric Blake <eblake@redhat.com>,
	Samuel Thibault <samuel.thibault@ens-lyon.org>
Cc: Jason Wang <jasowang@redhat.com>,
	qemu-devel@nongnu.org, "Daniel P. Berrange" <berrange@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 1/1] slirp: add SOCKS5 support
Date: Mon, 27 Mar 2017 20:58:50 +0200	[thread overview]
Message-ID: <3a712e65-045d-3dc2-9c07-a13c8268aa6f@vivier.eu> (raw)
In-Reply-To: <237b2650-da58-c844-d594-390f41e23e65@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 2056 bytes --]

Le 27/03/2017 à 20:41, Eric Blake a écrit :
> On 03/27/2017 01:21 PM, Laurent Vivier wrote:
>> When the VM is used behind a firewall, This allows
>> to use a SOCKS5 proxy server to connect the VM IP stack
> 
> "allows to $verb" is not idiomatic English; the correct forms are
> generally "allows $subject to $verb" or "allows ${verb}ing".  In this
> case, I'd lean towards "this allows the use of a SOCKS5 proxy server"

Thank you, and sorry.

> 
>> directly to the Internet.
>>
>> This implementation doesn't manage UDP packets, so they
>> are simply dropped (as with restrict=on), except for
>> the localhost as we need it for DNS.
>>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
> 
>> +++ b/qapi-schema.json
>> @@ -3680,6 +3680,9 @@
>>      '*ipv6-dns':         'str',
>>      '*smb':       'str',
>>      '*smbserver': 'str',
>> +    '*proxy-server': 'str',
>> +    '*proxy-user':   'str',
>> +    '*proxy-passwd': 'str',
> 
> Why can't we spell this out as password, instead of abbreviating?

because of unix command "passwd" ;) . I will fix.

> Should this hook into the "secrets object" framework so that someone
> does not have to pass the password in plaintext?
> 
>>      '*hostfwd':   ['String'],
>>      '*guestfwd':  ['String'] } }
> 
> Missing documentation.

I will fix.

> Do we want all three proxy elements to be in a substruct? The difference
> is between:
> 
> { ... "smb": "foo", "proxy-server": "bar", "proxy-user": "noone",
> "proxy-passwd": "hello" }
> 
> and a substruct:
> 
> { ... "smb": "foo", "proxy": { "server": "bar", "user", "noone",
> "passwd": "hello" } }

yes, substruct looks better.

> 
>>  
>> +@item proxy-server=@var{addr}:@var{port}[,proxy-user=@var{user},proxy-passwd=@var{passwd}]]
> 
> Yes, you DEFINITELY need to hook into the "secrets object" framework to
> avoid having to pass a password in plaintext on the command line.  Dan
> Berrange may have more advice on doing that.
> 

OK

Thank you for the review.

Laurent

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]