-
Notifications
You must be signed in to change notification settings - Fork 990
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
Interfaces and services for JWK management #10628
Conversation
These should be very fast, and thus don't need a separate queue.
It makes more sense to have the OIDC_PROVIDERS map be a map of names to issuer FQDNs, so that we don't have to store that information elsewhere.
Updated the
|
I haven't started on tests yet, but this should be good for another review pass. |
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.
Overall looks good. We need to make sure we're emitting meaningful metrics at the right places. We probably want to use sentry_sdk.capture_message
for the errors instead of logging them (would be nice to make this just output to the logger in development).
Co-authored-by: Dustin Ingram <[email protected]>
Co-authored-by: Dustin Ingram <[email protected]>
Adds a service factory for creating per-provider services.
warehouse/oidc/services.py
Outdated
class JWKServiceFactory: | ||
def __init__(self, provider, service_class=JWKService): | ||
self.provider = provider | ||
self.service_class = service_class | ||
|
||
def __call__(self, context, request): | ||
cache_url = request.registry.settings["oidc.jwk_cache_url"] | ||
|
||
return self.service_class(self.provider, cache_url) | ||
|
||
def __eq__(self, other): | ||
if not isinstance(other, JWKServiceFactory): | ||
return NotImplemented | ||
|
||
return (self.provider, self.service_class) == ( | ||
other.provider, | ||
other.service_class, | ||
) |
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.
cc @di: Let me know if this looks reasonable for the factory.
(I cargo-culted the __eq__
implementation from TokenServiceFactory
-- I assume it's necessary to help find_service
disambiguate the factory-produced services?)
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.
Some slight refactoring suggestions and one overall thought: Should we make this an OIDCProviderService
instead? I don't see a need for having separate JWK service and OIDC service (with an interface like .verify(token)
) for the same provider.
Yep, that makes sense to me! I'll make that change. |
Dependencies failure in the CI looks unrelated to these changes:
|
* python-version: bump to 3.8.9 * ci, Dockerfile: bump Python versions * requirements, warehouse: dependencies, skeleton for JWKs * warehouse/oidc: format * config, oidc, utils: fill in more groundwork * warehouse: add a basic `warehouse oidc` CLI, redis caching * tasks: remove the separate OIDC queue These should be very fast, and thus don't need a separate queue. * warehouse: decompose OIDC urls a bit It makes more sense to have the OIDC_PROVIDERS map be a map of names to issuer FQDNs, so that we don't have to store that information elsewhere. * warehouse/utils: docs * warehouse: refactor JWKs to fetch on first use * tests/unit: fix config test * Update requirements/main.txt Co-authored-by: Dustin Ingram <[email protected]> * Apply suggestions from code review Co-authored-by: Dustin Ingram <[email protected]> * warehouse: refactor JWKService Adds a service factory for creating per-provider services. * oidc/services: appease flake8 * warehouse: add metrics to JWKService, rewrite CLI * warehouse/cli: remove oidc subcommand This is no longer useful. * warehouse: rename JWKService to OIDCProviderService, refactor * warehouse/oidc: fix init * warehouse: remove oidc.utils, refactor * warehouse/oidc: re-add provider attribute * tests: unit tests for warehouse.oidc.services Fixes small bugs in the process. Co-authored-by: Dustin Ingram <[email protected]>
This adds the initial set of interfaces and services (and a small
warehouse oidc
CLI) for JSON Web Key (JWK) management.Some design notes:
warehouse oidc update-oidc-jwks
.issuer
key, but we should probably keep that hardcoded anyways.Needs:
Closes #10620.