fix: never construct id token claim templates in parallel

[AlbinoDrought]

Fixes #551

Related issue

#551

Proposed changes

Ensure we never call templates.New().Parse() across more than one thread. I have implemented this for mutator_id_token.go which solves my issue in #551.

Checklist

• I have read the contributing guidelines.
• I have read the security policy.
• I confirm that this pull request does not address a security
  vulnerability. If this pull request addresses a security. vulnerability, I
  confirm that I got green light (please contact
  [email protected]) from the maintainers to push
  the changes.
• I have added tests that prove my fix is effective or that my feature
  works. (I'm not sure how to test this kind of issue)
• I have added or changed the documentation. (not sure if required for a non-feature change)

Further comments

It appears like the same issue may exist in these files:

• pipeline/mutate/mutator_header.go
• pipeline/mutate/mutator_cookie.go
• pipeline/authz/remote.go
• pipeline/authz/remote_json.go
• pipeline/authz/keto_engine_acp_ory.go

Affected blocks may look something like this:

	t := a.t.Lookup(templateID)
	if t == nil {
		var err error
		// if two goroutines reach this at the same time, we panic
		t, err = a.t.New(templateID).Parse(templateString)
		if err != nil {
			return "", err
		}
	}

If this fix is acceptable, I can carry it over to those files as well.

[CLAassistant]

All committers have signed the CLA.

[aeneasr]

Awesome, thank you for your contribution! This looks pretty good but I have some ideas how to improve it further :)

[aeneasr]

The documentation says:

Because associated templates share underlying data, template construction cannot be done safely in parallel. Once the templates are constructed, they can be executed in parallel.

So I think it would be better to have a a.getTemplate(c.ClaimsTemplateID()) which looks up the template and if it doesn't exist creates it and adds it to the cache. The creation part would be locked while the read part would not have to be locked. This will increase throughput as we don't need to lock here for every request :)

[AlbinoDrought]

Hey, I'm not sure I understand. Do you have an example of what you're thinking?

Here are my assumptions right now with the current code:

	var templateClaims []byte
	if len(c.Claims) > 0 {
		// find the already-constructed template:
		t := a.templates.Lookup(c.ClaimsTemplateID())
		if t == nil {
			// if we couldn't find it, lock and construct a new one (only one time)
			var err error
			a.templatesLock.Lock()
			t, err = a.templates.New(c.ClaimsTemplateID()).Parse(c.Claims)
			a.templatesLock.Unlock()
			if err != nil {
				return errors.Wrapf(err, `error parsing claims template in rule "%s"`, rl.GetID())
			}
		}

		// after a template already been constructed once, requests should reach this point without locking:
		var b bytes.Buffer
		if err := t.Execute(&b, session); err != nil {
			return errors.Wrapf(err, `error executing claims template in rule "%s"`, rl.GetID())
		}

		templateClaims = b.Bytes()
		if err := json.NewDecoder(&b).Decode(&claims); err != nil {
			return errors.WithStack(err)
		}
	}

Thanks!

[aeneasr]

Sorry, my bad, I just overlooked the preceding if statement and template lookup - you are correct, this is indeed the way it can be solved! :)