Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions labgrid/remote/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@
# TODO: drop if Python >= 3.11 guaranteed
from exceptiongroup import ExceptionGroup # pylint: disable=redefined-builtin

from labgrid.remote.grpc.interceptor.client import (
IdentityClientStreamStreamInterceptor,
IdentityClientUnaryUnaryInterceptor,
)

from .common import (
ResourceEntry,
ResourceMatch,
Expand Down Expand Up @@ -120,9 +125,19 @@ def __attrs_post_init__(self):
("grpc.http2.max_pings_without_data", 0), # no limit
]

identity = {
"username": self.getuser(),
"hostname": self.gethostname(),
"user_agent": f"labgrid-client {labgrid_version()}",
}
interceptors = [
IdentityClientUnaryUnaryInterceptor(**identity),
IdentityClientStreamStreamInterceptor(**identity),
]
self.channel = grpc.aio.insecure_channel(
target=self.address,
options=channel_options,
interceptors=interceptors,
)
self.stub = labgrid_coordinator_pb2_grpc.CoordinatorStub(self.channel)

Expand Down
17 changes: 17 additions & 0 deletions labgrid/remote/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
import logging
from datetime import datetime
from fnmatch import fnmatchcase
from typing import Optional
import warnings

import attr

Expand Down Expand Up @@ -481,6 +483,21 @@ def from_pb2(cls, pb2: labgrid_coordinator_pb2.Reservation):
)


def get_metadata_single_value_by_key(metadata, key: str) -> Optional[str]:
"""Look up a single value by key in a metadata sequence of (key, value) pairs."""
values = [v for k, v in metadata or () if k == key]

if not values:
return None

if len(values) > 1:
warnings.warn(
"Multiple metadata KV pairs with the same key. The value of the first matching KV pair will be returned."
)

return values[0]


async def queue_as_aiter(q):
try:
while True:
Expand Down
63 changes: 58 additions & 5 deletions labgrid/remote/coordinator.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#!/usr/bin/env python3
import argparse
import contextvars
import logging
import asyncio
import traceback
Expand All @@ -10,11 +11,15 @@
import copy
import random
import signal
from typing import Optional

import attr
import grpc
from grpc_reflection.v1alpha import reflection

from labgrid.remote.grpc.interceptor.server import IdentityServerInterceptor
from labgrid.remote.identity import ClientIdentity, infer_peer_identity

