Fix caching of default credentials

[kerneis]

Two independent changes, please let me know if you'd rather have two separate pull requests:

• Make caching work with GOOGLE_APPLICATION_CREDENTIALS
• Fix oauth2l test

[shinfan]

Could you please provide more details, like a repro example, about the caching issue of the default location?

[andyrzhao]

I think we should separate the 2 changes. The first change to use exit code for "oauth2l test" is good in theory, but it would not be backwards compatible with older scripts that relied on "stdout" for the test function right? I'll let shinfan comment on whether this is acceptable.

As for the second change to support robust caching for GOOGLE_APPLICATION_CREDENTIALS: If my understanding is correct, the existing behavior used an "empty json" as part of the cache key when caching for GOOGLE_APPLICATION_CREDENTIALS (via util/cache.go), so only 1 version of the cached token is possible. And your proposed change allows us to cache multiple tokens obtained through different GOOGLE_APPLICATION_CREDENTIALS. I think in this particular scenario, it would probably be better to either not use caching or "reset" the cache beforehand - both of which are already supported. If the default credentials get changed, I doubt that it will ever be changed back to the previous state, which would make the cache on the old key useless. What do you think?

[kerneis]

I've split the test change in #85 and updated this pull request to focus on the caching change alone.

@andyrzhao has correctly described the change. Here is our use case:

• we have several environments (eg. prod and test)
• we have a playbook to debug or investigate issues, with a number of command-line snippets to copy-paste (those are not full-fledged scripts because remember the debugging, interactive nature of this task—which is also specifically what oauth2l is about, according to oauth2l is unnecessarily interactive, which gets in the way of automation #61 for instance),
• instructions in this playbook typically ask the user to set set GOOGLE_APPLICATION_CREDENTIALS to a json file they download from the web (to a location of your choice) for the appropriate environment (test or prod), and then use oauth2l --scope openid curl ... or something equivalent.

It is only natural for a user want to test in multiple environments to compare results, either by redefining the global variable or by switching between different terminal sessions. Having the cache unaware of those environment changes is surprising, which is harmful especially while debugging and violates the POLA. Having the cache disabled for such cases would break very legitimate use cases. In particular, any usage of the form $(oauth2l fetch --scope openid) would silently stall.

A work-around for us would be to explicitly maintain separate caches (which is more tedious), or use a separate shell variable and pass it explicitly with --credentials. But that wouldn't solve the similar violation of POLA for other users in the field (this PR originated from me being very puzzled for about 20 minutes until I figured out what the issue was).

The only downside I can see with the propose PR is that I was afraid to break existing workflows I may not envision or understand, and didn't go as far as failing the whole command if the JSON resolution fails. That means we could still corrupt the cache with empty json keys, eg. in the case of subtle race conditions where the environment changes between the two resolutions. I'm open to making that change if you think it's better to ensure we have exactly one default resolution for any command call.

[sethvargo]

Could you use oauth2l ... --cache=/dev/null to disable the cache?

[kerneis]

I could use --cache= to disable the cache but that doesn't solve my use case, which is to complete the interactive step once, and then rely on the cached value for non interactive commands, such as (simplified example): grpc_cli --credential=$(oauth2l fetch openid) dns:/// myservice.googleapis.com. Also it wouldn't the solve the confusion introduced by having a cache that doesn't cache the correct value and returns surprising results.

…

[andyrzhao]

Understood. I agree that the current caching behavior for default credentials is "broken" and should be fixed. I'd like to make 2 recommendations however:

1. For your particular use-case of toggling between credentials, using the explicit "--credentials" parameter is preferable and more predictable than updating default credentials env variable, which is intended to be static. Unless you are using GOOGLE_APPLICATION_CREDENTIALS for other tools besides OAuth2l at the same time, it is probably better to update your playbook to use the explicit parameter.
2. The caching logic in util/cache.go probably has other subtle issues that need to be fixed, such as how we construct the cache key in general. With this in mind, it is probably better to put your resolution logic inside util/cache.go instead of main.go to keep everything in one place. What do you think?

[kerneis]

For your particular use-case of toggling between credentials, using the

explicit "--credentials" parameter is preferable and more predictable Agreed, thanks.

it is probably better to put your resolution logic inside util/cache.go

instead of main.go That's what I did initially. The reason I changed my mind is that I wanted to make sure that the key (well, the `settings` used to build the key) in the cache would match exactly the settings passed to sgauth to perform the final call. It seemed less prone to introducing silent corruption that way: if we mess up with default resolution somehow, we mess up with the result provided to the user, they will notice and let us know. But I'm open to both approaches, I'll happily update the PR with your suggestion if it doesn't sound convincing.

[andyrzhao]

I see your concern regarding consistency. Here is one more alternative to consider: Updating sgauth/default.go to store the application default credentials back into the settings object, after it has been retrieved. This should give us the correct cache key and avoid the problem of trying to resolve the default credentials twice.

If the above alternative is not feasible, then I'd still prefer you add the logic to the caching util. Practically speaking, corruption due to concurrency etc. should not be a concern the way this tool is intended to be used. Thanks!

[kerneis]

Updating sgauth/default.go to store the application default credentials

back into the settings object That would be nice but won't work in practice because we need the correct kep to perform the initial Lookup before calling sgauth. We need resolution not only when storing but also when looking up.

I'd still prefer you add the logic to the caching util

Sure, I'll update the PR next week and let you know. Thanks for your time.

…