Conversation
3e96e96 to
cddedf5
Compare
41060bb to
6df7018
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #997 +/- ##
==========================================
+ Coverage 78.02% 78.13% +0.10%
==========================================
Files 328 335 +7
Lines 28919 29624 +705
==========================================
+ Hits 22565 23146 +581
- Misses 6354 6478 +124
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| if mount_stat != MountStat.MNT3_OK: | ||
| return mount_stat |
There was a problem hiding this comment.
Maybe just raising an exception would be better, so we have a consistent output of the deserialize function?
There was a problem hiding this comment.
I think I keep this as-is to closely follow the rfc here, and then throw an exception in a MountClient (to be created), which can be built on top of the sunrpc.Client. Or perhaps I add a mount method to the NfsClient, which is simpler.
I will add a comment in the demo
| class Serializer(ABC, Generic[Serializable]): | ||
| @abstractmethod | ||
| def serialize(self, _: Serializable) -> bytes: | ||
| pass |
There was a problem hiding this comment.
Maybe it is a better idea to have something like this:
class Serializable(ABC):
@abstractmethod
def serialize(self) -> bytes:
pass
@dataclass
class CookieVerf3(Serializer):
def serialize(self) -> bytes:
... # Do the serialize code hereThen the serialization can be done in the dataclasses instead of multiple different serializers.
There was a problem hiding this comment.
(see reply on following comment)
| class Deserializer(ABC, Generic[Serializable]): | ||
| def deserialize_from_bytes(self, payload: bytes) -> Serializable: | ||
| return self.deserialize(io.BytesIO(payload)) | ||
|
|
||
| @abstractmethod | ||
| def deserialize(self, _: io.BytesIO) -> Serializable: | ||
| pass |
There was a problem hiding this comment.
Might it be an idea to do something like this:
# Reference to self
from typing_extensions import Self
class Deserializable(ABC):
@classmethod
def from_bytes(cls, payload: bytes) -> Self:
return self.deserialize(io.BytesIO(payload))
@abstractmethod
@classmethod
def deserialize(cls, _: io.BytesIO) -> Self:
pass
...
@dataclass
class FileAttributes3(Deserializer):
def deserialize(cls, payload: io.BytesIO) -> Self:
...
return cls(...)All the _read_* functions would need to be converted to a classmethod to to use it in the deserialize functions.
However, it would bring the serialization/deserialization closer to the data it actually tries to deserialize/serialize which i feel makes more sense.
There was a problem hiding this comment.
Generally speaking, I think it is not a bad idea to treat serialization to a specific format as a separate concern, especially since the cost seems to be low.
Related, if we make deserializable a class method then I wonder how we can for example deserialize the params and result child members in the Message(Serializer). I don´t think python has support for reifying the type parameters at run time, and hence we would not know which deserializer to use for fields of generic dataclasses .
Finally coupling the data to the deserializer makes it harder to compose parsers.
I favored to keep things simple, but for example the _read_optional and _read_var_length could be made more flexible by returning parsers themselves if so required in later stories:
_read_var_length(self, payload: io.BytesIO, deserializer: Deserializer[ElementType]) -> Deserializer[list[ElementType]]:,
_read_optional(self, payload: io.BytesIO, deserializer: Deserializer[ElementType]) -> Deserializer[ElementType | None]
, so that they can be chained futher. Another example is a or combinator.
When we couple the data to the deserialization, we shut this down.
There was a problem hiding this comment.
Discussed with @Miauwkeru IRL:
His proposal is to use polymorphism and replace fields which are dependent on a type parameter with Deserializable, as in the following:
X = TypeVar("Verifier")
class Deserializable():
@classmethod
def deserialize(cls, data: bytes) -> Deserializable:
pass
@dataclass
class Z(Deserializable):
@classmethod
def deserialize(cls, data: bytes) -> Z:
return cls(s="Roel", i=5)
s: str
i: int
@dataclass
class K(Generic[X], Deserializable):
z: Deserializable
a: X
z = Z(s="Roel", i=5)
# Access through either Generic or Deserializable
k = K[Z](z=z, a=z)
k.z.i # type error (Any), access through super class
z2: Z = k.z, z.i # ok, but tedious
cast(Z, k.a).i # ok, but tedious
k.a.i # ok, access through field dependent on type parameter
The downside to this approach is that we lose type inference if we access z through k.z.i, compared to the approach with generics, as in k.a.i.
This type inference is for example used in Client::readdirplus, where results gets correctly inferred to be ReadDirPlusResult3 | NfsStat.
It needs to be carefully weighed if the strong locality of Serializer with its data offsets the loss of type inference.
Besided that, shutting down of composition of deserializers is also a concern.
| if messageType == MessageType.REPLY: | ||
| replyStat = self._read_enum(payload, ReplyStat) | ||
| if replyStat == ReplyStat.MSG_ACCEPTED: | ||
| reply = self._read_accepted_reply(payload) | ||
| elif replyStat == ReplyStat.MSG_DENIED: | ||
| reply = self._read_rejected_reply(payload) | ||
|
|
||
| return sunrpc.Message(xid, reply) | ||
|
|
||
| raise NotImplementedError("Only REPLY messages are deserializable") |
There was a problem hiding this comment.
Reduces indentation a bit
| if messageType == MessageType.REPLY: | |
| replyStat = self._read_enum(payload, ReplyStat) | |
| if replyStat == ReplyStat.MSG_ACCEPTED: | |
| reply = self._read_accepted_reply(payload) | |
| elif replyStat == ReplyStat.MSG_DENIED: | |
| reply = self._read_rejected_reply(payload) | |
| return sunrpc.Message(xid, reply) | |
| raise NotImplementedError("Only REPLY messages are deserializable") | |
| if messageType != MessageType.REPLY: | |
| raise NotImplementedError("Only REPLY messages are deserializable") | |
| replyStat = self._read_enum(payload, ReplyStat) | |
| if replyStat == ReplyStat.MSG_ACCEPTED: | |
| reply = self._read_accepted_reply(payload) | |
| elif replyStat == ReplyStat.MSG_DENIED: | |
| reply = self._read_rejected_reply(payload) | |
| return sunrpc.Message(xid, reply) |
dissect/target/helpers/nfs/nfs.py
Outdated
| MNT3_OK = 0 # no error | ||
| MNT3ERR_PERM = 1 # Not owner | ||
| MNT3ERR_NOENT = 2 # No such file or directory | ||
| MNT3ERR_IO = 5 # I/O error | ||
| MNT3ERR_ACCES = 13 # Permission denied | ||
| MNT3ERR_NOTDIR = 20 # Not a directory | ||
| MNT3ERR_INVAL = 22 # Invalid argument | ||
| MNT3ERR_NAMETOOLONG = 63 # Filename too long | ||
| MNT3ERR_NOTSUPP = 10004 # Operation not supported | ||
| MNT3ERR_SERVERFAULT = 10006 # A failure on the server |
There was a problem hiding this comment.
An idea to remove the MNT3 prefix for less visual noise?
| pass | ||
|
|
||
|
|
||
| class Client(Generic[Credentials, Verifier]): |
There was a problem hiding this comment.
A suggestion for a bit of additional usability, would it be an idea to also make this a context manager or create a context manager for this class?
that will allow you to do:
with Client(...):
...and will automatically clean up the connections it makes.
There was a problem hiding this comment.
Yeah thought about it but it did not make the cut.
Probably more pressing than I originally thought, given that the demo was eating up ports
|
|
||
|
|
||
| class PortMappingSerializer(Serializer[sunrpc.PortMapping]): | ||
| def serialize(self, portMapping: sunrpc.PortMapping) -> bytes: |
dissect/target/helpers/nfs/demo.py
Outdated
| mount_port = port_mapper_client.call(100000, 2, 3, params_mount, PortMappingSerializer(), UInt32Serializer()) | ||
| params_nfs = PortMapping(program=NFS_PROGRAM, version=NFS_V3, protocol=Protocol.TCP) | ||
| nfs_port = port_mapper_client.call(100000, 2, 3, params_nfs, PortMappingSerializer(), UInt32Serializer()) |
There was a problem hiding this comment.
What do the numbers mean?
fba07e5 to
fa54aee
Compare
38c8d64 to
dc42e0f
Compare
| replyStat = self._read_enum(payload, ReplyStat) | ||
| if replyStat == ReplyStat.MSG_ACCEPTED: | ||
| reply = self._read_accepted_reply(payload) | ||
| elif replyStat == ReplyStat.MSG_DENIED: | ||
| reply = self._read_rejected_reply(payload) | ||
|
|
||
| return sunrpc.Message(xid, reply) |
There was a problem hiding this comment.
just to avoid any weird shenanigans, maybe still a good idea to put reply as None above the conditions. So it is at least defined.
| @classmethod | ||
| def connect( | ||
| cls, hostname: str, port: int, auth: AuthScheme[Credentials, Verifier], local_port: int = 0 | ||
| ) -> "Client": |
There was a problem hiding this comment.
the annotations allow you to not use the "Client" but Client instead, if i r
| ) -> "Client": | |
| ) -> Client: |
| PMAP_PORT = 111 | ||
|
|
||
| @classmethod | ||
| def connect_port_mapper(cls, hostname: str) -> "Client": |
There was a problem hiding this comment.
| def connect_port_mapper(cls, hostname: str) -> "Client": | |
| def connect_port_mapper(cls, hostname: str) -> Client: |
| ) -> Results: | ||
| """Synchronously call an RPC procedure and return the result""" | ||
|
|
||
| callBody = sunrpc.CallBody( |
dissect/target/helpers/nfs/nfs3.py
Outdated
| filehandle: FileHandle3 | ||
| authFlavors: list[int] |
| paramsSerializer: XdrSerializer[ProcedureParams], | ||
| resultsDeserializer: XdrDeserializer[ProcedureResults], | ||
| credentialsSerializer: AuthSerializer[Credentials], | ||
| verifierSerializer: AuthSerializer[Verifier], |
tests/helpers/sunrpc/test_client.py
Outdated
| filehandle=FileHandle3( | ||
| opaque=b"\x01\x00\x07\x00\x02\x00\xec\x02\x00\x00\x00\x00\xb5g\x131&\xf1I\xed\xb8R\rx\\h8\xb4" | ||
| ), | ||
| authFlavors=[1], |
| def serialize(self, i: int) -> bytes: | ||
| return i.to_bytes(length=4, byteorder="big", signed=True) | ||
|
|
||
| def deserialize(self, payload: io.BytesIO) -> int: | ||
| return int.from_bytes(payload.read(4), byteorder="big", signed=True) | ||
|
|
||
|
|
||
| class UInt32Serializer(Serializer[int], Deserializer[int]): | ||
| def serialize(self, i: int) -> bytes: | ||
| return i.to_bytes(length=4, byteorder="big", signed=False) | ||
|
|
||
| def deserialize(self, payload: io.BytesIO) -> int: | ||
| return int.from_bytes(payload.read(4), byteorder="big", signed=False) |
There was a problem hiding this comment.
| def serialize(self, i: int) -> bytes: | |
| return i.to_bytes(length=4, byteorder="big", signed=True) | |
| def deserialize(self, payload: io.BytesIO) -> int: | |
| return int.from_bytes(payload.read(4), byteorder="big", signed=True) | |
| class UInt32Serializer(Serializer[int], Deserializer[int]): | |
| def serialize(self, i: int) -> bytes: | |
| return i.to_bytes(length=4, byteorder="big", signed=False) | |
| def deserialize(self, payload: io.BytesIO) -> int: | |
| return int.from_bytes(payload.read(4), byteorder="big", signed=False) | |
| _signed = True | |
| def serialize(self, i: int) -> bytes: | |
| return i.to_bytes(length=4, byteorder="big", signed=self._signed) | |
| def deserialize(self, payload: io.BytesIO) -> int: | |
| return int.from_bytes(payload.read(4), byteorder="big", signed=self._signed) | |
| class UInt32Serializer(Int32Serializer): | |
| _signed = False |
wouldn't something like this work?
| ProcedureParams = TypeVar("ProcedureParams") | ||
| ProcedureResults = TypeVar("ProcedureResults") | ||
| Credentials = TypeVar("Credentials") | ||
| Verifier = TypeVar("Verifier") | ||
| Serializable = TypeVar("Serializable") | ||
| AuthProtocol = TypeVar("AuthProtocol") | ||
| EnumType = TypeVar("EN", bound=Enum) | ||
| ElementType = TypeVar("ET") |
There was a problem hiding this comment.
Maybe an idea to add bound for every type var?
There was a problem hiding this comment.
On second thought, there is no bound on any of the other TypeVars
| from contextlib import AbstractContextManager | ||
| from types import TracebackType | ||
| from typing import Generic, Iterator, NamedTuple, TypeVar | ||
|
|
||
| from dissect.target.helpers.nfs.nfs3 import ( | ||
| CookieVerf3, | ||
| EntryPlus3, | ||
| FileAttributes3, | ||
| FileHandle3, | ||
| Nfs3Stat, | ||
| Read3args, | ||
| ReadDirPlusParams, | ||
| ReadDirPlusProc, | ||
| ReadFileProc, | ||
| ) | ||
| from dissect.target.helpers.nfs.serializer import ( | ||
| Read3ArgsSerializer, | ||
| Read3ResultDeserializer, | ||
| ReadDirPlusParamsSerializer, | ||
| ReadDirPlusResultDeserializer, | ||
| ) | ||
| from dissect.target.helpers.sunrpc.client import AuthScheme | ||
| from dissect.target.helpers.sunrpc.client import Client as SunRpcClient |
There was a problem hiding this comment.
| from contextlib import AbstractContextManager | |
| from types import TracebackType | |
| from typing import Generic, Iterator, NamedTuple, TypeVar | |
| from dissect.target.helpers.nfs.nfs3 import ( | |
| CookieVerf3, | |
| EntryPlus3, | |
| FileAttributes3, | |
| FileHandle3, | |
| Nfs3Stat, | |
| Read3args, | |
| ReadDirPlusParams, | |
| ReadDirPlusProc, | |
| ReadFileProc, | |
| ) | |
| from dissect.target.helpers.nfs.serializer import ( | |
| Read3ArgsSerializer, | |
| Read3ResultDeserializer, | |
| ReadDirPlusParamsSerializer, | |
| ReadDirPlusResultDeserializer, | |
| ) | |
| from dissect.target.helpers.sunrpc.client import AuthScheme | |
| from dissect.target.helpers.sunrpc.client import Client as SunRpcClient | |
| from contextlib import AbstractContextManager | |
| from typing import TYPE_CHECKING, Generic, Iterator, NamedTuple, TypeVar | |
| from dissect.target.helpers.nfs.nfs3 import ( | |
| CookieVerf3, | |
| EntryPlus3, | |
| FileAttributes3, | |
| FileHandle3, | |
| Nfs3Stat, | |
| Read3args, | |
| ReadDirPlusParams, | |
| ReadDirPlusProc, | |
| ReadFileProc, | |
| ) | |
| from dissect.target.helpers.nfs.serializer import ( | |
| Read3ArgsSerializer, | |
| Read3ResultDeserializer, | |
| ReadDirPlusParamsSerializer, | |
| ReadDirPlusResultDeserializer, | |
| ) | |
| from dissect.target.helpers.sunrpc.client import AuthScheme | |
| from dissect.target.helpers.sunrpc.client import Client as SunRpcClient | |
| if TYPE_CHECKING: | |
| from types import TracebackType |
| import random | ||
| import socket | ||
| from contextlib import AbstractContextManager | ||
| from dataclasses import dataclass | ||
| from types import TracebackType | ||
| from typing import Generic, TypeVar | ||
|
|
||
| from dissect.target.helpers.nfs.nfs3 import ProcedureDescriptor | ||
| from dissect.target.helpers.sunrpc import sunrpc | ||
| from dissect.target.helpers.sunrpc.serializer import ( | ||
| AuthNullSerializer, | ||
| AuthSerializer, | ||
| AuthUnixSerializer, | ||
| MessageSerializer, | ||
| XdrDeserializer, | ||
| XdrSerializer, | ||
| ) |
There was a problem hiding this comment.
| import random | |
| import socket | |
| from contextlib import AbstractContextManager | |
| from dataclasses import dataclass | |
| from types import TracebackType | |
| from typing import Generic, TypeVar | |
| from dissect.target.helpers.nfs.nfs3 import ProcedureDescriptor | |
| from dissect.target.helpers.sunrpc import sunrpc | |
| from dissect.target.helpers.sunrpc.serializer import ( | |
| AuthNullSerializer, | |
| AuthSerializer, | |
| AuthUnixSerializer, | |
| MessageSerializer, | |
| XdrDeserializer, | |
| XdrSerializer, | |
| ) | |
| import random | |
| import socket | |
| from contextlib import AbstractContextManager | |
| from dataclasses import dataclass | |
| from typing import TYPE_CHECKING, Generic, TypeVar | |
| from dissect.target.helpers.sunrpc import sunrpc | |
| from dissect.target.helpers.sunrpc.serializer import ( | |
| AuthNullSerializer, | |
| AuthSerializer, | |
| AuthUnixSerializer, | |
| MessageSerializer, | |
| XdrDeserializer, | |
| XdrSerializer, | |
| ) | |
| if TYPE_CHECKING: | |
| from types import TracebackType | |
| from dissect.target.helpers.nfs.nfs3 import ProcedureDescriptor | |
| import io | ||
| from abc import ABC, abstractmethod | ||
| from enum import IntEnum | ||
| from typing import Generic, Type, TypeVar |
There was a problem hiding this comment.
| from typing import Generic, Type, TypeVar | |
| from typing import Generic, TypeVar |
| def _read_uint64(self, payload: io.BytesIO) -> int: | ||
| return int.from_bytes(payload.read(8), byteorder="big", signed=False) | ||
|
|
||
| def _read_enum(self, payload: io.BytesIO, enum: Type[EnumType]) -> EnumType: |
There was a problem hiding this comment.
| def _read_enum(self, payload: io.BytesIO, enum: Type[EnumType]) -> EnumType: | |
| def _read_enum(self, payload: io.BytesIO, enum: type[EnumType]) -> EnumType: |
You don't need to use Type, as type does the same since python3.9
| else: | ||
| # assert_never is slightly better, but only available starting from Python 3.11 | ||
| raise ValueError(f"Unexpected value for reject_stat: {reject_stat}") |
There was a problem hiding this comment.
the else is technically not needed, only saying this due to indentation allergies
…ons. Change to dataclass
…ntext of a Generic. Use Union instead.
…ializer subclass. - Replace enums with intenums
This is an experimental NFS V3 client which supports:
A demo is also included.
Some coarse tests are included.
The code is considered POC quality and thus a bit rough around the edges.
References: