zamzterz/Flask-pyoidc

Duplicate call to handle_provider_config causing duplicate keys being loaded in keyjar

Closed this issue · 3 comments

kerim1 commented

I encountered a problem where the public keys are loaded twice in the keyjar, causing problems during token validation. I can trace it back to these two calls in the PyoidcFacade: https://github.com/zamzterz/Flask-pyoidc/blob/main/src/flask_pyoidc/pyoidc_facade.py#L41-L42

provider_metadata = provider_configuration.ensure_provider_metadata(self._client)
self._client.handle_provider_config(ProviderConfigurationResponse(**provider_metadata.to_dict()),
                                    provider_metadata['issuer'])

In the first call to ensure_provider_metadata, a call to handle_provider_config on the client is already made. So why is it necessary to make another call to handle_provider_config? The extra call results in keys being added to the keyjar again. My problem seems to be resolved when I remove the second call to handle_provider_config without causing any other problems as far as I can see now.

If you use Dynamic Provider Registration to discover the IdP's endpoints, then only it calls handle_provider_config.

def ensure_provider_metadata(self, client: Client):
if not self._provider_metadata:
resp = client.provider_config(self._issuer)
logger.debug(f'Received discovery response: {resp.to_dict()}')
self._provider_metadata = ProviderMetadata(**resp.to_dict())
return self._provider_metadata

The if branch here becomes true if dynamic discovery has to be made to register the IdP's endpoints. For Static Provider Registration, the if condition becomes false, so handle_provider_config has to be called explicitly to register it.

This was the reason behind this call, however I have reproduced the bug. handle_provider_config is being called twice during Dynamic Provider Registration which is appending the oic.utils.keyio.KeyBundle instance to the keyjar at this line:

https://github.com/CZ-NIC/pyoidc/blob/4535d4126d72b40c0518a9856b6e57250012f2bf/src/oic/utils/keyio.py#L507

try:
    self.issuer_keys[issuer].append(kc)
except KeyError:
    self.issuer_keys[issuer] = [kc]

return kc

I will raise PR to address it.

kerim1 commented

Thanks for the explanation. Now it makes sense to me why the extra call is made. I can also confirm that the issue is resolved on my side once I tried it with your fix in the linked PR.

Thanks for confirming. @zamzterz Can you review and merge?