Auth failure handling should be more complete, but it's still horrifically slow to test because the dovecot instance I run has rate-limits for auth failures. Perhaps public-inbox-imapd should learn to reject certain username + password combos, since the -imapd instance on public-inbox.org sees a fair amount of automated traffic looking to steal info off leaked credentials. 3/6 is me still learning to use APIs I invent :x Eric Wong (6): URInntps: add URI 5.08 release note lei q: ensure wq workers shutdown on IMAP auth failures lei tag: fix tagging of IMAP inputs lei_auth: rename {net_merge} to {net_merge_continue} net_reader: fix read-only "lei convert" auth failures xt/lei-auth-fail: test more failure cases lib/PublicInbox/LEI.pm | 24 ++++++++---------------- lib/PublicInbox/LeiAuth.pm | 17 ++++++++++------- lib/PublicInbox/LeiTag.pm | 6 +++++- lib/PublicInbox/NetReader.pm | 5 +++-- lib/PublicInbox/URInntps.pm | 1 + t/lei-import-imap.t | 1 + xt/lei-auth-fail.t | 11 +++++++---- 7 files changed, 35 insertions(+), 30 deletions(-)
I wanted to say 2031, but that's probably too aggressive a removal timeline. --- lib/PublicInbox/URInntps.pm | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/PublicInbox/URInntps.pm b/lib/PublicInbox/URInntps.pm index 69fe7163..231b247b 100644 --- a/lib/PublicInbox/URInntps.pm +++ b/lib/PublicInbox/URInntps.pm @@ -4,6 +4,7 @@ # deal with the lack of URI::nntps in upstream URI. # nntps is IANA registered, snews is deprecated # cf. https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=983419 +# Fixed in URI 5.08, we can drop this by 2035 when LTS distros all have it package PublicInbox::URInntps; use strict; use parent qw(URI::snews);
Leaving workers running on after auth failures is bad and messy, cleanup our process management to have consistent worker teardowns. Improve error reporting, too, instead of letting Mail::IMAPClient->exists fail due to undef. --- lib/PublicInbox/LEI.pm | 24 ++++++++---------------- lib/PublicInbox/LeiAuth.pm | 15 +++++++++------ lib/PublicInbox/NetReader.pm | 2 +- 3 files changed, 18 insertions(+), 23 deletions(-) diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm index f9361c68..cdb4b347 100644 --- a/lib/PublicInbox/LEI.pm +++ b/lib/PublicInbox/LEI.pm @@ -350,6 +350,11 @@ my %CONFIG_KEYS = ( my @WQ_KEYS = qw(lxs l2m imp mrr cnv p2q tag sol); # internal workers +sub _drop_wq { + my ($self) = @_; + for my $wq (grep(defined, delete(@$self{@WQ_KEYS}))) { $wq->DESTROY } +} + # pronounced "exit": x_it(1 << 8) => exit(1); x_it(13) => SIGPIPE sub x_it ($$) { my ($self, $code) = @_; @@ -360,10 +365,7 @@ sub x_it ($$) { send($s, "x_it $code", MSG_EOR); } elsif ($self->{oneshot}) { # don't want to end up using $? from child processes - for my $f (@WQ_KEYS) { - my $wq = delete $self->{$f} or next; - $wq->DESTROY; - } + _drop_wq($self); # cleanup anything that has tempfiles or open file handles %PATH2CFG = (); delete @$self{qw(ovv dedupe sto cfg)}; @@ -392,11 +394,8 @@ sub qerr ($;@) { $_[0]->{opt}->{quiet} or err(shift, @_) } sub fail_handler ($;$$) { my ($lei, $code, $io) = @_; - for my $f (@WQ_KEYS) { - my $wq = delete $lei->{$f} or next; - $wq->wq_wait_old(undef, $lei) if $wq->wq_kill_old; # lei-daemon - } close($io) if $io; # needed to avoid warnings on SIGPIPE + _drop_wq($lei); x_it($lei, $code // (1 << 8)); } @@ -983,14 +982,7 @@ sub accept_dispatch { # Listener {post_accept} callback sub dclose { my ($self) = @_; delete $self->{-progress}; - for my $f (@WQ_KEYS) { - my $wq = delete $self->{$f} or next; - if ($wq->wq_kill) { - $wq->wq_close(0, undef, $self); - } elsif ($wq->wq_kill_old) { - $wq->wq_wait_old(undef, $self); - } - } + _drop_wq($self); close(delete $self->{1}) if $self->{1}; # may reap_compress $self->close if $self->{-event_init_done}; # PublicInbox::DS::close } diff --git a/lib/PublicInbox/LeiAuth.pm b/lib/PublicInbox/LeiAuth.pm index 927fe550..48deca93 100644 --- a/lib/PublicInbox/LeiAuth.pm +++ b/lib/PublicInbox/LeiAuth.pm @@ -13,12 +13,15 @@ sub do_auth_atfork { # used by IPC WQ workers return if $wq->{-wq_worker_nr} != 0; my $lei = $wq->{lei}; my $net = $lei->{net}; - my $mics = $net->imap_common_init($lei); - my $nn = $net->nntp_common_init($lei); - pkt_do($lei->{pkt_op_p}, 'net_merge', $net) or - die "pkt_do net_merge: $!"; - $net->{mics_cached} = $mics if $mics; - $net->{nn_cached} = $nn if $nn; + eval { + my $mics = $net->imap_common_init($lei); + my $nn = $net->nntp_common_init($lei); + pkt_do($lei->{pkt_op_p}, 'net_merge', $net) or + die "pkt_do net_merge: $!"; + $net->{mics_cached} = $mics if $mics; + $net->{nn_cached} = $nn if $nn; + }; + $lei->fail($@) if $@; } sub net_merge_done1 { # bump merge-count in top-level lei-daemon diff --git a/lib/PublicInbox/NetReader.pm b/lib/PublicInbox/NetReader.pm index 6a52b479..c269d841 100644 --- a/lib/PublicInbox/NetReader.pm +++ b/lib/PublicInbox/NetReader.pm @@ -267,7 +267,7 @@ sub imap_common_init ($;$) { $mics->{$sec} //= mic_for($self, "$sec/", $mic_args, $lei); next unless $self->isa('PublicInbox::NetWriter'); my $dst = $uri->mailbox // next; - my $mic = $mics->{$sec}; + my $mic = $mics->{$sec} // die "Unable to continue\n"; next if $mic->exists($dst); # already exists $mic->create($dst) or die "CREATE $dst failed <$uri>: $@"; }
We need net_merge_all and to lock the number of worker jobs. Parallel inputs are not supported, yet (is it needed?, I don't expect this to be used for multiple files very often...). --- lib/PublicInbox/LeiTag.pm | 6 +++++- t/lei-import-imap.t | 1 + 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/PublicInbox/LeiTag.pm b/lib/PublicInbox/LeiTag.pm index d572a84a..06313139 100644 --- a/lib/PublicInbox/LeiTag.pm +++ b/lib/PublicInbox/LeiTag.pm @@ -112,7 +112,8 @@ sub lei_tag { # the "lei tag" method my $ops = { '' => [ \&tag_done, $lei ] }; $lei->{auth}->op_merge($ops, $self) if $lei->{auth}; $self->{vmd_mod} = $vmd_mod; - (my $op_c, $ops) = $lei->workers_start($self, 'lei_tag', 1, $ops); + my $j = $self->{-wq_nr_workers} = 1; # locked for now + (my $op_c, $ops) = $lei->workers_start($self, 'lei_tag', $j, $ops); $lei->{tag} = $self; net_merge_complete($self) unless $lei->{auth}; $op_c->op_wait_event($ops); @@ -175,4 +176,7 @@ sub _complete_tag { } grep(/$re\Q$cur/, @all); } +no warnings 'once'; # the following works even when LeiAuth is lazy-loaded +*net_merge_all = \&PublicInbox::LeiAuth::net_merge_all; + 1; diff --git a/t/lei-import-imap.t b/t/lei-import-imap.t index fd38037a..7e4d44b9 100644 --- a/t/lei-import-imap.t +++ b/t/lei-import-imap.t @@ -23,5 +23,6 @@ test_lei({ tmpdir => $tmpdir }, sub { my %r; for (@$out) { $r{ref($_)}++ } is_deeply(\%r, { 'HASH' => scalar(@$out) }, 'all hashes'); + lei_ok([qw(tag +kw:seen), "imap://$host_port/t.v2.0"], undef, undef); }); done_testing;
No reason for the hash key to differ from the subroutine name, here. --- lib/PublicInbox/LeiAuth.pm | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/PublicInbox/LeiAuth.pm b/lib/PublicInbox/LeiAuth.pm index 48deca93..e7b26c5f 100644 --- a/lib/PublicInbox/LeiAuth.pm +++ b/lib/PublicInbox/LeiAuth.pm @@ -16,8 +16,8 @@ sub do_auth_atfork { # used by IPC WQ workers eval { my $mics = $net->imap_common_init($lei); my $nn = $net->nntp_common_init($lei); - pkt_do($lei->{pkt_op_p}, 'net_merge', $net) or - die "pkt_do net_merge: $!"; + pkt_do($lei->{pkt_op_p}, 'net_merge_continue', $net) or + die "pkt_do net_merge_continue: $!"; $net->{mics_cached} = $mics if $mics; $net->{nn_cached} = $nn if $nn; }; @@ -46,7 +46,7 @@ sub net_merge_continue { sub op_merge { # prepares PktOp->pair ops my ($self, $ops, $wq) = @_; - $ops->{net_merge} = [ \&net_merge_continue, $wq ]; + $ops->{net_merge_continue} = [ \&net_merge_continue, $wq ]; $ops->{net_merge_done1} = [ \&net_merge_done1, $wq ]; }
"convert" is actually a bit more complicated than "lei import" since it may need auth for either input or output. --- lib/PublicInbox/NetReader.pm | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/PublicInbox/NetReader.pm b/lib/PublicInbox/NetReader.pm index c269d841..821e5d7f 100644 --- a/lib/PublicInbox/NetReader.pm +++ b/lib/PublicInbox/NetReader.pm @@ -264,10 +264,11 @@ sub imap_common_init ($;$) { my $mics = {}; # schema://authority => IMAPClient obj for my $uri (@{$self->{imap_order}}) { my $sec = uri_section($uri); - $mics->{$sec} //= mic_for($self, "$sec/", $mic_args, $lei); + my $mic = $mics->{$sec} //= + mic_for($self, "$sec/", $mic_args, $lei) // + die "Unable to continue\n"; next unless $self->isa('PublicInbox::NetWriter'); my $dst = $uri->mailbox // next; - my $mic = $mics->{$sec} // die "Unable to continue\n"; next if $mic->exists($dst); # already exists $mic->create($dst) or die "CREATE $dst failed <$uri>: $@"; }
Because failures are often overlooked, unfortunately. --- xt/lei-auth-fail.t | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/xt/lei-auth-fail.t b/xt/lei-auth-fail.t index e352aab3..06cb8533 100644 --- a/xt/lei-auth-fail.t +++ b/xt/lei-auth-fail.t @@ -2,17 +2,20 @@ # Copyright (C) 2021 all contributors <meta@public-inbox.org> # License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt> use strict; use v5.10.1; use PublicInbox::TestCommon; -require_mods(qw(Mail::IMAPClient)); +require_mods(qw(Mail::IMAPClient lei)); # TODO: mock IMAP server which fails at authentication so we don't # have to make external connections to test this: my $imap_fail = $ENV{TEST_LEI_IMAP_FAIL_URL} // 'imaps://AzureDiamond:Hunter2@public-inbox.org:994/INBOX'; +my ($ro_home, $cfg_path) = setup_public_inboxes; test_lei(sub { - for my $pfx ([qw(convert -o mboxrd:/dev/stdout)], ['import'], - [qw(tag +L:INBOX)]) { + for my $pfx ([qw(q z:0.. --only), "$ro_home/t1", '-o'], + [qw(convert -o mboxrd:/dev/stdout)], + [qw(convert t/utf8.eml -o), $imap_fail], + ['import'], [qw(tag +L:INBOX)]) { ok(!lei(@$pfx, $imap_fail), "IMAP auth failure on @$pfx"); - like($lei_err, qr!\bE:.*?imaps://.*?!sm, 'error shown'); + like($lei_err, qr!\bE:.*?imaps?://.*?!sm, 'error shown'); unlike($lei_err, qr!Hunter2!s, 'password not shown'); is($lei_out, '', 'nothing output'); }