refactor(rust): second iteration of now-proto-pdu library #7

pacmancoder · 2024-12-24T01:00:27Z

Updated protocol encode/decode logic according to the last NOW-PROTO specification
Removed internal types suck as VarStr/VarBuf from public API
Simplified Rust API
Added more docs
Fixed a lot of crate API issues from initial implementation
Reduced allocations.
- Decoded PDUs are borrowed by default. Ownership is only required for Error types.
- It is possible to avoid allocations completely during PDU construction when <'static> values for strings/buffers are used.
Improved tests

…vision

pacmancoder · 2024-12-24T11:26:08Z

doc/NOW-spec.md

@@ -973,17 +1014,13 @@ The NOW_EXEC_DATA_MSG message is used to send input/output data as part of a rem

 | Flag                                   | Meaning                         |
 |----------------------------------------|---------------------------------|
-| NOW_EXEC_FLAG_DATA_FIRST<br>0x00000001 | This is the first data message. |


protocol change: first flag for streams is not necessary and only complicates data API/protocol

CBenoit

Great job! Looks good to me, but I left a few high-level comments about documentation and protocol specification changes.

Here is another suggestion I have: rename the folder doc to docs. It’s typically called docs everywhere, so it’s best to stick to this convention. (Devolutions Gateway and rust-analyzer to cite those two)

CBenoit · 2025-01-08T15:50:18Z

README.md

+1. Update the protocol specification in `doc/NOW-spec.md`.
+    1. Bump the protocol version number.
+1. Update the Rust implementation of the protocol in `crates/now-proto-pdu`.
+    1. Bump current protocol version in `NowProtoVersion::CURRENT`
+1. Update the C# protocol implementation (WIP)
+1. Update C# clients (WIP)


question/suggestion: Is using 1. everywhere intended?

Suggested change

1. Update the protocol specification in `doc/NOW-spec.md`.

1. Bump the protocol version number.

1. Update the Rust implementation of the protocol in `crates/now-proto-pdu`.

1. Bump current protocol version in `NowProtoVersion::CURRENT`

1. Update the C# protocol implementation (WIP)

1. Update C# clients (WIP)

1. Update the protocol specification in `doc/NOW-spec.md`.

- Bump the protocol version number.

2. Update the Rust implementation of the protocol in `crates/now-proto-pdu`.

- Bump current protocol version in `NowProtoVersion::CURRENT`

3. Update the C# protocol implementation (WIP)

4. Update C# clients (WIP)

Yes, this was intended -- It is allowed by markdown and correctly displayed in rendered form as a numbered list -- subsequent structural changes in this list will not require changing all numbers manually when adding or removing steps and will produce smaller diff in PR. However, if you have a strong opinion on that, I could change this 🙂

Oh, I didn't know! I think it's fine! 🙂
It's a bit less nice to read in source, but I can see how it's useful when adding new elements in between. Maybe we don't need the sublists to be numbered though. I would just suggest to use bullets there if you don't mind, but up to you!

CBenoit · 2025-01-08T15:54:07Z

crates/now-proto-pdu/Cargo.toml

 ironrdp-core = { version = "0.1.2", features = ["alloc"] }
+ironrdp-error = { version = "0.1.1", features = ["alloc"] }


suggestion: As a rule of thumb, do not specify the patch number as you may be over-constraining the dependency graph. It’s fine for pre-1.x crates, but typically you’re better off never specifying them.

CBenoit · 2025-01-08T15:55:07Z

crates/now-proto-pdu/README.md

+## Library architecture details
+- The library only provides encoding/decoding functions for the NOW-proto protocol, transport layer
+  and session processing logic should be implemented by the client/server.
+- `#[no_std]` compatible. Requires `alloc`.
+- PDUs could contain borrowed data by default to avoid unnecessary string/vec allocations when
+  parsing. The only exception is error types which are `'static` and may allocate if optional
+  message is set.
+- PDUs are immutable by default. PDU constructors take only required fields, optional fields are
+  set using implicit builder pattern (consuming `.with_*` and `.set_*` methods).
+- User-facing `bitfield` types should be avoied in the public API if incorrect usage could lead to
+  invalid PDUs. E.g. `ExecData`'s stdio stream flags are represented as a bitfield, but exactly
+  one of the flags should be set at a time. The public API should provide a safe way to set and
+  retrieve these flags. Channel capabilities flags on the other hand could all be set independently,
+  therefore it is safe to expose them in the public API.
+- Primitive protocol types e.g `NOW_STRING` should not be exposed in the public API.
+- Message validition should be always checked in the PDU constructor(s). If the message have
+  variable fields, it should be ensured that it could fit into the message body (`u32`).
+- PDUs should NOT fail on deserialization if message body have more data to ensure backwards
+  compatibility with future protocol versions (e.g. new fields added to the end of the message in
+  the new protocol version).


