From adc4cbf51f212d1ac99095ab905aa22441806efe Mon Sep 17 00:00:00 2001 From: Kenichi Ishigaki Date: Fri, 26 Apr 2024 19:53:49 +0900 Subject: [PATCH 01/19] Add Auth::GoogleAuth to cpanfile --- cpanfile | 1 + 1 file changed, 1 insertion(+) diff --git a/cpanfile b/cpanfile index b0009040a..38f4d4550 100644 --- a/cpanfile +++ b/cpanfile @@ -1,4 +1,5 @@ requires 'Apache::Session::Counted'; +requires 'Auth::GoogleAuth', '1.05'; requires 'BSD::Resource'; requires 'CPAN::Checksums', '1.050'; requires 'CPAN::DistnameInfo'; From 78ffb032d2f47c941198b9bce2e3c483696e1bb5 Mon Sep 17 00:00:00 2001 From: Kenichi Ishigaki Date: Fri, 26 Apr 2024 19:54:37 +0900 Subject: [PATCH 02/19] Alter the authen_pause schema and add a one-off script to alter --- doc/authen_pause.schema.txt | 3 +++ doc/schemas/authen_pause.schema.sqlite | 3 +++ one-off-utils/schemachange-2024-04_2.sql | 3 +++ 3 files changed, 9 insertions(+) create mode 100644 one-off-utils/schemachange-2024-04_2.sql diff --git a/doc/authen_pause.schema.txt b/doc/authen_pause.schema.txt index 00b335dc8..450ed8205 100644 --- a/doc/authen_pause.schema.txt +++ b/doc/authen_pause.schema.txt @@ -56,6 +56,9 @@ CREATE TABLE usertable ( `changed` int(11) DEFAULT NULL, changedby char(10) DEFAULT NULL, lastvisit datetime DEFAULT NULL, + mfa tinyint(1) DEFAULT 0, + mfa_secret32 varchar(16) DEFAULT NULL, + mfa_recovery_codes text DEFAULT NULL, PRIMARY KEY (`user`), KEY usertable_password (`password`) ) ENGINE=InnoDB DEFAULT CHARSET=latin1 PACK_KEYS=1; diff --git a/doc/schemas/authen_pause.schema.sqlite b/doc/schemas/authen_pause.schema.sqlite index 189abb95a..36ae5725b 100644 --- a/doc/schemas/authen_pause.schema.sqlite +++ b/doc/schemas/authen_pause.schema.sqlite @@ -36,6 +36,9 @@ CREATE TABLE usertable ( changed int(11) DEFAULT NULL, changedby char(10) DEFAULT NULL, lastvisit datetime DEFAULT NULL, + mfa tinyint(1) DEFAULT 0, + mfa_secret32 varchar(16) DEFAULT NULL, + mfa_recovery_codes text DEFAULT NULL, PRIMARY KEY (user) ); diff --git a/one-off-utils/schemachange-2024-04_2.sql b/one-off-utils/schemachange-2024-04_2.sql new file mode 100644 index 000000000..b981d9722 --- /dev/null +++ b/one-off-utils/schemachange-2024-04_2.sql @@ -0,0 +1,3 @@ +ALTER TABLE usertable ADD COLUMN mfa tinyint(1) DEFAULT 0; +ALTER TABLE usertable ADD COLUMN mfa_secret32 varchar(16); +ALTER TABLE usertable ADD COLUMN mfa_recovery_codes text; From d7ed0e7aec49fb9b252fc66532dd8cf13a5ff6cd Mon Sep 17 00:00:00 2001 From: Kenichi Ishigaki Date: Fri, 26 Apr 2024 19:55:52 +0900 Subject: [PATCH 03/19] Add INCOMING_TMP (/tmp/ for now) where incoming files from MFA-enabled users are put first --- lib/PAUSE.pm | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/PAUSE.pm b/lib/PAUSE.pm index ec3ec466c..2127a90b5 100644 --- a/lib/PAUSE.pm +++ b/lib/PAUSE.pm @@ -92,6 +92,7 @@ $PAUSE::Config ||= HTTP_ERRORLOG => '/var/log/nginx/error.log', # harmless use in cron-daily INCOMING => 'file://data/pause/incoming/', INCOMING_LOC => '/data/pause/incoming', + INCOMING_TMP => '/data/pause/tmp/', MAIL_MAILER => ["sendmail"], MAXRETRIES => 16, MIRRORCONFIG => '/usr/local/mirror/mymirror.config', From 356fed5ad2d3393ed758ebd31757b967b73a66b4 Mon Sep 17 00:00:00 2001 From: Kenichi Ishigaki Date: Fri, 26 Apr 2024 19:57:22 +0900 Subject: [PATCH 04/19] Add WithMFAProtection plugin --- lib/pause_2017/PAUSE/Web.pm | 2 + .../PAUSE/Web/Plugin/WithMFAProtection.pm | 86 +++++++++++++++++++ 2 files changed, 88 insertions(+) create mode 100644 lib/pause_2017/PAUSE/Web/Plugin/WithMFAProtection.pm diff --git a/lib/pause_2017/PAUSE/Web.pm b/lib/pause_2017/PAUSE/Web.pm index f2ba1ae2f..d62cda8e5 100644 --- a/lib/pause_2017/PAUSE/Web.pm +++ b/lib/pause_2017/PAUSE/Web.pm @@ -33,6 +33,7 @@ sub startup { # Load plugins to modify path/set stash values/provide helper methods $app->plugin("WithCSRFProtection"); + $app->plugin("PAUSE::Web::Plugin::WithMFAProtection"); $app->plugin("PAUSE::Web::Plugin::ConfigPerRequest"); $app->plugin("PAUSE::Web::Plugin::IsPauseClosed"); $app->plugin("PAUSE::Web::Plugin::GetActiveUserRecord"); @@ -81,6 +82,7 @@ sub startup { for my $method (qw/get post/) { my $route = $private->$method("/$name"); $route->with_csrf_protection if $method eq "post" and $action->{x_csrf_protection}; + $route->with_mfa_protection if $method eq "post" and $action->{x_mfa_protection}; $route->to($action->{x_mojo_to}); } } diff --git a/lib/pause_2017/PAUSE/Web/Plugin/WithMFAProtection.pm b/lib/pause_2017/PAUSE/Web/Plugin/WithMFAProtection.pm new file mode 100644 index 000000000..60717728b --- /dev/null +++ b/lib/pause_2017/PAUSE/Web/Plugin/WithMFAProtection.pm @@ -0,0 +1,86 @@ +package PAUSE::Web::Plugin::WithMFAProtection; + +use Mojo::Base 'Mojolicious::Plugin'; + +our $VERSION = '1.00'; + +sub register { + my ( $self, $app ) = @_; + + my $routes = $app->routes; + + $routes->add_condition( + with_mfa_protection => sub { + my ( $route, $c ) = @_; + + my $u = $c->active_user_record; + + # XXX: The active user record does not have mfa when an admin user is pretending someone else. + return 1 unless $u->{mfa}; + + my $otp = $c->req->body_params->param('otp'); + if (defined $otp and $otp ne '') { + if ($otp =~ /\A[0-9]{6}\z/) { + return 1 if $c->app->pause->authenticator_for($u)->verify($otp); + } elsif ($otp =~ /\A[a-z0-9]{5}\-[a-z0-9]{5}\z/) { # maybe one of the recovery codes? + require PAUSE::Crypt; + my $pause = $c->stash(".pause"); + my @recovery_codes = split / /, $u->{mfa_recovery_codes} // ''; + for my $code (@recovery_codes) { + if (PAUSE::Crypt::password_verify($otp, $code)) { + my $new_codes = join ' ', grep { $_ ne $code } @recovery_codes; + my $dbh = $c->app->pause->authen_connect; + my $tbl = $PAUSE::Config->{AUTHEN_USER_TABLE}; + my $sql = "UPDATE $tbl SET mfa_recovery_codes = ?, changed = ?, changedby = ? WHERE user = ?"; + $dbh->do($sql, undef, $new_codes, time, $pause->{User}{userid}, $u->{userid}) + or push @{$pause->{ERROR}}, sprintf(qq{Could not enter the data into the database: %s.},$dbh->errstr); + return 1; + } + } + } + } + # special case for upload + if (my $upload = $c->req->upload("pause99_add_uri_httpupload")) { + if ($upload->size) { + $PAUSE::Config->{INCOMING_TMP} =~ s|/$||; + + my $filename = $upload->filename; + $filename =~ s(.*/)()gs; # no slash + $filename =~ s(.*\\)()gs; # no backslash + $filename =~ s(.*:)()gs; # no colon + $filename =~ s/[^A-Za-z0-9_\-\.\@\+]//g; # only ASCII-\w and - . @ + allowed + my $to = "$PAUSE::Config->{INCOMING_TMP}/$filename"; + # my $fhi = $upl->fh; + if (-f $to && -s _ == 0) { # zero sized files are a common problem + unlink $to; + } + if (eval { $upload->move_to($to) }){ + warn "h1[File successfully copied to '$to']filename[$filename]"; + } else { + die PAUSE::Web::Exception->new(ERROR => "Couldn't copy file '$filename' to '$to': $!"); + } + unless ($upload->filename eq $filename) { + require Dumpvalue; + my $dv = Dumpvalue->new; + $c->req->param("pause99_add_uri_httpupload",$filename); + $c->req->param("pause99_add_uri_httpupload_renamed_from",$upload->filename); + } + $c->req->param("pause99_add_uri_httpupload_stashed", $filename); + } + } + $c->render('mfa_check'); + return; + } + ); + + $routes->add_shortcut( + with_mfa_protection => sub { + my ($route) = @_; + return $route->requires( with_mfa_protection => 1 ); + } + ); + + return; +} + +1; From 1c6756bd6d52f26aacbcfac4d475ecd04aac59d3 Mon Sep 17 00:00:00 2001 From: Kenichi Ishigaki Date: Fri, 26 Apr 2024 19:58:42 +0900 Subject: [PATCH 05/19] Add x_mfa_protection to places where apparently appropriate --- lib/pause_2017/PAUSE/Web/Config.pm | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/lib/pause_2017/PAUSE/Web/Config.pm b/lib/pause_2017/PAUSE/Web/Config.pm index 04d4e1230..b936b3db3 100644 --- a/lib/pause_2017/PAUSE/Web/Config.pm +++ b/lib/pause_2017/PAUSE/Web/Config.pm @@ -111,6 +111,7 @@ our %Actions = ( cat => "User/01Files/01up", desc => "This is the heart of the Upload Server, the page most heavily used on PAUSE.", method => 'POST', + x_mfa_protection => 1, x_form => { HIDDENNAME => {form_type => "hidden_field"}, CAN_MULTIPART => {form_type => "hidden_field"}, @@ -428,6 +429,7 @@ our %Actions = ( desc => "Change your password any time you want.", method => 'POST', x_csrf_protection => 1, + x_mfa_protection => 1, x_form => { HIDDENNAME => {form_type => "hidden_field"}, ABRA => {form_type => "hidden_field"}, @@ -444,6 +446,7 @@ our %Actions = ( desc => "Edit your user name, your email addresses (both public and secret one), change the URL of your homepage.", method => 'POST', x_csrf_protection => 1, + x_mfa_protection => 1, x_form => { HIDDENNAME => {form_type => "hidden_field"}, pause99_edit_cred_fullname => {form_type => "text_field"}, @@ -456,6 +459,21 @@ our %Actions = ( pause99_edit_cred_sub => {form_type => "submit_button"}, }, }, + mfa => { + x_mojo_to => "user-mfa#edit", + verb => "Multifactor Auth", + priv => "user", + cat => "User/06Account/03", + desc => "Multifactor Authentication.", + method => 'POST', + x_csrf_protection => 1, + x_form => { + HIDDENNAME => {form_type => "hidden_field"}, + pause99_mfa_code => {form_type => "text_field"}, + pause99_mfa_reset => {form_type => "hidden_field"}, + pause99_mfa_sub => {form_type => "submit_button"}, + }, + }, pause_logout => { x_mojo_to => "user#pause_logout", verb => "About Logging Out", @@ -493,6 +511,7 @@ our %Actions = ( cat => "01usr/01add", desc => "Admins can add users or mailinglists.", method => 'POST', + x_mfa_protection => 1, x_form => { SUBMIT_pause99_add_user_Soundex => {form_type => "submit_button"}, SUBMIT_pause99_add_user_Metaphone => {form_type => "submit_button"}, @@ -520,6 +539,7 @@ our %Actions = ( cat => "01usr/02", desc => "Admins and mailing list representatives can change the name, address and description of a mailing list.", method => 'POST', + x_mfa_protection => 1, x_form => { HIDDENNAME => {form_type => "hidden_field"}, pause99_edit_ml_3 => {form_type => "select_field"}, # mailing lists @@ -544,6 +564,7 @@ our %Actions = ( cat => "01usr/03", desc => "Admins can access PAUSE as-if they were somebody else. Here they select a user/action pair.", method => 'POST', + x_mfa_protection => 1, x_form => { HIDDENNAME => {form_type => "select_field"}, ACTIONREQ => {form_type => "select_field"}, From db6139151407c50a40e4e101ec8d4bfe51564f51 Mon Sep 17 00:00:00 2001 From: Kenichi Ishigaki Date: Fri, 26 Apr 2024 20:02:45 +0900 Subject: [PATCH 06/19] Add authentication_for helper to hide the details of Auth::GoogleAuth stuff --- lib/pause_2017/PAUSE/Web/Context.pm | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lib/pause_2017/PAUSE/Web/Context.pm b/lib/pause_2017/PAUSE/Web/Context.pm index aa84e0b2f..324f7dc9d 100644 --- a/lib/pause_2017/PAUSE/Web/Context.pm +++ b/lib/pause_2017/PAUSE/Web/Context.pm @@ -10,6 +10,7 @@ use Email::MIME; use Data::Dumper; use PAUSE::Web::Config; use PAUSE::Web::Exception; +use Auth::GoogleAuth; our $VERSION = "1072"; @@ -40,6 +41,17 @@ sub version { $version; } +sub authenticator_for { + my ($self, $user) = @_; + my $cpan_alias = lc($user->{userid}) . '@cpan.org'; + my $secret32 = $user->{mfa_secret32}; + return Auth::GoogleAuth->new({ + secret32 => $secret32, + issuer => $PAUSE::Config->{MFA_ISSUER} || 'PAUSE', + key_id => $cpan_alias, + }); +} + sub hostname { my $self = shift; $PAUSE::Config->{SERVER_NAME} || Sys::Hostname::hostname(); From c28a4875799cf3c78bab7b7e97db31947603c41f Mon Sep 17 00:00:00 2001 From: Kenichi Ishigaki Date: Fri, 26 Apr 2024 20:05:04 +0900 Subject: [PATCH 07/19] Move the incoming files put in the INCOMING_TMP directory to INCOMING_LOC --- .../PAUSE/Web/Controller/User/Uri.pm | 40 ++++++++++++++++--- 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/lib/pause_2017/PAUSE/Web/Controller/User/Uri.pm b/lib/pause_2017/PAUSE/Web/Controller/User/Uri.pm index 7f5169e29..cde6bdbe4 100644 --- a/lib/pause_2017/PAUSE/Web/Controller/User/Uri.pm +++ b/lib/pause_2017/PAUSE/Web/Controller/User/Uri.pm @@ -12,6 +12,7 @@ sub add { my $req = $c->req; $PAUSE::Config->{INCOMING_LOC} =~ s|/$||; + $PAUSE::Config->{INCOMING_TMP} =~ s|/$||; my $u = $c->active_user_record; die PAUSE::Web::Exception @@ -26,11 +27,20 @@ sub add { if ($req->param("SUBMIT_pause99_add_uri_HTTPUPLOAD") || $req->param("SUBMIT_pause99_add_uri_httpupload")) { - my $upl = $req->upload('pause99_add_uri_httpupload'); - unless ($upl->size) { - warn "Warning: maybe they hit RETURN, no upload size, not doing HTTPUPLOAD"; - $req->param("SUBMIT_pause99_add_uri_HTTPUPLOAD",""); - $req->param("SUBMIT_pause99_add_uri_httpupload",""); + if (my $stashed = $req->param("pause99_add_uri_httpupload_stashed")) { + my $stashed_file = "$PAUSE::Config->{INCOMING_TMP}/$stashed"; + if (!-e $stashed_file) { + warn "Warning: maybe their files are already gone, not doing HTTPUPLOAD"; + $req->param("SUBMIT_pause99_add_uri_HTTPUPLOAD",""); + $req->param("SUBMIT_pause99_add_uri_httpupload",""); + } + } else { + my $upl = $req->upload('pause99_add_uri_httpupload'); + unless ($upl->size) { + warn "Warning: maybe they hit RETURN, no upload size, not doing HTTPUPLOAD"; + $req->param("SUBMIT_pause99_add_uri_HTTPUPLOAD",""); + $req->param("SUBMIT_pause99_add_uri_httpupload",""); + } } } if (! $req->param("SUBMIT_pause99_add_uri_HTTPUPLOAD") @@ -54,7 +64,25 @@ sub add { ) { { # $pause->{UseModuleSet} eq "ApReq" my $upl; - if ( + if (my $filename = $req->param("pause99_add_uri_httpupload_stashed")) { + my $stashed_file = "$PAUSE::Config->{INCOMING_TMP}/$filename"; + my $to = "$PAUSE::Config->{INCOMING_LOC}/$filename"; + rename $stashed_file => $to or die PAUSE::Web::Exception + ->new(ERROR => "Couldn't copy file '$filename' to '$to': $!"); + $pause->{successfully_copied_to} = $to; + warn "h1[File successfully copied to '$to']filename[$filename]"; + if ($req->param("pause99_add_uri_httpupload_renamed_from")) { + require Dumpvalue; + my $dv = Dumpvalue->new; + $req->param("pause99_add_uri_httpupload",$filename); + $pause->{upload_is_renamed} = { + from => $dv->stringify($req->param("pause99_add_uri_httpupload_renamed_from")), + to => $dv->stringify($filename), + }; + } + $uri = $filename; + } + elsif ( $upl = $req->upload("pause99_add_uri_httpupload") or # from 990806 $upl = $req->upload("HTTPUPLOAD") ) { From e6f7b1a049eca3dd32b688e7df7cb9a8890414b4 Mon Sep 17 00:00:00 2001 From: Kenichi Ishigaki Date: Fri, 26 Apr 2024 20:14:44 +0900 Subject: [PATCH 08/19] Add Mfa controller and a template for it --- .../PAUSE/Web/Controller/User/Mfa.pm | 80 +++++++++++++++++++ .../templates/user/mfa/edit.html.ep | 59 ++++++++++++++ 2 files changed, 139 insertions(+) create mode 100644 lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm create mode 100644 lib/pause_2017/templates/user/mfa/edit.html.ep diff --git a/lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm b/lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm new file mode 100644 index 000000000..90eaf8e19 --- /dev/null +++ b/lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm @@ -0,0 +1,80 @@ +package PAUSE::Web::Controller::User::Mfa; + +use Mojo::Base "Mojolicious::Controller"; +use Auth::GoogleAuth; +use PAUSE::Crypt; +use Crypt::URandom qw(urandom); +use Convert::Base32 qw(encode_base32); + +sub edit { + my $c = shift; + my $pause = $c->stash(".pause"); + my $mgr = $c->app->pause; + my $req = $c->req; + my $u = $c->active_user_record; + + my $auth = $c->app->pause->authenticator_for($u); + $pause->{mfa_qrcode} = $auth->qr_code; + if (!$u->{mfa_secret32}) { + my $dbh = $mgr->authen_connect; + my $tbl = $PAUSE::Config->{AUTHEN_USER_TABLE}; + my $sql = "UPDATE $tbl SET mfa_secret32 = ?, changed = ?, changedby = ? WHERE user = ?"; + $dbh->do($sql, undef, $auth->secret32, time, $pause->{User}{userid}, $u->{userid}) + or push @{$pause->{ERROR}}, sprintf(qq{Could not enter the data into the database: %s.},$dbh->errstr); + } + + if (uc $req->method eq 'POST' and $req->param("pause99_mfa_sub")) { + my $code = $req->param("pause99_mfa_code"); + $req->param("pause99_mfa_code", undef); + if ($code =~ /\A[0-9]{6}\z/ && !$auth->verify($code)) { + $pause->{error}{invalid_code} = 1; + return; + } elsif ($code =~ /\A[a-z0-9]{5}\-[a-z0-9]{5}\z/ && $u->{mfa_recovery_codes} && $req->param("pause99_mfa_reset")) { + my @recovery_codes = split / /, $u->{mfa_recovery_codes} // ''; + if (!grep { PAUSE::Crypt::password_verify($code, $_) } @recovery_codes) { + $pause->{error}{invalid_code} = 1; + return; + } + } + my ($mfa, $secret32, $recovery_codes); + if ($req->param("pause99_mfa_reset")) { + $mfa = 0; + $secret32 = undef; + $recovery_codes = undef; + $c->flash(mfa_disabled => 1); + } else { + $mfa = 1; + $secret32 = $auth->secret32; + $c->flash(mfa_enabled => 1); + my @codes = _generate_recovery_codes(); + $c->flash(recovery_codes => \@codes); + $recovery_codes = join " ", map { PAUSE::Crypt::hash_password($_) } @codes; + } + my $dbh = $mgr->authen_connect; + my $tbl = $PAUSE::Config->{AUTHEN_USER_TABLE}; + my $sql = "UPDATE $tbl SET mfa = ?, mfa_secret32 = ?, mfa_recovery_codes = ?, changed = ?, changedby = ? WHERE user = ?"; + if ($dbh->do($sql, undef, $mfa, $secret32, $recovery_codes, time, $pause->{User}{userid}, $u->{userid})) { + my $mailblurb = $c->render_to_string("email/user/mfa/edit", format => "email"); + my $header = {Subject => "User update for $u->{userid}"}; + my @to = $u->{secretemail}; + $mgr->send_mail_multi(\@to, $header, $mailblurb); + } else { + push @{$pause->{ERROR}}, sprintf(qq{Could not enter the data + into the database: %s.},$dbh->errstr); + } + $c->redirect_to('/authenquery?ACTION=mfa'); + } +} + +sub _generate_recovery_codes { + my @codes; + for (1 .. 8) { + my $code = encode_base32(urandom(6)); + $code =~ tr/lo/89/; + $code =~ s/^(.{5})/$1-/; + push @codes, $code; + } + @codes; +} + +1; diff --git a/lib/pause_2017/templates/user/mfa/edit.html.ep b/lib/pause_2017/templates/user/mfa/edit.html.ep new file mode 100644 index 000000000..2df809329 --- /dev/null +++ b/lib/pause_2017/templates/user/mfa/edit.html.ep @@ -0,0 +1,59 @@ +% layout 'layout'; +% my $pause = stash(".pause") || {}; +% my $cpan_alias = lc($pause->{HiddenUser}{userid}) . '@cpan.org'; + + + +% if (flash('mfa_enabled')) { +
+

