-
Notifications
You must be signed in to change notification settings - Fork 171
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
OIDC: Fix issues with subset of services. #1304
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
unleashed
approved these changes
Aug 30, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
By default, APICast stores all services in an array of Service Objects, also, another array with the OIDC objects, something like this: ``` { "services": [ { "id": 1, "issuer": "http://foo.com", "auth_type": "oidc", ... }, { "id": 2, "auth_type": "provider_key" ... }, { "id": 3, "issuer": "http://bar.com", "auth_type": "oidc", ... } ], "oidc": [ { "issuer": "http://foo.com", ... }, false, { "issuer": "http://bar.com", ... } ] } ``` The mapping, on APICast config is like this: ``` service[0] using oidc[0] service[1] using oidc[1] service[2] using oidc[2] ``` When we filter using `APICAST_SERVICE_LIST`, it filters based on the array, so it'll transform to this: export APICAST_SERVICE_LIST=3 ``` { "services": [ { "id": 3, "issuer": "http://bar.com", "auth_type": "oidc", ... } ], "oidc": [ false, { "issuer": "http://bar.com", ... } ] } ``` So, OIDC will fail, because the first entry of the OIDC array is false, because false is added on filtering. This PR added a new entry on oidc object, that it's service_id, so filtering can be done without issues, config will be like this: ``` { "services": [ { "id": 1, "issuer": "http://foo.com", "auth_type": "oidc", ... }, { "id": 2, "auth_type": "provider_key" ... }, { "id": 3, "issuer": "http://bar.com", "auth_type": "oidc", ... } ], "oidc": [ { "issuer": "http://foo.com", "service_id": 1, ... }, { "service_id": 2, }, { "issuer": "http://bar.com", "service_id": 3, ... } ] } ``` On non-oidc services, the oidc will be not hitted at all. On invalid fetch, It'll be not fail, because the issuer is not in there, so it'll not work as expected. OIDC config links: Service OIDC setup: https://github.com/3scale/APIcast/blob/c184ff3e904f3d75857032a3da0004f8d74eba00/gateway/src/apicast/configuration/service.lua#L221-L231 OIDC error on invalid oicd setup: https://github.com/3scale/APIcast/blob/c184ff3e904f3d75857032a3da0004f8d74eba00/gateway/src/apicast/oauth/oidc.lua#L55 Warning message: https://github.com/3scale/APIcast/blob/c184ff3e904f3d75857032a3da0004f8d74eba00/gateway/src/apicast/proxy.lua#L199-L205 Filtering part: https://github.com/3scale/APIcast/blob/c184ff3e904f3d75857032a3da0004f8d74eba00/gateway/src/apicast/configuration.lua#L173-L297 Fix: THREESCALE-6042 Reported-by: Kevin Price <kevprice@redhat.com> Reported-by: Samuele Illuminati <sillumin@redhat.com> Signed-off-by: Eloy Coto <eloy.coto@acalustra.com>
eloycoto
added a commit
that referenced
this pull request
Sep 1, 2021
This is a second patch, and it's related to PR #1304. When using remotev2 loader, the config.oidc is created in there, and there is also a caching mechanism for OIDC[0], so we need to append the service_id and send a valid table in case of non OIDC setup. Because this oidc is cached, we also do a deepcopy, to avoid to use always the same reference, so the config.oidc[i].service_id is always different and we didn't get a invalid filtering. This is only failing when using APICAST_SERVICE_LIST [0] Commit: 720cd99 Related to: THREESCALE-6042 Signed-off-by: Eloy Coto <eloy.coto@acalustra.com>
eloycoto
added a commit
that referenced
this pull request
Sep 1, 2021
This is a second patch, and it's related to PR #1304. When using remotev2 loader, the config.oidc is created in there, and there is also a caching mechanism for OIDC[0], so we need to append the service_id and send a valid table in case of non OIDC setup. Because this oidc is cached, we also do a deepcopy, to avoid to use always the same reference, so the config.oidc[i].service_id is always different and we didn't get a invalid filtering. This is only failing when using APICAST_SERVICE_LIST [0] Commit: 720cd99 Related to: THREESCALE-6042 Signed-off-by: Eloy Coto <eloy.coto@acalustra.com>
eloycoto
added a commit
that referenced
this pull request
Sep 1, 2021
This is a second patch, and it's related to PR #1304. When using remotev2 loader, the config.oidc is created in there, and there is also a caching mechanism for OIDC[0], so we need to append the service_id and send a valid table in case of non OIDC setup. Because this oidc is cached, we also do a deepcopy, to avoid to use always the same reference, so the config.oidc[i].service_id is always different and we didn't get a invalid filtering. This is only failing when using APICAST_SERVICE_LIST [0] Commit: 720cd99 Related to: THREESCALE-6042 Signed-off-by: Eloy Coto <eloy.coto@acalustra.com>
eloycoto
added a commit
that referenced
this pull request
Sep 1, 2021
This is a second patch, and it's related to PR #1304. When using remotev2 loader, the config.oidc is created in there, and there is also a caching mechanism for OIDC[0], so we need to append the service_id and send a valid table in case of non OIDC setup. Because this oidc is cached, we also do a deepcopy, to avoid to use always the same reference, so the config.oidc[i].service_id is always different and we didn't get a invalid filtering. This is only failing when using APICAST_SERVICE_LIST [0] Commit: 720cd99 Related to: THREESCALE-6042 Signed-off-by: Eloy Coto <eloy.coto@acalustra.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By default, APICast stores all services in an array of Service Objects, also,
another array with the OIDC objects, something like this:
The mapping, on APICast config is like this:
When we filter using
APICAST_SERVICE_LIST
, it filters based on the array, soit'll transform to this:
export APICAST_SERVICE_LIST=3
So, OIDC will fail, because the first entry of the OIDC array is false, because
false is added on filtering.
This PR added a new entry on oidc object, that it's service_id, so filtering can
be done without issues, config will be like this:
On non-oidc services, the oidc will be not hitted at all. On invalid fetch,
It'll be not fail, because the issuer is not in there, so it'll not work as
expected.
OIDC config links:
Service OIDC setup:
APIcast/gateway/src/apicast/configuration/service.lua
Lines 221 to 231 in c184ff3
OIDC error on invalid oicd setup:
APIcast/gateway/src/apicast/oauth/oidc.lua
Line 55 in c184ff3
Warning message:
APIcast/gateway/src/apicast/proxy.lua
Lines 199 to 205 in c184ff3
Filtering part:
https://github.com/3scale/APIcast/blob/c184ff3e904f3d75857032a3da0004f8d74eba00/gateway/src/apicast/configuration.lua#L173-L297
Fix: THREESCALE-6042
Reported-by: Kevin Price kevprice@redhat.com
Reported-by: Samuele Illuminati sillumin@redhat.com
Signed-off-by: Eloy Coto eloy.coto@acalustra.com