On Thu, Nov 11, 2010 at 10:51:46PM +0100, Martin Koegler wrote: > Two general comments:
Thanks for your comments, I've modified the patch and commited it as r4198. > * 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. I didn't remove the assertion for now so vncviewer on Windows is temporarily broken. I will implement showMsgBox and everything will be fine after that. > * 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. I didn't modified original logic so currently vncviewer does it same way as ssh. Regards, Adam -- Adam Tkac, Red Hat, Inc. ------------------------------------------------------------------------------ Beautiful is writing same markup. Internet Explorer 9 supports standards for HTML5, CSS3, SVG 1.1, ECMAScript5, and DOM L2 & L3. Spend less time writing and rewriting code and more time creating great experiences on the web. Be a part of the beta today http://p.sf.net/sfu/msIE9-sfdev2dev _______________________________________________ Tigervnc-devel mailing list Tigervnc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