Multifactor Authentication is enabled.

+

Recovery codes:

+ +
    +% for my $code (@{ flash('recovery_codes') }) { +
  • <%= $code %> +% } +
+ +

Please write down these codes, as they will not show again.

+
+% } elsif (flash('mfa_disabled')) { +
+

Multifactor Authentication is disabled.

+
+% } + +

<% if (!$pause->{HiddenUser}{mfa}) { %>Enable<% } else { %>Disable<% } %> Multifactor Authentication for <%= $pause->{HiddenUser}{userid} %> +% if (exists $pause->{UserGroups}{admin}) { + (lastvisit <%= $pause->{HiddenUser}{lastvisit} || "before 2005-12-02" %>) +% } +

+ +% if (my $error = $pause->{error}) { +
+ERROR: +% if ($error->{invalid_code}) { +Verification Code is invalid. +% } +
+
+% } +% if (!$pause->{HiddenUser}{mfa}) { +
+

Submit 6-digit code to enable Multifactor Authentication.

+ +
+% } else { +

Submit 6-digit code to disable Multifactor Authentication.

+<%= hidden_field "pause99_mfa_reset" => 1 %> +% } + +
+

CODE: <%= text_field "pause99_mfa_code" => '', + size => 10, + maxlength => 10, +%> +

+
+ +%= csrf_field From 34f087931aeda92ea469388994e38b4b1c05468f Mon Sep 17 00:00:00 2001 From: Kenichi Ishigaki Date: Fri, 26 Apr 2024 20:15:03 +0900 Subject: [PATCH 09/19] Add an email template for mfa --- .../templates/email/user/mfa/edit.email.ep | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 lib/pause_2017/templates/email/user/mfa/edit.email.ep diff --git a/lib/pause_2017/templates/email/user/mfa/edit.email.ep b/lib/pause_2017/templates/email/user/mfa/edit.email.ep new file mode 100644 index 000000000..454cf824d --- /dev/null +++ b/lib/pause_2017/templates/email/user/mfa/edit.email.ep @@ -0,0 +1,19 @@ +% my $pause = stash(".pause") || {}; +% +%#------------------------------------------------------------------ +% +Record update in the PAUSE users database: + +<%== sprintf "%11s: [%s]", "userid", $pause->{HiddenUser}{userid} %> + +% if ($pause->{mfa_enabled}) { +Multifactor Authentication is enabled. +% } elsif ($pause->{mfa_disabled}) { +Multifactor Authentication is disabled. +% } + +Data were entered by <%== $pause->{User}{userid} %> (<%== $pause->{User}{fullname} %>). +Please check if they are correct. + +Thanks, +The PAUSE Team From a3c6f7a5e51018f69a67acf7a5e6d553a26415f0 Mon Sep 17 00:00:00 2001 From: Kenichi Ishigaki Date: Fri, 26 Apr 2024 20:15:24 +0900 Subject: [PATCH 10/19] Add a template for authentication code --- lib/pause_2017/templates/mfa_check.html.ep | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 lib/pause_2017/templates/mfa_check.html.ep diff --git a/lib/pause_2017/templates/mfa_check.html.ep b/lib/pause_2017/templates/mfa_check.html.ep new file mode 100644 index 000000000..928aeb163 --- /dev/null +++ b/lib/pause_2017/templates/mfa_check.html.ep @@ -0,0 +1,16 @@ +% layout 'layout'; +% my $pause = stash(".pause") || {}; + +

