Potential race condition on reading and writing credentials cache

[ChrisKujawa]

Describe the bug

I'm not a go expert but I think I found a potential race condition in the go client on read an writing the cache file.

The following code is used to write the cache:

// writeCache will overwrite any contents in the current cache file, so use carefully
func (cache *oauthYamlCredentialsCache) writeCache() error {
	cache.lock.RLock()
	defer cache.lock.RUnlock()

	cacheContents, err := yaml.Marshal(&cache.audiences)
	if err != nil {
		return err
	}

	return ioutil.WriteFile(cache.path, cacheContents, 0600)
}

It locks the cache to read the content and then to write the file.

The following code is used to read the cache file and update the in memory cache.

// readCache will overwrite the current contents of cache.audiences, so use carefully
func (cache *oauthYamlCredentialsCache) readCache() error {
	cacheContents, err := ioutil.ReadFile(cache.path)
	if err != nil {
		return err
	}

	cache.lock.Lock()
	defer cache.lock.Unlock()

	err = yaml.Unmarshal(cacheContents, &cache.audiences)
	return err
}

The issue I see is that if read and write happens concurrently (which I assume since we using locks here otherwise it doesn't make sense to me) then I think it is likely that it can happen that we locked the in memory cache and write to the file that concurrently before we completed the file write we already read the file again. This reading of the file happens before the lock. We might update the in memory cache with old content.

Environment:

• Zeebe Version:SNAPSHOT

[npepinpe]

Makes sense to me - we should lock before reading the file as well. This still only protects from within the same process, but I imagine that's the current design anyway. I'll let @miguelpires confirm anyway in case I'm missing something.

[ChrisKujawa]

I think it is also problematic when update and refresh is called concurrently.

// Refresh overwrites the current in-memory contents with whatever is written in the cache file
func (cache *oauthYamlCredentialsCache) Refresh() error {
	return cache.readCache()
}

// Get returns the cached credentials for the given audience or nil
// Note: it does not read the cache again
func (cache *oauthYamlCredentialsCache) Get(audience string) *oauth2.Token {
	cache.lock.RLock()
	defer cache.lock.RUnlock()

	cachedCredentials := cache.audiences[audience]
	if cachedCredentials == nil {
		return nil
	}

	return cachedCredentials.Auth.Credentials
}

// Update updates the in-memory mapping for the given audience and credentials, and flushes its contents to disk
func (cache *oauthYamlCredentialsCache) Update(audience string, credentials *oauth2.Token) error {
	cache.put(audience, credentials)
	return cache.writeCache()
}

func (cache *oauthYamlCredentialsCache) put(audience string, credentials *oauth2.Token) {
	cache.lock.Lock()
	defer cache.lock.Unlock()
	cache.audiences[audience] = &oauthCachedCredentials{
		Auth: struct{ Credentials *oauth2.Token }{Credentials: credentials},
	}
}

// readCache will overwrite the current contents of cache.audiences, so use carefully
func (cache *oauthYamlCredentialsCache) readCache() error {
	cacheContents, err := ioutil.ReadFile(cache.path)
	if err != nil {
		return err
	}

	cache.lock.Lock()
	defer cache.lock.Unlock()

	err = yaml.Unmarshal(cacheContents, &cache.audiences)
	return err
}

In update we do put and writeCache which has separate locks but in between a refresh could happen which would re-read the cache and replace the in-mem cache.

[npepinpe]

As far as I can tell, even if we do a partial read, or read the wrong thing, it just means we do an extra request - this is problematic now with the job worker issue, but when that's fixed, then at least we should recover from this race condition. Although we should fix it eventually, and check if it wouldn't be the same in the Java client.