[Privoxy-devel] HTTPS filtering in Privoxy

[Vašek Švec]

On Sat, May 20, 2017 at 1:37 PM, Fabian Keil <fk at fabiankeil.de> wrote:

> 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.
>

Ok.  I'll start editing the code (changes described in this emails) at the
beginning of July and I'll put it on git.

>
> > > - 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).
>

Sounds reasonable for me. I'll try to change it.

>
> 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.


Ok. In July I will try to write the documentation of this new action.

>
>
> > - 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?
>

My reviewer would like to test it on Turris Omnia, but I don't know the
result. A lot of mutexes could cause some difficulties on this router
probably.


> Did you run any tests to confirm that using multiple mutexes is
> required at all?
>

No, I didn't test it. There could be only one, but I think it would slow
down certificates generating. Without any mutex, invalid key and
certificate pairs may arise. (certificate would be for different key) There
is a lot of mutexes to create certificates for different hosts at the same
time.

>
> 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.
>

There is probably a possibility to reduce mutex protected area, but I has
created this solution to be sure of its flawlessness. I will try to reduce
this area as much as possible.

> > - 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.
>
> Ok.


> > > - 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.
>
> I understand now.


> > > - 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.
>

I will try to use it.

>
> > >
> > > 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.
>

I'll try to get URL.

Best regards
Václav Švec