rfc1928: the keep-or-cut sweep + the honest refusal vocabulary (review-002 N-1, N-4)
- N-1 cut: auth_method_enums! (macro + generated StandardAuthentication/ StandardAuthenticationStarted/allow_no_auth), skip_auth_this_is_not_ rfc_compliant, CheckResult's Option/Result impls (bool stays) — the producer-replaced machinery; ADR-013's no-speculative-surface posture - N-1 keep-notes made current: ConnectError (hand-port companion; the fired trigger + keep disposition recorded in stream.rs), transfer/wait_on_tcp (differential-suite-consumed), Bug (upstream- verbatim, display intact) - N-4: new SocksServerError::Refused(&'static str) — the honest log vocabulary for the five in-stream policy refusal sites (the BIND command gate + dial/egress/bind-factory/bind-accept seams) and the harness's minimal producer; wire bytes untouched (Bug keeps its display; AuthenticationRejected stays the typestate's auth-reject) - Provenance extended, not replaced (convention 17): server.rs module doc's owned-changes paragraph + rfc1928.md's owned-code post-fork changes list + last_updated - Differential :103 comment reworded to the RFC's ordering rationale; AuthMethod doc updated for the macro cut - Tests: refusal-site session-error asserts moved to Refused, the mapping-table loop grew the variant, a display/shape test pins it Verified: cargo test (356 green incl. --all-features), clippy/fmt/doc clean, both wasm checks clean.
This commit is contained in:
1 parent
ff81f2d27b
commit
68d5be36a0
7 files changed
+211
-147
No files matched your search
@@ -1,10 +1,12 @@
|
||||
---
|
||||
status: draft
|
||||
last_updated: 2026-09-22 (the targeted fork — ADR-013 supersedes
|
||||
ADR-011's maintenance posture; the module is `src/rfc1928/`, the
|
||||
fork point is `5b18d4d`, the lint-allow posture dissolves;
|
||||
extraction review F-1: `ReplyError::from_u8`'s panic arm mapped to
|
||||
`GeneralFailure`; Phase 1 initial draft 2026-09-20)
|
||||
last_updated: 2026-09-24 (the keep-or-cut sweep + the refusal
|
||||
vocabulary variant — the fork's owned post-fork changes section;
|
||||
review 002 N-1/N-4; the targeted fork 2026-09-22 — ADR-013
|
||||
supersedes ADR-011's maintenance posture; the module is
|
||||
`src/rfc1928/`, the fork point is `5b18d4d`, the lint-allow posture
|
||||
dissolves; extraction review F-1: `ReplyError::from_u8`'s panic arm
|
||||
mapped to `GeneralFailure`; Phase 1 initial draft 2026-09-20)
|
||||
---
|
||||
|
||||
# alksocks — The RFC 1928 Core (the targeted fork)
|
||||
@@ -113,6 +115,33 @@ four dissolve with the kernel-net region:
|
||||
Dissolved: the `net` feature gate (no net code in-tree) and the
|
||||
`run_udp_proxy_custom` bind seam (the handler isn't forked).
|
||||
|
||||
Plus the **owned-code post-fork changes** (the fork is first-class
|
||||
code — extend this list, never replace it; convention 17):
|
||||
|
||||
- **The keep-or-cut sweep** (review 002 N-1): the producer-replaced
|
||||
machinery is cut from the typestate — the `auth_method_enums!`
|
||||
macro (upstream's enum generator) with its generated
|
||||
`StandardAuthentication` enum + `allow_no_auth`,
|
||||
`skip_auth_this_is_not_rfc_compliant`, and `CheckResult`'s
|
||||
`Option`/`Result` impls (`bool` stays; re-add with a
|
||||
typed-rejection design if a door ever wants it). None had an
|
||||
in-tree consumer once the producer drove `negotiate_auth` directly
|
||||
(`&[NoAuthentication]`; per-door auth is ADR-005's, not the
|
||||
typestate's). Kept with note (the hand-port map, ADR-013):
|
||||
`ConnectError` (constructed nowhere in-tree; the dial-error →
|
||||
reply-code mapping a future kernel-net port reuses) and
|
||||
`transfer`/`wait_on_tcp` (differential-suite-consumed — their
|
||||
semantics stay pinned there). The upstream `Bug` variant is
|
||||
likewise kept, its display intact.
|
||||
- **The refusal vocabulary variant** (review 002 N-4):
|
||||
`SocksServerError::Refused(&'static str)` — the honest
|
||||
log/observability vocabulary for the producer's in-stream policy
|
||||
refusals (the command gate + the four seam-refusal sites). Upstream
|
||||
has no such variant; it is an owned addition, never reaching the
|
||||
wire (the RFC reply goes out first; the variant's
|
||||
`to_reply_error` mapping is the `GeneralFailure` fall-through,
|
||||
pinned by the mapping-table test).
|
||||
|
||||
Plus the provenance notes: the fork point (upstream v1.0.0,
|
||||
extraction `57b107b..5b18d4d`, fork at the review commit) and the
|
||||
license notice (MIT, extended — ADR-013's license mechanics).
|
||||
|
||||
+19
-15
@@ -228,11 +228,15 @@ where
|
||||
// The command gate (ADR-006): BIND mints inbound
|
||||
// listeners — an identity with proxy egress must not
|
||||
// imply ingress. RFC-faithful refusal (the command is
|
||||
// known, the caller is not allowed).
|
||||
// known, the caller is not allowed). The session
|
||||
// error is the log/observability vocabulary (the wire
|
||||
// got its reply already) — `Refused`, the honest
|
||||
// shape (review 002 N-4; an ACL refusal is not an
|
||||
// auth failure).
|
||||
proto.reply_error(&ReplyError::ConnectionNotAllowed).await?;
|
||||
return Err(SocksError::ServerError(
|
||||
SocksServerError::AuthenticationRejected,
|
||||
));
|
||||
return Err(SocksError::ServerError(SocksServerError::Refused(
|
||||
"bind command gate: not allowed",
|
||||
)));
|
||||
}
|
||||
serve_tcp_bind(proto, target, accept).await
|
||||
}
|
||||
@@ -262,7 +266,7 @@ where
|
||||
Ok(handle) => handle,
|
||||
Err(err) => {
|
||||
proto.reply_error(&err.to_reply_error()).await?;
|
||||
return Err(SocksError::ServerError(SocksServerError::Bug(
|
||||
return Err(SocksError::ServerError(SocksServerError::Refused(
|
||||
"dial refused in-stream",
|
||||
)));
|
||||
}
|
||||
@@ -302,7 +306,7 @@ where
|
||||
Ok(egress) => egress,
|
||||
Err(err) => {
|
||||
proto.reply_error(&err.to_reply_error()).await?;
|
||||
return Err(SocksError::ServerError(SocksServerError::Bug(
|
||||
return Err(SocksError::ServerError(SocksServerError::Refused(
|
||||
"egress refused in-stream",
|
||||
)));
|
||||
}
|
||||
@@ -495,7 +499,7 @@ where
|
||||
Ok(accept) => accept,
|
||||
Err(err) => {
|
||||
proto.reply_error(&err.to_reply_error()).await?;
|
||||
return Err(SocksError::ServerError(SocksServerError::Bug(
|
||||
return Err(SocksError::ServerError(SocksServerError::Refused(
|
||||
"bind factory refused in-stream",
|
||||
)));
|
||||
}
|
||||
@@ -553,7 +557,7 @@ where
|
||||
.write_all(&new_reply(&err.to_reply_error(), sentinel_bind_peer_addr()))
|
||||
.await;
|
||||
let _ = inner.flush().await;
|
||||
return Err(SocksError::ServerError(SocksServerError::Bug(
|
||||
return Err(SocksError::ServerError(SocksServerError::Refused(
|
||||
"bind accept refused in-stream",
|
||||
)));
|
||||
}
|
||||
@@ -919,7 +923,7 @@ mod tests {
|
||||
.expect_err("the session surfaces the refusal in-stream");
|
||||
assert!(matches!(
|
||||
err,
|
||||
SocksError::ServerError(SocksServerError::Bug("dial refused in-stream"))
|
||||
SocksError::ServerError(SocksServerError::Refused("dial refused in-stream"))
|
||||
));
|
||||
}
|
||||
|
||||
@@ -1524,7 +1528,7 @@ mod tests {
|
||||
.expect_err("the refusal surfaces in-stream");
|
||||
assert!(matches!(
|
||||
err,
|
||||
SocksError::ServerError(SocksServerError::Bug("egress refused in-stream"))
|
||||
SocksError::ServerError(SocksServerError::Refused("egress refused in-stream"))
|
||||
));
|
||||
}
|
||||
|
||||
@@ -1873,7 +1877,7 @@ mod tests {
|
||||
.expect_err("the refusal surfaces in-stream");
|
||||
assert!(matches!(
|
||||
err,
|
||||
SocksError::ServerError(SocksServerError::AuthenticationRejected)
|
||||
SocksError::ServerError(SocksServerError::Refused("bind command gate: not allowed"))
|
||||
));
|
||||
|
||||
// CONNECT still serves the same identity (egress unaffected).
|
||||
@@ -2026,7 +2030,7 @@ mod tests {
|
||||
.expect_err("the refusal surfaces in-stream");
|
||||
assert!(matches!(
|
||||
err,
|
||||
SocksError::ServerError(SocksServerError::Bug("bind factory refused in-stream"))
|
||||
SocksError::ServerError(SocksServerError::Refused("bind factory refused in-stream"))
|
||||
));
|
||||
}
|
||||
|
||||
@@ -2065,7 +2069,7 @@ mod tests {
|
||||
.expect_err("the refusal surfaces in-stream");
|
||||
assert!(matches!(
|
||||
err,
|
||||
SocksError::ServerError(SocksServerError::Bug("bind accept refused in-stream"))
|
||||
SocksError::ServerError(SocksServerError::Refused("bind accept refused in-stream"))
|
||||
));
|
||||
}
|
||||
|
||||
@@ -2334,7 +2338,7 @@ mod tests {
|
||||
.expect_err("the refusal surfaces in-stream");
|
||||
assert!(matches!(
|
||||
err,
|
||||
SocksError::ServerError(SocksServerError::AuthenticationRejected)
|
||||
SocksError::ServerError(SocksServerError::Refused("bind command gate: not allowed"))
|
||||
));
|
||||
|
||||
// CONNECT still serves the same producer (egress unaffected).
|
||||
@@ -2471,7 +2475,7 @@ mod tests {
|
||||
.expect_err("the refusal surfaces in-stream");
|
||||
assert!(matches!(
|
||||
err,
|
||||
SocksError::ServerError(SocksServerError::AuthenticationRejected)
|
||||
SocksError::ServerError(SocksServerError::Refused("bind command gate: not allowed"))
|
||||
));
|
||||
}
|
||||
|
||||
|
||||
+61
-110
@@ -16,15 +16,28 @@
|
||||
//! inventory row 1 in `docs/architecture/rfc1928.md`.
|
||||
//!
|
||||
//! Extraction-time adjustments (mechanical, ADR-011 §Amendment):
|
||||
//! `log` calls dropped; `anyhow::Context` dropped; the
|
||||
//! `#[macro_export]` on `auth_method_enums!` converted to `pub`
|
||||
//! scoping within this module (same adjustment as `read_exact!`);
|
||||
//! `reply_error`'s `"0.0.0.0:0".parse().unwrap()` replaced with a
|
||||
//! `const` parsed at compile time (no `unwrap` outside tests).
|
||||
//! The upstream `Config<A>`/`Authentication` impls that consumed
|
||||
//! `log` calls dropped; `anyhow::Context` dropped; `reply_error`'s
|
||||
//! `"0.0.0.0:0".parse().unwrap()` replaced with a `const` parsed at
|
||||
//! compile time (no `unwrap` outside tests). The upstream
|
||||
//! `Config<A>`/`Authentication` impls that consumed
|
||||
//! `SocksServerError::AuthenticationRejected` are cut; the variant
|
||||
//! stays (the typestate's `accept_password_auth` produces it).
|
||||
//!
|
||||
//! Post-fork owned changes (extend, never replace — convention 17):
|
||||
//! the keep-or-cut sweep (review 002 N-1) cut the producer-replaced
|
||||
//! machinery — the `auth_method_enums!` macro (upstream's enum
|
||||
//! generator) with its generated `StandardAuthentication` enum +
|
||||
//! `allow_no_auth`, `skip_auth_this_is_not_rfc_compliant`, and
|
||||
//! `CheckResult`'s `Option`/`Result` impls (`bool` stays) — none had
|
||||
//! an in-tree consumer once the producer drove `negotiate_auth`
|
||||
//! directly. Kept with note (the hand-port map, ADR-013):
|
||||
//! `transfer`/`wait_on_tcp` are differential-suite-consumed (their
|
||||
//! semantics stay pinned there), and the error vocabulary keeps its
|
||||
//! upstream variants. One variant was added, not absorbed:
|
||||
//! `SocksServerError::Refused` — the honest refusal vocabulary for
|
||||
//! the producer's in-stream policy refusals (review 002 N-4; never
|
||||
//! reaches the wire). Recorded in docs/architecture/rfc1928.md.
|
||||
//!
|
||||
//! The BIND note (ADR-007): BIND's driving is producer-side code —
|
||||
//! the typestate already returns `TCPBind` from `read_command()`,
|
||||
//! `reply_success(sock_addr)` carries reply#1's BND.ADDR, and
|
||||
@@ -71,6 +84,8 @@ pub enum SocksServerError {
|
||||
AddrError(#[from] AddrError),
|
||||
#[error("BUG: {0}")] // should be unreachable
|
||||
Bug(&'static str),
|
||||
#[error("Refused: {0}")]
|
||||
Refused(&'static str),
|
||||
#[error("Auth method unacceptable `{0:?}`.")]
|
||||
AuthMethodUnacceptable(Vec<u8>),
|
||||
#[error("Unsupported SOCKS version `{0}`.")]
|
||||
@@ -142,13 +157,11 @@ impl<T> Socks5ServerProtocol<T, states::Opened> {
|
||||
}
|
||||
|
||||
/// Closure-result adapter for [`Socks5ServerProtocol
|
||||
/// ::accept_password_auth`]'s credential checker: `bool`,
|
||||
/// `Option<T>`, and `Result<T, E>` all work (`is_good`). In-tree
|
||||
/// callers use `bool` only; the `Option`/`Result` impls anticipate
|
||||
/// the producer layer's username/password mapping (typed rejections
|
||||
/// want `Result`-returning checkers — review 001 N-3). Same
|
||||
/// keep-or-cut decision as `StandardAuthentication` when the producer
|
||||
/// layer lands.
|
||||
/// ::accept_password_auth`]'s credential checker: `bool` (`is_good`).
|
||||
/// The `Option`/`Result` impls upstream-adjacent shape were cut in
|
||||
/// the keep-or-cut sweep (review 002 N-1) — every in-tree checker
|
||||
/// returns `bool`; re-add with a typed-rejection design if a door
|
||||
/// ever wants it.
|
||||
pub trait CheckResult {
|
||||
fn is_good(&self) -> bool;
|
||||
}
|
||||
@@ -159,18 +172,6 @@ impl CheckResult for bool {
|
||||
}
|
||||
}
|
||||
|
||||
impl<T> CheckResult for Option<T> {
|
||||
fn is_good(&self) -> bool {
|
||||
self.is_some()
|
||||
}
|
||||
}
|
||||
|
||||
impl<T, E> CheckResult for Result<T, E> {
|
||||
fn is_good(&self) -> bool {
|
||||
self.is_ok()
|
||||
}
|
||||
}
|
||||
|
||||
impl<T> Socks5ServerProtocol<T, states::Authenticated> {
|
||||
/// Finish handling the authentication method-specific part of the protocol,
|
||||
/// returning back to the overall SOCKS5 flow.
|
||||
@@ -178,14 +179,6 @@ impl<T> Socks5ServerProtocol<T, states::Authenticated> {
|
||||
Self::new(auth.into_inner())
|
||||
}
|
||||
|
||||
/// Wrap a socket in a SOCKS5 flow handler that's already marked as authenticated.
|
||||
///
|
||||
/// This is not actually part of the official SOCKS5 protocol, but allows you to
|
||||
/// only use the post-authentication subset of it.
|
||||
pub fn skip_auth_this_is_not_rfc_compliant(inner: T) -> Self {
|
||||
Self::new(inner)
|
||||
}
|
||||
|
||||
/// Handle the SOCKS5 auth negotiation supporting only the `NoAuthentication` method.
|
||||
pub async fn accept_no_auth(inner: T) -> Result<Self, SocksServerError>
|
||||
where
|
||||
@@ -244,8 +237,11 @@ pub trait AuthMethodSuccessState<T> {
|
||||
/// A metadata trait for authentication methods, essentially binding an ID value
|
||||
/// (as used in the method negotiation) to an actual implementation of the method.
|
||||
///
|
||||
/// Use blank structs for individual protocol implementations and
|
||||
/// enums for sets of supported protocols (you'll need a matching enum for the `Impl`).
|
||||
/// Use blank structs for individual protocol implementations;
|
||||
/// `negotiate_auth` takes any slice of `AuthMethod`s, so a custom
|
||||
/// selection is a slice of the method structs (upstream's
|
||||
/// enum-generator macro was cut — review 002 N-1; nothing in-tree
|
||||
/// drove a multi-method slice).
|
||||
pub trait AuthMethod<T>: Copy {
|
||||
type StartingState;
|
||||
fn method_id(self) -> u8;
|
||||
@@ -409,78 +405,6 @@ macro_rules! try_notify {
|
||||
};
|
||||
}
|
||||
|
||||
macro_rules! auth_method_enums {
|
||||
(
|
||||
$(#[$enum_meta:meta])*
|
||||
$vis:vis enum $enum:ident / $(#[$state_enum_meta:meta])* $state_enum:ident<$state_enum_par:ident> {
|
||||
$($method:ident($state:ty)),+ $(,)?
|
||||
}
|
||||
) => {
|
||||
$(#[$state_enum_meta])*
|
||||
$vis enum $state_enum<$state_enum_par> {
|
||||
$($method($state)),+
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy)]
|
||||
$(#[$enum_meta])*
|
||||
$vis enum $enum {
|
||||
$($method($method)),+
|
||||
}
|
||||
|
||||
impl<T> AuthMethod<T> for $enum {
|
||||
type StartingState = $state_enum<T>;
|
||||
|
||||
fn method_id(self) -> u8 {
|
||||
match self {
|
||||
$($enum::$method(auth) => AuthMethod::<T>::method_id(auth)),+
|
||||
}
|
||||
}
|
||||
|
||||
fn into_starting_state(self, inner: T) -> Self::StartingState {
|
||||
match self {
|
||||
$($enum::$method(auth) => $state_enum::$method(auth.into_starting_state(inner))),+
|
||||
}
|
||||
}
|
||||
}
|
||||
};
|
||||
}
|
||||
|
||||
auth_method_enums! {
|
||||
/// The combination of all authentication methods supported by this crate out of the box,
|
||||
/// as an enum appropriate for static dispatch.
|
||||
///
|
||||
/// If you want to add your own custom methods, you can generate a similar enum using the `auth_method_enums` macro.
|
||||
///
|
||||
/// Posture (review 001 C-2): currently uncalled in-tree — the
|
||||
/// typestate's `negotiate_auth` is driven with the single-method
|
||||
/// slices everywhere. Kept as the negotiation machinery's
|
||||
/// combinator: the producer layer's per-door auth selection
|
||||
/// (ADR-005's auth-split) is its anticipated consumer. If the
|
||||
/// producer layer lands without consuming it, cut it then.
|
||||
pub enum StandardAuthentication / StandardAuthenticationStarted<T> {
|
||||
NoAuthentication(NoAuthenticationImpl<T>),
|
||||
PasswordAuthentication(PasswordAuthenticationImpl<T, password_states::Started>),
|
||||
}
|
||||
}
|
||||
|
||||
impl StandardAuthentication {
|
||||
/// Return a slice containing either both supported methods or only `PasswordAuthentication`.
|
||||
pub fn allow_no_auth(allow: bool) -> &'static [StandardAuthentication] {
|
||||
if allow {
|
||||
&[
|
||||
// The order of authentication methods can be tested by clients in sequence,
|
||||
// so list more secure or preferred methods first
|
||||
StandardAuthentication::PasswordAuthentication(PasswordAuthentication),
|
||||
StandardAuthentication::NoAuthentication(NoAuthentication),
|
||||
]
|
||||
} else {
|
||||
&[StandardAuthentication::PasswordAuthentication(
|
||||
PasswordAuthentication,
|
||||
)]
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl<T: AsyncRead + AsyncWrite + Unpin> Socks5ServerProtocol<T, states::Opened> {
|
||||
/// Negotiate an authentication method from a list of supported ones and initialize it.
|
||||
///
|
||||
@@ -685,9 +609,14 @@ mod tests {
|
||||
/// variant falls through to `GeneralFailure` — the mapping the
|
||||
/// producer's error surface relies on when mapping session errors
|
||||
/// to reply codes (`try_notify!`'s secondary-error write). The
|
||||
/// fall-through arms (`Io`/`FromUtf8`/`EOF`/auth variants) are
|
||||
/// exercised here directly, since driving each from the wire is
|
||||
/// not one-test-cheap. Upstream `src/server.rs:62-70`.
|
||||
/// fall-through arms (`Io`/`FromUtf8`/`EOF`/auth/refusal variants)
|
||||
/// are exercised here directly, since driving each from the wire
|
||||
/// is not one-test-cheap. Upstream `src/server.rs:62-70`. The
|
||||
/// table also pins the vocabulary honesty (review 002 N-4):
|
||||
/// `Bug` and `Refused` are log-vocabulary variants — both fall
|
||||
/// through to `GeneralFailure` (they never reach the wire from
|
||||
/// the typestate; the producer's refusal sites write the RFC
|
||||
/// reply before the session error).
|
||||
#[test]
|
||||
fn to_reply_error_mapping_table() {
|
||||
assert!(matches!(
|
||||
@@ -706,6 +635,7 @@ mod tests {
|
||||
SocksServerError::AuthenticationRejected,
|
||||
SocksServerError::UnsupportedSocksVersion(0x04),
|
||||
SocksServerError::Bug("test"),
|
||||
SocksServerError::Refused("test"),
|
||||
SocksServerError::AuthMethodUnacceptable(vec![0x02]),
|
||||
SocksServerError::UnexpectedUdpControlGarbage(0xde),
|
||||
SocksServerError::Io {
|
||||
@@ -724,6 +654,27 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
/// The refusal vocabulary (review 002 N-4): `Refused` is the
|
||||
/// honest display for the producer's in-stream policy refusals —
|
||||
/// not `BUG:` (a working-as-designed refusal is not a bug) and
|
||||
/// not "Authentication rejected" (an ACL refusal is not an auth
|
||||
/// failure). It is log/observability vocabulary: never encoded
|
||||
/// on the wire by the typestate (only `to_reply_error` maps it,
|
||||
/// and the producer's refusal sites write the RFC reply before
|
||||
/// the session error). Upstream has no such variant — the
|
||||
/// fork's owned addition, recorded in the module doc.
|
||||
#[test]
|
||||
fn refused_variant_display_is_honest_and_wire_invisible() {
|
||||
assert_eq!(
|
||||
SocksServerError::Refused("policy says no").to_string(),
|
||||
"Refused: policy says no"
|
||||
);
|
||||
assert!(matches!(
|
||||
SocksServerError::Refused("policy says no").to_reply_error(),
|
||||
ReplyError::GeneralFailure
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn new_reply_v4_form() {
|
||||
let addr: SocketAddr = "127.0.0.1:1080".parse().unwrap();
|
||||
|
||||
@@ -14,12 +14,13 @@
|
||||
//!
|
||||
//! [`ConnectError`] is currently constructed nowhere in-tree (review
|
||||
//! 001 C-1 — upstream's sole construction site was the cut
|
||||
//! `tcp_connect` dialer). It is kept as the hand-port map's
|
||||
//! companion: the dial-error → reply-code mapping
|
||||
//! `tcp_connect` dialer). The keep-or-cut trigger (cut if the
|
||||
//! producer layer landed without consuming it) fired and the
|
||||
//! disposition is **keep** (review 002 N-1): it stays as the
|
||||
//! hand-port map's companion — the dial-error → reply-code mapping
|
||||
//! (`to_reply_error`) is what a future kernel-net port (e.g. the
|
||||
//! dial-callback shape, ADR-004) would reuse, and its mapping is
|
||||
//! pinned by the module's test. If the producer layer lands without
|
||||
//! consuming it, cut it then.
|
||||
//! pinned by the module's test.
|
||||
|
||||
/// Easy to destructure bytes buffers by naming each fields:
|
||||
///
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
---
|
||||
id: implementation/fix-rfc1928-posture
|
||||
name: "Fix — rfc1928 keep-or-cut pass + honest refusal vocabulary (review-002 N-1, N-4)"
|
||||
status: pending
|
||||
status: completed
|
||||
depends_on: []
|
||||
scope: narrow
|
||||
risk: medium
|
||||
@@ -86,20 +86,20 @@ display/shape if the vocabulary is table-tested elsewhere.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `StandardAuthentication`, `allow_no_auth`, the generated
|
||||
- [x] `StandardAuthentication`, `allow_no_auth`, the generated
|
||||
enum, `skip_auth_this_is_not_rfc_compliant`, and
|
||||
`CheckResult`'s `Option`/`Result` impls are cut; `bool` stays
|
||||
- [ ] `ConnectError`, `transfer`/`wait_on_tcp`, `Bug` kept — each
|
||||
- [x] `ConnectError`, `transfer`/`wait_on_tcp`, `Bug` kept — each
|
||||
with its keep-note current (module docs name the posture)
|
||||
- [ ] The differential suite stays green (the cut machinery was not
|
||||
- [x] The differential suite stays green (the cut machinery was not
|
||||
load-bearing for it; the `:103` comment reworded)
|
||||
- [ ] The five in-stream refusal sites carry the honest vocabulary
|
||||
- [x] The five in-stream refusal sites carry the honest vocabulary
|
||||
(one variant or the renamed display — the choice recorded);
|
||||
the wire bytes are untouched (the reply-code assertions stay
|
||||
green)
|
||||
- [ ] The fork's provenance notice is extended, not replaced, if the
|
||||
- [x] The fork's provenance notice is extended, not replaced, if the
|
||||
carried-changes note names the new variant (convention 17)
|
||||
- [ ] `cargo test` green; clippy/fmt clean; both wasm checks clean;
|
||||
- [x] `cargo test` green; clippy/fmt clean; both wasm checks clean;
|
||||
`cargo doc --no-deps` clean (module docs changed)
|
||||
|
||||
## References
|
||||
@@ -116,8 +116,87 @@ display/shape if the vocabulary is table-tested elsewhere.
|
||||
|
||||
## Notes
|
||||
|
||||
> Agent fills this during implementation.
|
||||
- **N-4 shape chosen: the dedicated variant** —
|
||||
`SocksServerError::Refused(&'static str)`, display `"Refused:
|
||||
{0}"` — not the `Bug` display rename. The review's sketch favored
|
||||
the dedicated variant ("the honest shape"), and the vocabulary
|
||||
split is real: `Bug` stays upstream-verbatim (`BUG: {0}`, its
|
||||
"should be unreachable" contract intact) while policy refusals
|
||||
get their own honest label. The five sites now read
|
||||
`Refused("bind command gate: not allowed")`,
|
||||
`Refused("dial refused in-stream")`,
|
||||
`Refused("egress refused in-stream")`,
|
||||
`Refused("bind factory refused in-stream")`,
|
||||
`Refused("bind accept refused in-stream")` — same
|
||||
`&'static str` payload shape as `Bug`, zero wire impact (the RFC
|
||||
reply went out before each `Err`).
|
||||
- **The harness's `Bug` site was migrated too** (the task said
|
||||
"leave unless it wants the new variant"): `tests/harness.rs`'s
|
||||
minimal CONNECT producer returned `Bug("dial refused in-stream")`
|
||||
— the same working-as-designed refusal shape, so it takes the new
|
||||
variant for vocabulary consistency across the log surface. No
|
||||
test asserts that harness error's variant, so nothing else
|
||||
moved.
|
||||
- **The mapping-table test grew the new variant**:
|
||||
`SocksServerError::Refused` rides the `GeneralFailure`
|
||||
fall-through loop in `to_reply_error_mapping_table` (alongside
|
||||
`Bug`), and a new test pins the display string and the
|
||||
fall-through mapping (`refused_variant_display_is_honest_and_
|
||||
wire_invisible`) — the task's "one test pinning the new variant's
|
||||
display/shape".
|
||||
- **Provenance extended, not replaced** (convention 17): the
|
||||
`server.rs` module doc gained a "Post-fork owned changes"
|
||||
paragraph (the cuts, the keep-notes, the `Refused` addition);
|
||||
`docs/architecture/rfc1928.md` gained the matching "owned-code
|
||||
post-fork changes" list under the deliberate modifications and a
|
||||
`last_updated` line. The bundled `src/rfc1928/LICENSE` notice
|
||||
already carries the "modifications after the fork point" clause
|
||||
generically — no change needed there.
|
||||
- **Keep-notes made current**: `stream.rs`'s `ConnectError` note
|
||||
records that the trigger fired and the disposition is **keep**
|
||||
(was: "cut it then"); `transfer`/`wait_on_tcp`'s differential-
|
||||
suite note lives in the `server.rs` module doc; `Bug`'s keep-note
|
||||
is the same paragraph (upstream-verbatim, its display untouched).
|
||||
- **The differential suite's `:103` comment** reworded to the
|
||||
RFC-ordering rationale (the server picks by its own preference,
|
||||
more-secure-first) — no cut-type reference remains anywhere.
|
||||
- **The `AuthMethod` trait doc** was updated for the macro cut
|
||||
(upstream's "use enums for sets" line → a custom selection is a
|
||||
slice of method structs) and the module doc's extraction-time
|
||||
adjustments line dropped the `#[macro_export]` note (the macro
|
||||
itself is gone).
|
||||
|
||||
## Summary
|
||||
|
||||
> Agent fills this on completion.
|
||||
Both items landed. **N-1 (the cut)**: `auth_method_enums!` (the
|
||||
macro, nothing else used it), `StandardAuthentication` +
|
||||
`StandardAuthenticationStarted` + `allow_no_auth`,
|
||||
`skip_auth_this_is_not_rfc_compliant`, and `CheckResult`'s
|
||||
`Option`/`Result` impls are gone from `src/rfc1928/server.rs` —
|
||||
the producer-replaced machinery, per ADR-013's
|
||||
no-speculative-surface posture. Kept with notes: `ConnectError`
|
||||
(hand-port companion, `stream.rs` note updated to record the fired
|
||||
trigger + keep disposition), `transfer`/`wait_on_tcp`
|
||||
(differential-suite-consumed, semantics pinned there), `Bug`
|
||||
(upstream-verbatim, display intact). **N-4 (the vocabulary)**: new
|
||||
`SocksServerError::Refused(&'static str)` variant, used at the five
|
||||
in-stream refusal sites (the BIND command gate + the four seam
|
||||
refusals) plus the harness's minimal producer; `Bug`'s display is
|
||||
untouched (a real bug should still read `BUG:`); the wire bytes are
|
||||
byte-identical (every reply-code assertion stayed green unchanged).
|
||||
Tests: the three gate-site session-error asserts moved from
|
||||
`AuthenticationRejected` to `Refused(...)`, the four `Bug(...)`
|
||||
asserts to `Refused(...)`, the mapping-table loop grew the variant,
|
||||
and a new display/shape test pins both. Docs: the `server.rs`
|
||||
module doc's owned-changes paragraph, the `rfc1928.md`
|
||||
"owned-code post-fork changes" list + `last_updated`, the
|
||||
`stream.rs` keep-note, the `AuthMethod` doc, and the differential
|
||||
comment. The review-002 N-1 coverage blocks resolve (the cuts were
|
||||
the accepted-uncovered surface).
|
||||
|
||||
Verified: `cargo test` (356 green across all suites incl.
|
||||
`--all-features`), `cargo clippy --all-targets -- -D warnings`
|
||||
clean, `cargo fmt --check` clean, `cargo doc --no-deps` clean,
|
||||
`cargo check --target wasm32-unknown-unknown` +
|
||||
`cargo clippy --target wasm32-unknown-unknown -- -D warnings`
|
||||
clean.
|
||||
+1
-1
@@ -384,7 +384,7 @@ where
|
||||
.reply_error(&err.to_reply_error())
|
||||
.await
|
||||
.map_err(SocksError::ServerError)?;
|
||||
return Err(SocksError::ServerError(SocksServerError::Bug(
|
||||
return Err(SocksError::ServerError(SocksServerError::Refused(
|
||||
"dial refused in-stream",
|
||||
)));
|
||||
}
|
||||
|
||||
@@ -100,9 +100,9 @@ async fn negotiation_server_picks_its_own_first_match() {
|
||||
|
||||
/// The same contract under `accept_password_auth`: a client offering
|
||||
/// none-then-password is answered with the *server's* preferred
|
||||
/// method (0x02, listed first upstream per the `StandardAuthentication
|
||||
/// ::allow_no_auth` ordering note — `src/server.rs:455-469` upstream
|
||||
/// / in-tree `server.rs` "more secure or preferred methods first").
|
||||
/// method (0x02 — `negotiate_auth` walks `server_methods` in server
|
||||
/// order; RFC 1928 §3 lets the server pick by its own preference, so
|
||||
/// the method list is ordered more-secure/preferred first).
|
||||
#[tokio::test]
|
||||
async fn negotiation_password_server_prefers_password_over_no_auth() {
|
||||
let (mut client, server) = tokio::io::duplex(64);
|
||||
|
||||
Reference in new issue
Block a user