plugin authn/authz rfc - #19
Conversation
Signed-off-by: Patrick Koss <pati.koss@gmx.de>
|
Hi! I've updated #10 to renumber the RFCs 5 and 6 that were in there to RFCs 8 and 9 to avoid conflicts with the now merged RFC 5, this PR, and #13 which proposes an RFC 7. In the future, I'd recommend the following to avoid more numbering conflicts:
Of course that only works if everybody does it, but I think it's worth trying. In my opinion, a better solution would be to stop numbering the RFC's, but presumably that's a broader community discussion. |
| route. That duplication is the single hardest thing to maintain in a plugin | ||
| approach, and it is exactly what this RFC is designed to prevent. | ||
|
|
||
| The demand is concrete and named in the issue: Kubernetes `TokenReview` + |
There was a problem hiding this comment.
Could you stay generic but just use these as examples? The goal of the RFC is not to support those but rather define a contract for plugins.
| username: str | ||
|
|
||
| # Richer attributes; None when the provider does not supply them. | ||
| email: str | None = None |
There was a problem hiding this comment.
Rather than having these as first class attributes, what about allowing each plugin to define arbitrary metadata? We can keep this class tightly scoped to required fields + a plugin metadata field?
There was a problem hiding this comment.
I think some first class attributes like display_name is nice so we can use it for UX.
I was thinking it might also be nice if it returns a profile_url which we store in the JIT user table for optionally hyperlinking the user in the MLflow UI. For example of it's a Github OIDC, it could link you to their GitHub page, which would be neat.
| email: str | None = None | ||
| display_name: str | None = None | ||
| groups: tuple[str, ...] = () # IdP groups/roles, consumed by group→permission mapping | ||
| is_admin: bool = False # provider may assert super-admin (e.g. an IdP claim) |
There was a problem hiding this comment.
I don't think this is sufficient as a bool because not all auth systems identify admins. This is also an authorization concept that I think is leaking into identity.
There was a problem hiding this comment.
I don't see why this is needed at all, I think we should drop it.
| class AuthChallenge: | ||
| status_code: int = 401 | ||
| headers: Mapping[str, str] = field(default_factory=dict) # WWW-Authenticate, Location, Set-Cookie | ||
| body: str = "" |
There was a problem hiding this comment.
When is the body ever needed for auth?
| next provider in the chain. `challenge()` means "this *is* mine but it's | ||
| absent or invalid; here is how the client should authenticate." Only after | ||
| *every* provider skips does core emit the default challenge. This is what lets | ||
| a chain coexist — bearer token, then session cookie, then basic auth — without |
| next provider in the chain. `challenge()` means "this *is* mine but it's | ||
| absent or invalid; here is how the client should authenticate." Only after | ||
| *every* provider skips does core emit the default challenge. This is what lets | ||
| a chain coexist — bearer token, then session cookie, then basic auth — without |
There was a problem hiding this comment.
How can an auth provider determine if the token is not for them vs just invalid? If it can't, the challenge info would confusingly come from the last provider or from all the providers presumably?
My overall preference is to limit this RFC to just one auth and one authorization provider.
| chokepoint calls a small `IdentityStore.ensure_user(identity)` that creates the | ||
| local user row keyed by `username`, populating email/display_name and | ||
| (optionally) syncing group→role assignments, *before* authorization runs. | ||
| Providers never write to the auth database. |
There was a problem hiding this comment.
I'm assuming this is an upsert operation. It may be worthwhile explaining why this is needed (e.g. for the review queue feature's user assignment).
I think it also makes sense to have a separate table for external users vs mixing responsibilities with the basic auth table.
There was a problem hiding this comment.
Agreed on the new table, the basic auth will be behind the same plugin boundary, so core should be agnostic to its tables.
|
|
||
| MLflow runs both Flask (WSGI, `_before_request` at `:2552`) and FastAPI/Starlette | ||
| (`_find_fastapi_validator` at `:4079`). These are two separate auth code paths | ||
| today, and the FastAPI path *rejects any non-default `authorization_function`* |
There was a problem hiding this comment.
Some recent changes were made to this code path. It'd be worth having a coding agent ensure this is still accurate.
There was a problem hiding this comment.
Yes the FastAPI does support authorization functions now, it simply adds an adapter to the WSGI request/response. I don't think it impacts the RFC much, maybe worth just updating the language to better reflect reality.
| MLflow runs both Flask (WSGI, `_before_request` at `:2552`) and FastAPI/Starlette | ||
| (`_find_fastapi_validator` at `:4079`). These are two separate auth code paths | ||
| today, and the FastAPI path *rejects any non-default `authorization_function`* | ||
| (`:4141`) — so custom auth doesn't even work for gateway routes right now. We |
There was a problem hiding this comment.
This is technically possible by not using the basic-auth app and registering your own FastAPI middleware. This is what Kubeflow does.
| def body_json(self) -> dict | None: ... # cached parse, shared with dispatch + handler | ||
| @property | ||
| def framework(self) -> Literal["flask", "starlette"]: ... |
There was a problem hiding this comment.
I don't think these are necessary.
| validate the token against JWKS, map the claims — is identical regardless of | ||
| framework. Two entry points would double the surface that can drift. | ||
|
|
||
| Reference authentication adapters: |
There was a problem hiding this comment.
As mentioned previously, let's keep this RFC scoped to the abstraction. Plugins will come later and those plugins maybe community maintained as opposed to being directly shipped in MLflow.
| class Decision: | ||
| allowed: bool | ||
| effective_permission: str | None = None # READ/USE/EDIT/MANAGE/NO_PERMISSIONS; None if the backend can't express a level | ||
| is_admin: bool = False # backend may assert the subject is super-admin |
There was a problem hiding this comment.
As mentioned previously, I think is_admin needs a third option to indicate no admin concept in this auth system.
| effective_permission: str | None = None # READ/USE/EDIT/MANAGE/NO_PERMISSIONS; None if the backend can't express a level | ||
| is_admin: bool = False # backend may assert the subject is super-admin | ||
| reason: str | None = None # surfaced in the 403 body and the audit log | ||
| cache_ttl_seconds: int | None = None # backend's cache hint; None => use the configured default |
There was a problem hiding this comment.
I think each plugin should maintain its own cache and not require MLflow to maintain this cache. I think each plugin may have their own ways to perform cache invalidation and TTL.
| name: str | ||
| def authorize(self, query: AuthorizationQuery) -> Decision: ... | ||
| # Batch entry point for list/search filtering (see "search filtering" below). | ||
| def authorize_batch(self, queries: Sequence[AuthorizationQuery]) -> Sequence[Decision]: ... |
There was a problem hiding this comment.
What's the purpose of this over running authorize in parallel?
| # grant query; remote backends fall back to authorize_batch. | ||
| def list_readable( | ||
| self, subject: Identity, resource_type: str, workspace: str | None, | ||
| candidate_ids: Sequence[str], |
There was a problem hiding this comment.
What happens if candidate_ids is not set? Can there be a way to ask the authorization system if the user has read permission on all entities of this resource type in the workspace?
That way, MLflow can skip post request modifications on list/search API endpoints.
|
|
||
| ### Configuration | ||
|
|
||
| Keep the existing INI `[mlflow]` section (`mlflow/server/auth/config.py`) and the |
There was a problem hiding this comment.
My preference is keep the ini format for just basic auth but the plugins can determine their own configuration mechanisms (e.g. a config file, env vars, etc.).
| `authn_providers = <that function, wrapped as a provider>` and | ||
| `authz_backend = database`. Existing configs keep working unchanged. | ||
|
|
||
| ### Caching, error handling, fail-closed |
There was a problem hiding this comment.
As mentioned in another comment, let's leave caching to each plugin. MLflow shouldn't be concerned about how their caching works.
|
|
||
| # Open questions | ||
|
|
||
| - **Where does group → MLflow-role mapping live?** In the authn provider (it |
There was a problem hiding this comment.
I think MLflow roles should be limited to basic auth. The authorization system can map permissions to roles in its own plugin code.
| def framework(self) -> Literal["flask", "starlette"]: ... | ||
| ``` | ||
|
|
||
| The one subtle point is body reads. A Starlette body is single-read and async |
There was a problem hiding this comment.
Could we also have MLflow overwrite the user that created a run based on the resolved authenticated user rather than something that can be arbitrarily set in the request body? The Kubeflow MLflow auth plugin does this.
| | `k8s-tokenreview` | POSTs a `TokenReview` to the API server; reads `status.user` | `Identity(username, groups=status.user.groups)` | | ||
| | `proxy-header` | trusts `X-Forwarded-User` / `X-Forwarded-Groups` from a vetted upstream proxy | `Identity(username, groups)` | | ||
|
|
||
| ### AuthorizationBackend: the permission store that owns the decision |
There was a problem hiding this comment.
I think it's implied, but it'd be good to state somewhere that all the authorization checks will remain the same after the migration.
| cache_ttl_seconds: int | None = None # backend's cache hint; None => use the configured default | ||
|
|
||
|
|
||
| class AuthorizationBackend(Protocol): |
There was a problem hiding this comment.
We need a method so that on startup, MLflow can pass a list of all resources that can be authorized (e.g. experiments, registeredmodels, etc.) and the plugin needs to respond on if it can handle all those resource types.
That way, if an auth system is tied to specific MLflow version, you can't inadvertently update MLflow and run it with an supported authorization plugin.
There was a problem hiding this comment.
Agreed on defining the support boundary contract between plugin and MLflow.
I think we can let users choose from two modes (1) strictly blcok MLflow upgrade (2) allow MLflow upgrade while rejecting unsupported resources/actions at request time. The latter is mainly for users who don't need new features but want to get updates and patches for existing features.
| is_redirect: bool = False | ||
|
|
||
|
|
||
| class AuthenticationResult: |
There was a problem hiding this comment.
Aren't you missing some key details here like:
class AuthenticationResult:
kind: Literal["authenticated", "skip", "challenge"]
identity: Identity | None = None
challenge: AuthChallenge | None = None
...
@property
def is_authenticated(self) -> bool:
return self.kind == "authenticated"?
| class AuthenticationResult: | ||
| """Exactly one outcome per provider call.""" | ||
| @staticmethod | ||
| def authenticated(identity: Identity) -> "AuthenticationResult": ... |
There was a problem hiding this comment.
When would authenticated be called vs. AuthenticationProvider.authenticate ?
| class AuthorizationQuery: | ||
| subject: Identity | ||
| requirement: AuthorizationRequirement | ||
| context: "RequestContext" # method, path, request_id, claims passthrough for OPA / SAR |
There was a problem hiding this comment.
Can you add more about "RequestContext"? This feels underspecified, who builds it (core?), what lives in it?
6 participants
RFC 0006: Pluggable Authentication and Authorization
Tracking issue: mlflow/mlflow#21240
Summary
Adds a new RFC proposing two small plugin contracts —
AuthenticationProviderand
AuthorizationBackend— to replace MLflow's singleauthorization_functionhook. The split separates who you are from what you may do, and keeps the
load-bearing
route → requirementmapping in core so plugins never need totrack MLflow's routing surface.
This is the extension point that RFC 0005 ("Role-Based Access Control for
MLflow OSS") flagged as future work. It builds on 0005's role model and
resolver surface, and the default plugins reproduce today's behavior
byte-for-byte — operators who upgrade and change nothing see no difference.
What's in this PR
rfcs/0006-pluggable-auth/0006-pluggable-auth.md(823 lines, onecommit on top of
main).No code changes, no implementation — this is the design document. Reference
adapters described in the RFC (OIDC, Kubernetes
TokenReview/SubjectAccessReview, OPA, upstream proxy headers) are sketched in enoughdetail to validate the interface shape but are not built here.
Why now
The existing surface has three structural problems:
authorization_functionreturns awerkzeug.datastructures.Authorizationcarrying only a username — too thinfor bearer tokens, OIDC claims, group membership, or JIT provisioning.
non-default function (
mlflow/server/auth/__init__.py:4141), so the hookis effectively Flask-only.
dispatch structures. Any external authorization system (Kubernetes SAR,
OPA, a corporate policy engine) has to rediscover and re-sync that mapping
every time MLflow adds a route.
Design rule worth calling out
Core retains sole ownership of the
route → requirementmapping via a singleauthoritative
OPERATION_REGISTRY. Plugins only ever see the normalized tuple(resource_type, resource_id, action, workspace)— never a route, a protobufclass, or a GraphQL field. A CI guard fails the build if any route ships
without a declared requirement.
Out of scope (intentionally)
READ / USE / EDIT / MANAGE.Reviewer guide
Suggested reading order if you're short on time:
rest of the design hangs off this.
of truth as routes evolve.
like pushback.
Open questions I'd like input on
These are spelled out at the bottom of the RFC; flagging them here so they
don't get lost:
authn_providersshould be an ordered chain or a single providerwith explicit fallback rules.
workspaceshould be for the Kubernetes SAR adapter(namespace? label selector? both?).
Checklist
0000-template.mdstructurestart_dateset,mlflow_issuelinked,rfc_prleft empty pertemplate instructions
mlflow/server/auth/