unofficial mirror of meta@public-inbox.org
 help / color / mirror / Atom feed
* [PATCH 0/6] lei auth-related fixes
@ 2021-04-03  2:24 Eric Wong
  2021-04-03  2:24 ` [PATCH 1/6] URInntps: add URI 5.08 release note Eric Wong
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Eric Wong @ 2021-04-03  2:24 UTC (permalink / raw)
  To: meta

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/6] URInntps: add URI 5.08 release note
  2021-04-03  2:24 [PATCH 0/6] lei auth-related fixes Eric Wong
@ 2021-04-03  2:24 ` Eric Wong
  2021-04-03  2:24 ` [PATCH 2/6] lei q: ensure wq workers shutdown on IMAP auth failures Eric Wong
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Eric Wong @ 2021-04-03  2:24 UTC (permalink / raw)
  To: meta

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/6] lei q: ensure wq workers shutdown on IMAP auth failures
  2021-04-03  2:24 [PATCH 0/6] lei auth-related fixes Eric Wong
  2021-04-03  2:24 ` [PATCH 1/6] URInntps: add URI 5.08 release note Eric Wong
@ 2021-04-03  2:24 ` Eric Wong
  2021-04-03  2:24 ` [PATCH 3/6] lei tag: fix tagging of IMAP inputs Eric Wong
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Eric Wong @ 2021-04-03  2:24 UTC (permalink / raw)
  To: meta

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>: $@";
 	}

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 3/6] lei tag: fix tagging of IMAP inputs
  2021-04-03  2:24 [PATCH 0/6] lei auth-related fixes Eric Wong
  2021-04-03  2:24 ` [PATCH 1/6] URInntps: add URI 5.08 release note Eric Wong
  2021-04-03  2:24 ` [PATCH 2/6] lei q: ensure wq workers shutdown on IMAP auth failures Eric Wong
@ 2021-04-03  2:24 ` Eric Wong
  2021-04-03  2:24 ` [PATCH 4/6] lei_auth: rename {net_merge} to {net_merge_continue} Eric Wong
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Eric Wong @ 2021-04-03  2:24 UTC (permalink / raw)
  To: meta

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;

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 4/6] lei_auth: rename {net_merge} to {net_merge_continue}
  2021-04-03  2:24 [PATCH 0/6] lei auth-related fixes Eric Wong
                   ` (2 preceding siblings ...)
  2021-04-03  2:24 ` [PATCH 3/6] lei tag: fix tagging of IMAP inputs Eric Wong
@ 2021-04-03  2:24 ` Eric Wong
  2021-04-03  2:24 ` [PATCH 5/6] net_reader: fix read-only "lei convert" auth failures Eric Wong
  2021-04-03  2:24 ` [PATCH 6/6] xt/lei-auth-fail: test more failure cases Eric Wong
  5 siblings, 0 replies; 7+ messages in thread
From: Eric Wong @ 2021-04-03  2:24 UTC (permalink / raw)
  To: meta

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 ];
 }
 

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 5/6] net_reader: fix read-only "lei convert" auth failures
  2021-04-03  2:24 [PATCH 0/6] lei auth-related fixes Eric Wong
                   ` (3 preceding siblings ...)
  2021-04-03  2:24 ` [PATCH 4/6] lei_auth: rename {net_merge} to {net_merge_continue} Eric Wong
@ 2021-04-03  2:24 ` Eric Wong
  2021-04-03  2:24 ` [PATCH 6/6] xt/lei-auth-fail: test more failure cases Eric Wong
  5 siblings, 0 replies; 7+ messages in thread
From: Eric Wong @ 2021-04-03  2:24 UTC (permalink / raw)
  To: meta

"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>: $@";
 	}

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 6/6] xt/lei-auth-fail: test more failure cases
  2021-04-03  2:24 [PATCH 0/6] lei auth-related fixes Eric Wong
                   ` (4 preceding siblings ...)
  2021-04-03  2:24 ` [PATCH 5/6] net_reader: fix read-only "lei convert" auth failures Eric Wong
@ 2021-04-03  2:24 ` Eric Wong
  5 siblings, 0 replies; 7+ messages in thread
From: Eric Wong @ 2021-04-03  2:24 UTC (permalink / raw)
  To: meta

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');
 	}

^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-04-03  2:24 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-03  2:24 [PATCH 0/6] lei auth-related fixes Eric Wong
2021-04-03  2:24 ` [PATCH 1/6] URInntps: add URI 5.08 release note Eric Wong
2021-04-03  2:24 ` [PATCH 2/6] lei q: ensure wq workers shutdown on IMAP auth failures Eric Wong
2021-04-03  2:24 ` [PATCH 3/6] lei tag: fix tagging of IMAP inputs Eric Wong
2021-04-03  2:24 ` [PATCH 4/6] lei_auth: rename {net_merge} to {net_merge_continue} Eric Wong
2021-04-03  2:24 ` [PATCH 5/6] net_reader: fix read-only "lei convert" auth failures Eric Wong
2021-04-03  2:24 ` [PATCH 6/6] xt/lei-auth-fail: test more failure cases Eric Wong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).