New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Channel security #234
base: master
Are you sure you want to change the base?
Channel security #234
Conversation
Thanks! I'll try to review on Friday. In the meantime, can you have a look at the CI build failure for Windows? |
@bk138 Thanks! The error is
which I don't really understand the meaning of. Does it have anything to do with API stability? What is unclear to me is how API stability is dealt with. Both patches changes the rfb.h requiring recompilation of users (new vfuncs, new state enum value). It could be avoided by adding the new struct fields/state enums in the end I suppose, but as far as I could tell (and I might very well be wrong), things have been added without such a concern in the past, and compilation flags might change the struct field placement as well. |
That's a pretty big change that needs a while to digest ;-) Before getting into the details, I'd like to understand a bit more the bigger picture: So this kinda splits up the usual RFB session setup into a twofold process where one first sets up "channel" security and then afterwards ordinary RFB security type? Isn' that very similar to what https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#tight-security-type already does? I've the feeling this would'nt work with standard VNC clients as there's a two stage sectype setup? I might be wrong with above assumptions, TBH, I've to dive into the proto spec again to better understand what you're trying to achieve. |
Right, first set up the channel, then if that is successful (client supports a security channel negotiation type), continue with the authorization. This is how Vino adds TLS support, and it is this what I'm trying to reimplement without using Vino's LibVNCServer fork.
Seems somewhat similar indeed, and would be good to have. What I'm initially targeting, however, is to support what ever security mechanism that Vino supports. I have relied on reading source code for this though, as I've not find any documentation of how the TLS security type (18) should work.
My assumption is that any client that worked with Vino with TLS encryption enabled (which is the default) should support this. I've so far successfully tested with Vinagre and Remmina. |
Thanks for the explanation. This is just to let you know I haven't forgotten about this, I need some time for thinking and digging up info on sectype 18. |
@jadahl can you explain what you're aiming for on a user perspective level? I reckon it's
|
Yes, or, all clients that worked when using encryption in Vino. I don't know what other clients supports security type 18. |
FYI I started a bit of work on unifying encryption and crypto between LibVNCServer and LibVNCClient. As Remmina is using LibVNCClient (dunno about vinagre?), I'll try to model basic server encryption based on the client counterpart that already exists. The code in your PR here will be quite helpful I guess. |
@bk138 any update on this? Any way I can implement anon-tls and other channel security using e.g. rfbTLS? (to keep backward compatibility with the clients currently supported). |
79738f4
to
5c25f49
Compare
Fixed a couple of bugs in this PR. Any update on getting it merged, or anything that would make it possible to implement the same level of channel security out-of-tree? |
Replying to an earlier question regarding client support: Vinagre and GNOME Boxes both use gtk-vnc as a client side library. |
Thank you, @jadahl ! Currently, simply lacking time as day job consumes too much. Hopefully over the holidays... |
Can this be rebased to resolve conflicts please? |
Add API to make it possible to channel RFB input and output through another layer, for example TLS. This is done by making it possible to override the default read/write/peek functions.
Add another type of security handler that is meant to be used initially to set up a secure channel. Regular security handlers would be advertised and processed after any channel security have succeeded. For example, this, together with the custom I/O functions allows a LibVNCServer user to implement TLS in combination with VNCAuth. This is done by adding a single channel security handler with the rfbTLS (18) with a handler that initiates a TLS session, and when a TLS session is initiated, the regular security handler list is sent.
5c25f49
to
30b947d
Compare
Rebased (but untested). |
I still have this on my TODO list, thanks @jadahl ! |
Any news about this PR? |
int | ||
rfbPeekAtSocket(rfbClientPtr cl, char *buf, int len) | ||
{ | ||
cl->peekAtSocket(cl, buf, len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is causing compilation error when buiding with -Werror=return-type
I assume it should be return cl->peekAtSocket(cl, buf, len);
Seems ok? |
|
||
ClientReadFromSocket readFromSocket; /* Read data from socket */ | ||
ClientPeekAtSocket peekAtSocket; /* Peek at data from socket */ | ||
ClientHasPendingOnSocket hasPendingOnSocket; /* Peek at data from socket */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a minor copy&paste error in the comment.
|
||
if (cl->protocolMajorVersion==3 && cl->protocolMinorVersion < 7) | ||
{ | ||
/* Make sure we use only RFB 3.3 compatible security types. */ | ||
if (channelSecurityHandlers) { | ||
rfbLog("VNC channel security enabled - RFB 3.3 client rejected\n"); | ||
rfbClientConnFailed(cl, "Your viewer cannot hnadler required " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: hnadler -> handle
Hi,
In GNOME Remote Desktop I'm using LibVNCServer to add VNC support. Currently I see no way to add my own encryption layer (I currently want to add TLS-ANON to begin with), so I have come with a couple of API additions allowing me to add it.
This allows me to replace read with the TLS receive function etc, in a similar way as of how WebSockets TLS have been implemented. What I currently do is to first use the default socket I/O functions, then after a secure channel is set up, I override the I/O functions with ones that use the TLS library.
A "channel" security handler here is simply a security handler that LibVNCServer already has, but that is advertised prior to any other security handler. This allows me to require using rfbTLS before continuing with VNC-AUTH etc. It works more or less by first advertising only channel security handlers (e.g. could be only rfbTLS but any other that sets up a secure channel). When that is done, the channel security handler will send the security handler list excluding the channel security handlers.
With these two additions, I can get a flow looking like this: