[pve-devel] [PATCH v3 apiclient] fix #2227: enable totp codes to be passed in cli

[Wolfgang Bumiller]

On Wed, Aug 14, 2019 at 02:15:40PM +0200, Thomas Lamprecht wrote:
> Am 7/18/19 um 4:44 PM schrieb Oguz Bektas:
> > this patch enables to pass totp codes during cluster join if tfa has been
> > enabled for root at pam (or any other user actually, but root seems to cause the
> > most problems).
> > 
> > u2f support is still not implemented.
> > 
> > Co-developed-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
> > Co-developed-by: Wolfgang Bumiller <w.bumiller at proxmox.com>
> > Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
> > ---
> > 
> > v2 -> v3:
> > implement thomas' suggestions, namely:
> > * factor out code and handle it outside of the login func
> > * pass $type and $challenge after detecting a two-factor-auth ticket
> > (the detection regex can be improved i think, but i'll leave it like
> > this for now)
> > * also if $type isn't detected, abort after doing a raise
> > 
> >  PVE/APIClient/LWP.pm | 30 ++++++++++++++++++++++++------
> >  1 file changed, 24 insertions(+), 6 deletions(-)
> > 
> > diff --git a/PVE/APIClient/LWP.pm b/PVE/APIClient/LWP.pm
> > index c0e30ff..2cf7ac7 100755
> > --- a/PVE/APIClient/LWP.pm
> > +++ b/PVE/APIClient/LWP.pm
> > @@ -92,6 +92,23 @@ sub update_ticket {
> >      $agent->default_header('Cookie', $cookie);
> >  }
> >  
> > +sub two_factor_auth_login {
> > +    my ($self, $type, $challenge) = @_;
> > +
> > +    if ($type eq 'PVE:tfa') {
> > +	raise("TFA-enabled join currently works only with a TTY.") if !-t STDIN;
> 
> This has nothing to do with a PVE node join, it hits us there, yes,
> but that's only the case because we currently only use the apiclient
> there.
> But we may ecxtend it's use in the future, and also other, external
> people and projects may already use it for different cases.
> So please adapt the error message(s) to something more general and
> fitting, i.e., you're doing a login which failed.
> 
> > +	print "\nEnter OTP code for user $self->{username}: ";
> > +	my $tfa_response = <STDIN>;
> > +	chomp $tfa_response;
> > +	return $self->post('/api2/json/access/tfa', {response => $tfa_response});
> > +    } elsif ($type eq 'PVE:u2f') {
> > +	# TODO: implement u2f-enabled join
> > +	raise("U2F-enabled join is currently not implemented.");
> 
> same here
> 
> > +    } else {
> > +	raise("Authentication type not recognized, aborting!");
> 
> maybe include the unrecognised type in the error to help us understand
> why someone ran into this, if it happens.
> 
> > +    }
> > +}
> > +
> >  sub login {
> >      my ($self) = @_;
> >  
> > @@ -129,15 +146,16 @@ sub login {
> >      my $res = from_json($response->decoded_content, {utf8 => 1, allow_nonref => 1});
> >  
> >      my $data = $extract_data->($res);
> > -
> > -    # TODO: make it possible to use tfa
> > -    if ($data->{ticket} =~ m/^PVE:tfa!/) {
> > -	raise("Two Factor Auth is not yet implemented! Try disabling TFA for the user '$username'.\n");
> > -    }
> > -
> >      $self->update_ticket($data->{ticket});
> >      $self->update_csrftoken($data->{CSRFPreventionToken});
> >  
> > +    # handle two-factor login
> > +    if ($data->{ticket} =~ m{^([^\s!]+)![^!]*(!([0-9a-zA-Z/.=_\-+]+))?$}) {
> 
> maybe a
> 
> my $tfa_ticket_re = qr/^([^\s!]+)![^!]*(!([0-9a-zA-Z/.=_\-+]+))?$/;

While the base64 pattern there is currently only used specifically with u2f
($data =~ /^u2f!...), whereas the 'tfa' case is actually matched as
`$data =~ /^tfa!(.*)$/` in the code currently. (Iow. the number of
"parameters" and restrictions on the content depends on the authentication
type.)

So rather thant restricting the tail, just end with `(?:!(.*))$`, also note
that you are including the '!' character in $2.

> if ($data->{ticket} =~ m/$tfa_ticket_re/) ...

No need to for the m//, jus use =~ $tfa_ticket_re ;-)