Rex icon indicating copy to clipboard operation
Rex copied to clipboard

INI file with server auth setting sudo=true does not use sudo

Open zmughal opened this issue 4 years ago • 1 comments

Describe the bug

Using sudo=true in an INI file as in the example for Rex::Group::Lookup::INI does not use sudo because it checks if the value is == 1, but this is a numeric comparison so "true" == 1 is false.

How to reproduce it

~~Steps to reproduce the behavior:~~

Shortest code example that demonstrates the bug:

# [= Rexfile =]
use Rex -feature => ['1.4','use_server_auth'];

use Rex::Group::Lookup::INI;

groups_file 'server-min.ini';

desc 'Install cpanminus';
task 'cpanminus', group => 'instr', sub {
	pkg "cpanminus", ensure => "present";
};
; [= server-min.ini =]
[instr]
;;; The below works without any Perl warnings

instrument-remote sudo=1


;;; Using the below instead says
;;;
;;; Argument "true" isn't numeric in numeric eq (==)
;;; at Rex/Interface/Connection/OpenSSH.pm
;;; on the line
;;;
;;;   if ( $self->{is_sudo} && $self->{is_sudo} == 1 ) {
;;;
;;; and
;;;
;;; Argument "true" isn't numeric in numeric eq (==)
;;; at Rex.pm
;;; on the line
;;;
;;;  if ( exists $CONNECTION_STACK[-1]->{server}->{auth}->{sudo}
;;;    && $CONNECTION_STACK[-1]->{server}->{auth}->{sudo} == 1 )
;;;

; instrument-remote sudo=true

Expected behavior

Should use sudo when sudo=true is in the INI file.

Per discussion on IRC with @ferki:

[ FErki[m]] at first glance, one possible fix might be the following:
[ FErki[m]] - change the test at https://github.com/RexOps/Rex/blob/master/t/ini.t#L100 to check for
            the value being `1`
[ FErki[m]] - change https://github.com/RexOps/Rex/blob/master/lib/Rex/Group/Entry/Server.pm#L78-L81
            to recognize the `"true"` string, and assign the value of `1` to the internal
            `$self->{auth}->{sudo}` (and perhaps add logic to recognize `"false"` as 0 as well)
[ FErki[m]] alternatively, we could clarify the docs (and the tests) that the value should be 0 or 1,
            but `true/false` seems to be more human-friendly to me in the context of auth info inside
            INI files
[ FErki[m]] a good next step could be to open a formal bug report as a GitHub issue to make it more
            visible for others (including the workaround) and to further discuss/design actual fixing
            steps

Circumstances

  • Rex version: (R)?ex 1.13.5
  • Perl version: perl 5, version 26, subversion 1 (v5.26.1) built for x86_64-linux
  • OS running rex: Debian GNU/Linux 5.15.0 amd64
  • OS managed by rex: Debian GNU/Linux 5.10.17 armv7l
  • How rex was installed: CPAN client

Debug log

zmughal avatar Dec 19 '21 23:12 zmughal

Thanks for your report, @zmughal!

I'll take a look at the tests and related code + docs to see how it's best to address this.

If I find it possible without breaking any backwards compatibility, I'll probably go for supporting true and yes as 1, and false and no as 0 values, while passing anything else as-is.

It's unlikely, but if something blocks this, I'll update the docs instead.

ferki avatar Jan 22 '22 00:01 ferki