Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SECAUTH-1475 Calls jwks endpoint with x-zone_uuid header #16

Merged
merged 10 commits into from
Jul 26, 2021

Conversation

nenaraab
Copy link
Contributor

@nenaraab nenaraab commented Jul 2, 2021

  • handles 400 (Bad request) status code
  • memorizes the zones that were already accepted or denied for up to 15 minutes
  • avoids jwks call in case zone was already accepted and keys are not yet expired
  • DOES NOT call jwks endpoints before cache entry gets expired => tokens signed with new token keys are not accepted immediately but after a delay of up to 15 minutes
  • test cache handling fro jwks and acceptedZoneIDs
  • removes singleFlight here, as GetJWKs is parameterized with zone id
    • -> implement using sync.RWMutex to avoid concurrent remote calls to fetch jwks per zone

- handles 400 (Bad request) status code
- memorizes the zones that were already accepted
- avoids jwks call in case zone was already accepted and keys are not yet expired
- removes singleFlight here, as GetJWKs is parameterized with zone id -> sync required?
@nenaraab nenaraab requested a review from a team as a code owner July 2, 2021 15:10
@f-blass
Copy link
Member

f-blass commented Jul 5, 2021

Hi @nenaraab
thanks for the contribution. Is there a specific reason you removed the singleFlight completely? One could add the zone-id in the key of the singleflight to make it unique.

Another possible improvement is the use of map for acceptedZoneIDs to reduce the complexity from O(n) to O(1) in comparison to a slice. One benchmark I found is this one: https://www.darkcoding.net/software/go-slice-search-vs-map-lookup
Best regards
Felix

@nenaraab
Copy link
Contributor Author

nenaraab commented Jul 5, 2021

Hi @f-blass
Thanks for your initial review!

The usage of map[string]bool for zoneIDs is a good idea.

In the first draft I’ve added zoneID as key but then I realized that the jwks are the same for all zones (normally up to 20, but for shared ias tenant it could be 1000) that are “assigned” to the same identity tenant… in order to reduce memory I decided to remove the single flight, but maybe we can synchronize the call to identity differently - considering the url AND the x-zone_uuid header…
any idea?

@nenaraab nenaraab requested a review from f-blass July 20, 2021 18:56
mocks/mockServer.go Outdated Show resolved Hide resolved
oidcclient/jwk.go Outdated Show resolved Hide resolved
oidcclient/jwk.go Outdated Show resolved Hide resolved
oidcclient/jwk.go Show resolved Hide resolved
@nenaraab nenaraab requested a review from f-blass July 26, 2021 12:49
Copy link
Member

@f-blass f-blass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now. 👍🏼
For the future we can maybe think about adding a concurrent test to be run with race detector.

@nenaraab nenaraab merged commit b759afa into master Jul 26, 2021
@nenaraab nenaraab deleted the SECAUTH-1475 branch July 26, 2021 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants