On 10/27/2010 11:15 PM, Martin Koegler wrote: 1) > On Wed, Oct 27, 2010 at 02:36:46PM +0200, Guillaume Destuynder wrote:
> Basic question: Wouldn't a generic messagebox (like in > http://e9925248.users.sourceforge.net/0005-client-side-TLS-tunnel.patch > File common/rfb/UserMsgBox.h) be more useful? No difference for me, I did as recommended on the IRC channel. I can put it directly there, too. If there's already a patch for this however I don't mind if it's used instead, in fact, it would be great too. 2) > So it only stores the last used certificate. Users with more than one > vnc server can take no advantage, as the certificate is overwritten > again and again. My mistake, it should append instead. 3) > If you intend to use the last saved certificate by default, why do you > use two different constructions patterns for this file name (with one being > fixed)? That's for being mostly coherent with the defaults. The way VNC handle them is not really making this clear. If you do ./vncviewer -h and you get 'cafile default = ""' you would expect that it loads nothing. You would have to dynamically parse the home path just for this option to show the real default path, which is not very nice either. The ~ is not parsed by the shell unfortunately in this case, hence parsing it in VNC. Anyway, it's a bit of a hack (since using ~ you don't need to resolve the path to display), it can be replaced by an empty default and silently load the default certificates anyway (actually I also did that in another patch), it's just not very coherent to me. > Wishlist: Being able to ignore expired certificates via yes/no question. > Good idea ;) If the msgbox issue from 1) is sorted I don't mind adding this. 4) >> + if ((CSecurityTLS::ctd)->getCertificateReply(certinfo)) { >> + free(certinfo); >> + exit(1); > > I would throw an AuthFailureException here. I also asked this question in IRC and how they'd like it best, and it ended up being exit. The exception would display a generic dialog box in the client, so it would need a custom exception that is ignored by the client (or feel ok with having yet another dialog telling you that there was an exception). I don't mind with either solution, actually I don't like the exit from this part of the code too much. 5) >> + if ((CSecurityTLS::ctd)->saveCertificate(out_buf)) >> + AuthFailureException("certificate save failed"); > > I wouldn't abort here. Failing to save the certificate is not fatal for all > users. Point taken. With the msgbox issue again, in 1), solved, I can add a info msg here as well instead of failing. 6) > The vncDir mkdir code is the same as in main of vncviewer.cpp - maybe something could be merged. Yeah I suppose so. There's been a commit for that so I'm using it now. Going to check Atkac's feedback on this and implement whatever is requested for this feature (well, if this feature is going to be implemented via this patch that is) Thanks :) -- Guillaume Destuynder - m-privacy GmbH - Am Köllnischen Park 1, 10179 Berlin Tel: +49 30 24342334 Fax: +49 30 24342336 Web: http://www.m-privacy.de, http://oss.m-privacy.de Handelsregister: Amtsgericht Charlottenburg HRB 84946 Geschäftsführer: Dipl.-Kfm. Holger Maczkowsky, Roman Maczkowsky GnuPG-Key-ID: 0x3FB1D217 ------------------------------------------------------------------------------ The Next 800 Companies to Lead America's Growth: New Video Whitepaper David G. Thomson, author of the best-selling book "Blueprint to a Billion" shares his insights and actions to help propel your business during the next growth cycle. Listen Now! http://p.sf.net/sfu/SAP-dev2dev _______________________________________________ Tigervnc-devel mailing list Tigervnc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