from .common import (
ResourceEntry,
ResourceMatch,
Expand All @@ -30,6 +35,10 @@
from .generated import labgrid_coordinator_pb2_grpc
from ..util import atomic_replace, labgrid_version, yaml, Timeout

client_identity_context: contextvars.ContextVar[Optional[ClientIdentity]] = contextvars.ContextVar(
"client_identity", default=None
)
Comment thread
Emantor marked this conversation as resolved.
Comment on lines +38 to +40

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been trying to understand why you chose to use a ContextVar instead of using the gRPC Server Side context object passed to each API method to inspect the invocation_metadata? It feels like the ContextVar adds a mechanism similar to what is already provided by the context parameter.

Later, on the authorization side, a function decorator could also access the ServicerContext parameter of the call to decide whether to allow/deny the call.

Without a ContextVar, we don't have easy access to the ClientIdentity, but as auth checks should be done early, I'm skeptical if that access is needed at all. Without the ContextVar, the complexity of the Interceptors wouldn't be needed either.

I'm not saying we must avoid ContextVar and Interceptors, but I want to understand why they are the right choice.



@contextmanager
def warn_if_slow(prefix, *, level=logging.WARNING, limit=0.1):
Expand Down Expand Up @@ -317,9 +326,16 @@ async def ClientStream(self, request_iterator, context):
assert peer not in self.clients
out_msg_queue = asyncio.Queue()

identity = client_identity_context.get()
if identity:
logging.debug("client identity provided in gRPC metadata")
logging.debug(identity)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be one logging.debug() line.

self.clients[peer] = ClientSession(self, peer, identity.id, out_msg_queue, identity.user_agent)

Comment thread
Emantor marked this conversation as resolved.
async def request_task():
name = None
version = None
session = self.clients.get(peer)
try:
async for in_msg in request_iterator:
in_msg: labgrid_coordinator_pb2.ClientInMessage
Expand All @@ -330,8 +346,18 @@ async def request_task():
out_msg.sync.id = in_msg.sync.id
out_msg_queue.put_nowait(out_msg)
elif kind == "startup":
version = in_msg.startup.version
if identity:
logging.debug("ignoring legacy startup message; session initialised from metadata")
continue
if session:
logging.warning("ignoring duplicate startup message from client %s", peer)
continue
name = in_msg.startup.name
version = in_msg.startup.version
logging.warning(
"client %s did not provide identity metadata; using deprecated startup identity",
peer,
)
session = self.clients[peer] = ClientSession(self, peer, name, out_msg_queue, version)
logging.debug("Received startup from %s with %s", name, version)
asyncio.current_task().set_name(f"client-{peer}-rx/started-{name}")
Expand Down Expand Up @@ -409,9 +435,22 @@ async def ExporterStream(self, request_iterator, context):
out_msg.hello.version = labgrid_version()
yield out_msg

identity = client_identity_context.get()
if identity:
logging.debug("exporter identity provided in gRPC metadata")
logging.debug(identity)
Comment on lines +440 to +441

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a single logging.debug line.

if existing := self.get_exporter_by_name(identity.id):
await context.abort(
grpc.StatusCode.ALREADY_EXISTS,
f"startup failed: exporter with name '{identity.id}' is already connected from {existing.peer}",
)
self.exporters[peer] = ExporterSession(self, peer, identity.id, command_queue, identity.user_agent)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as for ClientSession above, should be created in a single place.

startup_done.set()

async def request_task():
name = None
version = None
session = self.exporters.get(peer)
try:
async for in_msg in request_iterator:
in_msg: labgrid_coordinator_pb2.ExporterInMessage
Expand All @@ -422,8 +461,18 @@ async def request_task():
cmd.complete(in_msg.response)
logging.debug("Command %s is done", cmd)
elif kind == "startup":
version = in_msg.startup.version
if identity:
logging.debug("ignoring legacy startup message; session initialized from metadata")
continue
if session:
logging.warning("ignoring duplicate startup message from exporter %s", peer)
continue
name = in_msg.startup.name
version = in_msg.startup.version
logging.warning(
"exporter %s did not provide identity metadata; using deprecated startup identity",
peer,
)
if existing := self.get_exporter_by_name(name):
raise ExporterError(
f"exporter with name '{name}' is already connected from {existing.peer}"
Expand Down Expand Up @@ -852,7 +901,7 @@ async def AcquirePlace(self, request, context):
peer = context.peer()
name = request.placename
try:
username = self.clients[peer].name
username = infer_peer_identity(self.clients, context, client_identity_context)
except KeyError:
await context.abort(grpc.StatusCode.FAILED_PRECONDITION, f"Peer {peer} does not have a valid session")
print(request)
Expand Down Expand Up @@ -922,7 +971,7 @@ async def AllowPlace(self, request, context):
user = request.user
peer = context.peer()
try:
username = self.clients[peer].name
username = infer_peer_identity(self.clients, context, client_identity_context)
except KeyError:
await context.abort(grpc.StatusCode.FAILED_PRECONDITION, f"Peer {peer} does not have a valid session")
try:
Expand Down Expand Up @@ -1077,7 +1126,10 @@ async def CreateReservation(self, request: labgrid_coordinator_pb2.CreateReserva
await context.abort(grpc.StatusCode.INVALID_ARGUMENT, f"Value {v} is invalid")
fltr[k] = v

owner = self.clients[peer].name
try:
owner = infer_peer_identity(self.clients, context, client_identity_context)
except KeyError:
await context.abort(grpc.StatusCode.FAILED_PRECONDITION, f"Peer {peer} does not have a valid session")
res = Reservation(owner=owner, prio=request.prio, filters=fltrs)
self.reservations[res.token] = res
self.schedule_reservations()
Expand Down Expand Up @@ -1127,6 +1179,7 @@ async def serve(listen, cleanup) -> None:
]
server = grpc.aio.server(
options=channel_options,
interceptors=[IdentityServerInterceptor(client_identity_context)],
)
coordinator = Coordinator()
labgrid_coordinator_pb2_grpc.add_CoordinatorServicer_to_server(coordinator, server)
Expand Down
10 changes: 10 additions & 0 deletions labgrid/remote/exporter.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@
import attr
import grpc

from labgrid.remote.grpc.interceptor.client import (
IdentityClientStreamStreamInterceptor,
IdentityClientUnaryUnaryInterceptor,
)

from .config import ResourceConfig
from .common import ResourceEntry, queue_as_aiter
from .generated import labgrid_coordinator_pb2, labgrid_coordinator_pb2_grpc
Expand Down Expand Up @@ -834,9 +839,14 @@ def __init__(self, config) -> None:
if urlsplit(f"//{config['coordinator']}").port is None:
config["coordinator"] += ":20408"

identity = (None, self.name, f"labgrid-exporter {labgrid_version()}")
self.channel = grpc.aio.insecure_channel(
target=config["coordinator"],
options=channel_options,
interceptors=[
IdentityClientUnaryUnaryInterceptor(*identity),
IdentityClientStreamStreamInterceptor(*identity),
],
)
self.stub = labgrid_coordinator_pb2_grpc.CoordinatorStub(self.channel)
self.out_queue = asyncio.Queue()
Expand Down
242 changes: 124 additions & 118 deletions labgrid/remote/generated/labgrid_coordinator_pb2.py

Large diffs are not rendered by default.

Empty file added labgrid/remote/grpc/__init__.py
Empty file.
Empty file.
32 changes: 32 additions & 0 deletions labgrid/remote/grpc/interceptor/client.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
from typing import Optional

from grpc.aio import ClientCallDetails, ClientInterceptor, StreamStreamClientInterceptor, UnaryUnaryClientInterceptor

from labgrid.remote.identity import HOSTNAME_KEY, USER_AGENT_KEY, USERNAME_KEY


class BaseIdentityClientInterceptor(ClientInterceptor):
def __init__(self, username: Optional[str], hostname: str, user_agent: Optional[str]):
super().__init__()
self.username = username
self.hostname = hostname
self.user_agent = user_agent

def _inject(self, client_call_details: ClientCallDetails):
if self.username:
client_call_details.metadata.add(USERNAME_KEY, self.username)
client_call_details.metadata.add(HOSTNAME_KEY, self.hostname)
if self.user_agent:
client_call_details.metadata.add(USER_AGENT_KEY, self.user_agent)


class IdentityClientUnaryUnaryInterceptor(UnaryUnaryClientInterceptor, BaseIdentityClientInterceptor):
Comment thread
Emantor marked this conversation as resolved.
async def intercept_unary_unary(self, continuation, client_call_details, request):
self._inject(client_call_details)
return await continuation(client_call_details, request)


class IdentityClientStreamStreamInterceptor(StreamStreamClientInterceptor, BaseIdentityClientInterceptor):
async def intercept_stream_stream(self, continuation, client_call_details, request_iterator):
self._inject(client_call_details)
return await continuation(client_call_details, request_iterator)
33 changes: 33 additions & 0 deletions labgrid/remote/grpc/interceptor/server.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import contextvars
import logging
from asyncio import iscoroutine

from grpc.aio import ServerInterceptor

from labgrid.remote.identity import ClientIdentity, NoIdentityPresent


class IdentityServerInterceptor(ServerInterceptor):
def __init__(self, client_identity_contextvar: contextvars.ContextVar):
super().__init__()
self.client_identity_contextvar = client_identity_contextvar

async def intercept_service(self, continuation, handler_call_details):
# continuation may return a handler
# OR an awaitable depending on grpcio build
maybe_handler = continuation(handler_call_details)
handler = await maybe_handler if iscoroutine(maybe_handler) else maybe_handler
if handler is None:
return None

metadata = handler_call_details.invocation_metadata
logging.debug(metadata)

try:
client_identity = ClientIdentity.from_metadata(metadata)
logging.debug(client_identity)
self.client_identity_contextvar.set(client_identity)
except NoIdentityPresent:
pass

return handler
65 changes: 65 additions & 0 deletions labgrid/remote/identity.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import contextvars
import logging
from typing import Optional

from labgrid.remote.common import get_metadata_single_value_by_key

USERNAME_KEY = "x-lg-username"
HOSTNAME_KEY = "x-lg-hostname"
USER_AGENT_KEY = "x-lg-user-agent"


class NoIdentityPresent(Exception):
"""Raised when metadata-based identity information is missing from the request."""


class ClientIdentity:
"""Represents the identity of a connected client, derived from gRPC metadata."""

def __init__(self, identity_id: str, user_agent: Optional[str]):
self.id = identity_id
self.user_agent = user_agent

def __str__(self):
return f"ClientIdentity(id={self.id}, user_agent={self.user_agent})"

@classmethod
def from_metadata(cls, metadata: tuple):
"""Construct a ClientIdentity from gRPC request metadata.

Args:
metadata: A sequence of (key, value) pairs from the gRPC context.

Returns:
A ClientIdentity with id set to ``hostname/username`` (or just
``hostname`` if no username is present) and (optional) user_agent.

Raises:
NoIdentityPresent: If the hostname key is missing from metadata.
"""
username = get_metadata_single_value_by_key(metadata, USERNAME_KEY)
hostname = get_metadata_single_value_by_key(metadata, HOSTNAME_KEY)
user_agent = get_metadata_single_value_by_key(metadata, USER_AGENT_KEY)

if not hostname:
raise NoIdentityPresent()

if username:
return cls(f"{hostname}/{username}", user_agent)

return cls(hostname, user_agent)


def infer_peer_identity(clients, context, identity_contextvar: contextvars.ContextVar[Optional[ClientIdentity]]):
logger = logging.getLogger("infer_peer_identity")

user = identity_contextvar.get()
if user:
logger.debug("identity sourced from metadata")
return user.id

try:
logger.debug("identity sourced from self.clients")
return clients[context.peer()].name
except KeyError:
raise
Comment on lines +64 to +65

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No-op, drop that.

Loading
Loading