From 86b86ba3434ef372f86a8361c931fc3e6b87fb7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Cortier?= Date: Mon, 18 Sep 2023 15:03:39 -0400 Subject: [PATCH] doc: elaborate high-level documentation (#192) --- ARCHITECTURE.md | 234 +++++++++++++---- README.md | 6 +- crates/ironrdp-acceptor/Cargo.toml | 2 +- crates/ironrdp-acceptor/README.md | 4 +- crates/ironrdp-blocking/README.md | 2 +- crates/ironrdp-client/Cargo.toml | 2 +- crates/ironrdp-client/README.md | 12 +- crates/ironrdp-cliprdr/Cargo.toml | 2 +- crates/ironrdp-cliprdr/README.md | 10 +- crates/ironrdp-pdu/README.md | 356 ++++++++++++++++++++++---- crates/ironrdp-pdu/src/cursor.rs | 22 ++ crates/ironrdp-pdu/src/write_buf.rs | 2 +- crates/ironrdp-rdpdr/Cargo.toml | 3 +- crates/ironrdp-rdpdr/README.md | 4 +- crates/ironrdp-server/Cargo.toml | 2 +- crates/ironrdp-server/README.md | 6 +- crates/ironrdp-svc/src/lib.rs | 37 ++- crates/ironrdp/Cargo.toml | 12 +- crates/ironrdp/examples/screenshot.rs | 7 +- 19 files changed, 608 insertions(+), 117 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 63f6b86d2..e52815930 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -12,78 +12,226 @@ This document describes the high-level architecture of IronRDP. This section talks briefly about various important directories and data structures. +Note also which crates are **API Boundaries**. +Remember, [rules at the boundary are different](https://www.tedinski.com/2018/02/06/system-boundaries.html). + ### Core Tier Set of foundational libraries for which strict quality standards must be observed. +Note that all crates in this tier are **API Boundaries**. Pay attention to the "**Architecture Invariant**" sections. -- `crates/ironrdp`: meta crate re-exporting important crates. -- `crates/ironrdp-pdu`: PDU encoding and decoding. (TODO: talk about important types and traits such as PduDecode, PduEncode…) -- `crates/ironrdp-graphics`: image processing primitives. -- `crates/ironrdp-svc`: traits to implement RDP static virtual channels. -- `crates/ironrdp-dvc`: DRDYNVC static channel implementation and traits to implement dynamic virtual channels. -- `crates/ironrdp-rdpdr`: RDPDR channel implementation. -- `crates/ironrdp-rdpsnd`: RDPSND static channel for audio output implemented as described in MS-RDPEA. -- `crates/ironrdp-connector`: state machines to drive an RDP connection sequence. -- `crates/ironrdp-session`: state machines to drive an RDP session. -- `crates/ironrdp-input`: utilities to manage and build input packets. -- `crates/ironrdp-rdcleanpath`: RDCleanPath PDU structure used by IronRDP web client and Devolutions Gateway. -- `crates/ironrdp-error`: lightweight and `no_std`-compatible generic `Error` and `Report` types. - The `Error` type wraps a custom consumer-defined type for domain-specific details (such as `PduErrorKind`). - **Architectural Invariant**: doing I/O is not allowed for these crates. **Architectural Invariant**: all these crates must be fuzzed. -**Architectural Invariant**: no non-essential dependency is allowed. - **Architectural Invariant**: must be `#[no_std]`-compatible (optionally using the `alloc` crate). Usage of the standard library must be opt-in through a feature flag called `std` that is enabled by default. When the `alloc` crate is optional, a feature flag called `alloc` must exist to enable its use. +**Architectural Invariant**: no non-essential dependency is allowed. + +**Architectural Invariant**: no proc-macro dependency. Dependencies such as `syn` should be pushed +as far as possible from the foundational crates so it doesn’t become too much of a compilation +bottleneck. The paper [Developer Productivity For Humans, Part 4: Build Latency, Predictability, +and Developer Productivity][developer-productivity] by Ciera Jaspan and Collin Green, Google +researchers, elaborates on why it is important to keep build times low. + +**Architectural Invariant**: unless the performance, usability or ergonomic gain is really worth +it, the amount of [monomorphization] incured in downstream user code should be minimal to avoid +binary bloating and to keep the compilation as parallel as possible. Large generic functions should +be avoided if possible. + +[developer-productivity]: https://www.computer.org/csdl/magazine/so/2023/04/10176199/1OAJyfknInm +[monomorphization]: https://rustc-dev-guide.rust-lang.org/backend/monomorph.html + +#### [`crates/ironrdp`](./crates/ironrdp) + +Meta crate re-exporting important crates. + +**Architectural Invariant**: this crate re-exports other crates and does not provide anything else. + +_TODO_: clean up the dependencies + +#### [`crates/ironrdp-pdu`](./crates/ironrdp-pdu) + +PDU encoding and decoding. + +_TODO_: talk about important types and traits such as PduDecode, PduEncode… + +#### [`crates/ironrdp-graphics`](./crates/ironrdp-graphics) + +Image processing primitives. + +_TODO_: break down into multiple smaller crates + +_TODO_: clean up the dependencies + +#### [`crates/ironrdp-svc`](./crates/ironrdp-svc) + +Traits to implement RDP static virtual channels. + +#### [`crates/ironrdp-dvc`](./crates/ironrdp-dvc) + +DRDYNVC static channel implementation and traits to implement dynamic virtual channels. + +#### [`crates/ironrdp-cliprdr`](./crates/ironrdp-cliprdr) + +CLIPRDR static channel for clipboard implemented as described in MS-RDPECLIP. + +#### [`crates/ironrdp-rdpdr`](./crates/ironrdp-rdpdr) + +RDPDR channel implementation. + +#### [`crates/ironrdp-rdpsnd`](./crates/ironrdp-rdpsnd) + +RDPSND static channel for audio output implemented as described in MS-RDPEA. + +#### [`crates/ironrdp-connector`](./crates/ironrdp-connector) + +State machines to drive an RDP connection sequence. + +#### [`crates/ironrdp-session`](./crates/ironrdp-session) + +State machines to drive an RDP session. + +#### [`crates/ironrdp-input`](./crates/ironrdp-input) + +Utilities to manage and build input packets. + +#### [`crates/ironrdp-rdcleanpath`](./crates/ironrdp-rdcleanpath) + +RDCleanPath PDU structure used by IronRDP web client and Devolutions Gateway. + +#### [`crates/ironrdp-error`](./crates/ironrdp-error) + +Lightweight and `no_std`-compatible generic `Error` and `Report` types. +The `Error` type wraps a custom consumer-defined type for domain-specific details (such as `PduErrorKind`). + ### Extra Tier Higher level libraries and binaries built on top of the core tier. Guidelines and constraints are relaxed to some extent. -- `crates/ironrdp-blocking`: blocking I/O abstraction wrapping the state machines conveniently. -- `crates/ironrdp-async`: provides `Future`s wrapping the state machines conveniently. -- `crates/ironrdp-tokio`: `Framed*` traits implementation above `tokio`’s traits. -- `crates/ironrdp-futures`: `Framed*` traits implementation above `futures`’s traits. -- `crates/ironrdp-tls`: TLS boilerplate common with most IronRDP clients. -- `crates/ironrdp-client`: portable RDP client without GPU acceleration using softbuffer and winit for windowing. -- `crates/ironrdp-web`: WebAssembly high-level bindings targeting web browsers. -- `web-client/iron-remote-gui`: core frontend UI used by `iron-svelte-client` as a Web Component. -- `web-client/iron-svelte-client`: web-based frontend using `Svelte` and `Material` frameworks. +#### [`crates/ironrdp-blocking`](./crates/ironrdp-blocking) + +Blocking I/O abstraction wrapping the state machines conveniently. + +This crate is an **API Boundary**. + +#### [`crates/ironrdp-async`](./crates/ironrdp-async) + +Provides `Future`s wrapping the state machines conveniently. + +This crate is an **API Boundary**. + +#### [`crates/ironrdp-tokio`](./crates/ironrdp-tokio) + +`Framed*` traits implementation above `tokio`’s traits. + +This crate is an **API Boundary**. + +#### [`crates/ironrdp-futures`](./crates/ironrdp-futures) + +`Framed*` traits implementation above `futures`’s traits. + +This crate is an **API Boundary**. + +#### [`crates/ironrdp-tls`](./crates/ironrdp-tls) + +TLS boilerplate common with most IronRDP clients. + +NOTE: it’s not yet clear if this crate is an API Boundary or an implementation detail for the native clients. + +#### [`crates/ironrdp-client`](./crates/ironrdp-client) + +Portable RDP client without GPU acceleration. + +#### [`crates/ironrdp-web`](./crates/ironrdp-web) + +WebAssembly high-level bindings targeting web browsers. + +This crate is an **API Boundary** (WASM module). + +#### [`web-client/iron-remote-gui`](./web-client/iron-remote-gui) + +Core frontend UI used by `iron-svelte-client` as a Web Component. + +This crate is an **API Boundary**. + +#### [`web-client/iron-svelte-client`](./web-client/iron-svelte-client) + +Web-based frontend using `Svelte` and `Material` frameworks. ### Internal Tier Crates that are only used inside the IronRDP project, not meant to be published. This is mostly test case generators, fuzzing oracles, build tools, and so on. -- `crates/ironrdp-pdu-generators`: `proptest` generators for `ironrdp-pdu` types. -- `crates/ironrdp-session-generators`: `proptest` generators for `ironrdp-session` types. -- `crates/ironrdp-testsuite-core`: contains all integration tests for code living in the core tier, in a single binary, - organized in modules. **Architectural Invariant**: no dependency from another tier is allowed. - It must be the case that compiling and running the core test suite does not require building any library from - the extra tier. This is to keep iteration time short. -- `crates/ironrdp-testsuite-extra`: contains all integration tests for code living in the extra tier, in a single binary, - organized in modules. (WIP: this crate does not exist yet.) -- `crates/ironrdp-fuzzing`: provides test case generators and oracles for use with fuzzing. -- `fuzz`: fuzz targets for code in core tier. -- `xtask`: IronRDP’s free-form automation using Rust code. - **Architecture Invariant**: these crates are not, and will never be, an **API Boundary**. +#### [`crates/ironrdp-pdu-generators`](./crates/ironrdp-pdu-generators) + +`proptest` generators for `ironrdp-pdu` types. + +#### [`crates/ironrdp-session-generators`](./crates/ironrdp-session-generators) + +`proptest` generators for `ironrdp-session` types. + +#### [`crates/ironrdp-testsuite-core`](./crates/ironrdp-testsuite-core) + +Contains all integration tests for code living in the core tier, in a single binary, organized in modules. + +**Architectural Invariant**: no dependency from another tier is allowed. It must be the case that +compiling and running the core test suite does not require building any library from the extra tier. +This is to keep iteration time short. + +#### [`crates/ironrdp-testsuite-extra`](./crates/ironrdp-testsuite-extra) + +Contains all integration tests for code living in the extra tier, in a single binary, organized in modules. + +(WIP: this crate does not exist yet.) + +#### [`crates/ironrdp-fuzzing`](./crates/ironrdp-fuzzing) + +Provides test case generators and oracles for use with fuzzing. + +#### [`fuzz`](./fuzz) + +Fuzz targets for code in core tier. + +#### [`xtask`](./xtask) + +IronRDP’s free-form automation using Rust code. + ### Community Tier -Crates provided and maintained by the community. -Core maintainers will not invest a lot of time into these. -One or several community maintainers are associated to each one +Crates provided and maintained by the community. Core maintainers will not invest a lot of time into +these. One or several community maintainers are associated to each one. + +The IronRDP team is happy to accept new crates but may not necessarily commit to keeping them +working when changing foundational libraries. We promise to notify you if such a crate breaks, and +will always try to fix things when it's a minor change. + +#### [`crates/ironrdp-acceptor`](./crates/ironrdp-acceptor) (@mihneabuz) + +State machines to drive an RDP connection acceptance sequence + +#### [`crates/ironrdp-server`](./crates/ironrdp-server) (@mihneabuz) + +Extendable skeleton for implementing custom RDP servers. + +#### [`crates/ironrdp-glutin-renderer`](./crates/ironrdp-glutin-renderer) (no maintainer) + +`glutin` primitives for OpenGL rendering. + +#### [`crates/ironrdp-client-glutin`](./crates/ironrdp-client-glutin) (no maintainer) + +GPU-accelerated RDP client using glutin. + +#### [`crates/ironrdp-replay-client`](./crates/ironrdp-replay-client) (no maintainer) -- `crates/ironrdp-glutin-renderer` (no maintainer): `glutin` primitives for OpenGL rendering. -- `crates/ironrdp-client-glutin` (no maintainer): GPU-accelerated RDP client using glutin. -- `crates/ironrdp-replay-client` (no maintainer): utility tool to replay RDP graphics pipeline for debugging purposes. +Utility tool to replay RDP graphics pipeline for debugging purposes. ## Cross-Cutting Concerns diff --git a/README.md b/README.md index 3de70b990..e3e12ea91 100644 --- a/README.md +++ b/README.md @@ -21,7 +21,7 @@ Supported codecs: A full-fledged RDP client based on IronRDP crates suite, and implemented using non-blocking, asynchronous I/O. -```bash +```shell cargo run --bin ironrdp-client -- --username --password ``` @@ -35,9 +35,9 @@ of code by leveraging the IronRDP crates suite. In this basic client implementation, the client establishes a connection with the destination server, decodes incoming graphics updates, and saves the -resulting output as a BMP image file on the local disk. +resulting output as a BMP image file on the disk. -```bash +```shell cargo run --example=screenshot -- --host --username --password --output out.bmp ``` diff --git a/crates/ironrdp-acceptor/Cargo.toml b/crates/ironrdp-acceptor/Cargo.toml index 2fbd70511..1ffa729bc 100644 --- a/crates/ironrdp-acceptor/Cargo.toml +++ b/crates/ironrdp-acceptor/Cargo.toml @@ -2,7 +2,7 @@ name = "ironrdp-acceptor" version = "0.1.0" readme = "README.md" -description = "" +description = "State machines to drive an RDP connection acceptance sequence" edition.workspace = true license.workspace = true homepage.workspace = true diff --git a/crates/ironrdp-acceptor/README.md b/crates/ironrdp-acceptor/README.md index ac6a31fb2..4ffc9ca5e 100644 --- a/crates/ironrdp-acceptor/README.md +++ b/crates/ironrdp-acceptor/README.md @@ -1,3 +1,5 @@ # IronRDP Acceptor -State machine for the server connection acceptance sequence. +State machines to drive an RDP connection acceptance sequence. + +For now, it requires the [Tokio runtime](https://tokio.rs/). diff --git a/crates/ironrdp-blocking/README.md b/crates/ironrdp-blocking/README.md index 521f6d71e..d1d3cc74c 100644 --- a/crates/ironrdp-blocking/README.md +++ b/crates/ironrdp-blocking/README.md @@ -3,5 +3,5 @@ Blocking I/O abstraction wrapping the IronRDP state machines conveniently. This crate is a higher level abstraction for IronRDP state machines using blocking I/O instead of -asynchronous I/O. This results in a simpler API with fewer dependencies that should be used +asynchronous I/O. This results in a simpler API with fewer dependencies that may be used instead of `ironrdp-async` when concurrency is not a requirement. \ No newline at end of file diff --git a/crates/ironrdp-client/Cargo.toml b/crates/ironrdp-client/Cargo.toml index edb5e7f3e..fb9b328d9 100644 --- a/crates/ironrdp-client/Cargo.toml +++ b/crates/ironrdp-client/Cargo.toml @@ -2,7 +2,7 @@ name = "ironrdp-client" version = "0.1.0" readme = "README.md" -description = "Portable RDP client without GPU acceleration using softbuffer and winit for windowing" +description = "Portable RDP client without GPU acceleration" edition.workspace = true license.workspace = true homepage.workspace = true diff --git a/crates/ironrdp-client/README.md b/crates/ironrdp-client/README.md index a89d52b09..ad85f1163 100644 --- a/crates/ironrdp-client/README.md +++ b/crates/ironrdp-client/README.md @@ -1,10 +1,14 @@ # IronRDP client -A full-fledged RDP client based on IronRDP crates suite, and implemented using non-blocking, asynchronous I/O. +Portable RDP client without GPU acceleration. + +This is a a full-fledged RDP client based on IronRDP crates suite, and implemented using +non-blocking, asynchronous I/O. Portability is achieved by using softbuffer for rendering +and winit for windowing. ## Sample usage -```bash +```shell ironrdp-client --username --password ``` @@ -12,7 +16,7 @@ ironrdp-client --username --password The `IRONRDP_LOG` environment variable is used to set the log filter directives. -```bash +```shell IRONRDP_LOG="info,ironrdp_connector=trace" ironrdp-client --username --password ``` @@ -29,7 +33,7 @@ This file can be read by Wireshark so that in can decrypt the packets. ### Example -```bash +```shell SSLKEYLOGFILE=/tmp/tls-secrets ironrdp-client --username --password ``` diff --git a/crates/ironrdp-cliprdr/Cargo.toml b/crates/ironrdp-cliprdr/Cargo.toml index e031d5d21..947f7c661 100644 --- a/crates/ironrdp-cliprdr/Cargo.toml +++ b/crates/ironrdp-cliprdr/Cargo.toml @@ -1,8 +1,8 @@ [package] name = "ironrdp-cliprdr" version = "0.1.0" -description = "RDP PDU encoding and decoding" readme = "README.md" +description = "CLIPRDR static channel for clipboard implemented as described in MS-RDPECLIP" edition.workspace = true license.workspace = true diff --git a/crates/ironrdp-cliprdr/README.md b/crates/ironrdp-cliprdr/README.md index 610c2d188..e1c18396a 100644 --- a/crates/ironrdp-cliprdr/README.md +++ b/crates/ironrdp-cliprdr/README.md @@ -1,6 +1,8 @@ -# IronRDP clipboard library +# IronRDP CLIPRDR + +CLIPRDR static channel for clipboard implemented as described in MS-RDPECLIP. -Cliboard static virtual channel(SVC) implementation. This library includes: - - Cliboard SVC PDUs parsing - - Clipboard SVC processing (TODO) + +- CLIPRDR PDUs parsing +- CLIPRDR processing (TODO) diff --git a/crates/ironrdp-pdu/README.md b/crates/ironrdp-pdu/README.md index faf360d22..6bb5c9500 100644 --- a/crates/ironrdp-pdu/README.md +++ b/crates/ironrdp-pdu/README.md @@ -19,88 +19,154 @@ TODO: elaborate this section ## Difference between `WriteBuf` and `WriteCursor` +`WriteCursor` is a wrapper around `&mut [u8]` and its purpose is to: + +- Provide convenient methods such as `write_u8`, `write_u16`, `write_u16_be`, etc. +- Guarantee syscall-free, infallible write access to a continuous slice of memory. +- Keep track of the number of bytes written. +- Allow backtracking to override a value previously written or skipped. +- Be `no-std` and `no-alloc` friendly, which `std::io::Cursor` is not as of today. + +The underlying storage could be abstracted over, but it’s deliberately hardcoded to `&mut [u8]` +so traits such as `PduEncode` using `WriteCursor` in their associated methods are object-safe. + `WriteBuf` is used in APIs where the required space cannot be known in advance. For instance, `ironrdp_connector::Sequence::step` is taking `&mut WriteBuf` instead of `&mut WriteCursor` because it’s unlikely the user knows exactly how much space is required to encode a -specific response before actually processing the input payload, and as such there is no way +specific response before actually processing the input payload, and as such there is no easy way to communicate the caller how much space is required before calling the function. -`WriteCursor`, in essence, is a helper for this kind of code: +Consider this piece of code: ```rust - let pdu = /* some PDU structure */; +fn process(&mut self, payload: &[u8], output: &mut WriteBuf) -> PduResult<()> { + let server_request = ServerRequest: ironrdp_pdu::decode(payload)?; + + match server_request.order { + ServerOrder::DoThis => { + // do this + let response = DoThisResponse { … }; + + // buffer is grown, or not, as appropriate, and `DoThisResponse` is encoded in the "unfilled" region + ironrdp_pdu::encode_buf(response, output)?; + } + ServerOrder::DoThat => { + // do that + let response = DoThatResponse { … }; + + // same as above + ironrdp_pdu::encode_buf(response, output)?; + } + } + + Ok(()) +} +``` + +Methods such as `write_u8` are overlapping with the `WriteCursor` API, but it’s mostly for +convenience if one needs to manually write something in an ad-hoc fashion, and using `WriteCursor` +is preferred in order to write `no-std` and `no-alloc` friendly code. - // Find the required space +## Difference between `WriteBuf` and `Vec` + +`WriteBuf` roles include: + +- Maintaining a non-trivial piece of initialized memory consistently, all while ensuring that the + internal `Vec` doesn't grow excessively large in memory throughout the program's + execution. Keeping a piece of initialized memory around is useful to amortize the initialization + cost of `Vec::resize` when building a `WriteCursor` (which requires a mutable slice of + initialized memory, `&mut [u8]`). + +- Keep track of the filled region in order to easily write multiple items sequentially within the + same buffer. + +`WriteCursor`, in essence, is a helper for this kind of code: + +```rust +pub fn encode_buf(pdu: &T, buf: &mut Vec, filled_len: usize) -> PduResult { let pdu_size = pdu.size(); // Resize the buffer, making sure there is enough space to fit the serialized PDU if buf.len() < pdu_size { - buf.resize(pdu_size, 0); + buf.resize(filled_len + pdu_size, 0); } // Proceed to actually serialize the PDU into the buffer… - let mut cursor = WriteCursor::new(dst); + let mut cursor = WriteCursor::new(&mut buf[filled_len..]); encode_cursor(pdu, &mut cursor)?; + + let written = cursor.pos(); + + Ok(written) +} + +fn somewhere_else() -> PduResult<()> { + let mut state_machine = /* … */; + let mut buf = Vec::new(); + let mut filled_len; + + while !state_machine.is_terminal() { + let pdus = state_machine.step(); + + filled_len = 0; + buf.shrink_to(16384); // Maintain up to 16 kib around + + for pdu in pdus { + filled_len += encode_buf(&pdu, &mut buf, filled_len)?; + } + + let filled = &buf[..filled_len]; + + // Do something with `filled` + } +} ``` -But it’s going a bit further by keeping track of the already filled region (as opposed to the -initialized yet unfilled region of the inner `Vec`). One purpose is to facilitate buffer re- -use: it’s okay to keep around the same `WriteBuf` and repeatedly write into it. Contrary to -`Vec`, bytes will be written at the beginning of the unfilled region (a `Vec` will write -bytes at the beginning of the uninitialized region). This way, it’s easy and safe to retrieve a -mutable slice of the unfilled (but already initialized) region. This slice can be used to build -a `WriteCursor`. Another purpose of the filled region tracking is to enable multiple items to be -written consecutively in the same buffer. For instance, the `encode_buf` function looks like this: +Observe that this code, which relies on `Vec`, demands extra bookkeeping for the filled region +and a cautious approach when clearing and resizing it. + +In comparison, the same code using `WriteBuf` looks like this: ```rust pub fn encode_buf(pdu: &T, buf: &mut WriteBuf) -> PduResult { let pdu_size = pdu.size(); + let dst = buf.unfilled_to(pdu_size); + let mut cursor = WriteCursor::new(dst); encode_cursor(pdu, &mut cursor)?; + let written = cursor.pos(); buf.advance(written); + Ok(written) } -``` - -A big enough mutable slice of the unfilled region is retrieved by using the `unfilled_to` helper -method (allocating more memory as necessary), a `WriteCursor` wrapping this slice is created and -used to actually encode the PDU. The filled region cursor of the `WriteBuf` is moved forward so that -the `filled` method returns the right region, and subsequent calls to `encode_buf` do not override -it until `clear` is called. -Consider this piece of code: +fn somewhere_else() -> PduResult<()> { + let mut state_machine = /* … */; + let mut buf = WriteBuf::new(); -```rust -fn process(&mut self, payload: &[u8], output: &mut WriteBuf) -> PduResult<()> { - let server_request = ServerRequest: ironrdp_pdu::decode(payload)?; + while !state_machine.is_terminal() { + let pdus = state_machine.step(); - match server_request.order { - ServerOrder::DoThis => { - // do this - let response = DoThisResponse { … }; + buf.clear(); - // buffer is grown, or not, as appropriate, and `DoThisResponse` is encoded in the "unfilled" region - ironrdp_pdu::encode_buf(response, output)?; + for pdu in pdus { + encode_buf(&pdu, &mut buf)?; } - ServerOrder::DoThat => { - // do that - let response = DoThatResponse { … }; - // same as above - ironrdp_pdu::encode_buf(response, output)?; - } - } + let filled = buf.filled(); - Ok(()) + // Do something with `filled` + } } ``` -Methods such as `write_u8` are overlapping with the `WriteCursor` API, but it’s mostly for -convenience if one needs to manually write something in an ad-hoc fashion. - -Otherwise, using `WriteCursor` is preferred in order to write `no-std` and `no-alloc` friendly code. +A big enough mutable slice of the unfilled region is retrieved by using the `unfilled_to` helper +method (allocating more memory as necessary), a `WriteCursor` wrapping this slice is created and +used to actually encode the PDU. The filled region cursor of the `WriteBuf` is moved forward so that +the `filled` method returns the right region, and subsequent calls to `encode_buf` do not override +it until `clear` is called. ## Most PDUs are "plain old data" structures with public fields @@ -130,3 +196,207 @@ When hiding some fields is really required, one of the following approach is sug [2]: https://doc.rust-lang.org/reference/expressions/struct-expr.html#functional-update-syntax [3]: https://matklad.github.io/2022/05/29/builder-lite.html [4]: https://xaeroxe.github.io/init-struct-pattern/ + +## Enumeration-like types should allow resilient parsing + +The **TL;DR** is that enums in Rust should not be used when parsing resilience is required. + +Network protocols are generally designed with forward and backward compatibility in mind. Items +like status codes, error codes, and other enumeration-like types may later get extended to include +new values. Additionally, it's not uncommon for different implementations of the same protocol to +disagree with each other. Some implementations may exhibit a bug, inserting an unexpected value, +while others may intentionally extend the original protocol with a custom value. + +Therefore, implementations of such protocols should decode these into a more flexible data type +that can accommodate a broader range of values, even when working with a language like Rust. As +long as the unknown value is not critical and can be handled gracefully, the implementation should +make every effort not to fail, especially during parsing. + +For example, let’s consider the following `enum` in Rust: + +```rust +#[repr(u32)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum MyNetworkCode { + FirstValue = 0, + SecondValue = 1, +} +``` + +This type cannot be used to hold new values that may be added to the protocol in the future. Thus, +it is not a suitable option. + +Many Rust developers will instinctively write the following instead: + +```rust +#[repr(u32)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum MyNetworkCode { + FirstValue = 0, + SecondValue = 1, + // Fallback variant + Unknown(u32), +} +``` + +This type can indeed hold additional values in its fallback variant, and the implementation can +remain compatible with future protocol versions as desired. + +However, this solution is not without its issues. There is a hard to catch forward-compatibility +hazard at the library level: when a new value, such as `ThirdValue = 2`, is added to the enum, the +`Unknown` variant for this value will no longer be emitted by the library (the library does not +construct and return the value anymore). This can lead to _silent_ failures in downstream code that +relies on matching `Unknown` to handle the previously unknown value. + +For instance: + +```rust +// Library code + +impl MyNetworkCode { + fn parse_network_code(value: u32) -> MyNetworkCode { + match value { + 0 => MyNetworkCode::FirstValue, + 1 => MyNetworkCode::SecondValue, + _ => MyNetworkCode::Unknown(value), + } + } +} + +// User code + +fn handle_network_code(reader: /* … */) { + let value = reader.read_u32(); + let code = MyNetworkCode::from_u32(value); + + if code == MyNetworkCode::Unknown(2) { + // The library doesn’t know about this value yet, but we need to handle it because […] + } +} +``` + +Once the library is updated to handle the third value: + +```rust +// Library code + +#[repr(u32)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum MyNetworkCode { + FirstValue = 0, + SecondValue = 1, + ThirdValue = 2, // NEW + Unknown(u32), +} + +impl MyNetworkCode { + fn parse_network_code(value: u32) -> MyNetworkCode { + match value { + 0 => MyNetworkCode::FirstValue, + 1 => MyNetworkCode::SecondValue, + 2 => MyNetworkCode::ThirdValue, // NEW + _ => MyNetworkCode::Unknown(value), // This branch is not entered anymore when value = 2 + } + } +} + +// User code (unchanged) + +fn handle_network_code(reader: /* … */) { + let value = reader.read_u32(); + let code = MyNetworkCode::from_u32(value); + + if code == MyNetworkCode::Unknown(2) { // <- The library does not construct this value as it used to… + // … the special case is not handled anymore; no warning and no error + // is emitted by the compiler, so it’s very easy to overlook this + } +} +``` + +Several other concerns arise: + +- `Unknown(2)` and `ThirdValue` are conceptually the same thing, but are represented differently in memory. +- The default `PartialEq` implementation that can be derived will return `false` when testing for + equality (i.e.: `Unknown(2) != ThirdValue`). Fixing this requires manual implementation of `PartialEq`. +- Even if `PartialEq` is fixed, the pattern matching issue can’t be fixed. +- The size of this type is bigger than necessary. + +All in all, this can be considered a potential source of bugs in code consuming the API, and the +bottom line is to avoid type definitions that allow for the same thing to be represented in two +different ways, i.e: different "type-level values", because the library will not return or "emit" +both at the same time; it’s as if one of the two value was implicitly "dead"[^deranged-note]. + +[^deranged-note]: Note that [`RangedU32::new_static`][RangedU32_new_static] from [`deranged`][deranged] +(ranged integers library) could help here, but in this case the end result is not ergonomic and still +error-prone as it’s natural to reach for [`RangedU32::get`][RangedU32_get] instead when comparing the +value. This approach would work if a lint was emitted when the compiler detects that the condition +operand will always evaluate to `false`. Built-in ranged integers would be great here. + +Another approach would be for `Unknown` not to hold the original value at all, ensuring it can never +overlap with another variant. In this case, users would need to wait for the library to be updated +before they can implement special handling for `ThirdValue`: + +```rust +#[repr(u32)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum MyNetworkCode { + FirstValue = 0, + SecondValue = 1, + // Fallback variant; do not rely on this variant + Unknown, +} +``` + +However, there is no guarantee that users will not rely on `Unknown` being emitted anyway. This +approach merely discourages them from doing so by making the fallback variant much less powerful. + +The downside here is that one can no longer determine the original payload's value, and the "round- +trip" property is lost because this becomes destructive parsing, with no way to determine how the +structure should be (re)encoded. + +Overall, it’s not a very good option either. + +Instead, consider providing a newtype struct wrapping the appropriate integer type: + +```rust +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct MyNetworkCode(u32); + +impl MyNetworkCode { + pub const FIRST_VALUE: Self = Self(0); + pub const SECOND_VALUE: Self = Self(1); +} +``` + +This is exactly [how the `http` crate deals with status codes][http-status-code]. + +This pattern is used in several places: + +- `FailureCode` in the `ironrdp_pdu::nego` module. +- `Code` in the `ironrdp_graphics::rle` module. +- `ClipboardFormatId` in the `ironrdp_cliprdr::pdu::format_list` module. + +Of course, the main drawback is that exhaustive matching is not possible. + +The question to consider is whether it's genuinely necessary to handle all the possible values +explicitly. It may not often be required or desirable. Since neither the client nor the server +should typically break when the protocol is extended, an older client (not handling the new value) +should generally be able to function properly with a newer server, and vice versa. In other words, +not handling the new value must not be an immediate problem. However, it is often desirable to +handle a subset of possible values or to use it for logging or metric purposes. For these purposes, +it’s actually fine to not do exhaustive matching, and ability to work with unknown values is useful +(e.g.: logging unknown values). + +Yet, it can be useful to provide a pattern-matching-friendly API in some cases. In such situations, +it is advisable to also offer a higher-level abstraction, such as an enum, that can be obtained from +the "raw" `MyNetworkCode` value. This pattern is applied in the `ironrdp-rdcleanpath` crate to convert +from a lower-level, future-proof, resilient but awkward-to-use type, `RDCleanPathPdu`, to a high-level, +easier-to-use type, `RDCleanPath`. + +Don’t forget that in some cases, the protocol specification explicitly states that other values +MUST always be rejected. In such cases, an `enum` is deemed appropriate. + +[RangedU32_new_static]: https://docs.rs/deranged/0.3.8/deranged/struct.RangedU32.html#method.new_static +[RangedU32_get]: https://docs.rs/deranged/0.3.8/deranged/struct.RangedU32.html#method.get +[deranged]: https://docs.rs/deranged/0.3.8/ +[http-status-code]: https://docs.rs/http/0.2.9/http/status/struct.StatusCode.html diff --git a/crates/ironrdp-pdu/src/cursor.rs b/crates/ironrdp-pdu/src/cursor.rs index bfc84810a..d59ed097d 100644 --- a/crates/ironrdp-pdu/src/cursor.rs +++ b/crates/ironrdp-pdu/src/cursor.rs @@ -204,6 +204,16 @@ impl<'a> ReadCursor<'a> { } } +#[cfg(feature = "std")] +impl std::io::Read for ReadCursor<'_> { + fn read(&mut self, buf: &mut [u8]) -> std::io::Result { + let n_to_copy = core::cmp::min(buf.len(), self.len()); + let to_copy = self.read_slice(n_to_copy); + buf.copy_from_slice(to_copy); + Ok(n_to_copy) + } +} + #[derive(Debug)] pub struct WriteCursor<'a> { inner: &'a mut [u8], @@ -308,3 +318,15 @@ impl<'a> WriteCursor<'a> { } } } + +#[cfg(feature = "std")] +impl std::io::Write for WriteCursor<'_> { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.write_slice(buf); + Ok(buf.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } +} diff --git a/crates/ironrdp-pdu/src/write_buf.rs b/crates/ironrdp-pdu/src/write_buf.rs index 9c7957547..4183e3813 100644 --- a/crates/ironrdp-pdu/src/write_buf.rs +++ b/crates/ironrdp-pdu/src/write_buf.rs @@ -1,4 +1,4 @@ -use std::ops::{Index, Range, RangeFrom, RangeFull, RangeInclusive, RangeTo, RangeToInclusive}; +use core::ops::{Index, Range, RangeFrom, RangeFull, RangeInclusive, RangeTo, RangeToInclusive}; /// Max capacity to keep for the inner Vec when `WriteBuf::clear` is called. const MAX_CAPACITY_WHEN_CLEARED: usize = 16384; // 16 kib diff --git a/crates/ironrdp-rdpdr/Cargo.toml b/crates/ironrdp-rdpdr/Cargo.toml index 9b8a02d3b..7a8405e9f 100644 --- a/crates/ironrdp-rdpdr/Cargo.toml +++ b/crates/ironrdp-rdpdr/Cargo.toml @@ -1,11 +1,10 @@ [package] name = "ironrdp-rdpdr" version = "0.1.0" -description = "RDPDR channel implementation." readme = "README.md" +description = "RDPDR channel implementation." edition.workspace = true license.workspace = true - homepage.workspace = true repository.workspace = true authors.workspace = true diff --git a/crates/ironrdp-rdpdr/README.md b/crates/ironrdp-rdpdr/README.md index 3994d2f9b..97c3f4399 100644 --- a/crates/ironrdp-rdpdr/README.md +++ b/crates/ironrdp-rdpdr/README.md @@ -1 +1,3 @@ -# IronRDP RDPDR library +# IronRDP RDPDR + +RDPDR channel implementation. \ No newline at end of file diff --git a/crates/ironrdp-server/Cargo.toml b/crates/ironrdp-server/Cargo.toml index 6229a318c..10587bb62 100644 --- a/crates/ironrdp-server/Cargo.toml +++ b/crates/ironrdp-server/Cargo.toml @@ -2,7 +2,7 @@ name = "ironrdp-server" version = "0.1.0" readme = "README.md" -description = "" +description = "Extendable skeleton for implementing custom RDP servers" edition.workspace = true license.workspace = true homepage.workspace = true diff --git a/crates/ironrdp-server/README.md b/crates/ironrdp-server/README.md index 2e7ba92e5..2d50f035d 100644 --- a/crates/ironrdp-server/README.md +++ b/crates/ironrdp-server/README.md @@ -1,8 +1,11 @@ # IronRDP Server -Library for implementing custom async RDP servers on the tokio runtime. +Extendable skeleton for implementing custom RDP servers. + +For now, it requires the [Tokio runtime](https://tokio.rs/). --- + The server currently supports: **Security** @@ -16,6 +19,7 @@ The server currently supports: - bitmap display updates with RDP 6.0 compression --- + Custom logic for your RDP server can be added by implementing these traits: - `RdpServerInputHandler` - callbacks used when the server receives input events from a client - `RdpServerDisplay` - notifies the server of display updates diff --git a/crates/ironrdp-svc/src/lib.rs b/crates/ironrdp-svc/src/lib.rs index 54e092be1..822958de8 100644 --- a/crates/ironrdp-svc/src/lib.rs +++ b/crates/ironrdp-svc/src/lib.rs @@ -204,7 +204,7 @@ macro_rules! impl_as_any { /// /// To ensure uniqueness, each trait object is associated to the [`TypeId`] of it’s original type. /// Once joined, channels may have their ID attached using [`Self::attach_channel_id()`], effectively -/// associating them together so it’s possible. +/// associating them together. /// /// At this point, it’s possible to retrieve the trait object using either /// the type ID ([`Self::get_by_type_id()`]), the original type ([`Self::get_by_type()`]) or @@ -229,14 +229,19 @@ impl StaticChannelSet { } } + /// Inserts a [`StaticVirtualChannel`] into this [`StaticChannelSet`]. + /// + /// If a static virtual channel of this type already exists, it is returned. pub fn insert(&mut self, val: T) -> Option> { self.channels.insert(TypeId::of::(), Box::new(val)) } + /// Gets a reference to a [`StaticVirtualChannel`] trait object by looking up its [`TypeId`]. pub fn get_by_type_id(&self, type_id: TypeId) -> Option<&dyn StaticVirtualChannel> { self.channels.get(&type_id).map(|boxed| boxed.as_ref()) } + /// Gets a mutable reference to a [`StaticVirtualChannel`] trait object by looking up its [`TypeId`]. pub fn get_by_type_id_mut(&mut self, type_id: TypeId) -> Option<&mut dyn StaticVirtualChannel> { if let Some(boxed) = self.channels.get_mut(&type_id) { Some(boxed.as_mut()) @@ -245,46 +250,72 @@ impl StaticChannelSet { } } + /// Gets a reference to a [`StaticVirtualChannel`] trait object by looking up its [`TypeId`]. pub fn get_by_type(&self) -> Option<&dyn StaticVirtualChannel> { self.get_by_type_id(TypeId::of::()) } + /// Gets a mutable reference to a [`StaticVirtualChannel`] trait object by looking up its [`TypeId`]. pub fn get_by_type_mut(&mut self) -> Option<&mut dyn StaticVirtualChannel> { self.get_by_type_id_mut(TypeId::of::()) } + /// Gets a reference to a [`StaticVirtualChannel`] trait object by looking up its channel ID. pub fn get_by_channel_id(&self, channel_id: StaticChannelId) -> Option<&dyn StaticVirtualChannel> { self.get_type_id_by_channel_id(channel_id) .and_then(|type_id| self.get_by_type_id(type_id)) } + /// Gets a mutable reference to a [`StaticVirtualChannel`] trait object by looking up its channel ID. pub fn get_by_channel_id_mut(&mut self, channel_id: StaticChannelId) -> Option<&mut dyn StaticVirtualChannel> { self.get_type_id_by_channel_id(channel_id) .and_then(|type_id| self.get_by_type_id_mut(type_id)) } + /// Removes a [`StaticVirtualChannel`] from this [`StaticChannelSet`]. + /// + /// If a static virtual channel of this type existed, it will be returned. + pub fn remove_by_type_id(&mut self, type_id: TypeId) -> Option> { + let svc = self.channels.remove(&type_id); + if let Some(channel_id) = self.to_channel_id.remove(&type_id) { + self.to_type_id.remove(&channel_id); + } + svc + } + + /// Removes a [`StaticVirtualChannel`] from this [`StaticChannelSet`]. + /// + /// If a static virtual channel of this type existed, it will be returned. pub fn remove_by_type(&mut self) -> Option> { - self.channels.remove(&TypeId::of::()) + let type_id = TypeId::of::(); + self.remove_by_type_id(type_id) } + /// Attaches a channel ID to a static virtual channel. + /// + /// If a channel ID was already attached, it will be returned. pub fn attach_channel_id(&mut self, type_id: TypeId, channel_id: StaticChannelId) -> Option { self.to_type_id.insert(channel_id, type_id); self.to_channel_id.insert(type_id, channel_id) } + /// Gets the attached channel ID for a given static virtual channel. pub fn get_channel_id_by_type_id(&self, type_id: TypeId) -> Option { self.to_channel_id.get(&type_id).copied() } + /// Gets the attached channel ID for a given static virtual channel. pub fn get_channel_id_by_type(&self) -> Option { self.get_channel_id_by_type_id(TypeId::of::()) } + /// Gets the [`TypeId`] of the static virtual channel associated to this channel ID. pub fn get_type_id_by_channel_id(&self, channel_id: StaticChannelId) -> Option { self.to_type_id.get(&channel_id).copied() } - pub fn detach_id(&mut self, type_id: TypeId) -> Option { + /// Detaches the channel ID associated to a given static virtual channel. + pub fn detach_channel_id(&mut self, type_id: TypeId) -> Option { if let Some(channel_id) = self.to_channel_id.remove(&type_id) { self.to_type_id.remove(&channel_id); Some(channel_id) diff --git a/crates/ironrdp/Cargo.toml b/crates/ironrdp/Cargo.toml index 6caef384a..663a6c8fe 100644 --- a/crates/ironrdp/Cargo.toml +++ b/crates/ironrdp/Cargo.toml @@ -11,10 +11,6 @@ authors.workspace = true keywords.workspace = true categories.workspace = true -[package.metadata.docs.rs] -cargo-args = ["-Zunstable-options", "-Zrustdoc-scrape-examples"] -all-features = true - [lib] doctest = false test = false @@ -57,3 +53,11 @@ pico-args = "0.5" x509-cert = { version = "0.2.1", default-features = false, features = ["std"] } tracing.workspace = true tracing-subscriber = { version = "0.3", features = ["env-filter"] } + +[package.metadata.docs.rs] +cargo-args = ["-Zunstable-options", "-Zrustdoc-scrape-examples"] +all-features = true + +[[example]] +name = "screenshot" +doc-scrape-examples = true diff --git a/crates/ironrdp/examples/screenshot.rs b/crates/ironrdp/examples/screenshot.rs index 632e1933f..82a4ddb2d 100644 --- a/crates/ironrdp/examples/screenshot.rs +++ b/crates/ironrdp/examples/screenshot.rs @@ -6,11 +6,13 @@ //! //! In this basic client implementation, the client establishes a connection //! with the destination server, decodes incoming graphics updates, and saves the -//! resulting output as a BMP image file on the local disk. +//! resulting output as a BMP image file on the disk. //! //! ## Usage example //! +//! ```shell //! cargo run --example=screenshot -- --host -u -p -o out.bmp +//! ``` #[macro_use] extern crate tracing; @@ -210,7 +212,8 @@ fn build_config(username: String, password: String, domain: Option) -> c #[cfg(target_os = "netbsd")] platform: MajorPlatformType::UNIX, - no_server_pointer: false, + // Disable custom pointers (there is no user interaction anyway) + no_server_pointer: true, } }