Skip to content
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

X509 client authentication #787 #1842

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

opentissandy
Copy link

Add feature: X509 client authentication.

Copy link
Member

@CendioOssman CendioOssman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! This looks nice and clean.

But there are some user experience issues I think we need to discuss and resolve before we can merge this.

common/rfb/CSecurityTLS.cxx Outdated Show resolved Hide resolved
common/rfb/CSecurityTLS.cxx Outdated Show resolved Hide resolved
vncviewer/vncviewer.man Show resolved Hide resolved
@@ -281,6 +287,9 @@ void CSecurityTLS::setParam()
if (gnutls_certificate_set_x509_crl_file(cert_cred, X509CRL, GNUTLS_X509_FMT_PEM) < 0)
vlog.error("Could not load user specified certificate revocation list");

if (gnutls_certificate_set_x509_key_file (cert_cred, X509CERT, X509KEY, GNUTLS_X509_FMT_PEM) < 0)
vlog.error("Could not load user specified client certificate");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we need to have a bit more explicit error handling. What is the user experience if there is some issue with the certificate or key file?

Comment on lines +735 to +748

ty += INPUT_LABEL_OFFSET;
certInput = new Fl_Input(tx + INDENT, ty,
width - INDENT * 2, INPUT_HEIGHT,
_("Path to X509 client certificate"));
certInput->align(FL_ALIGN_LEFT | FL_ALIGN_TOP);
ty += INPUT_HEIGHT + TIGHT_MARGIN;

ty += INPUT_LABEL_OFFSET;
keyInput = new Fl_Input(tx + INDENT, ty,
width - INDENT * 2, INPUT_HEIGHT,
_("Path to X509 client private key"));
keyInput->align(FL_ALIGN_LEFT | FL_ALIGN_TOP);
ty += INPUT_HEIGHT + TIGHT_MARGIN;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are authentication settings, not encryption. So I think they fit better in the other section.

@CendioOssman
Copy link
Member

These changes are just for the client. What server have you used for this?

@opentissandy
Copy link
Author

opentissandy commented Oct 8, 2024

Thanks reviewed by @CendioOssman. I try to fixed the code.

I deployed libvirt/QEMU server with Client Certificate VNC based on https://wiki.libvirt.org/VNCTLSSetup.html. I use tigervnc/vncviewer with the Client Certificate version several weeks, that's fine and useful. And now I removed Vinagre/remmina from my openSUSE.

I like use tigervnc nor Vinagre/remmina. So I try to find the way to remove tigervnc from "VNC clients known to NOT work with TLS" completely. Then I find tigervnc Framework is so flexible and gnutls api is simply. In fact ONLY use one gnutls_certificate_set_x509_key_file API to add the last feature to support client TLS fully.

I find remmina UI with CA and client certificate in one page. So I agree move these to new section. But I have no ability to do so big UI change. Can you make a planning? @CendioOssman.
Screenshot_20241008_213637

@CendioOssman
Copy link
Member

Thanks for the link. So it seems QEMU has had this behaviour for a while, which makes things more problematic.

Which means I'm even more keen on getting some insight from @berrange about these design choices.

Effectively, this means double authentication, which is not the norm in VNC.

@berrange
Copy link

berrange commented Oct 9, 2024

I deployed libvirt/QEMU server with Client Certificate VNC based on https://wiki.libvirt.org/VNCTLSSetup.html.
Thanks for the link. So it seems QEMU has had this behaviour for a while, which makes things more problematic.

FYI, on the QEMU side the following is our most up2date docs

https://www.qemu.org/docs/master/system/vnc-security.html

Effectively, this means double authentication, which is not the norm in VNC.

What do you mean by double authentication here ? Are you referring to the way VeNCrypt lets you run nested "VNC" or "SASL" auth, after first negotiating the outer auth ?

As a general design concept, the way VeNCrypt auth was designed is kind of frowned upon as a way for doing TLS upgrades on a live connection. Best practice design these days would be to either

  • Mandate TLS from the very moment the connection is opening, with no negotiation
  • Support a simple "STARTTLS" mechanism, which negotiates TLS, and then restarts the entire VNC protocol handshake from the beginning, but under TLS.

In both of these approaches, the client would not know a priori whether the server is expecting it to present a client cert - it just has to be ready present one if asked & one is available.

Anyway, one day I would like to define a "STARTTLS" auth scheme for VNC as a cleaner & simpler way to upgrade to TLS in VNC.

@opentissandy
Copy link
Author

I don't know the VNC or TLS technology details. But I can use "With x509 certificates, client verification and passwords" of QEMU (https://www.qemu.org/docs/master/system/vnc-security.html) with my simply patch code. So I share my idea.

If the project tigervnc leaders can promote the feature moving to better with general effectively design, that is fortunate of users like me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants