Coordinator Capability implementation - #1955
Conversation
Add client and server interceptors which attach labgrid identity metadata to gRPC calls and expose it to coordinator RPC handlers. Use the metadata identity to register client and exporter stream sessions while keeping startup-message handling as a deprecated fallback for older clients and exporters. Signed-off-by: Asher Pemberton <asher.pemberton@arm.com> Reviewed-by: Asher Pemberton <asher.pemberton@arm.com> # gatekeeper Co-authored-by: Luke Beardsmore <luke.beardsmore2@arm.com>
Allow AcquirePlace, ReleasePlace and CreateReservation to identify the caller from gRPC metadata instead of requiring identity to come only from an established ClientStream session. Keep the existing ClientStream session lookup as a fallback so older clients which still send startup messages on the stream continue to work. Signed-off-by: Asher Pemberton <asher.pemberton@arm.com> Reviewed-by: Asher Pemberton <asher.pemberton@arm.com> # gatekeeper Co-authored-by: Luke Beardsmore <luke.beardsmore2@arm.com>
Send StartupDone alongside identity metadata so new clients and exporters remain compatible with older coordinators. Create sessions immediately from identity metadata when available, while retaining StartupDone as a deprecated fallback. Signed-off-by: Asher Pemberton <asher.pemberton@arm.com> Reviewed-by: Asher Pemberton <asher.pemberton@arm.com> # gatekeeper
Taken verbatim from the authentication github discussion [1]. [1]: labgrid-project#1883 Signed-off-by: Rouven Czerwinski <rouven.czerwinski@linaro.org>
This is a stop-gap until we have policies to assign capabilities. Signed-off-by: Rouven Czerwinski <rouven.czerwinski@linaro.org>
Implement capabilities by using a decorator for unary calls and checking permissions manually for stream calls. Capabilities with more scoping (owned vs any) do not use the decorator and instead check manually. With the capabilities checked by the Coordinator, future Labgrid versions will be able to use policies to assign capabilities to specific identities and thus restrict which gRPC calls can be performed on the coordinator. Signed-off-by: Rouven Czerwinski <rouven.czerwinski@linaro.org>
To make ClientSession testing easier, return the token back to the user when calling create_reservation(). Signed-off-by: Rouven Czerwinski <rouven.czerwinski@linaro.org>
asher-pem-arm
left a comment
asher-pem-arm
left a comment
There was a problem hiding this comment.
picked up a few small issues but overall looks good. It would be helpful to document what is planned to be included in this PR/future ones to help us clarify the review scope
| parser.add_argument( | ||
| "--pystuck-port", metavar="PORT", type=int, default=6666, help="use a different pystuck port than 6666" | ||
| ) | ||
| parser.add_argument("--capabilities", action="store_true", default=False, help="enable using capabilities, which also enforces client identities") |
There was a problem hiding this comment.
what's the plan for this flag? is this intended as a migration mechanism before full auth?
There was a problem hiding this comment.
Yes, with this flag we indicate that the new identities are required to enable capabilities.
There was a problem hiding this comment.
Any reason why we would want a flag? If capabilities are populated by the configured authentication plugin, the coordinator could always enforce them. The default/legacy plugin can preserve current behaviour by granting all capabilities, while secure plugins return restricted sets. With a flag we could get into issues such as auth enabled but capability checks accidentally disabled.
There was a problem hiding this comment.
I think the naming here may be wrong, in essence this enforces the new authentication method and disables the fallback since only the new client identities can have capabilities and we require this feature switch for backwards compatibility at least in the initial release.
Apply comments from asher & run ruff. Signed-off-by: Rouven Czerwinski <rouven.czerwinski@linaro.org>
1b88506 to
deff9a2
Compare
asher-pem-arm
left a comment
asher-pem-arm
left a comment
There was a problem hiding this comment.
Can you clarify the intended plugin-wiring boundary? The coordinator, client, and exporter currently instantiate identity interceptors and the server interceptor parses metadata directly into ClientIdentity. Is this deliberately temporary, or should the interceptors accept injected ClientAuthPlugin/ServerAuthPlugin implementations now with plugin discovery and configuration added later?
| await context.abort(grpc.StatusCode.UNAUTHENTICATED, "Client identity is required when using capabilities") | ||
| if identity and (capability not in identity.capabilities): | ||
| await context.abort( | ||
| grpc.StatusCode.PERMISSION_DENIED, f"Capability {req_cap} not in client capabilities {ctx.capabilities}" |
There was a problem hiding this comment.
req_cap and ctx undefined
|
Yes, I deliberatly added the capabilities to the identity in the interceptor to have something to test against, the intention is to revert that change and have the plugins handle the capability fillout for the identity. Your suggestion to take this a step further and introduce dummy plugins to prevent any confusion on how the handling is supposed to be done makes sense if I understand it correctly, I will look into it. |
We can contstruct the ClientIdentity from the context.invocation_metadata() instead of relying on the server interceptor, which is more pythonic and saves code. If we need the identity further down in the call stack, it is still constructed either in the decorator or in the RPC itself. WIP because the server side tests are now broken. Signed-off-by: Rouven Czerwinski <rouven.czerwinski@linaro.org>
df4c26c to
64d7221
Compare
Signed-off-by: Rouven Czerwinski <rouven.czerwinski@linaro.org>
3 participants
Description
Implement capabilities and checks on the coordinator side.
This relies on the existing identity pull request and will require changes.
Happy for any feedback on the implementation, I know that documentation is still missing, but at this point we should start discussing the implementation.
The Tests currently only really test the happy path, testing the denied path will become easier once we have policies, but I can try to figure this out for this PR if desired.
Checklist