Re: [Evolution-hackers] Preliminary patches to add inline PGP support

[Not Zed <notzed ximian com>]

On Tue, 2005-04-05 at 14:11 -0400, Jeffrey Stedfast wrote:
> camel patch comments:
>
> -gpg_verify (CamelCipherContext *context, CamelMimePart *ipart,
> CamelException *ex)
> +gpg_doverify (CamelCipherContext *context, CamelStream *istream, CamelMimePart *sigpart, CamelException *ex)
>
> maybe just "verify"? I'm not a big fan of doverify

I'm totally not a fan of having one function called in one place at the end of another function which is only called in one place, either.

i.e. adding a new subroutine is redundant in this case, i'd just put all the code in gpg_verify, all it is is a slight re-order of the existing code anyway.

As for the processor code - it isn't particularly elegant, relies on brute-force and bulky handling, etc.  As much code as possible is implemented using stream processes.  These are harder to write but are more efficient.

i.e. you write the whole block to a memory stream just to scan it - then you write almost the same data all over again to another separate memory block just to scan it again.

Other things which could be done better:

+	
+	d(printf("\tCopying from %x for %d chars into mime-part\n", in, len);)
+	g_byte_array_append(buf, in, len);
+	
+	/* Create a mime part from the byte array */
+	part = createpart(buf, "text/plain");

This stuff goes and copies a memory buffer into another memory buffer, then createpart does the same thing all over again.  It would be much more efficient just to create a streammem and write to it.

You could even get 'tricky' and use a 'seekable substream' on the one original memory stream - but these are kind of clunky and may not work right in all situations.


So essentially, I would envisage it working a little differently (mainly where things are done, not how)

a scanner which looks for the armoured parts.
a formatter which handles that type of part.
do more streaming.

Not to say there are real 'practical' problems with the patch, I think it could be better :)

Also, since you do have control over all of it, you could perhaps do the camel stuff too from the plugin itself, but i guess there is a lot of extra work in doing that if you can't re-use the gpg-context as is.  Its just that if you're going to have to make code changes to core camel outside of the plugin, there might not be much point in doing it as a plugin, since the inline scanner already available could be modified only slightly to do most of the work required, and just add a new x-inline-pgp handler for the other glue (similar to the multipart/signed handler, etc).


> +	// XXX: Hash only seems to be used for signing... 
> +	//gpg_ctx_set_hash (gpg, camel_cipher_id_to_hash(context, camel_content_type_param(ct, "micalg")));
>
> don't use c++ style commenting... bad. anyway, the above code doesn't
> hurt (it doesn't do anything with the data, but maybe someday it could)
> doesn't really matter, could probably just be removed.
>
> +	if (sigfile) {
> +		gpg_ctx_set_sigfile (gpg, sigfile);
> +	}
>
> if it's just 1 statement under the if (and no comment), don't bother
> with the {}'s
>
> +		}
> +
> +	} else if (camel_content_type_is(ct, "application", "x-inline-pgp")) {
>
> I'd prefer the extra blank line not be there. same with a few other
> places in the code (and after the return statement you did the same
> thing)
>
> the code looks fine tho... the only thing I'd question is the faking of
> a MIME part to do the verification of non-MIME pgp stuff. I'm undecided
> about that :\
>
>
> plugin comments:
>
> you do a lot of unnecessary casting. for example:
>
> +	inptr = (char *)data->data;
> +	inend = inptr+(int)data->len;
>
> neither of those is needed.
>
> nor is this:
>
> +				g_byte_array_append(out,(guint8*)"-", 1);
>
>
> Jeff
>
> On Sun, 2005-04-03 at 03:20 +1200, Matt Brown wrote:
> > Hi, 
> > 
> > I've been working on some patches for this bounty (Bug #127521) for the
> > past couple of weeks. 
> > 
> > Given the high level of interest I've received in these patches and the
> > number of people emailing me, I am pleased to report that I have some
> > preliminary patches that I think are working well enough to make them
> > available for testing. 
> > 
> > 1) Modify the gpg_verify routine in camel-gpg-context.c to support
> > verifying application/inline-pgp mime parts. Clearsigned verifications
> > are different to PGP/Mime verifications so we can't just make a fake
> > multipart/signed unfortunately.
> > http://www.mattb.net.nz/evolution/eds-camel-inline-pgp.patch
> > 
> > 2) This patch is the actual plugin that hooks the formatting handlers to
> > detect and process inline pgp messages. 
> > http://www.mattb.net.nz/evolution/evo-plugin-inline-pgp.patch
> > 
> > I have to stress that both these patches are BETA and have passed only
> > the most preliminary testing so far, and there are a few known issues.
> > 
> > Known Issues
> > 1) The first 10 characters of the decrypted text are truncated when
> > processing an encrypted block. I'm really stumped on this one!
> > 2) Encrypted binary data (JPGs etc) will probably be displayed as
> > text/plain atm
> > 
> > Many thanks to Tommi Komulainen who contributed the code for
> > constructing a multipart/encrypted mime part.
> > 
> > 
> > Please test these patches and report back what works / what doesn't also
> > if you have weird and wacky examples of inline pgp messages (whether
> > valid or invalid) please email them to me at pgptest mattb net nz,
> > please include in the subject a short description of what the message is
> > (ie. "encrypted jpeg" or "invalid inline signature") 
> > 
> > Looking forward to your feedback :)
> > 
> > Regards
> >