Add support for reading pwd from Windows and Linux/GNOME keyrings

[giulianopz]

Use zalando/go-keyring to support targets different from macOS.

Please, note that the PR introduces new flags to properly tell apart the different credentials sources (or no credentials at all).

This should improve user experience IMHO: before passing the domain flag implicitly meant "prompt me for a password" and a failure when retrieving the credentials from a source was handled by just disabling proxy auth.

The README was updated accordingly.

[samuong]

Hi @giulianopz, thanks for sending this! It would be great to be able to support Linux and Windows.

I'm travelling at the moment, so apologies in advance, I might not be able to get back to you very quickly.

Please, note that the PR introduces new flags to properly tell apart the different credentials sources (or no credentials at all).

This should improve user experience IMHO: before passing the domain flag implicitly meant "prompt me for a password" and a failure when retrieving the credentials from a source was handled by just disabling proxy auth.

I see what you mean, it makes sense to have explicit flags for each source of credentials. The way it currently works is the result of evolution, with various credential sources being added over time.

That said, I'm reluctant to change this as the existing behaviour has been documented (e.g. internal confluence pages) and scripted (e.g. in shell scripts and launchd agents). I'm not sure exactly how much would break if we change the command-line flags now, but I know of at least a few examples. Are you ok to leave the changes to the flags out of this PR?

[giulianopz]

Hi @samuong,

I removed the -k flag, but I cannot remove the -i flag since I need the domain (and the username) to be passed also in the case of the keyring used as the credentials source. In fact, I was forced to move both variables to a global scope to be accessed in keyring.go. This is not needed on macOS because you resort to NoMAD to retrieve them.

To remove the -i flag, the code should be refactored a little bit to silently try fetching the password from shell env or keyring, using the shell prompt just as a fallback if the two previous options are not available.

What do you think?

[samuong]

Just one more thing, I still feel hesitant to add the -i flag in this way. The existing behaviour has also been documented in lot of internal wiki pages (which I have no way of updating) and requiring -i will break the existing behaviour for people who are reading these docs. Also the "interactive" behaviour is the only option that is supported on all platforms, and doesn't require any prior configuration, so I feel it should be the default when running alpaca with no flags.

For what it's worth, the original behaviour might make more sense if you think of it like this:

1. If the user specifies the domain from the terminal (via a flag) then we also get the username (either from a flag, or from their local username) and password (from an interactive prompt) from the terminal.
2. Otherwise, if they have an env var set, then get all three values from that env var.
3. Otherwise, if they're using NoMAD on a Mac, then get all three values from NoMAD's keychain entries.

In all cases, all three values come from the same place, which is either the terminal, the environment, or NoMAD.

But it starts to get confusing with the -i and -k flags because in the case of -k, only the password comes from the keyring, but the domain and username come from the command-line flags.

So I think it might make more sense to fetch all of the values (including domain and username) from the keyring, and also separate these from NoMAD's entries. So the logic in main() goes back to looking like this:

	if *domain != "" {
		src = fromTerminal().forUser(*domain, *username)
	} else if value := os.Getenv("NTLM_CREDENTIALS"); value != "" {
		src = fromEnvVar(value)
	} else {
		src = fromKeyring()
	}

And then in keyring.GetCredentials(), we have something like:

domain, err := ring.Get("alpaca", "domain")
// check err
username, err := ring.Get("alpaca", "username")
// check err
password, err := ring.Get("alpaca", "password")
// check err

So this would achieve two things: 1) we maintain the existing defaults for those that rely on it, and 2) all the credentials come from a single source. What do you think?

[giulianopz]

Hi @samuong,

That library doesn't let you search for a secret by arbitrary attributes: the second argument passed to Get is always expected to be the "username" of a given "service", see here. Anyway, I don't know if storing username and domain in the keyring would be the best thing to do. They should probably be read from a config file.

I don't want to force you to introduce breaking changes. At the end I can use my own fork, that's enough for me. So, feel free to just close this PR.

[samuong]

Hi @giulianopz, I agree that the domain and username would be better off in a config file than in the keyring. Unfortunately alpaca doesn't have a config file so we don't have anywhere to put this.

I'd be interested in merging this feature if you still are, maybe one way to do it is to just have the logic of:

1. If the domain is on the command line, then check if the entry is in the keyring, otherwise prompt the user.
2. Otherwise check the environment variable.
3. Otherwise try the nomad entries.

This is a bigger change though, as it requires having a separate credential source for the keyring and for nomad. If you're keen on giving that a go, let me know?

[giulianopz]

Hi @samuong, probably reading username and domain as environment variables could be an adequate workaround to avoid the refactoring you proposed. I changed a few lines to do so in 2e27810.