Two general comments: * This patch will probably break the windows build, as it adds an msg!=NULL assert without setting the variable on Windows. The best solution would be to add the windows implementation too.
* CSecurityTLS is only available, if tigervnc is built with GNUTLS. So it should break any non-GNUTLS build. I would msg to CSecurity to avoid many ifdefs. In the following, I'll assume, that msg has been move to CSecurity: > Index: unix/vncviewer/CConn.cxx > =================================================================== > --- unix/vncviewer/CConn.cxx (revision 4194) > +++ unix/vncviewer/CConn.cxx (working copy) > @@ -21,6 +21,9 @@ > // > > #include <unistd.h> > +#include <errno.h> > +#include <sys/stat.h> > +#include <sys/types.h> I can't spot any change in this file, requiring this. > Index: unix/vncviewer/vncviewer.cxx > =================================================================== > --- unix/vncviewer/vncviewer.cxx (revision 4194) > +++ unix/vncviewer/vncviewer.cxx (working copy) > @@ -33,6 +33,7 @@ > #include <locale.h> > #include <os/os.h> > #include <rfb/Logger_stdio.h> > +#include <rfb/CSecurityTLS.h> > #include <rfb/LogWriter.h> > #include <network/TcpSocket.h> > #include "TXWindow.h" > @@ -278,6 +279,8 @@ > "Copyright (C) 2004-2009 Peter Astrand for > Cendio AB\n" > "See http://www.tigervnc.org for information > on TigerVNC."); > > + rfb::CSecurityTLS::setDefaults(); > + > // Write about text to console, still using normal locale codeset > snprintf(aboutText, sizeof(aboutText), > gettext(englishAbout), PACKAGE_VERSION, buildtime); This will break non-GNUTLS builds. Just an idea: As the whole thing (with ifdefs) is needed on unix/windows server, why not add a SecurityClient::setDefaults: void SecurityClient::setDefaults() { #ifdef HAVE_GNUTLS CSecurityTLS::setDefaults(); #endif } So only SecurityClient/Server needs to know, if TLS is supported. > Index: common/rfb/CSecurityTLS.cxx > =================================================================== > --- common/rfb/CSecurityTLS.cxx (revision 4194) > +++ common/rfb/CSecurityTLS.cxx (working copy) > @@ -76,6 +82,24 @@ > crlfile = x509crl.getData(); > } > > +void CSecurityTLS::setDefaults() > +{ > + char* homeDir = NULL; > + > + if (strlen(x509ca.getData()) == 0) { Why didn't you just unconditionally change the default value, regardless of the user setting? This way, the default value should be consitent. > + if (gethomedir(&homeDir) == -1) > + vlog.error("Could not obtain home directory path, failed to open > ~/.vnc/x509_certs"); Wouldn't be the first part of the error message more meaningful: + vlog.error("Could not obtain home directory path"); > + else { > + CharArray caDefault(strlen(homeDir)+17); > + sprintf(caDefault.buf, "%s/.vnc/x509_certs", homeDir); > + delete [] homeDir; > + > + if (!access(caDefault.buf, R_OK)) > + x509ca.setDefaultStr(strdup(caDefault.buf)); else vlog.error("Failed to open ~/.vnc/x509_certs"); Question: Why do you need to do the access check? In my option, a different default (dependend on the existance of a file) is something unexpected. Is the access check necessary, because the TLS setup bails out, if there is no CA file? In this case, I would change the TLS setup, so that it handles a missing CA file like an empty CA file parameter. > + if (status & GNUTLS_CERT_SIGNER_NOT_FOUND) { > + size_t out_size; > + char *homeDir = NULL; > + char *out_buf; > + char *certinfo; > + int len = 0; > + > + vlog.debug("certificate issuer unknown"); > + > + len = snprintf(NULL, 0, "This certificate has been signed by an > unknown authority:\n\n%s\n\nDo you want to save it and continue?\n ", > info.data); > + if (len < 0) > + AuthFailureException("certificate decoding error"); > + > + vlog.debug("%s", info.data); > + > + certinfo = (char*) malloc(len); > + snprintf(certinfo, len, "This certificate has been signed by an > unknown authority:\n\n%s\n\nDo you want to save it and continue? ", > info.data); I would split the possibilty to accept and save. So you can first try out a server without having to manually delete a certificate from a file, if the server does not match the expectations. On the other hand, ssh does the same. Regards, Martin Kögler ------------------------------------------------------------------------------ Centralized Desktop Delivery: Dell and VMware Reference Architecture Simplifying enterprise desktop deployment and management using Dell EqualLogic storage and VMware View: A highly scalable, end-to-end client virtualization framework. Read more! http://p.sf.net/sfu/dell-eql-dev2dev _______________________________________________ Tigervnc-devel mailing list Tigervnc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