Authentication code

+%= text_field 'otp'; + +% for my $name (@{ $c->req->params->names }) { + % for my $value (@{ $c->req->every_param($name) }) { + % next if $name eq 'ACTION'; + % next if $name eq 'otp'; + %= hidden_field $name => $value; + % } +% } + +%= submit_button 'verify'; + From d53a7e4d8cb390c3655a5ce391fbb191cf79f723 Mon Sep 17 00:00:00 2001 From: Kenichi Ishigaki Date: Fri, 26 Apr 2024 21:16:43 +0900 Subject: [PATCH 11/19] Add x_csrf_and_mfa_protection helper --- lib/pause_2017/PAUSE/Web.pm | 5 +++-- lib/pause_2017/PAUSE/Web/Config.pm | 6 ++---- lib/pause_2017/PAUSE/Web/Plugin/WithMFAProtection.pm | 7 +++++++ 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/lib/pause_2017/PAUSE/Web.pm b/lib/pause_2017/PAUSE/Web.pm index d62cda8e5..80a855849 100644 --- a/lib/pause_2017/PAUSE/Web.pm +++ b/lib/pause_2017/PAUSE/Web.pm @@ -81,8 +81,9 @@ sub startup { my $action = $app->pause->config->action($name); for my $method (qw/get post/) { my $route = $private->$method("/$name"); - $route->with_csrf_protection if $method eq "post" and $action->{x_csrf_protection}; - $route->with_mfa_protection if $method eq "post" and $action->{x_mfa_protection}; + $route->with_csrf_protection if $method eq "post" and $action->{x_csrf_protection}; + $route->with_mfa_protection if $method eq "post" and $action->{x_mfa_protection}; + $route->with_csrf_and_mfa_protection if $method eq "post" and $action->{x_csrf_and_mfa_protection}; $route->to($action->{x_mojo_to}); } } diff --git a/lib/pause_2017/PAUSE/Web/Config.pm b/lib/pause_2017/PAUSE/Web/Config.pm index b936b3db3..4df7bee8b 100644 --- a/lib/pause_2017/PAUSE/Web/Config.pm +++ b/lib/pause_2017/PAUSE/Web/Config.pm @@ -428,8 +428,7 @@ our %Actions = ( cat => "User/06Account/02", desc => "Change your password any time you want.", method => 'POST', - x_csrf_protection => 1, - x_mfa_protection => 1, + x_csrf_and_mfa_protection => 1, x_form => { HIDDENNAME => {form_type => "hidden_field"}, ABRA => {form_type => "hidden_field"}, @@ -445,8 +444,7 @@ our %Actions = ( cat => "User/06Account/01", desc => "Edit your user name, your email addresses (both public and secret one), change the URL of your homepage.", method => 'POST', - x_csrf_protection => 1, - x_mfa_protection => 1, + x_csrf_and_mfa_protection => 1, x_form => { HIDDENNAME => {form_type => "hidden_field"}, pause99_edit_cred_fullname => {form_type => "text_field"}, diff --git a/lib/pause_2017/PAUSE/Web/Plugin/WithMFAProtection.pm b/lib/pause_2017/PAUSE/Web/Plugin/WithMFAProtection.pm index 60717728b..fe4a1a0e8 100644 --- a/lib/pause_2017/PAUSE/Web/Plugin/WithMFAProtection.pm +++ b/lib/pause_2017/PAUSE/Web/Plugin/WithMFAProtection.pm @@ -80,6 +80,13 @@ sub register { } ); + $routes->add_shortcut( + with_csrf_and_mfa_protection => sub { + my ($route) = @_; + return $route->requires( with_csrf_protection => 1, with_mfa_protection => 1 ); + } + ); + return; } From 684f2f05c7474d86c4d31f7736ee9b0bcb712a60 Mon Sep 17 00:00:00 2001 From: Kenichi Ishigaki Date: Sun, 28 Apr 2024 21:14:13 +0900 Subject: [PATCH 12/19] Invert the mfa verification logic (wolfsage++) --- lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm b/lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm index 90eaf8e19..e30b5c832 100644 --- a/lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm +++ b/lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm @@ -26,16 +26,19 @@ sub edit { if (uc $req->method eq 'POST' and $req->param("pause99_mfa_sub")) { my $code = $req->param("pause99_mfa_code"); $req->param("pause99_mfa_code", undef); - if ($code =~ /\A[0-9]{6}\z/ && !$auth->verify($code)) { - $pause->{error}{invalid_code} = 1; - return; + my $verified; + if ($code =~ /\A[0-9]{6}\z/ && $auth->verify($code)) { + $verified = 1; } elsif ($code =~ /\A[a-z0-9]{5}\-[a-z0-9]{5}\z/ && $u->{mfa_recovery_codes} && $req->param("pause99_mfa_reset")) { my @recovery_codes = split / /, $u->{mfa_recovery_codes} // ''; - if (!grep { PAUSE::Crypt::password_verify($code, $_) } @recovery_codes) { - $pause->{error}{invalid_code} = 1; - return; + if (grep { PAUSE::Crypt::password_verify($code, $_) } @recovery_codes) { + $verified = 1; } } + unless ($verified) { + $pause->{error}{invalid_code} = 1; + return; + } my ($mfa, $secret32, $recovery_codes); if ($req->param("pause99_mfa_reset")) { $mfa = 0; From ca5ea284250293120610be611bbf0302292ff933 Mon Sep 17 00:00:00 2001 From: Kenichi Ishigaki Date: Sun, 28 Apr 2024 21:15:22 +0900 Subject: [PATCH 13/19] Remove an unused variable from mfa/edit.html.ep --- lib/pause_2017/templates/user/mfa/edit.html.ep | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/pause_2017/templates/user/mfa/edit.html.ep b/lib/pause_2017/templates/user/mfa/edit.html.ep index 2df809329..f256f5b77 100644 --- a/lib/pause_2017/templates/user/mfa/edit.html.ep +++ b/lib/pause_2017/templates/user/mfa/edit.html.ep @@ -1,6 +1,5 @@ % layout 'layout'; % my $pause = stash(".pause") || {}; -% my $cpan_alias = lc($pause->{HiddenUser}{userid}) . '@cpan.org'; From efdf93115a3fec43c8291f39c4791e68c6689fc0 Mon Sep 17 00:00:00 2001 From: Kenichi Ishigaki Date: Sun, 28 Apr 2024 21:16:56 +0900 Subject: [PATCH 14/19] Fix the length of mfa_code field to match the current length of recovery codes --- lib/pause_2017/templates/user/mfa/edit.html.ep | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/pause_2017/templates/user/mfa/edit.html.ep b/lib/pause_2017/templates/user/mfa/edit.html.ep index f256f5b77..698bb4749 100644 --- a/lib/pause_2017/templates/user/mfa/edit.html.ep +++ b/lib/pause_2017/templates/user/mfa/edit.html.ep @@ -49,8 +49,8 @@ Verification Code is invalid.

CODE: <%= text_field "pause99_mfa_code" => '', - size => 10, - maxlength => 10, + size => 11, + maxlength => 11, %>

From abb0141293fe982fd641110611e3da0fb04bca1a Mon Sep 17 00:00:00 2001 From: Kenichi Ishigaki Date: Sun, 28 Apr 2024 21:19:36 +0900 Subject: [PATCH 15/19] PAUSE userid should be enough for the key_id passed to Auth::GoogleAuth --- lib/pause_2017/PAUSE/Web/Context.pm | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/pause_2017/PAUSE/Web/Context.pm b/lib/pause_2017/PAUSE/Web/Context.pm index 324f7dc9d..cb104bb8a 100644 --- a/lib/pause_2017/PAUSE/Web/Context.pm +++ b/lib/pause_2017/PAUSE/Web/Context.pm @@ -43,12 +43,12 @@ sub version { sub authenticator_for { my ($self, $user) = @_; - my $cpan_alias = lc($user->{userid}) . '@cpan.org'; - my $secret32 = $user->{mfa_secret32}; + my $userid = lc($user->{userid}); + my $secret32 = $user->{mfa_secret32}; return Auth::GoogleAuth->new({ secret32 => $secret32, issuer => $PAUSE::Config->{MFA_ISSUER} || 'PAUSE', - key_id => $cpan_alias, + key_id => $userid, }); } From c2472f1afce567909548f37d56d3f0d3cf4a74ec Mon Sep 17 00:00:00 2001 From: Kenichi Ishigaki Date: Sun, 28 Apr 2024 21:58:33 +0900 Subject: [PATCH 16/19] Generate qrcode image by ourselves --- lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm b/lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm index e30b5c832..903efa2c9 100644 --- a/lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm +++ b/lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm @@ -5,6 +5,8 @@ use Auth::GoogleAuth; use PAUSE::Crypt; use Crypt::URandom qw(urandom); use Convert::Base32 qw(encode_base32); +use Imager::QRCode qw(plot_qrcode); +use URI; sub edit { my $c = shift; @@ -14,7 +16,7 @@ sub edit { my $u = $c->active_user_record; my $auth = $c->app->pause->authenticator_for($u); - $pause->{mfa_qrcode} = $auth->qr_code; + $pause->{mfa_qrcode} = _generate_qrcode($auth); if (!$u->{mfa_secret32}) { my $dbh = $mgr->authen_connect; my $tbl = $PAUSE::Config->{AUTHEN_USER_TABLE}; @@ -80,4 +82,16 @@ sub _generate_recovery_codes { @codes; } +# using $auth->qr_code directly is handy but insecure +sub _generate_qrcode { + my $auth = shift; + my $otpauth = $auth->qr_code(undef, undef, undef, 1); + my $img = plot_qrcode($otpauth, { casesensitive => 1 }); + $img->write(data => \my $qr_png, type => 'png') or die "Failed to write image: " . $img->errstr; + my $data = URI->new("data:"); + $data->data($qr_png); + $data->media_type('image/png'); + $data; +} + 1; From a93739efa8f1cc3042fcc376b9dbbc044f217b15 Mon Sep 17 00:00:00 2001 From: Kenichi Ishigaki Date: Sun, 28 Apr 2024 22:11:12 +0900 Subject: [PATCH 17/19] Requires Imager::QRCode --- cpanfile | 1 + 1 file changed, 1 insertion(+) diff --git a/cpanfile b/cpanfile index 38f4d4550..a8da8a4e6 100644 --- a/cpanfile +++ b/cpanfile @@ -23,6 +23,7 @@ requires 'HTML::Entities'; requires 'HTTP::Date'; requires 'HTTP::Status'; requires 'HTTP::Tiny', '0.059'; +requires 'Imager::QRCode'; requires 'IO::Socket::SSL', '1.56'; requires 'IPC::Run3'; requires 'JSON'; From 0d09dbd3f79e836564bc85b971a97f2242bd5def Mon Sep 17 00:00:00 2001 From: Kenichi Ishigaki Date: Sun, 28 Apr 2024 22:15:28 +0900 Subject: [PATCH 18/19] Modify qrcode parameters --- lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm b/lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm index 903efa2c9..e13adcc0b 100644 --- a/lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm +++ b/lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm @@ -86,7 +86,7 @@ sub _generate_recovery_codes { sub _generate_qrcode { my $auth = shift; my $otpauth = $auth->qr_code(undef, undef, undef, 1); - my $img = plot_qrcode($otpauth, { casesensitive => 1 }); + my $img = plot_qrcode($otpauth, { casesensitive => 1, size => 4, margin => 4, version => 1, level => 'M' }); $img->write(data => \my $qr_png, type => 'png') or die "Failed to write image: " . $img->errstr; my $data = URI->new("data:"); $data->data($qr_png); From e1549a564d4e9ccd3aac35c2f730c14e71f77a7b Mon Sep 17 00:00:00 2001 From: Kenichi Ishigaki Date: Sun, 28 Apr 2024 23:01:09 +0900 Subject: [PATCH 19/19] Remove mfa column and use mfa_secret32 instead --- doc/authen_pause.schema.txt | 1 - doc/schemas/authen_pause.schema.sqlite | 1 - lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm | 8 +++----- lib/pause_2017/PAUSE/Web/Plugin/WithMFAProtection.pm | 2 +- lib/pause_2017/templates/user/mfa/edit.html.ep | 4 ++-- one-off-utils/schemachange-2024-04_2.sql | 1 - 6 files changed, 6 insertions(+), 11 deletions(-) diff --git a/doc/authen_pause.schema.txt b/doc/authen_pause.schema.txt index 450ed8205..a801abf3e 100644 --- a/doc/authen_pause.schema.txt +++ b/doc/authen_pause.schema.txt @@ -56,7 +56,6 @@ CREATE TABLE usertable ( `changed` int(11) DEFAULT NULL, changedby char(10) DEFAULT NULL, lastvisit datetime DEFAULT NULL, - mfa tinyint(1) DEFAULT 0, mfa_secret32 varchar(16) DEFAULT NULL, mfa_recovery_codes text DEFAULT NULL, PRIMARY KEY (`user`), diff --git a/doc/schemas/authen_pause.schema.sqlite b/doc/schemas/authen_pause.schema.sqlite index 36ae5725b..a1046d5be 100644 --- a/doc/schemas/authen_pause.schema.sqlite +++ b/doc/schemas/authen_pause.schema.sqlite @@ -36,7 +36,6 @@ CREATE TABLE usertable ( changed int(11) DEFAULT NULL, changedby char(10) DEFAULT NULL, lastvisit datetime DEFAULT NULL, - mfa tinyint(1) DEFAULT 0, mfa_secret32 varchar(16) DEFAULT NULL, mfa_recovery_codes text DEFAULT NULL, PRIMARY KEY (user) diff --git a/lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm b/lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm index e13adcc0b..a0e48f78e 100644 --- a/lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm +++ b/lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm @@ -41,14 +41,12 @@ sub edit { $pause->{error}{invalid_code} = 1; return; } - my ($mfa, $secret32, $recovery_codes); + my ($secret32, $recovery_codes); if ($req->param("pause99_mfa_reset")) { - $mfa = 0; $secret32 = undef; $recovery_codes = undef; $c->flash(mfa_disabled => 1); } else { - $mfa = 1; $secret32 = $auth->secret32; $c->flash(mfa_enabled => 1); my @codes = _generate_recovery_codes(); @@ -57,8 +55,8 @@ sub edit { } my $dbh = $mgr->authen_connect; my $tbl = $PAUSE::Config->{AUTHEN_USER_TABLE}; - my $sql = "UPDATE $tbl SET mfa = ?, mfa_secret32 = ?, mfa_recovery_codes = ?, changed = ?, changedby = ? WHERE user = ?"; - if ($dbh->do($sql, undef, $mfa, $secret32, $recovery_codes, time, $pause->{User}{userid}, $u->{userid})) { + my $sql = "UPDATE $tbl SET mfa_secret32 = ?, mfa_recovery_codes = ?, changed = ?, changedby = ? WHERE user = ?"; + if ($dbh->do($sql, undef, $secret32, $recovery_codes, time, $pause->{User}{userid}, $u->{userid})) { my $mailblurb = $c->render_to_string("email/user/mfa/edit", format => "email"); my $header = {Subject => "User update for $u->{userid}"}; my @to = $u->{secretemail}; diff --git a/lib/pause_2017/PAUSE/Web/Plugin/WithMFAProtection.pm b/lib/pause_2017/PAUSE/Web/Plugin/WithMFAProtection.pm index fe4a1a0e8..95f0ae171 100644 --- a/lib/pause_2017/PAUSE/Web/Plugin/WithMFAProtection.pm +++ b/lib/pause_2017/PAUSE/Web/Plugin/WithMFAProtection.pm @@ -16,7 +16,7 @@ sub register { my $u = $c->active_user_record; # XXX: The active user record does not have mfa when an admin user is pretending someone else. - return 1 unless $u->{mfa}; + return 1 unless $u->{mfa_secret32}; my $otp = $c->req->body_params->param('otp'); if (defined $otp and $otp ne '') { diff --git a/lib/pause_2017/templates/user/mfa/edit.html.ep b/lib/pause_2017/templates/user/mfa/edit.html.ep index 698bb4749..452f85cf5 100644 --- a/lib/pause_2017/templates/user/mfa/edit.html.ep +++ b/lib/pause_2017/templates/user/mfa/edit.html.ep @@ -22,7 +22,7 @@ % } -

<% if (!$pause->{HiddenUser}{mfa}) { %>Enable<% } else { %>Disable<% } %> Multifactor Authentication for <%= $pause->{HiddenUser}{userid} %> +

<% if (!$pause->{HiddenUser}{mfa_secret32}) { %>Enable<% } else { %>Disable<% } %> Multifactor Authentication for <%= $pause->{HiddenUser}{userid} %> % if (exists $pause->{UserGroups}{admin}) { (lastvisit <%= $pause->{HiddenUser}{lastvisit} || "before 2005-12-02" %>) % } @@ -37,7 +37,7 @@ Verification Code is invalid.
% } -% if (!$pause->{HiddenUser}{mfa}) { +% if (!$pause->{HiddenUser}{mfa_secret32}) {

Submit 6-digit code to enable Multifactor Authentication.

diff --git a/one-off-utils/schemachange-2024-04_2.sql b/one-off-utils/schemachange-2024-04_2.sql index b981d9722..241660ce1 100644 --- a/one-off-utils/schemachange-2024-04_2.sql +++ b/one-off-utils/schemachange-2024-04_2.sql @@ -1,3 +1,2 @@ -ALTER TABLE usertable ADD COLUMN mfa tinyint(1) DEFAULT 0; ALTER TABLE usertable ADD COLUMN mfa_secret32 varchar(16); ALTER TABLE usertable ADD COLUMN mfa_recovery_codes text;