On Sun, Sep 25, 2011 at 9:26 PM, Olli Salli <olli.salli at collabora.co.uk> wrote:
>
> setPasswordRequired(false);
>
> For added security, you could generate a random password, and export
> that as a tube parameter via the optional params parameter to
> exportTcpSocket. The other end (krdc/alike) would pick this up from its
> IncomingStreamTubeChannel::parameters() and authenticate with it using
> the RFB protocol. (Using StreamTubeClient in the other end, this can be
> done by inspecting the tube channel from the tubeAcceptedAsTcp signal).
>
> This is only anyway locally exploitable as your socket listens on local
> loopback, but still anybody from the computer, including different
> users, can connect to it, which might be undesired for some terminal
> server -like uses (think remote access to your account on some box -
> you'd run krfb there, but without a password anybody else having access
> of their own to the box could access your session!).
>
Nice, I didn't know I could do that. I will add this as a feature
after porting both sides.
>
>> void TubesRfbServer::onTubeRequested()
>> {
>> kDebug() << "Got a tube";
>> }
>
>> void TubesRfbServer::onTubeClosed()
>> {
>> kDebug() << "tube closed";
>> }
>
> tp-qt4 already emits you plenty of debug for the tubes being requested
> and closed (including the object path, close error etc) so if you aren't
> going to do anything on those events, I'd just drop these slots altogether.
>
Yeah, I just wanted to see how the api behaves. I will remove them in
the final version.
>
>> //try a few times with different ports
>> bool ok = false;
>> for (int i=0; !ok && i<5; i++) {
>> setListeningPort((KRandom::random() % 16383) + 49152);
>> ok = start();
>> }
>
>> if (!ok) {
>> kError() << "Failed to start tubes rfb server";
>> return;
>> }
>
> OK, I know this is not really TP, but giving up after trying a few
> random ports seems a bit shady. I'd also like the error message to be
> more explicit about the cause.
>
> Is the port being already taken the only reason why start() could fail?
> Is start() a particularly heavy operation, or is it pretty much just
> socket(), bind() & listen()? If it's cheap and (at least usually) fails
> because the port is taken, I'd rather just randomize the starting port,
> then try every successive port after that (and rolling over on 65535),
> and only stop when all 16384 ports have been tried. Something like:
I don't fully know when start() can fail, as it just calls some
functions from libvncserver to initialize the screen and start
listening, then checks if the screen is active. It is most likely that
it will fail if one of the socket functions fails, that's why I added
the code to try different ports, but if it fails for some other reason
(not sure what), I wouldn't like it to try thousands of times before
it gives up.
>
>>d->stubeServer->exportTcpSocket(QHostAddress(QString::fromAscii(listeningAddress())),
>> listeningPort());
>
> You should check that stubeServer->isRegistered(), and at least kError()
> or ideally somehow signal to krdc that you aren't actually going to get
> any connections (freeing up the listen port etc). This could happen e.g.
> because something else has already taken your bus name (another
> instance? is there some pre-existing instance activation logic in krfb?)
> or you don't have a working dbus connection.
>
Good catch. I thought this was a KUniqueApplication, but apparently it
is not, so there could be multiple instances. I should probably change
it to be a KUniqueApplication, then there will be no way to start two
of them. I will add a check for isRegistered() though, just in case.
>
>> QHash<quint16, Tp::ContactPtr> contactsPerPort;
>> d->contactsPerPort[sourcePort] = contact;
>> d->contactsPerPort.remove(sourcePort);
>> if (d->contactsPerPort.contains(port)) {
>
> Do you really need to maintain this mapping yourself?
> StreamTubeServer::tcpConnections() gives you the same info - though
> granted, it'll be slightly clumsier to look up the contact from as it
> has the account too (and you need to include QHostAddress::LocalHost in
> the key you look up with).
To be honest I didn't realize tcpConnections() was there (this line in
the header file is unreadable...) and I just reused the old code that
was in krfb. Now that you mention it, I could probably use
tcpConnections(), although I don't find much value in it. I don't need
the account pointer and also I don't need to keep track of the source
address (why do we have this anyway, isn't it always localhost?).
> However, if there is some kind of virtual fn
> notifying about clients disconnecting you can implement to clean up
> clients from the clientsPerPort mapping, you could then do away with
> onTcpConnectionClosed entirely.
There is no such virtual function, although I could add one, given
that the parent class knows when a client disconnects. But it is
easier to get this information from telepathy...
> If you think the return type of STS::tcpConnections is too clumsy, I'm
> open to suggestions on how to simplify it (split to multiple accessors?
> make some small struct with account/contact rather than using raw
> pairs?). We can still change it as the APIs haven't yet been released in
> tp-qt4.
>
Maybe a small struct would make it more clear, yes. I don't much like
pair return values, since it is not very clear what you are doing if
you use one of its members directly, for example:
setContact(sts->tcpConnections()[qMakePair(QHostAddress::LocalHost,
port)].second);
is not as clear as:
setContact(sts->tcpConnections()[qMakePair(QHostAddress::LocalHost,
port)].contact);
and I would have to write something like this to make it more clear:
ContactPtr contact =
tcpConnections()[qMakePair(QHostAddress::LocalHost, port)].second;
setContact(contact);
Regards,
George