[PATCH] PublicInbox::Import Extend add with a optional raw message parameter

[PATCH] PublicInbox::Import Extend add with a optional raw message parameter

[Eric W. Biederman]

I don't trust the MIME type to not munge my email messages in horrible
ways upon occasion.  Therefore  allow for passing in the raw message
value instead of trusting the mime object to preserve it.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---

The context here is because the only copy of messages that I save
I save in public-inbox I don't want to have to worry about losing
information.  So I just pass the raw email_str to add.

I expect if I were to export these lists public I would want to do
some more but for now I am just putting them in public-inbox
so that I can read and archive the lists locally.

 lib/PublicInbox/Import.pm     | 10 +++++-----
 lib/PublicInbox/V2Writable.pm |  8 ++++----
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/lib/PublicInbox/Import.pm b/lib/PublicInbox/Import.pm
index 81a38fb6987d..0a63784414f2 100644
--- a/lib/PublicInbox/Import.pm
+++ b/lib/PublicInbox/Import.pm
@@ -359,7 +359,7 @@ sub clean_tree_v2 ($$$) {
 # returns undef on duplicate
 # returns the :MARK of the most recent commit
 sub add {
-	my ($self, $mime, $check_cb) = @_; # mime = Email::MIME
+	my ($self, $mime, $check_cb, $email_str) = @_; # mime = Email::MIME
 
 	my ($name, $email) = extract_author_info($mime);
 	my $hdr = $mime->header_obj;
@@ -396,16 +396,16 @@ sub add {
 	}
 
 	my $blob = $self->{mark}++;
-	my $str = $mime->as_string;
-	my $n = length($str);
+	$email_str ||= $mime->as_string;
+	my $n = length($email_str);
 	$self->{bytes_added} += $n;
 	print $w "blob\nmark :$blob\ndata ", $n, "\n" or wfail;
-	print $w $str, "\n" or wfail;
+	print $w $email_str, "\n" or wfail;
 
 	# v2: we need this for Xapian
 	if ($self->{want_object_info}) {
 		my $oid = $self->get_mark(":$blob");
-		$self->{last_object} = [ $oid, $n, \$str ];
+		$self->{last_object} = [ $oid, $n, \$email_str ];
 	}
 	my $ref = $self->{ref};
 	my $commit = $self->{mark}++;
diff --git a/lib/PublicInbox/V2Writable.pm b/lib/PublicInbox/V2Writable.pm
index afcac4d2db86..713d3817cc59 100644
--- a/lib/PublicInbox/V2Writable.pm
+++ b/lib/PublicInbox/V2Writable.pm
@@ -109,14 +109,14 @@ sub init_inbox {
 # returns undef on duplicate or spam
 # mimics Import::add and wraps it for v2
 sub add {
-	my ($self, $mime, $check_cb) = @_;
+	my ($self, $mime, $check_cb, $email_str) = @_;
 	$self->{-inbox}->with_umask(sub {
-		_add($self, $mime, $check_cb)
+		_add($self, $mime, $check_cb, $email_str)
 	});
 }
 
 sub _add {
-	my ($self, $mime, $check_cb) = @_;
+	my ($self, $mime, $check_cb, $email_str) = @_;
 
 	# spam check:
 	if ($check_cb) {
@@ -135,7 +135,7 @@ sub _add {
 	defined $num or return; # duplicate
 	defined $mid0 or die "BUG: $mid0 undefined\n";
 	my $im = $self->importer;
-	my $cmt = $im->add($mime);
+	my $cmt = $im->add($mime, undef, $email_str);
 	$cmt = $im->get_mark($cmt);
 	$self->{last_commit}->[$self->{epoch_max}] = $cmt;
 
-- 
2.17.1

Re: [PATCH] PublicInbox::Import Extend add with a optional raw message parameter

[Eric Wong]

"Eric W. Biederman" <ebiederm@xmission.com> wrote:
> 
> I don't trust the MIME type to not munge my email messages in horrible
> ways upon occasion.  Therefore  allow for passing in the raw message
> value instead of trusting the mime object to preserve it.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

I've had the same concern in the past about Email::MIME and
Email::Simple.  But after reading the code for Email::MIME,
Email::Simple and Email::{MIME,Simple}::Header, I don't think
the implementation of Email::MIME->as_string and all methods it
calls does anything unreasonable.

The only notable munging they seem to do is make irrelevant
whitespace changes in headers and maybe fix quoting in headers.
No body changes AFAIK.

> The context here is because the only copy of messages that I save
> I save in public-inbox I don't want to have to worry about losing
> information.  So I just pass the raw email_str to add.
> 
> I expect if I were to export these lists public I would want to do
> some more but for now I am just putting them in public-inbox
> so that I can read and archive the lists locally.

I worry about public archives getting badly munged, too.

>  lib/PublicInbox/Import.pm     | 10 +++++-----
>  lib/PublicInbox/V2Writable.pm |  8 ++++----
>  2 files changed, 9 insertions(+), 9 deletions(-)

Did you have plans to modify -mda/-watch or another script to
use this?

> diff --git a/lib/PublicInbox/Import.pm b/lib/PublicInbox/Import.pm
> index 81a38fb6987d..0a63784414f2 100644
> --- a/lib/PublicInbox/Import.pm
> +++ b/lib/PublicInbox/Import.pm
> @@ -359,7 +359,7 @@ sub clean_tree_v2 ($$$) {
>  # returns undef on duplicate
>  # returns the :MARK of the most recent commit
>  sub add {
> -	my ($self, $mime, $check_cb) = @_; # mime = Email::MIME
> +	my ($self, $mime, $check_cb, $email_str) = @_; # mime = Email::MIME

I usually place callback args at the end of the arg list so
it's easy to write:

	$im->add($mime, sub {
		# ...
	});

So having a parameter after the sub{} is a bit ugly...
If I had to support this, I think I'd accept $mime being
a plain hashref:

	if (ref($mime) eq 'HASH') {
		$raw = $mime->{raw};
		$mime = $mime->{mime};
	} else {
		$raw = $mime->as_string;
	}

But, I'm still on the fence about the idea...

Side note: I'm also taking the opportunity to use "$raw" instead
of "$str", because I've been bitten by the difference header_raw
vs header_str in the Email::MIME API, so consistency with
that API would be good, here.

Re: [PATCH] PublicInbox::Import Extend add with a optional raw message parameter

[Eric W. Biederman]

Eric Wong <e@80x24.org> writes:

> "Eric W. Biederman" <ebiederm@xmission.com> wrote:
>> 
>> I don't trust the MIME type to not munge my email messages in horrible
>> ways upon occasion.  Therefore  allow for passing in the raw message
>> value instead of trusting the mime object to preserve it.
>> 
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> I've had the same concern in the past about Email::MIME and
> Email::Simple.  But after reading the code for Email::MIME,
> Email::Simple and Email::{MIME,Simple}::Header, I don't think
> the implementation of Email::MIME->as_string and all methods it
> calls does anything unreasonable.
>
> The only notable munging they seem to do is make irrelevant
> whitespace changes in headers and maybe fix quoting in headers.
> No body changes AFAIK.

I was hoping I could skip the Email::MIME stuff entirely but the headers
do need a bit of parsing to derive the git commit information.

I will take a second look at the code.  I did not make it all of the
way through last time.  I just know the bazillions of warnings I could
not easily track down with old emails did not make me comfortable
with that code base as anything other than a suggestion.

>> The context here is because the only copy of messages that I save
>> I save in public-inbox I don't want to have to worry about losing
>> information.  So I just pass the raw email_str to add.
>> 
>> I expect if I were to export these lists public I would want to do
>> some more but for now I am just putting them in public-inbox
>> so that I can read and archive the lists locally.
>
> I worry about public archives getting badly munged, too.
>
>>  lib/PublicInbox/Import.pm     | 10 +++++-----
>>  lib/PublicInbox/V2Writable.pm |  8 ++++----
>>  2 files changed, 9 insertions(+), 9 deletions(-)
>
> Did you have plans to modify -mda/-watch or another script to
> use this?

I have been using this for a while with my own imap fetcher
script.  As for the others I certain could.  I don't use -mda
or -watch so they have not been a priority.

>> diff --git a/lib/PublicInbox/Import.pm b/lib/PublicInbox/Import.pm
>> index 81a38fb6987d..0a63784414f2 100644
>> --- a/lib/PublicInbox/Import.pm
>> +++ b/lib/PublicInbox/Import.pm
>> @@ -359,7 +359,7 @@ sub clean_tree_v2 ($$$) {
>>  # returns undef on duplicate
>>  # returns the :MARK of the most recent commit
>>  sub add {
>> -	my ($self, $mime, $check_cb) = @_; # mime = Email::MIME
>> +	my ($self, $mime, $check_cb, $email_str) = @_; # mime = Email::MIME
>
> I usually place callback args at the end of the arg list so
> it's easy to write:
>
> 	$im->add($mime, sub {
> 		# ...
> 	});
>
> So having a parameter after the sub{} is a bit ugly...
> If I had to support this, I think I'd accept $mime being
> a plain hashref:
>
> 	if (ref($mime) eq 'HASH') {
> 		$raw = $mime->{raw};
> 		$mime = $mime->{mime};
> 	} else {
> 		$raw = $mime->as_string;
> 	}
>
> But, I'm still on the fence about the idea...
>
> Side note: I'm also taking the opportunity to use "$raw" instead
> of "$str", because I've been bitten by the difference header_raw
> vs header_str in the Email::MIME API, so consistency with
> that API would be good, here.

Yes the distinction about "$raw" is fair,
and the placement in the argument list makes sense.

As for the hashref.  Perhaps what I should do is modify
PublicInbox::MIME to at least conditionally keep the original raw email
around.  Then the logic to get the raw email could be kept in
PublicInbox::MIME as well.

Which let's me think the general solution is to have a configuration
option somewhere that says we want to archive the raw email.  We
update PublicInbox::MIME to always keep the original raw email.

If add is not configured to drop any headers we use the raw original
email str.  If add drops any headers we do what we do today.

I think perhaps we could move all of the scrubbing into
PublicInbox::Filter::Base.pm::scrub.  Instead of having a hard coded
drop_unwanted_headers in PublicInbox::Import.  That would make it very
straight forward to just make this a knob that the user controls
for how they want their email received/imported.

If that general idea sounds palatable I will investigate to see
if I can move the caching of the raw email into PublicInbox::MIME,
see about moving the dropping of headers into an appropriate config
knob.

Eric

Re: [PATCH] PublicInbox::Import Extend add with a optional raw message parameter

[Eric Wong]

"Eric W. Biederman" <ebiederm@xmission.com> wrote:
> Eric Wong <e@80x24.org> writes:
> 
> > "Eric W. Biederman" <ebiederm@xmission.com> wrote:
> >> 
> >> I don't trust the MIME type to not munge my email messages in horrible
> >> ways upon occasion.  Therefore  allow for passing in the raw message
> >> value instead of trusting the mime object to preserve it.
> >> 
> >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> >
> > I've had the same concern in the past about Email::MIME and
> > Email::Simple.  But after reading the code for Email::MIME,
> > Email::Simple and Email::{MIME,Simple}::Header, I don't think
> > the implementation of Email::MIME->as_string and all methods it
> > calls does anything unreasonable.
> >
> > The only notable munging they seem to do is make irrelevant
> > whitespace changes in headers and maybe fix quoting in headers.
> > No body changes AFAIK.
> 
> I was hoping I could skip the Email::MIME stuff entirely but the headers
> do need a bit of parsing to derive the git commit information.

Perhaps parsing git commit information can be skipped, entirely
(but it's still necessary for indexing, so I'm not sure if there's
 any gain, there).

> I will take a second look at the code.  I did not make it all of the
> way through last time.  I just know the bazillions of warnings I could
> not easily track down with old emails did not make me comfortable
> with that code base as anything other than a suggestion.

Ah, I added ce18b29d175ef5f01f05d59c95bcf8e0cd40e611
("index: warn with info about the message as context") exactly
for that.

> >> The context here is because the only copy of messages that I save
> >> I save in public-inbox I don't want to have to worry about losing
> >> information.  So I just pass the raw email_str to add.
> >> 
> >> I expect if I were to export these lists public I would want to do
> >> some more but for now I am just putting them in public-inbox
> >> so that I can read and archive the lists locally.
> >
> > I worry about public archives getting badly munged, too.
> >
> >>  lib/PublicInbox/Import.pm     | 10 +++++-----
> >>  lib/PublicInbox/V2Writable.pm |  8 ++++----
> >>  2 files changed, 9 insertions(+), 9 deletions(-)
> >
> > Did you have plans to modify -mda/-watch or another script to
> > use this?
> 
> I have been using this for a while with my own imap fetcher
> script.  As for the others I certain could.  I don't use -mda
> or -watch so they have not been a priority.

OK.  Tangent: which IMAP module(s) did you choose? (and why?)
I haven't gotten a chance to evaluate Perl IMAP client modules,
yet; but I want to extend -watch to support IMAP.

> >> diff --git a/lib/PublicInbox/Import.pm b/lib/PublicInbox/Import.pm
> >> index 81a38fb6987d..0a63784414f2 100644
> >> --- a/lib/PublicInbox/Import.pm
> >> +++ b/lib/PublicInbox/Import.pm
> >> @@ -359,7 +359,7 @@ sub clean_tree_v2 ($$$) {
> >>  # returns undef on duplicate
> >>  # returns the :MARK of the most recent commit
> >>  sub add {
> >> -	my ($self, $mime, $check_cb) = @_; # mime = Email::MIME
> >> +	my ($self, $mime, $check_cb, $email_str) = @_; # mime = Email::MIME
> >
> > I usually place callback args at the end of the arg list so
> > it's easy to write:
> >
> > 	$im->add($mime, sub {
> > 		# ...
> > 	});
> >
> > So having a parameter after the sub{} is a bit ugly...
> > If I had to support this, I think I'd accept $mime being
> > a plain hashref:
> >
> > 	if (ref($mime) eq 'HASH') {
> > 		$raw = $mime->{raw};
> > 		$mime = $mime->{mime};
> > 	} else {
> > 		$raw = $mime->as_string;
> > 	}
> >
> > But, I'm still on the fence about the idea...
> >
> > Side note: I'm also taking the opportunity to use "$raw" instead
> > of "$str", because I've been bitten by the difference header_raw
> > vs header_str in the Email::MIME API, so consistency with
> > that API would be good, here.
> 
> Yes the distinction about "$raw" is fair,
> and the placement in the argument list makes sense.
> 
> As for the hashref.  Perhaps what I should do is modify
> PublicInbox::MIME to at least conditionally keep the original raw email
> around.  Then the logic to get the raw email could be kept in
> PublicInbox::MIME as well.

Actually, since Perl "objects" are open-ended hashrefs with no
restrictions, we could set:

	$mime->{-public_inbox_raw} = $raw;
	$im->add($mime);

That would work for any code using Email::MIME, as well.  I
don't want the PublicInbox::MIME wrapper to be necessary forever
(it's only to workaround old bugs in Email::MIME).

And I doubt Email::* authors will start using "-public_inbox"
prefixes in their code to mess with us.

One caveat is we might need to modify our (Simple|MIME)->new callers:

-	::(Simple|MIME)->new(\$raw);
+	::(Simple|MIME)->new($raw);

since Email::Simple->new(\$raw) can clobber $raw in the
interest of memory savings, while the former preserves
the original.

> Which let's me think the general solution is to have a configuration
> option somewhere that says we want to archive the raw email.  We
> update PublicInbox::MIME to always keep the original raw email.
> 
> If add is not configured to drop any headers we use the raw original
> email str.  If add drops any headers we do what we do today.
> 
> I think perhaps we could move all of the scrubbing into
> PublicInbox::Filter::Base.pm::scrub.  Instead of having a hard coded
> drop_unwanted_headers in PublicInbox::Import.  That would make it very
> straight forward to just make this a knob that the user controls
> for how they want their email received/imported.

Not calling drop_unwanted_headers can have dangerous side-effects
(training loops, bugs in other consumers, private data exposure),
so I'm very hesitant to move it to Filter::Base

@PublicInbox::MDA::BAD_HEADERS is exposed via `our', so it could
remain stable-but-undocumented API if people really feel the
need to tweak it.  At most, we'd add a comment for that variable
asking potential hackers not to move/rename it.

> If that general idea sounds palatable I will investigate to see
> if I can move the caching of the raw email into PublicInbox::MIME,
> see about moving the dropping of headers into an appropriate config
> knob.

I think using $mime->{-public_inbox_raw} will be sufficient for
now.  Thanks.

Re: [PATCH] PublicInbox::Import Extend add with a optional raw message parameter

[Eric W. Biederman]

Eric Wong <e@80x24.org> writes:

> "Eric W. Biederman" <ebiederm@xmission.com> wrote:
>> Eric Wong <e@80x24.org> writes:
>> 
>> > "Eric W. Biederman" <ebiederm@xmission.com> wrote:
>> >> 
>> >> I don't trust the MIME type to not munge my email messages in horrible
>> >> ways upon occasion.  Therefore  allow for passing in the raw message
>> >> value instead of trusting the mime object to preserve it.
>> >> 
>> >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> >
>> > I've had the same concern in the past about Email::MIME and
>> > Email::Simple.  But after reading the code for Email::MIME,
>> > Email::Simple and Email::{MIME,Simple}::Header, I don't think
>> > the implementation of Email::MIME->as_string and all methods it
>> > calls does anything unreasonable.
>> >
>> > The only notable munging they seem to do is make irrelevant
>> > whitespace changes in headers and maybe fix quoting in headers.
>> > No body changes AFAIK.
>> 
>> I was hoping I could skip the Email::MIME stuff entirely but the headers
>> do need a bit of parsing to derive the git commit information.
>
> Perhaps parsing git commit information can be skipped, entirely
> (but it's still necessary for indexing, so I'm not sure if there's
>  any gain, there).
>
>> I will take a second look at the code.  I did not make it all of the
>> way through last time.  I just know the bazillions of warnings I could
>> not easily track down with old emails did not make me comfortable
>> with that code base as anything other than a suggestion.
>
> Ah, I added ce18b29d175ef5f01f05d59c95bcf8e0cd40e611
> ("index: warn with info about the message as context") exactly
> for that.

Knowing what the code is doing in those modules would be interesting.

>> >> The context here is because the only copy of messages that I save
>> >> I save in public-inbox I don't want to have to worry about losing
>> >> information.  So I just pass the raw email_str to add.
>> >> 
>> >> I expect if I were to export these lists public I would want to do
>> >> some more but for now I am just putting them in public-inbox
>> >> so that I can read and archive the lists locally.
>> >
>> > I worry about public archives getting badly munged, too.
>> >
>> >>  lib/PublicInbox/Import.pm     | 10 +++++-----
>> >>  lib/PublicInbox/V2Writable.pm |  8 ++++----
>> >>  2 files changed, 9 insertions(+), 9 deletions(-)
>> >
>> > Did you have plans to modify -mda/-watch or another script to
>> > use this?
>> 
>> I have been using this for a while with my own imap fetcher
>> script.  As for the others I certain could.  I don't use -mda
>> or -watch so they have not been a priority.
>
> OK.  Tangent: which IMAP module(s) did you choose? (and why?)
> I haven't gotten a chance to evaluate Perl IMAP client modules,
> yet; but I want to extend -watch to support IMAP.

The first one I found Mail::IMAPClient.  There are a few hiccups but it
seems to work for my purposes.  The hiccups are basically exceptions
thrown on failure and the PublicInbox modules not cleaning up after
themselves.  So I fork a process and let it exit when an die is called.
Instead of running the entire thing in a eval block.

I hope one of these days to upgrade it to use idle support but I haven't
gotten that far.  So far I just have a polling loop that runs every 5
minutes.

Oh that and IO::Socket::SSL.  Something that is currently missing  (or
at least missing until recently I haven't check in the last couple of
months) is ssl support for our nntp sockets.  So do imap or prevent
mischief for our nntp streams we need to use tls.  Which IO::Socket::SSL
seems to do.

But again I grabbed the first implementation that seems to work.  Going
through those modules in detail to make certain nothing goofy is going
on might be wise.

>> >> diff --git a/lib/PublicInbox/Import.pm b/lib/PublicInbox/Import.pm
>> >> index 81a38fb6987d..0a63784414f2 100644
>> >> --- a/lib/PublicInbox/Import.pm
>> >> +++ b/lib/PublicInbox/Import.pm
>> >> @@ -359,7 +359,7 @@ sub clean_tree_v2 ($$$) {
>> >>  # returns undef on duplicate
>> >>  # returns the :MARK of the most recent commit
>> >>  sub add {
>> >> -	my ($self, $mime, $check_cb) = @_; # mime = Email::MIME
>> >> +	my ($self, $mime, $check_cb, $email_str) = @_; # mime = Email::MIME
>> >
>> > I usually place callback args at the end of the arg list so
>> > it's easy to write:
>> >
>> > 	$im->add($mime, sub {
>> > 		# ...
>> > 	});
>> >
>> > So having a parameter after the sub{} is a bit ugly...
>> > If I had to support this, I think I'd accept $mime being
>> > a plain hashref:
>> >
>> > 	if (ref($mime) eq 'HASH') {
>> > 		$raw = $mime->{raw};
>> > 		$mime = $mime->{mime};
>> > 	} else {
>> > 		$raw = $mime->as_string;
>> > 	}
>> >
>> > But, I'm still on the fence about the idea...
>> >
>> > Side note: I'm also taking the opportunity to use "$raw" instead
>> > of "$str", because I've been bitten by the difference header_raw
>> > vs header_str in the Email::MIME API, so consistency with
>> > that API would be good, here.
>> 
>> Yes the distinction about "$raw" is fair,
>> and the placement in the argument list makes sense.
>> 
>> As for the hashref.  Perhaps what I should do is modify
>> PublicInbox::MIME to at least conditionally keep the original raw email
>> around.  Then the logic to get the raw email could be kept in
>> PublicInbox::MIME as well.
>
> Actually, since Perl "objects" are open-ended hashrefs with no
> restrictions, we could set:
>
> 	$mime->{-public_inbox_raw} = $raw;
> 	$im->add($mime);
>
> That would work for any code using Email::MIME, as well.  I
> don't want the PublicInbox::MIME wrapper to be necessary forever
> (it's only to workaround old bugs in Email::MIME).
>
> And I doubt Email::* authors will start using "-public_inbox"
> prefixes in their code to mess with us.
>
> One caveat is we might need to modify our (Simple|MIME)->new callers:
>
> -	::(Simple|MIME)->new(\$raw);
> +	::(Simple|MIME)->new($raw);
>
> since Email::Simple->new(\$raw) can clobber $raw in the
> interest of memory savings, while the former preserves
> the original.

Noted.

>> Which let's me think the general solution is to have a configuration
>> option somewhere that says we want to archive the raw email.  We
>> update PublicInbox::MIME to always keep the original raw email.
>> 
>> If add is not configured to drop any headers we use the raw original
>> email str.  If add drops any headers we do what we do today.
>> 
>> I think perhaps we could move all of the scrubbing into
>> PublicInbox::Filter::Base.pm::scrub.  Instead of having a hard coded
>> drop_unwanted_headers in PublicInbox::Import.  That would make it very
>> straight forward to just make this a knob that the user controls
>> for how they want their email received/imported.
>
> Not calling drop_unwanted_headers can have dangerous side-effects
> (training loops, bugs in other consumers, private data exposure),
> so I'm very hesitant to move it to Filter::Base

Usually it is my experience that dropping headers is more likely
to cause loops than keeping them.  But I definitely understand
the private data exposure angle.  However since this is my primary
archive I don't want to loose the information, in case I need it to
debug something.

> @PublicInbox::MDA::BAD_HEADERS is exposed via `our', so it could
> remain stable-but-undocumented API if people really feel the
> need to tweak it.  At most, we'd add a comment for that variable
> asking potential hackers not to move/rename it.
>
>> If that general idea sounds palatable I will investigate to see
>> if I can move the caching of the raw email into PublicInbox::MIME,
>> see about moving the dropping of headers into an appropriate config
>> knob.
>
> I think using $mime->{-public_inbox_raw} will be sufficient for
> now.  Thanks.

Then I will move in that direction.  It seems a straight forward
and simple change to make.

Eric

Re: [PATCH] PublicInbox::Import Extend add with a optional raw message parameter

[Eric Wong]

"Eric W. Biederman" <ebiederm@xmission.com> wrote:
> Eric Wong <e@80x24.org> writes:
> 
> > "Eric W. Biederman" <ebiederm@xmission.com> wrote:
> >> Eric Wong <e@80x24.org> writes:
> >> 
> >> > "Eric W. Biederman" <ebiederm@xmission.com> wrote:
> >
> >> I will take a second look at the code.  I did not make it all of the
> >> way through last time.  I just know the bazillions of warnings I could
> >> not easily track down with old emails did not make me comfortable
> >> with that code base as anything other than a suggestion.
> >
> > Ah, I added ce18b29d175ef5f01f05d59c95bcf8e0cd40e611
> > ("index: warn with info about the message as context") exactly
> > for that.
> 
> Knowing what the code is doing in those modules would be interesting.

Yes, I considered adding Carp::longmess to it, but didn't need
it since (AFAIK) all the warnings were easily found by reading
the Email::* sources.

<snip> thanks for the info on IMAP modules

> Oh that and IO::Socket::SSL.  Something that is currently missing  (or
> at least missing until recently I haven't check in the last couple of
> months) is ssl support for our nntp sockets.  So do imap or prevent
> mischief for our nntp streams we need to use tls.  Which IO::Socket::SSL
> seems to do.

Yup, IO::Socket::SSL will be what we use for STARTTLS support,
not sure when I'll get around to it.

Fwiw, it's also used by Perlbal for non-blocking HTTPS with
Danga::Socket.  I plan on keeping PublicInbox::DS close to
Danga::Socket, for now[1])...

Using the TLS support in the kernel might be a fun option to
add, too; but portable stuff, first.

> But again I grabbed the first implementation that seems to work.  Going
> through those modules in detail to make certain nothing goofy is going
> on might be wise.

Yes; at least choosing IO::Socket::SSL seems straightforward.

Looking at Perl IMAP libraries in the past has made my head spin
in the past; and lack of IDLE support (or the cost of having an
entire process wait on it) has turned me off of going further,
so far.

In any case, I think anything we do for IMAP should use
git's credential system (same as git-imap-send) to minimize
setup.

> >> I think perhaps we could move all of the scrubbing into
> >> PublicInbox::Filter::Base.pm::scrub.  Instead of having a hard coded
> >> drop_unwanted_headers in PublicInbox::Import.  That would make it very
> >> straight forward to just make this a knob that the user controls
> >> for how they want their email received/imported.
> >
> > Not calling drop_unwanted_headers can have dangerous side-effects
> > (training loops, bugs in other consumers, private data exposure),
> > so I'm very hesitant to move it to Filter::Base
> 
> Usually it is my experience that dropping headers is more likely
> to cause loops than keeping them.  But I definitely understand
> the private data exposure angle.  However since this is my primary
> archive I don't want to loose the information, in case I need it to
> debug something.
> 
> > @PublicInbox::MDA::BAD_HEADERS is exposed via `our', so it could
> > remain stable-but-undocumented API if people really feel the
> > need to tweak it.  At most, we'd add a comment for that variable
> > asking potential hackers not to move/rename it.
> >
> >> If that general idea sounds palatable I will investigate to see
> >> if I can move the caching of the raw email into PublicInbox::MIME,
> >> see about moving the dropping of headers into an appropriate config
> >> knob.
> >
> > I think using $mime->{-public_inbox_raw} will be sufficient for
> > now.  Thanks.
> 
> Then I will move in that direction.  It seems a straight forward
> and simple change to make.

Alright, so treating @PublicInbox::MDA::BAD_HEADERS as a stable
interface ought to be enough?

Keeping qw(bytes lines content-length) could throw off
consumers.  The HTTP mbox.gz code filters them out, too;
but I just noticed the NNTP code doesn't, maybe it should...

[1] I've also been thinking about making PublicInbox::DS switch
    to EPOLLONESHOT/EV_ONESHOT to map better to my mind when
    distinguishing between SSL_ERROR_WANT_READ/WRITE..  And it
    would give me an excuse to pick up and dogfood an epoll API
    to reduce syscall overhead:
    https://lore.kernel.org/lkml/1393206162-18151-1-git-send-email-n1ght.4nd.d4y@gmail.com/

TLS support and event loops

[Eric Wong]

Eric Wong <e@80x24.org> wrote:
> "Eric W. Biederman" <ebiederm@xmission.com> wrote:
> > Oh that and IO::Socket::SSL.  Something that is currently missing  (or
> > at least missing until recently I haven't check in the last couple of
> > months) is ssl support for our nntp sockets.  So do imap or prevent
> > mischief for our nntp streams we need to use tls.  Which IO::Socket::SSL
> > seems to do.
> 
> Yup, IO::Socket::SSL will be what we use for STARTTLS support,
> not sure when I'll get around to it.
> 
> Fwiw, it's also used by Perlbal for non-blocking HTTPS with
> Danga::Socket.  I plan on keeping PublicInbox::DS close to
> Danga::Socket, for now[1])...
> 
> Using the TLS support in the kernel might be a fun option to
> add, too; but portable stuff, first.

> [1] I've also been thinking about making PublicInbox::DS switch
>     to EPOLLONESHOT/EV_ONESHOT to map better to my mind when
>     distinguishing between SSL_ERROR_WANT_READ/WRITE..  And it
>     would give me an excuse to pick up and dogfood an epoll API
>     to reduce syscall overhead:
>     https://lore.kernel.org/lkml/1393206162-18151-1-git-send-email-n1ght.4nd.d4y@gmail.com/

Or the userspace pollable epoll stuff:
     https://lore.kernel.org/lkml/20190516085810.31077-1-rpenyaev@suse.de/

But yeah, I think the first step would be to convert to
EPOLLONESHOT or EPOLLET to simplify socket state management.
That should make dealing with non-blocking OpenSSL easier.

Fwiw, I never understood the appeal of level-triggering or the
"reactor pattern", but that's what everybody else seems to
pick for event loops.