feat(framework): Add authn abstraction layer#6758
Conversation
… more closely match current policy
msheller
changed the title
msheller
changed the title
github-actions
bot
added
the
Contributor
msheller
changed the title
msheller
requested review from
danieljanes,
panh99 and
tanertopal
as code owners
There was a problem hiding this comment.
Pull request overview
This PR introduces a transport-agnostic authentication (“authn”) abstraction layer for AppIo services, aiming to separate mechanism handling, policy/decision logic, and identity representation to simplify future authn changes.
Changes:
- Added core authn primitives (auth input, caller identity, decision engine, and a token authenticator).
- Added method-level auth policy types plus validation helper for policy tables.
- Added constants and unit tests for the new authn layer.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
framework/py/flwr/supercore/auth/appio_auth.py |
Adds the core authn domain model and decision engine, plus token mechanism implementation. |
framework/py/flwr/supercore/auth/policy.py |
Introduces method auth policy types and a helper to validate per-RPC policy maps. |
framework/py/flwr/supercore/auth/constant.py |
Defines shared mechanism/caller-type identifiers and metadata key constants. |
framework/py/flwr/supercore/auth/appio_auth_test.py |
Adds unit tests covering engine behavior, token auth, signed-metadata presence semantics, and policy table validation. |
framework/py/flwr/supercore/auth/__init__.py |
Exposes the new authn layer public API via package exports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| # Expected SuperExec plugin scope (for allowlist/policy checks). | ||
| plugin_type: str | None = None |
There was a problem hiding this comment.
Could remove the plugin_type? It would nice to make the auth abstract agnostic to plugins.
panh99
left a comment
panh99
left a comment
There was a problem hiding this comment.
General question: just curious, can this abstraction be applied to node authentication as well?
| # True means signed-metadata auth material was supplied on the request path. | ||
| # This can be True while ``signed_metadata`` is None when extraction sees a | ||
| # partial/malformed signed-metadata payload. | ||
| signed_metadata_present: bool = False | ||
| signed_metadata: SignedMetadataAuthInput | None = None | ||
|
|
||
| def __post_init__(self) -> None: | ||
| """Validate signed metadata presence invariants.""" | ||
| if self.signed_metadata is not None and not self.signed_metadata_present: | ||
| raise ValueError( | ||
| "signed_metadata_present must be True when signed_metadata is set." | ||
| ) |
There was a problem hiding this comment.
Could we remove signed_metadata_present? Or is there an case when signed_metadata_present == True and signed_metadata == None but still pass the check?
Issue
This is PR 1.2 in the AppIo Auth changes.
Changing authn mechanisms (e.g. token, signed metadata) and/or policy (e.g. which interfaces require which mechanisms) is too intrusive and bespoke, complicating implementation, review and testing of essential security code.
Proposal
Add a lightweight authn abstraction layer to seperate authn logic into:
This enables new mechanisms or policies with minimal change and makes reasoning and testing much simpler.
Explanation
Created a light-weight auth abstraction intended to simplify the next set of PRs. See internal RFC for abstraction details.
The mental model is:
Checklist
#contributions)Any other comments?
NOTE: this solution version conflates authentication-mechanism policy with authorization policy: SuperExecs and *Apps are distinguished by authentication mechanism (token vs. signed metadata). While I believe this is okay for now, we should separate these concerns as soon as we add authorization complexity, or if we ever decide to support multiple mechanisms for a given process (i.e. if *Apps can use tokens or signed metadata).