praise: This kind of architecture overview is nice! 👍

CBenoit · 2025-01-08T16:03:48Z

crates/now-proto-pdu/README.md

+## Versioning
+Crate version is not tied to the protocol version (e.g. Introduction of breaking changes in the
+crate API does not necessarily mean a protocol version bump and vice versa). The currently
+implemented protocol version is stored in [`NowProtoVersion::CURRENT`] and should be updated
+accordingly when the protocol is updated.


suggestion: Maybe a good idea to mention that a handshake/negotiation packet is used to accommodate older servers and vice-versa, meaning that even a major bump in the protocol version does not mean we need a major bump for the crate, as the older protocol version is still supported.

CBenoit · 2025-01-08T16:08:08Z

doc/NOW-spec.md


    - `NOW_STATUS_ERROR_KIND_WINAPI`: code contains standard WinAPI error.
    - `NOW_STATUS_ERROR_KIND_UNIX`: code contains standard UNIX error code.

-**errorMessage(variable, optional)**: this value contains either optional error message if
+**errorMessage(variable)**: this value contains either optional error message if


Suggested change

**errorMessage(variable)**: this value contains either optional error message if

**errorMessage(variable)**: this value contains either an error message if

CBenoit · 2025-01-08T16:08:22Z

doc/NOW-spec.md


    - `NOW_STATUS_ERROR_KIND_WINAPI`: code contains standard WinAPI error.
    - `NOW_STATUS_ERROR_KIND_UNIX`: code contains standard UNIX error code.

-**errorMessage(variable, optional)**: this value contains either optional error message if
+**errorMessage(variable)**: this value contains either optional error message if
 `NOW_STATUS_ERROR_MESSAGE` flag is set, or empty sting if the flag is not set.


Suggested change

`NOW_STATUS_ERROR_MESSAGE` flag is set, or empty sting if the flag is not set.

`NOW_STATUS_ERROR_MESSAGE` flag is set, or empty string if the flag is not set.

CBenoit · 2025-01-08T16:14:33Z

doc/NOW-spec.md

+The NOW_EXEC_ABORT_MSG message is used to abort a remote execution immediately. See NOW_EXEC_CANCEL_REQ if
+the graceful session cancellation is needed instead. This message can be sent by the client at any point of session lifetime. The session is considered aborted as soon as this message is sent.


suggestion: I recommend you to use the "sentence-per-line" style: https://github.com/Devolutions/IronRDP/blob/25bbb2682c95419fc18a61cab81e29a0ab9f23d4/STYLE.md#sentence-per-line-style

Suggested change

The NOW_EXEC_ABORT_MSG message is used to abort a remote execution immediately. See NOW_EXEC_CANCEL_REQ if

the graceful session cancellation is needed instead. This message can be sent by the client at any point of session lifetime. The session is considered aborted as soon as this message is sent.

The NOW_EXEC_ABORT_MSG message is used to abort a remote execution immediately.

See NOW_EXEC_CANCEL_REQ if the graceful session cancellation is needed instead.

This message can be sent by the client at any point of session lifetime.

The session is considered aborted as soon as this message is sent.

CBenoit · 2025-01-08T16:16:08Z

doc/NOW-spec.md

@@ -743,7 +784,6 @@ The NOW_EXEC_MSG message is used to execute remote commands or scripts.

 | Value | Meaning |
 |-------|---------|
-| NOW_EXEC_CAPSET_MSG_ID<br>0x00 | NOW_EXEC_CAPSET_MSG |


question: Are we removing this message? Is it superseded by something else?

Yep. This was previosuly in exec message class -- now it was moved to channel message class

CBenoit · 2025-01-08T16:20:12Z

doc/NOW-spec.md

@@ -385,6 +387,45 @@ the specified interval, it should consider the connection as lost.

 **msgFlags (2 bytes)**: The message flags.

+#### NOW_CHANNEL_TERMINATE_MSG


naming: What about this naming?

Suggested change

#### NOW_CHANNEL_TERMINATE_MSG

#### NOW_CHANNEL_CLOSE_MSG

CBenoit · 2025-01-08T16:21:09Z

doc/NOW-spec.md

@@ -761,7 +801,8 @@ The NOW_EXEC_MSG message is used to execute remote commands or scripts.

 #### NOW_EXEC_ABORT_MSG

