vncconfig: add option to force view-only remote client connections #1670
Conversation
casantos
force-pushed
the
casantos-rhbz_2180903
branch
from
7fe09c5
to
e75b5e7
Compare
| @@ -181,7 +181,7 @@ typedef struct { | |||
| CARD8 vncExtReqType; /* always VncExtConnect */ | |||
| CARD16 length B16; | |||
| CARD8 strLen; | |||
| CARD8 pad0; | |||
| CARD8 viewOnly; | |||
There was a problem hiding this comment.
Hi, I'm not really an expert here, it's also where I ended up when I was looking into this issue myself. I assume this is following a certain specification (in this case some RealVNC extension?).
From the documentation:
Several message formats include padding bits or bytes. For maximum
compatibility, messages should be generated with padding set to zero,
but message recipients should not assume padding has any particular
value
I'm just trying to say that I'm not sure you can change the message format here, even though you are not technically changing it.
There was a problem hiding this comment.
This is not the RFB protocol; it's the TigerVNC X11 extension. vncconfig interacts with Xvnc as an X11 client. The messages flow over the X11 connection, usually using the Unix socket at /tmp/.X11-unix/X<diplay-number>.
There was a problem hiding this comment.
I see, this makes much more sense now.
There was a problem hiding this comment.
The feature looks reasonable, but I think some more work is needed on the implementation.
My primary concern is that I want to avoid having "view only" as a concept in the core code. That is a high-level abstraction. In the core code, we are more concerned with access bits. So an interface based around those would be ideal.
Could you please elaborate that? I don't see a contradiction between implementing this feature and a high-level abstraction. Is there an official description of such abstraction? |
|
"View only" is not something that currently exists in the core code. It deals with access bits. So something between the core code and vncconfig needs to do a mapping of what "view only" means in terms of access bits. The current places where "view only" is handled is in |
[rephrasing my comment] I could not find a way to make those methods know that we are performing a reverse connection and if we want to force view-only mode or not. That's why I had to modify the A more sophisticated solution would require the implementation of new Security sub-classes whose What really annoys me in the current solution is the change in |
|
I'm attempting to craft an alternate implementation but I can't find where SConnection.security is initialized. |
|
It's set here: tigervnc/common/rfb/SConnection.cxx Line 184 in 2d0d516 So right now it is entirely controlled by the security type handler. We probably need to add another mechanism. |
That code is executed only if the client needs protocol 3.3 (see the test in line 169), so I think it's actually initialized here in most of the cases: tigervnc/common/rfb/SConnection.cxx Line 234 in 2d0d516 The main problem here is that the various SSecurity classes do three things at the same time:
So setting the access rights of a reverse connection via security type handler requires more extensive changes, since we must find a way to force |
This is is where SConnection.ssecurity is initialized, not SConnection.security, but I found that it is implicitly initialized by the compiler, since the SecurityServer constructor does not need any argument. |
|
OK, I have a second attempt in #1701 but quite frankly I don't like it. Seems to be overkill. |
casantos
force-pushed
the
casantos-rhbz_2180903
branch
from
e75b5e7
to
49f7434
Compare
I think that's reasonable, if we replace What I don't really like is that these RFB specific things are added to code that's in the general |
|
Suggested removal of diff --git a/common/network/Socket.h b/common/network/Socket.h
index 117851c1..7085d73a 100644
--- a/common/network/Socket.h
+++ b/common/network/Socket.h
@@ -111,41 +111,6 @@ namespace network {
SocketException(const char* text, int err_) : rdr::SystemException(text, err_) {}
};
- class SocketServer {
- public:
- virtual ~SocketServer() {}
-
- // addSocket() tells the server to serve the Socket. The caller
- // retains ownership of the Socket - the only way for the server
- // to discard a Socket is by calling shutdown() on it.
- // outgoing is set to true if the socket was created by connecting out
- // to another host, or false if the socket was created by accept()ing
- // an incoming connection.
- virtual void addSocket(network::Socket* sock, bool outgoing=false) = 0;
-
- // removeSocket() tells the server to stop serving the Socket. The
- // caller retains ownership of the Socket - the server must NOT
- // delete the Socket! This call is used mainly to cause per-Socket
- // resources to be freed.
- virtual void removeSocket(network::Socket* sock) = 0;
-
- // getSockets() gets a list of sockets. This can be used to generate an
- // fd_set for calling select().
- virtual void getSockets(std::list<network::Socket*>* sockets) = 0;
-
- // processSocketReadEvent() tells the server there is a Socket read event.
- // The implementation can indicate that the Socket is no longer active
- // by calling shutdown() on it. The caller will then call removeSocket()
- // soon after processSocketEvent returns, to allow any pre-Socket
- // resources to be tidied up.
- virtual void processSocketReadEvent(network::Socket* sock) = 0;
-
- // processSocketReadEvent() tells the server there is a Socket write event.
- // This is only necessary if the Socket has been put in non-blocking
- // mode and needs this callback to flush the buffer.
- virtual void processSocketWriteEvent(network::Socket* sock) = 0;
- };
-
}
#endif // __NETWORK_SOCKET_H__
diff --git a/common/rfb/VNCServer.h b/common/rfb/VNCServer.h
index 3f97634b..6d75f8f8 100644
--- a/common/rfb/VNCServer.h
+++ b/common/rfb/VNCServer.h
@@ -23,17 +23,46 @@
#ifndef __RFB_VNCSERVER_H__
#define __RFB_VNCSERVER_H__
-#include <network/Socket.h>
-
#include <rfb/UpdateTracker.h>
#include <rfb/SSecurity.h>
#include <rfb/ScreenSet.h>
+namespace network { class Socket; }
+
namespace rfb {
- class VNCServer : public UpdateTracker,
- public network::SocketServer {
+ class VNCServer : public UpdateTracker {
public:
+ // addSocket() tells the server to serve the Socket. The caller
+ // retains ownership of the Socket - the only way for the server
+ // to discard a Socket is by calling shutdown() on it.
+ // outgoing is set to true if the socket was created by connecting out
+ // to another host, or false if the socket was created by accept()ing
+ // an incoming connection.
+ virtual void addSocket(network::Socket* sock, bool outgoing=false) = 0;
+
+ // removeSocket() tells the server to stop serving the Socket. The
+ // caller retains ownership of the Socket - the server must NOT
+ // delete the Socket! This call is used mainly to cause per-Socket
+ // resources to be freed.
+ virtual void removeSocket(network::Socket* sock) = 0;
+
+ // getSockets() gets a list of sockets. This can be used to generate an
+ // fd_set for calling select().
+ virtual void getSockets(std::list<network::Socket*>* sockets) = 0;
+
+ // processSocketReadEvent() tells the server there is a Socket read event.
+ // The implementation can indicate that the Socket is no longer active
+ // by calling shutdown() on it. The caller will then call removeSocket()
+ // soon after processSocketEvent returns, to allow any pre-Socket
+ // resources to be tidied up.
+ virtual void processSocketReadEvent(network::Socket* sock) = 0;
+
+ // processSocketReadEvent() tells the server there is a Socket write event.
+ // This is only necessary if the Socket has been put in non-blocking
+ // mode and needs this callback to flush the buffer.
+ virtual void processSocketWriteEvent(network::Socket* sock) = 0;
+
// blockUpdates()/unblockUpdates() tells the server that the pixel buffer
// is currently in flux and may not be accessed. The attributes of the
// pixel buffer may still be accessed, but not the frame buffer itself.
diff --git a/common/rfb/VNCServerST.cxx b/common/rfb/VNCServerST.cxx
index f0cfc31a..a4a6e8db 100644
--- a/common/rfb/VNCServerST.cxx
+++ b/common/rfb/VNCServerST.cxx
@@ -55,6 +55,8 @@
#include <assert.h>
#include <stdlib.h>
+#include <network/Socket.h>
+
#include <rfb/ComparingUpdateTracker.h>
#include <rfb/Exception.h>
#include <rfb/KeyRemapper.h>
@@ -127,7 +129,7 @@ VNCServerST::~VNCServerST()
}
-// SocketServer methods
+// VNCServer methods
void VNCServerST::addSocket(network::Socket* sock, bool outgoing)
{
@@ -232,8 +234,6 @@ void VNCServerST::processSocketWriteEvent(network::Socket* sock)
throw rdr::Exception("invalid Socket in VNCServerST");
}
-// VNCServer methods
-
void VNCServerST::blockUpdates()
{
blockCounter++;
diff --git a/common/rfb/VNCServerST.h b/common/rfb/VNCServerST.h
index 028903c9..be81fc2a 100644
--- a/common/rfb/VNCServerST.h
+++ b/common/rfb/VNCServerST.h
@@ -51,7 +51,7 @@ namespace rfb {
virtual ~VNCServerST();
- // Methods overridden from SocketServer
+ // Methods overridden from VNCServer
// addSocket
// Causes the server to allocate an RFB-protocol management
@@ -76,9 +76,6 @@ namespace rfb {
// Flush pending data from the Socket on to the network.
virtual void processSocketWriteEvent(network::Socket* sock);
-
- // Methods overridden from VNCServer
-
virtual void blockUpdates();
virtual void unblockUpdates();
virtual void setPixelBuffer(PixelBuffer* pb, const ScreenSet& layout);
diff --git a/unix/x0vncserver/XDesktop.cxx b/unix/x0vncserver/XDesktop.cxx
index b3d5e54c..da298d35 100644
--- a/unix/x0vncserver/XDesktop.cxx
+++ b/unix/x0vncserver/XDesktop.cxx
@@ -26,6 +26,8 @@
#include <signal.h>
#include <unistd.h>
+#include <network/Socket.h>
+
#include <rfb/LogWriter.h>
#include <rfb/Exception.h>
diff --git a/unix/xserver/hw/vnc/XserverDesktop.cc b/unix/xserver/hw/vnc/XserverDesktop.cc
index b5a58671..a2160a72 100644
--- a/unix/xserver/hw/vnc/XserverDesktop.cc
+++ b/unix/xserver/hw/vnc/XserverDesktop.cc
@@ -311,7 +311,7 @@ void XserverDesktop::handleSocketEvent(int fd, bool read, bool write)
bool XserverDesktop::handleListenerEvent(int fd,
std::list<SocketListener*>* sockets,
- SocketServer* sockserv)
+ VNCServer* sockserv)
{
std::list<SocketListener*>::iterator i;
@@ -332,7 +332,7 @@ bool XserverDesktop::handleListenerEvent(int fd,
}
bool XserverDesktop::handleSocketEvent(int fd,
- SocketServer* sockserv,
+ VNCServer* sockserv,
bool read, bool write)
{
std::list<Socket*> sockets;
diff --git a/unix/xserver/hw/vnc/XserverDesktop.h b/unix/xserver/hw/vnc/XserverDesktop.h
index 677097a6..a42b5440 100644
--- a/unix/xserver/hw/vnc/XserverDesktop.h
+++ b/unix/xserver/hw/vnc/XserverDesktop.h
@@ -42,7 +42,7 @@ namespace rfb {
class VNCServerST;
}
-namespace network { class SocketListener; class Socket; class SocketServer; }
+namespace network { class SocketListener; class Socket; }
class XserverDesktop : public rfb::SDesktop, public rfb::FullFramePixelBuffer,
public rfb::Timer::Callback {
@@ -107,9 +107,9 @@ public:
protected:
bool handleListenerEvent(int fd,
std::list<network::SocketListener*>* sockets,
- network::SocketServer* sockserv);
+ rfb::VNCServer* sockserv);
bool handleSocketEvent(int fd,
- network::SocketServer* sockserv,
+ rfb::VNCServer* sockserv,
bool read, bool write);
virtual bool handleTimeout(rfb::Timer* t);
diff --git a/win/rfb_win32/SocketManager.cxx b/win/rfb_win32/SocketManager.cxx
index b7cc1cce..8e88b79b 100644
--- a/win/rfb_win32/SocketManager.cxx
+++ b/win/rfb_win32/SocketManager.cxx
@@ -24,8 +24,12 @@
#include <winsock2.h>
#include <list>
+
+#include <network/Socket.h>
+
#include <rfb/LogWriter.h>
#include <rfb/Timer.h>
+#include <rfb/VNCServer.h>
#include <rfb/util.h>
#include <rfb_win32/SocketManager.h>
@@ -55,7 +59,7 @@ static void requestAddressChangeEvents(network::SocketListener* sock_) {
void SocketManager::addListener(network::SocketListener* sock_,
- network::SocketServer* srvr,
+ VNCServer* srvr,
AddressChangeNotifier* acn) {
WSAEVENT event = WSACreateEvent();
long flags = FD_ACCEPT | FD_CLOSE;
@@ -103,7 +107,7 @@ void SocketManager::remListener(network::SocketListener* sock) {
}
-void SocketManager::addSocket(network::Socket* sock_, network::SocketServer* srvr, bool outgoing) {
+void SocketManager::addSocket(network::Socket* sock_, VNCServer* srvr, bool outgoing) {
WSAEVENT event = WSACreateEvent();
if (!event || !addEvent(event, this) ||
(WSAEventSelect(sock_->getFd(), event, FD_READ | FD_CLOSE) == SOCKET_ERROR)) {
@@ -135,7 +139,7 @@ void SocketManager::remSocket(network::Socket* sock_) {
throw rdr::Exception("Socket not registered");
}
-bool SocketManager::getDisable(network::SocketServer* srvr)
+bool SocketManager::getDisable(VNCServer* srvr)
{
std::map<HANDLE,ListenInfo>::iterator i;
for (i=listeners.begin(); i!=listeners.end(); i++) {
@@ -146,7 +150,7 @@ bool SocketManager::getDisable(network::SocketServer* srvr)
throw rdr::Exception("Listener not registered");
}
-void SocketManager::setDisable(network::SocketServer* srvr, bool disable)
+void SocketManager::setDisable(VNCServer* srvr, bool disable)
{
bool found = false;
std::map<HANDLE,ListenInfo>::iterator i;
diff --git a/win/rfb_win32/SocketManager.h b/win/rfb_win32/SocketManager.h
index e5ca02e6..809c470e 100644
--- a/win/rfb_win32/SocketManager.h
+++ b/win/rfb_win32/SocketManager.h
@@ -19,22 +19,28 @@
// -=- SocketManager.h
// Socket manager class for Win32.
-// Passed a network::SocketListener and a network::SocketServer when
+// Passed a network::SocketListener and a rfb::VNCServer when
// constructed. Uses WSAAsyncSelect to get notifications of network
// connection attempts. When an incoming connection is received,
-// the manager will call network::SocketServer::addClient(). If
+// the manager will call rfb::VNCServer::addClient(). If
// addClient returns true then the manager registers interest in
// network events on that socket, and calls
-// network::SocketServer::processSocketEvent().
+// rfb::VNCServer::processSocketEvent().
#ifndef __RFB_WIN32_SOCKET_MGR_H__
#define __RFB_WIN32_SOCKET_MGR_H__
#include <map>
-#include <network/Socket.h>
#include <rfb_win32/EventManager.h>
+namespace network {
+ class SocketListener;
+ class Socket;
+}
+
namespace rfb {
+ class VNCServer;
+
namespace win32 {
class SocketManager : public EventManager, EventHandler {
@@ -52,21 +58,21 @@ namespace rfb {
};
// Add a listening socket. Incoming connections will be added to the supplied
- // SocketServer.
+ // VNCServer.
void addListener(network::SocketListener* sock_,
- network::SocketServer* srvr,
+ VNCServer* srvr,
AddressChangeNotifier* acn = 0);
// Remove and delete a listening socket.
void remListener(network::SocketListener* sock);
// Add an already-connected socket. Socket events will cause the supplied
- // SocketServer to be called. The socket must ALREADY BE REGISTERED with
- // the SocketServer.
- void addSocket(network::Socket* sock_, network::SocketServer* srvr, bool outgoing=true);
+ // VNCServer to be called. The socket must ALREADY BE REGISTERED with
+ // the VNCServer.
+ void addSocket(network::Socket* sock_, VNCServer* srvr, bool outgoing=true);
- bool getDisable(network::SocketServer* srvr);
- void setDisable(network::SocketServer* srvr, bool disable);
+ bool getDisable(VNCServer* srvr);
+ void setDisable(VNCServer* srvr, bool disable);
protected:
virtual int checkTimeouts();
@@ -75,11 +81,11 @@ namespace rfb {
struct ConnInfo {
network::Socket* sock;
- network::SocketServer* server;
+ VNCServer* server;
};
struct ListenInfo {
network::SocketListener* sock;
- network::SocketServer* server;
+ VNCServer* server;
AddressChangeNotifier* notifier;
bool disable;
};
diff --git a/win/winvnc/ManagedListener.cxx b/win/winvnc/ManagedListener.cxx
index a93ca298..1a278678 100644
--- a/win/winvnc/ManagedListener.cxx
+++ b/win/winvnc/ManagedListener.cxx
@@ -45,7 +45,7 @@ ManagedListener::~ManagedListener() {
}
-void ManagedListener::setServer(network::SocketServer* svr) {
+void ManagedListener::setServer(rfb::VNCServer* svr) {
if (svr == server)
return;
vlog.info("set server to %p", svr);
diff --git a/win/winvnc/ManagedListener.h b/win/winvnc/ManagedListener.h
index 39223c79..20503c33 100644
--- a/win/winvnc/ManagedListener.h
+++ b/win/winvnc/ManagedListener.h
@@ -27,7 +27,7 @@ namespace winvnc {
// -=- ManagedListener
// Wrapper class which simplifies the management of a listening socket
- // on a specified port, attached to a SocketManager and SocketServer.
+ // on a specified port, attached to a SocketManager and VNCServer.
// Reopens sockets & reconfigures filters & callbacks as appropriate.
// Handles addition/removal of Listeners from SocketManager internally.
@@ -36,7 +36,7 @@ namespace winvnc {
ManagedListener(rfb::win32::SocketManager* mgr);
~ManagedListener();
- void setServer(network::SocketServer* svr);
+ void setServer(rfb::VNCServer* svr);
void setPort(int port, bool localOnly=false);
void setFilter(const char* filter);
void setAddressChangeNotifier(rfb::win32::SocketManager::AddressChangeNotifier* acn);
@@ -49,7 +49,7 @@ namespace winvnc {
network::TcpFilter* filter;
rfb::win32::SocketManager* manager;
rfb::win32::SocketManager::AddressChangeNotifier* addrChangeNotifier;
- network::SocketServer* server;
+ rfb::VNCServer* server;
int port;
bool localOnly;
}; |
|
Thanks for the suggestion. I will merge it to my code, test locally and update the PR. |
casantos
force-pushed
the
casantos-rhbz_2180903
branch
from
49f7434
to
91d48c8
Compare
|
OK, I updated the code. I still believe that the AccessRights type and the corresponding constants should belong to the rfb namespace, not to the SConnection class, but this change can be made in a subsequent PR. |
I agree. Maybe it should be included in this PR, though, as you are already moving those constants around. Annoying to have multiple moves, that makes it difficult to run |
Yes, but what would be the initial value of accessRights? Currently it's initialized to zero, so a binary AND will always return zero. |
|
Keep that part of your patch, where you can give the base access rights to |
I suspect I did not understanding your reasoning. Both VNCSConnectionST and VNCServerST are subclasses of SConnection, so it's necessary to pass accessRights as an argument to their constructors. What exactly do you believe I should not change in the security types? |
|
Indeed, |
Ah, it makes sense, now. Indeed the changes in |
casantos
force-pushed
the
casantos-rhbz_2180903
branch
from
91d48c8
to
4bfc5c6
Compare
unix/vncconfig/vncconfig.cxx
Outdated
| i++; | ||
| if (strcmp(argv[i], "-viewOnly") == 0 || | ||
| strcmp(argv[i], "-view-only") == 0) { |
There was a problem hiding this comment.
Sorry, I missed this the first rounds. But why two variants? And case-sensitive upper case seems a bit surprising.
There was a problem hiding this comment.
Yup, they are redundant. I must admit I forgot why I added both. It was probably because of some test script. I will change the code to use just -view-only.
common/rfb/SSecurity.cxx
Outdated
| @@ -25,7 +25,7 @@ | |||
| using namespace rfb; | |||
|
|
|||
| SSecurity::SSecurity(SConnection* sc) | |||
| : sc(sc), accessRights(AccessDefault) | |||
| : sc(sc), accessRights(sc->getAccessRights()) | |||
There was a problem hiding this comment.
Won't this get overridden by some security types?
Isn't it better to let the SSecurity derived classes keep their own accessRights, and then merge it with the one in SConnection after authentication?
There was a problem hiding this comment.
Do you mean, by the code in SConnection::processSecurityMsg()?
setAccessRights(ssecurity->getAccessRights());
I will need to review the code and run tests to confirm that it it would work with all security types.
There was a problem hiding this comment.
Indeed. It would probably be replaced with:
setAccessRights(accessRights & ssecurity->getAccessRights());That should merge the most restrictive set between the connection and security type rights.
|
Have you had time to look at the last two details, @casantos? I'm starting to think about making a release, and it would be fantastic if this could be included. |
Hello. Sorry, I got sick and could not finish my tests, last week. I'll resume working on this feature today. |
|
How are things progressing here, @casantos? :) |
casantos
force-pushed
the
casantos-rhbz_2180903
branch
from
4bfc5c6
to
02992ec
Compare
|
I removed the redundant “-viewOnly” argument from vncconfig. I also attempted to follow your suggestion to use and to leave |
|
That's odd. Works beautifully here. This is the change I applied to test: diff --git a/common/rfb/SConnection.cxx b/common/rfb/SConnection.cxx
index 94a5b9d64..7a930af56 100644
--- a/common/rfb/SConnection.cxx
+++ b/common/rfb/SConnection.cxx
@@ -242,7 +242,7 @@ bool SConnection::processSecurityMsg()
}
state_ = RFBSTATE_QUERYING;
- setAccessRights(ssecurity->getAccessRights());
+ setAccessRights(accessRights & ssecurity->getAccessRights());
queryConnection(ssecurity->getUserName());
// If the connection got approved right away then we can continue
diff --git a/common/rfb/SSecurity.cxx b/common/rfb/SSecurity.cxx
index 109882d34..1cc5c4dc0 100644
--- a/common/rfb/SSecurity.cxx
+++ b/common/rfb/SSecurity.cxx
@@ -25,7 +25,7 @@
using namespace rfb;
SSecurity::SSecurity(SConnection* sc)
- : sc(sc), accessRights(sc->getAccessRights())
+ : sc(sc), accessRights(AccessDefault)
{
} |
They must belong to the rfb namespace, not to the SConnection class. Also add an AccessNone constant, since it's better to use a mnemonic symbol rather than zero to initialize the accessRights members. Signed-off-by: Carlos Santos <casantos@redhat.com>
OK, so you are applying this after the second commit of the series, which introduced |
|
I tried that now as well, and seems to work fine. To be fully clear, what I did was:
|
Move these RFB specific things to rfb::VNCServer, for clarity. Signed-off-by: Pierre Ossman <ossman@cendio.se> Signed-off-by: Carlos Santos <casantos@redhat.com>
I followed the steps above (had to |
Specifies that the server must ignore all keyboard or mouse events sent by the client. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2180903 Signed-off-by: Carlos Santos <casantos@redhat.com>
casantos
force-pushed
the
casantos-rhbz_2180903
branch
from
02992ec
to
7597f8d
Compare
|
All merged! Thank you for working with us to get this done. :) |
Specifies that the server must ignore all keyboard or mouse events sent by the client.
Doing this trough SocketServer::addSocket() looks as a dirty trick. It probably requires some refining.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2180903