command: Fix panic for nil credentials source

[alisdair]

If loading credentials fails, the credentials source can be nil. Calling any of its methods would then result in a panic. This commit adds nil receiver checks to each of the public methods to cope with this case.

The user-facing bug that triggered this work is that running init when the HOME environment variable is unset would result in a panic on macOS.

The terraform-svchost package does attempt to nil-check the credentials source before calling its methods. But since the credentials source is an interface, this nil check is always false.

I think the correct fix here is for all implementations of the interface to cope with a nil value, rather than trying to use reflection or similar in svchost. Assuming that's agreed-upon, I'll create a PR to svchost to remove these checks.

Fixes #24520.

[codecov]

Codecov Report

Merging #25110 into master will increase coverage by 0.00%.
The diff coverage is 60.00%.

Impacted Files	Coverage Δ
main.go	34.05% <60.00%> (+0.42%)	⬆️

[jbardin]

This seems a bit unusual to handle an incorrectly assigned interface value by checking all receivers. If the original bug isn't fixed, any other implementations would also need to check every receiver call. Can we find out where the dynamic type is assigned and check for nil there?

[alisdair]

@jbardin Sorry, I don't completely understand your review comment. Is the change in f1f24df what you were thinking of? If not, could you clarify what you mean by "where the dynamic type is assigned"?

Unfortunately I don't think we can run an automated test for this approach, but I've verified it manually locally.

[jbardin]

That's more what I had in mind, but I don't think it prevents the issue, because the dynamic type is added to the interface here:

terraform/commands.go

Line 415 in 6fbd394

func credentialsSource(config *cliconfig.Config) (auth.CredentialsSource, error) {

That function should probably return a *cliconfig.CredentialsSource (following the axiom "accept interfaces and return structs"), though returning an explicit null there would prevent the assignment of the type too.

[alisdair]

I'm pretty sure it does prevent the issue (I've locally tested it with a repro for #24520). Note that we're not passing the return value of credentialsSource to disco.NewWithCredentialsSource if its value is (typed) nil; instead we pass an explicit nil literal to ensure that it is untyped.

Changing the return type of credentialsSource to the concrete type doesn't fix the issue. I am beyond my understanding of the dark recesses of Go's type system now, but I think it may be caused by assigning the value to the disco struct member, which has an interface type, thus adding the dynamic type to the nil pointer.

Below are some debug lines.

(Code which generates these logs)

	credsSrc, err := credentialsSource(config)
	if err != nil {
		// Most commands don't actually need credentials, and most situations
		// that would get us here would already have been reported by the config
		// loading above, so we'll just log this one as an aid to debugging
		// in the unlikely event that it _does_ arise.
		log.Printf("[WARN] Cannot initialize remote host credentials manager: %s", err)
		log.Printf("[DEBUG] credsSrc = %#v, == nil = %v", credsSrc, credsSrc == nil)
	}
	services := disco.NewWithCredentialsSource(credsSrc)
	{
		cs := services.CredentialsSource()
		log.Printf("[DEBUG] services.CredentialsSource() = %#v, == nil = %v", cs, cs == nil)
	}

First, the return value from the new func credentialsSource(config *cliconfig.Config) (*cliconfig.CredentialsSource, error):

2020/06/03 11:02:27 [DEBUG] credsSrc = (*cliconfig.CredentialsSource)(nil), == nil = true

Seems okay so far: it's a typed nil, but comparing it to nil gives the expected result.

Now if we immediately call services.CredentialsSource() to get the return value:

2020/06/03 11:02:27 [DEBUG] services.CredentialsSource() = (*cliconfig.CredentialsSource)(nil), == nil = false

This should return an auth.StaticCredentialsSource, but instead it's the same typed nil we passed in, and now it is not equal to nil.

[jbardin]

Sorry, I didn't mean to confuse the issue further; I was thinking you were checking the value was nil rather than the error. You're correct that this will fix the problem.

The root of the problem is technically the credentialsSource not returning nil. However because a return value should generally not be used when an error is returned, calling NewWithCredentialsSource with nil is also the correct thing to do here as well.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.