-The NOW_EXEC_ABORT_MSG message is used to abort a remote execution immediately due to an unrecoverable error. This message can be sent at any time without an explicit response message. The session is considered aborted as soon as this message is sent.
+The NOW_EXEC_ABORT_MSG message is used to abort a remote execution immediately. See NOW_EXEC_CANCEL_REQ if


praise: +1 for referencing other relevant messages.

CBenoit · 2025-01-08T16:28:09Z

Another remark: the commit type for refactoring is refactor. I’m also unsure if we want to go with a "rust" scope, but I’ll let you decide what’s best here. (Not specifying a scope at all purposefully may make sense too.)

pacmancoder added 2 commits December 24, 2024 03:04

test(proto): Add tests for initial proto implementation

6f4184e

refactoring(rust): Refactoring of now-proto according to new proto re…

b17ed2c

…vision

pacmancoder force-pushed the feat/now-proto-improvements branch from 9fb68f0 to 767d128 Compare December 24, 2024 01:04

refactoring(rust): updated unit tests for now-proto-pdu

805c4b5

pacmancoder force-pushed the feat/now-proto-improvements branch from 767d128 to 805c4b5 Compare December 24, 2024 01:11

refactoring(rust): Updated docs, fixed tests

09cbe85

pacmancoder changed the title ~~Feat/now proto improvements~~ refactoring(rust): second iteration of NOW-proto library Dec 24, 2024

pacmancoder commented Dec 24, 2024

View reviewed changes

pacmancoder requested a review from CBenoit December 24, 2024 11:26

pacmancoder marked this pull request as ready for review December 24, 2024 11:59

pacmancoder self-assigned this Dec 24, 2024

refactoring(agent): fixed minor documentation errors

53f0058

CBenoit approved these changes Jan 8, 2025

View reviewed changes

CBenoit changed the title ~~refactoring(rust): second iteration of NOW-proto library~~ refactor(rust): second iteration of now-proto-pdu library Jan 8, 2025

refactor(pdu-rust): refactoring after code review

5acba5f

pacmancoder merged commit 050fb03 into master Jan 14, 2025
9 checks passed

pacmancoder deleted the feat/now-proto-improvements branch January 14, 2025 11:01

pacmancoder mentioned this pull request Jan 14, 2025

refactor(pdu-dotnet): second iteration of NowProto dotnet package #9

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(rust): second iteration of now-proto-pdu library #7

refactor(rust): second iteration of now-proto-pdu library #7

pacmancoder commented Dec 24, 2024 •

edited

Loading

pacmancoder Dec 24, 2024 •

edited

Loading

CBenoit left a comment

CBenoit Jan 8, 2025

pacmancoder Jan 9, 2025 •

edited

Loading

CBenoit Jan 9, 2025

CBenoit Jan 8, 2025

CBenoit Jan 8, 2025

CBenoit Jan 8, 2025

CBenoit Jan 8, 2025

CBenoit Jan 8, 2025

CBenoit Jan 8, 2025

CBenoit Jan 8, 2025

pacmancoder Jan 9, 2025

CBenoit Jan 8, 2025

CBenoit Jan 8, 2025

CBenoit commented Jan 8, 2025

		ironrdp-core = { version = "0.1.2", features = ["alloc"] }
		ironrdp-error = { version = "0.1.1", features = ["alloc"] }

	errorMessage(variable): this value contains either optional error message if
	errorMessage(variable): this value contains either an error message if

	`NOW_STATUS_ERROR_MESSAGE` flag is set, or empty sting if the flag is not set.
	`NOW_STATUS_ERROR_MESSAGE` flag is set, or empty string if the flag is not set.

		The NOW_EXEC_ABORT_MSG message is used to abort a remote execution immediately. See NOW_EXEC_CANCEL_REQ if
		the graceful session cancellation is needed instead. This message can be sent by the client at any point of session lifetime. The session is considered aborted as soon as this message is sent.

		@@ -385,6 +387,45 @@ the specified interval, it should consider the connection as lost.

		msgFlags (2 bytes): The message flags.

		#### NOW_CHANNEL_TERMINATE_MSG

refactor(rust): second iteration of now-proto-pdu library #7

refactor(rust): second iteration of now-proto-pdu library #7

Conversation

pacmancoder commented Dec 24, 2024 • edited Loading

pacmancoder Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

CBenoit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pacmancoder Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CBenoit commented Jan 8, 2025

pacmancoder commented Dec 24, 2024 •

edited

Loading

pacmancoder Dec 24, 2024 •

edited

Loading

pacmancoder Jan 9, 2025 •

edited

Loading