Creating a Sudo() Client copy leads to nil pointer dereference

[kaplan-michael]

I create the connections like this. yet when I check with a debugger, the SudoClient seems to have nil as it's connection. I'm guessing it should either have a pointer to the original, or a copy of the original one? Is it a bug or am I doing something wrong?

// setupHost configures and connects to a single host
func setupHost(hostConfig *HostConfig) (*Host, error) {
	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
	defer cancel()

	conn, err := createConnection(hostConfig)
	if err != nil {
		return nil, err
	}
	client, err := rig.NewClient(rig.WithConnection(conn), rig.WithLogger(logger.Slog))
	if err != nil {
		return nil, fmt.Errorf("failed to create rig client: %w", err)
	}

	err = client.Connect(ctx)
	if err != nil {
		return nil, fmt.Errorf("failed to connect: %w", err)
	}

	// Initialize sudo client at the same time
	sudoClient := client.Sudo()
	if sudoClient == nil {
		return nil, fmt.Errorf("failed to initialize sudo client")
	}

	return &Host{Client: client, Sudo: sudoClient, Vars: hostConfig.Vars, Fs: remotefs.NewFS(client)}, nil
}

[kaplan-michael]

From looking at it, it probably doesn't clone all required bits(doesn't init them) https://github.com/k0sproject/rig/blob/main/client.go#L252

so some internal Client bits end up being nil.

edit: seems the WithConnection, etc. don't properly propagate it, no clue why yet.

[kaplan-michael]

I think it is missing a call to the setup() method on the cloned client, hence the options don't get propagated.
not sure what is the best place for it though.

func (c *Client) Clone(opts ...ClientOption) *Client {
	options := c.options.Clone()
	options.Apply(opts...)
	client := &Client{options: options}
	if err := client.setup(); err != nil {
		return nil
	}
	return client
}

[kke]

Hmm, setup does c.Runner = c.options.GetRunner(c.connection) which will overwrite the sudo Runner 🤔

Doesn't the nil panic backtrace show what is nil?

The Client already memoizes the sudoClone when you do h.Sudo(), so keeping your own copy of it shouldn't be necessary. But it probably doesn't solve this nil problem.

[kaplan-michael]

I have tried to not keeping my copy and it behaved the same, so I started doing that so it's easier to debug.

The backtrace wasn't very helpful(I'll try to share later), but from what I found I guess either the connection/the runner is nil. It doesn't get applied from the options to the Sudo client itself(and I assume something is trying to use that)

[kke]

I think @james-nesbitt solved it, PR incoming

[james-nesbitt]

#199 makes sure that any client.Clone() has .setup() run before returning, which includes .Sudo()

[james-nesbitt]

oh. I didn't check the error as @kaplan-michael suggested. Let me update that.

Edit: actually, I think that the error catch is superfluous and maybe a lint ignore is better.

[kaplan-michael]

Thanks @james-nesbitt. works good.
I do have a question in terms of sudo, do you plan/want to support sudo with a password? probably injected through stdin?

[james-nesbitt]

for encrypted private keys I use ssh-agent and pre-load the key.

[kke]

I do have a question in terms of sudo, do you plan/want to support sudo with a password? probably injected through stdin?

I'm undecided. I suppose it wouldn't be that hard to do, the biggest hurdle is when the sudo times out (could be after each command) and password is needed again, so the password would have to be kept in memory. Something like https://github.com/awnumar/memguard could help in doing it somewhat securely.

[kaplan-michael]

yeah, I do agree it would be nice to use memguard, but I feel like any password that are passed in will be through a flag or some kind of config, so I'm not sure if it justifies the effort to then try to keep it safe in memory? + It would be in memory on the system rig is running, which I guess in most cases is a single user personal system, so that mitigates it a bit more?