[Privoxy-devel] HTTPS filtering in Privoxy

[Fabian Keil]

Vašek Švec <va.svec at gmail.com> wrote (quoting adjusted):

> On Thu, May 18, 2017 at 2:22 PM, Fabian Keil <fk at fabiankeil.de> wrote:
> 
> > Vašek Švec <va.svec at gmail.com> wrote:
> >  
> > > file with patches (git format-patch) is in attachment. I hereby
> > > release this code under the GNU GPLv2 or later.  
> >
> > Thanks a lot for the patch.
> >
> > Unfortunately Mailman stripped it due to the mime type
> > (application/octet-stream) but I received it as part of the
> > moderation message an will attach it (renamed) to this mail
> > so other list subscribers can take a look at it as well.
> >
> > A couple of comments after looking at the patch without
> > testing it yet:

> I agree with most of yours comments. However I'm just learning for my  
> final exams and I don't have enough time to work on this codes or
> documentation. From the end of June, I should have more free time and
> then I'd like to continue this work. It would be quicker if these
> changes were made by someone else.

There is no time pressure. Assuming the changes required to
commit the code to CVS are made in July they should make it
into the next release.

By then we may even have migrated to git which would allow
better attribution.
 
> > - The commit message "final change" is meaningless and should
> >   be changed to reflect what the commit does.  
> 
> I wasn't using VCS for changes. This commit was created yesterday for
> purpose of this diff file.

I see. I recommend that you continue to use git when working on this.

> > - The patch adds two separate boolean actions (disable-ssl-filtering
> >   and ignore-certificate-errors) that probably shouldn't be separated.
> >
> >   A multi action like filter-encrypted-content with a parameter that
> >   specifies whether or not certificates should be validates seems more
> >   appropriate to me.
> >  
> 
> Maybe it would be better solution. I didn't realize that these parameters
> could/should be merged.

The advantage I see is that it seems more intuitive (to me) and would
prevent invalid combinations like
(-disable-ssl-filtering -ignore-certificate-errors).

As a bonus it would require one bit less in the action bit field.

> > - The added action(s) need documentation (in
> > doc/source/user-manual.sgml). 
> 
> Yes, it definitely should be, but I didn't change anything in
> documentation for now. I'm not really sure, if my English is good enough
> to be published in documentation.

Documentation doesn't have to be perfect to be useful and we
can review and improve it before the release.

> > - Chunks like this look dubious to me:
> >
> > @@ -3256,6 +3743,20 @@ static void initialize_mutexes(void)
> >     /*
> >      * Prepare global mutex semaphores
> >      */
> > +
> > +   /*Added by Vaclav Svec*/
> > +#ifdef LIMIT_MUTEX_NUMBER
> > +   int i = 0;
> > +   for (i = 0; i < 32; i++)
> > +#else
> > +   int i = 0;
> > +   for (i = 0; i < 65536; i++)
> > +#endif /* LIMIT_MUTEX_NUMBER */
> >
> > It's not obvious to me why 32 and 65536 are used here.
> >
> > It seems preferable to me to use a single define for the
> > number of certificates_mutexes[] slots and use sizeof or
> > the define for the loop boundaries.  
> 
> 
> I used these two values to simplify the calculation of used mutex number.
> (in function get_certificate_mutex_id) The number is calculated from
> bytes of host hash.  65536 - first two bytes of hash, and 32 is fallback
> for platform which must save mutexes count.

Did you come across such platforms?
Did you run any tests to confirm that using multiple mutexes is
required at all?

My impression is that only a small part of generate_webpage_certificate()
needs mutex protection anyway. Reducing the protected area should reduce
the mutex contention and seems preferable to increasing the number of
mutexes.

> > - The copyright header in ssl.c and ssl.h is unclear.
> >   Is "FIT CVUT" supposed to be a copyright holder?
> >  
> 
> I'm implementation author and FIT CVUT is there only mentioned to
> emphasize that I'm a student of this faculty (and university). FIT CVUT
> isn't a copyright holder.

Great. 
 
> > - create_server_ssl_connection() seems to parse the CA
> >   file for each connection which seems wasteful. Probably
> >   the file content should be cached and only reloaded when
> >   the file changed.
> >  
> 
> I agree, but I'm not able to implement this feature right now. I'm also
> not sure how could be this functionality implemented.

This isn't critical and could be done after the integration.

> > - Many functions in ssl.c only need a small part of csp
> >   in which case they shouldn't get access to the whole
> >   struct.
> >  
> 
> Yes. Maybe there could be created new structure (maybe as part of csp)
> with necessary parameters for SSL connections.

Looks like my comment was unclear. I was referring to functions
like get_certificate_serial() which gets passed a pointer to the
whole client_state structure when it only needs to access
csp->http->hash_of_host[].

Adjusting the function prototype would be sufficient to address
this.

This isn't critical for the integration either.

> > - The code has to be rebased on CVS HEAD before it can
> >   be committed.
> >  
> 
> I don't have many experiences with CVS, therefore I didn't use it.

The rebase could be done with git.

> > Do you have time and interest to address these change
> > requests or a subset of them and do they seem reasonable
> > to you?
> >  
> 
> These days, I don't have enough time to work on these changes
> (approximately next one and half month). In future, I'd like to work on
> these changes or new features for SSL.

Great.

> >
> > Is the bachelor thesis public?
> >  
> 
> Yes, it's public, but it's written in Czech. I send it as an attachment
> (because I don't know URL for public).

Thanks. I don't understand Czech but assuming it eventually gets
a stable URL we could reference it in the documentation.

Fabian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.privoxy.org/pipermail/privoxy-devel/attachments/20170520/756d5afe/attachment.bin>