diff --git a/README.md b/README.md index af83107..343a284 100644 --- a/README.md +++ b/README.md @@ -58,15 +58,17 @@ Skipped this step? The probe notices: on a first-run timeout it prints this exac | Check | What it proves | |---|---| | **Status-Server** | An [RFC 5997](https://www.rfc-editor.org/rfc/rfc5997) liveness ping that runs first and **consumes no authentication attempt** — nothing shows up in the server's auth log. PASS if the server answers; a neutral INFO (never a failure) if it doesn't, since many servers leave it off. See [Liveness & multi-server](#liveness--comparing-servers). | -| **Reachability** | The server answers on UDP/1812 — and how fast. A timeout means unreachable, not listening, **or the probe isn't whitelisted / the secret is wrong** (servers silently drop unverifiable requests). | +| **Reachability** | The server answers on UDP/1812 — and how fast. The request is retransmitted the way a real NAS does before "no reply" is declared, so one lost packet shows as a WARN about loss rather than "server down". A timeout means unreachable, not listening, **or the probe isn't whitelisted / the secret is wrong** (servers silently drop unverifiable requests). Latency comes from the Status-Server reply when the server answers one; otherwise from a rejected test login, which FreeRADIUS delays by `reject_delay` (1 s by default), and the output says so. | | **Shared secret** | Cryptographically verifies the server's reply signature. A pass *proves* the secret matches — no more guessing whether "everyone's getting rejected" is a secret problem or something else. | | **BlastRADIUS posture** | Observes whether the server signs its replies with a **Message-Authenticator** — the mitigation for the RADIUS/UDP reply-forgery flaw [CVE-2024-3596](https://blastradius.fail) ("BlastRADIUS"). PASS if it does; WARN, with config pointers, if it accepts the probe's (signed) request but replies unsigned. Observation only — see below. | + +Reachability, shared secret and BlastRADIUS posture are answered by **one** request (a PAP-shaped Access-Request for the synthetic user `authhound-probe`, which the server rejects), so a run leaves one rejected-login line in the server's log rather than three. Only Status-Server consumes no authentication attempt at all. When the server does not answer that request, the auth, certificate and MTU checks are skipped immediately instead of each waiting out the timeout. | **PAP authentication** | A real login with credentials you supply → Accept or Reject, decoded. Also detects an **MFA/second-factor challenge** and reports it (the probe does not complete push/OTP — see below). | | **PEAP-MSCHAPv2** | The method most enterprise 802.1X networks actually run: a real inner authentication inside the PEAP TLS tunnel. Reports success — and verifies the server's own MSCHAPv2 proof (mutual auth) — or the decoded reason on rejection. The "can my users actually log in?" test. | | **EAP-TTLS (PAP)** | A real login inside the TTLS tunnel using inner PAP. Because the password is checked in cleartext (safe inside the tunnel), TTLS-PAP works against *any* backend — including hashed stores that MSCHAPv2 can't use. If PEAP-MSCHAPv2 fails but this passes, the directory can't produce an NT hash. | | **EAP-TLS** | Certificate-based login (no password): presents a client certificate and reports whether the server accepts it — with a plain-English reason on failure (untrusted CA, expired cert, policy reject). See [EAP-TLS: preparing a client certificate](#eap-tls-preparing-a-client-certificate). | | **Authorization / VLAN** | On any successful login, decodes and prints the authorization the Access-Accept returned (VLAN, Filter-Id, Session-Timeout, vendor attributes) — and lets you **assert** on it with `--expect-vlan` / `--expect-attr` so "auth works, wrong VLAN" fails loudly. See [Verifying policy](#verifying-policy-not-just-connectivity). | -| **Server certificate** | Establishes the PEAP/TLS tunnel over RADIUS, captures the server's certificate, and flags **expiry**, an incomplete intermediate chain, a **name mismatch** against `--server-name` (FAIL — clients validating that name would reject the handshake), and the negotiated TLS version. Without `--server-name`, name validation is **skipped and reported as a WARN** — the probe never claims "valid" for a name it didn't check. The "Wi-Fi died overnight" outage, caught early. | +| **Server certificate** | Establishes the PEAP/TLS tunnel over RADIUS, captures the server's certificate, and flags **expiry** (of the certificate and of any CA certificate sent with it), a missing intermediate (only when the issuer is demonstrably an intermediate — a certificate issued straight from a private root your clients trust is normal and is not warned about), a **name mismatch** against `--server-name` (FAIL — clients validating that name would reject the handshake; a match by Common Name only, with no SAN, is a WARN because Windows accepts it while Android/iOS do not), and the negotiated TLS version. Without `--server-name`, name validation is **skipped and reported as a WARN** — the probe never claims "valid" for a name it didn't check. The "Wi-Fi died overnight" outage, caught early. | | **Path MTU / fragmentation** (`--mtu`) | Finds the largest RADIUS packet that survives the round trip. Pinpoints the invisible failure where a firewall or VPN drops large / IP-fragmented UDP, so the multi-kilobyte EAP-TLS certificate flight never arrives and 802.1X silently stalls — while every server-side log looks clean. | | **RadSec** (`radsec test`) | Checks a RADIUS/TLS endpoint on TCP/2083: reachability, TLS handshake, server certificate, and a RADIUS exchange over the tunnel. For modern deployments and UDP→RadSec migration readiness. | @@ -206,7 +208,9 @@ $ authhound-probe radsec test --server radius.corp.com \ | `--expect-vlan ID` | Assert the Access-Accept assigns this VLAN (`Tunnel-Private-Group-ID`). Mismatch = **FAIL** — see [Verifying policy](#verifying-policy-not-just-connectivity). | | `--expect-attr Name=Value` | Assert a returned authorization attribute (repeatable), e.g. `--expect-attr Filter-Id=staff`. Mismatch = **FAIL**. | | `--mtu` | Run the path-MTU / fragmentation probe (sends a few padded packets). | -| `--count N` | Run the checks `N` times (2–50) and report aggregate statistics — see [Chasing intermittent failures](#chasing-intermittent-failures). | +| `--count N` | Run the checks `N` times (2–50) and report aggregate statistics — see [Chasing intermittent failures](#chasing-intermittent-failures). Stops early if the server rejects the test credentials (repeating a rejected password locks accounts). | +| `--called-station-id VALUE` | Send a Called-Station-Id, e.g. `AA-BB-CC-DD-EE-FF:CorpWiFi`. Wireless policies (NPS conditions, FreeRADIUS unlang) often match the SSID here; without it the probe can land on a different policy than real clients. | +| `--calling-station-id VALUE` | Send a Calling-Station-Id, e.g. the client MAC `11-22-33-44-55-66`, for MAC-based policies. | | `--interval DURATION` | Pause between `--count` iterations (default `2s`; a hard-coded safety floor applies). | | `--nas-port-type wireless\|ethernet\|virtual` | How the probe presents itself, so server policies match (default `wireless`). | | `--server-name NAME` | Name the server certificate must be valid for (also sent as TLS SNI). Mismatch = **FAIL**; omitted = name validation skipped, reported as a **WARN** — see [Certificate name validation](#certificate-name-validation---server-name). | @@ -265,6 +269,20 @@ $ authhound-probe radius test --server radius.corp.com --peap alice \ --nas-port-type ethernet --expect-vlan 30 ``` +`NAS-Port-Type` is not the only input a policy branches on. Wireless policies +routinely match the **SSID** in `Called-Station-Id`, and MAC-based rules match +the client MAC in `Calling-Station-Id`; if the probe doesn't send them, it can +match a different policy than your real clients do (a different VLAN, or a +"did not match any network policy" reject). Present the probe the way the real +NAS does: + +```console +# as an AP on the CorpWiFi SSID, from a specific client MAC +$ authhound-probe radius test --server radius.corp.com --peap alice \ + --called-station-id 'AA-BB-CC-DD-EE-FF:CorpWiFi' \ + --calling-station-id '11-22-33-44-55-66' --expect-vlan 20 +``` + `--expect-attr` generalises this to any returned attribute (repeatable), so you can pin a whole policy in a scheduled check: @@ -323,6 +341,10 @@ How to read it: Iterations run sequentially, `--interval` apart (default `2s`). The probe's hard-coded rate ceiling still bounds everything: intervals below the safety floor are stretched (and the stretch announced), and `--count` is capped at 50. +If the server **rejects the test credentials**, the loop stops after that +iteration and says so: re-sending a rejected password 50 times is how a test +account trips the domain's lockout threshold, and the aggregate would then +report a lockout as "consistent failure". Fix the credentials and re-run. This is a diagnosis loop you babysit, not monitoring — it never schedules, repeats forever, or stores anything between runs. @@ -513,6 +535,18 @@ transmitted or logged. If the cert is untrusted, expired, or rejected by policy, the probe says exactly which — so you know whether to fix the cert, the CA trust, or the server's authorization rules. +## What's new in v0.5.0 + +- **Retransmits.** A lost UDP datagram is re-sent the way a real NAS does before "no reply" is declared, so one dropped packet is a warning about packet loss rather than a false "server is down". Scheduled `--strict` runs stop paging on single drops. +- **One request for three checks.** Reachability, shared secret and BlastRADIUS posture share a single Access-Request, so a run leaves one rejected test login in the server's log instead of three. Checks that depend on reachability skip immediately when the server is silent, instead of each waiting out the timeout. +- **Honest latency.** The round-trip figure comes from the Status-Server reply when the server answers one; otherwise the output says it includes FreeRADIUS's `reject_delay`. +- **Better certificate verdicts.** A certificate issued straight from a private root is no longer reported as an incomplete chain; CA certificates sent with it are checked for expiry; a Common-Name-only match (no SAN) is a warning rather than a failure, because Windows accepts it and modern clients don't. +- **Clearer rejects.** A wrong PEAP password is reported as a credential reject instead of "the server may not offer PEAP", and an EAP-TLS handshake that failed before the server asked for a certificate is no longer blamed on the certificate. +- **Lockout guard.** `--count` stops as soon as the server rejects the test credentials, instead of re-sending a rejected password up to 50 times. +- **New flags** `--called-station-id` and `--calling-station-id`, plus IPv6 literals in `--server`. + +Full detail in the [release notes](https://github.com/authhound/probe/releases/tag/v0.5.0). + ## Install **Binary** (Linux, macOS, Windows — single static file, no runtime): diff --git a/cmd/authhound-probe/main.go b/cmd/authhound-probe/main.go index d7600a5..274c7d8 100644 --- a/cmd/authhound-probe/main.go +++ b/cmd/authhound-probe/main.go @@ -187,6 +187,8 @@ func cmdRadiusTest(args []string) int { clientKey := fs.String("client-key", "", "client private key (PEM) for the EAP-TLS test") nasID := fs.String("nas-id", "authhound-probe", "NAS-Identifier to send") nasPortType := fs.String("nas-port-type", "wireless", "NAS-Port-Type: wireless, ethernet, or virtual") + calledStation := fs.String("called-station-id", "", "Called-Station-Id to send, e.g. 'AA-BB-CC-DD-EE-FF:CorpWiFi' (policies often match the SSID here)") + callingStation := fs.String("calling-station-id", "", "Calling-Station-Id to send, e.g. the client MAC '11-22-33-44-55-66' (MAC-based policies match here)") serverName := fs.String("server-name", "", "name the server certificate must be valid for (also sent as TLS SNI); omitted = name validation skipped and reported as a warning") expectVLAN := fs.String("expect-vlan", "", "assert the Access-Accept assigns this VLAN (Tunnel-Private-Group-ID); a mismatch is a FAIL") var expectAttr stringSliceFlag @@ -274,11 +276,16 @@ func cmdRadiusTest(args []string) int { // Address is set per server below; everything else is shared across a // multi-server comparison. target := check.Target{ - Secret: secretValue, - Timeout: *timeout, - NASIdentifier: *nasID, - NASPortType: portType, - LocalAddr: localAddr, + Secret: secretValue, + Timeout: *timeout, + NASIdentifier: *nasID, + NASPortType: portType, + CalledStation: *calledStation, + CallingStation: *callingStation, + LocalAddr: localAddr, + } + if provided["timeout"] && *timeout < 2*time.Second { + fmt.Fprintf(os.Stderr, "note: --timeout %s is short: FreeRADIUS delays every Access-Reject by 1s (reject_delay), so a rejected reply can be misread as a lost packet; 2s or more is safer\n", *timeout) } papUser, papPass, err := resolveCreds(prompter, *pap, "--pap", *passwordFile) @@ -317,17 +324,21 @@ func cmdRadiusTest(args []string) int { // Status-Server runs first: an RFC 5997 liveness ping that doesn't consume an // auth attempt. The rest follow in dependency order (reachability/secret before // the auth methods that rely on them). + // One shared exchange answers reachability, shared-secret and posture (one + // rejected-login line in the server log per run instead of three), and its + // outcome lets the dependent checks skip at once when the server is silent. + base := &check.BaseExchange{} checks := []check.Check{ - check.StatusServer{}, - check.Reachability{}, - check.SharedSecret{}, - check.BlastRADIUS{}, - check.PAP{User: papUser, Pass: papPass}, - check.PEAPMSCHAPv2{User: peapUser, Pass: peapPass, ServerName: *serverName}, - check.EAPTTLS{User: ttlsUser, Pass: ttlsPass, ServerName: *serverName}, - check.EAPTLS{CertFile: *clientCert, KeyFile: *clientKey, ServerName: *serverName}, - check.ServerCert{ServerName: *serverName}, - check.MTUProbe{Enabled: *mtu}, + check.StatusServer{Base: base}, + check.Reachability{Base: base}, + check.SharedSecret{Base: base}, + check.BlastRADIUS{Base: base}, + check.PAP{User: papUser, Pass: papPass, Base: base}, + check.PEAPMSCHAPv2{User: peapUser, Pass: peapPass, ServerName: *serverName, Base: base}, + check.EAPTTLS{User: ttlsUser, Pass: ttlsPass, ServerName: *serverName, Base: base}, + check.EAPTLS{CertFile: *clientCert, KeyFile: *clientKey, ServerName: *serverName, Base: base}, + check.ServerCert{ServerName: *serverName, Base: base}, + check.MTUProbe{Enabled: *mtu, Base: base}, } if len(servers) > 1 { @@ -368,7 +379,12 @@ func parseServers(raw string) ([]string, error) { if s == "" { continue } - if !strings.Contains(s, ":") { + switch { + case net.ParseIP(s) != nil && strings.Contains(s, ":"): + s = "[" + s + "]:1812" // bare IPv6 literal + case strings.HasPrefix(s, "[") && !strings.Contains(s, "]:"): + s += ":1812" // bracketed IPv6 literal without a port + case !strings.HasPrefix(s, "[") && !strings.Contains(s, ":"): s += ":1812" } if seen[s] { @@ -498,7 +514,9 @@ func runRepeat(plan check.Plan, sink resultSink, count int, interval time.Durati fmt.Fprintln(os.Stderr, "error:", err) return 2 } - if completed := len(run.Iterations); completed < count { + if run.StoppedReason != "" { + fmt.Fprintln(os.Stderr, "note:", run.StoppedReason) + } else if completed := len(run.Iterations); completed < count { fmt.Fprintf(os.Stderr, "interrupted — aggregating the %d completed iteration(s)\n", completed) if completed == 0 { return 1 diff --git a/docs/json-schema.md b/docs/json-schema.md index b3fa113..6a4d2e3 100644 --- a/docs/json-schema.md +++ b/docs/json-schema.md @@ -69,7 +69,7 @@ Each entry in `results`: | `summary` | string | always | One plain-English line describing the outcome. | | `detail` | string | when present | Extra context. Omitted when empty. | | `hint` | string | when present | Multi-line, paste-ready remediation. Newline formatting is significant. Omitted when empty. Never contains secrets. | -| `fields` | object (string→string) | when present | Structured extras such as `rtt_ms`, `tls_version`, `not_after`, `subject`, `san`, `chain_len`, `source_ip`. On the `status-server` check, `supported` is `"true"` (the server answered the RFC 5997 liveness query) or `"false"` (it didn't — which is fine; that check never fails). `blastradius_posture` (on the `blastradius-posture` check) is `"signed"` or `"unsigned"` — whether the server signed its reply with a Message-Authenticator (see [BlastRADIUS posture](../README.md#blastradius--message-authenticator-posture)). `timeout: "true"` marks a request that got no reply at all (a *lost* request, as opposed to a processed rejection). On `server-cert` and `radsec-cert`, `name_validation` is `"match"`, `"mismatch"` (a `fail`), or `"skipped"` (no `--server-name` given — a `warn` when everything else is healthy, so a run that never validated the name is never a silent pass). Aggregate verdicts under `--count` add `success_rate`, `attempts`, `successes`, `timeouts`, and `latency_{min,median,p95,max}_ms`. Keys vary by check; values are always strings. Omitted when there are none. | +| `fields` | object (string→string) | when present | Structured extras such as `rtt_ms`, `tls_version`, `not_after`, `subject`, `san`, `chain_len`, `source_ip`. On the `status-server` check, `supported` is `"true"` (the server answered the RFC 5997 liveness query) or `"false"` (it didn't — which is fine; that check never fails). `blastradius_posture` (on the `blastradius-posture` check) is `"signed"` or `"unsigned"` — whether the server signed its reply with a Message-Authenticator (see [BlastRADIUS posture](../README.md#blastradius--message-authenticator-posture)). `timeout: "true"` marks a request that got no reply at all (a *lost* request, as opposed to a processed rejection). On `server-cert` and `radsec-cert`, `name_validation` is `"match"`, `"mismatch"` (a `fail`), or `"skipped"` (no `--server-name` given — a `warn` when everything else is healthy, so a run that never validated the name is never a silent pass). Aggregate verdicts under `--count` add `success_rate`, `attempts`, `successes`, `timeouts`, and `latency_{min,median,p95,max}_ms`. On `reachability`, `rtt_source` is `"status-server"` (undelayed liveness reply; `auth_rtt_ms` then carries the auth request's time) or `"access-reject"` (measured on a rejected test login, which FreeRADIUS delays by `reject_delay`); `retransmits` (also on the EAP checks) counts datagrams that had to be re-sent before a reply, i.e. observed packet loss. On the auth checks, `credential_rejected: "true"` marks a reject the server issued after processing the credentials (repeat mode stops on it). On `server-cert`/`radsec-cert`, `name_validation` also has the value `"cn-only"` (a `warn`: matches by Common Name only, no SAN) and `chain` is `"complete"`, `"self-signed"`, or `"leaf-only"`. Keys vary by check; values are always strings. Omitted when there are none. | | `duration_ns` | integer | when present | How long the check took, in nanoseconds. Omitted when zero. | | `authorization` | object | when present | On an auth check that reached an Access-Accept, the authorization attributes the server returned (VLAN/Filter-Id/…) and the outcome of any `--expect-vlan`/`--expect-attr` assertions. See below. Omitted otherwise. | @@ -129,6 +129,7 @@ the raw material: | `interval_ms` | integer | Pause between iterations actually used. | | `requested_interval_ms` | integer | The `--interval` that was asked for. | | `interval_stretched` | boolean | `true` when the requested interval was below the hard-coded safety floor and got stretched. | +| `stopped_early` | string | when present | Additive: the loop ended before `count` on purpose because the server rejected the test credentials (repeating would lock the account). Absent otherwise. | | `iterations` | array | One entry per completed iteration, each with its `results` (same result-object shape as the top level). | | `aggregate` | array | Per-check tallies. `successes` = the server answered and processed the request (pass/warn/info); `timeouts` = the subset of `failures` where no reply arrived at all. `latency_ms` (nearest-rank percentiles over answered runs) is omitted when nothing was answered. | diff --git a/internal/check/base.go b/internal/check/base.go new file mode 100644 index 0000000..fcbda22 --- /dev/null +++ b/internal/check/base.go @@ -0,0 +1,94 @@ +package check + +import ( + "errors" + "time" + + "github.com/authhound/probe/internal/radius" +) + +// BaseExchange shares ONE credential-shaped Access-Request between the +// reachability, shared-secret and BlastRADIUS-posture checks. They used to send +// three near-identical requests, each ending in an Access-Reject for the +// synthetic user "authhound-probe" — three failed-logon lines in the server's +// log per run (150 under --count 50), which SIEMs alert on. One request answers +// all three questions: did a reply arrive, does its Response Authenticator +// verify, and did the server sign it with a Message-Authenticator. +// +// It also records whether the server was reachable at all, so the checks that +// depend on reachability (auth methods, certificate, MTU) can skip immediately +// instead of each waiting out the full timeout on a server that is not there. +// +// A nil *BaseExchange keeps every check fully self-contained (tests, and any +// caller composing checks by hand). +type BaseExchange struct { + // Status-Server result for this iteration, recorded by StatusServer so + // reachability can report an undelayed round-trip time (an Access-Reject is + // delayed by FreeRADIUS's reject_delay, 1 s by default). + statusOK bool + statusRTT time.Duration + + done bool + err error // non-nil when no usable reply arrived + timedOut bool + localIP string + raw []byte + reqAuth [16]byte + rtt time.Duration + retransmits int +} + +// resetForIteration clears the previous iteration's exchange but keeps the +// Status-Server observation made moments ago by the StatusServer check, which +// always runs first. +func (b *BaseExchange) resetForIteration() { + *b = BaseExchange{statusOK: b.statusOK, statusRTT: b.statusRTT} +} + +// run performs the shared exchange: a PAP-shaped Access-Request (RFC 2865 §4.1 +// requires a credential attribute) with a throwaway password. Accept and Reject +// are equally useful; only the reply's presence and signatures matter. +func (b *BaseExchange) run(t Target) { + b.done = true + p, err := radius.NewAccessRequest(2) + if err != nil { + b.err = err + return + } + p.AddString(radius.AttrUserName, "authhound-probe") + p.SetUserPassword("authhound-probe-check", t.Secret) + addCommon(p, t) + b.reqAuth = p.Authenticator + + res, err := radius.ExchangeR(t.Address, t.Secret, p, t.Timeout, t.LocalAddr) + if res != nil { + b.rtt = res.RTT + b.retransmits = res.Retransmits + b.raw = res.Raw + } + b.err = err + if errors.Is(err, radius.ErrTimeout) { + b.timedOut = true + var te *radius.TimeoutError + if errors.As(err, &te) { + b.localIP = te.LocalIP + } + } +} + +// Unreachable reports whether this iteration's shared exchange got no reply. +func (b *BaseExchange) Unreachable() bool { return b != nil && b.done && b.timedOut } + +// skipIfUnreachable returns the SKIP result a dependent check should emit when +// the server never answered the reachability probe: there is no point waiting +// another full timeout. The result is marked as a timeout so --count still +// counts the iteration as lost. +func (b *BaseExchange) skipIfUnreachable(check string) (Result, bool) { + if !b.Unreachable() { + return Result{}, false + } + return markTimeout(Result{ + Check: check, Status: StatusSkip, + Summary: "Skipped — the server did not answer the reachability probe; fix that first", + }), true +} diff --git a/internal/check/blastradius.go b/internal/check/blastradius.go index 017e41f..6084308 100644 --- a/internal/check/blastradius.go +++ b/internal/check/blastradius.go @@ -2,7 +2,6 @@ package check import ( "context" - "errors" "github.com/authhound/probe/internal/radius" ) @@ -22,34 +21,32 @@ const BlastRADIUSField = "blastradius_posture" // technique — presence/absence on a single exchange, nothing more. And it can // only speak to behaviour toward THIS probe's client entry: a server may sign // for one client and not another, so the result is scoped to what we can see. -type BlastRADIUS struct{} +type BlastRADIUS struct { + Base *BaseExchange // shared with Reachability; nil = send its own request +} func (BlastRADIUS) Name() string { return "blastradius-posture" } -func (BlastRADIUS) Run(ctx context.Context, t Target) Result { - p, err := radius.NewAccessRequest(6) - if err != nil { - return Result{Check: "blastradius-posture", Status: StatusFail, Summary: "internal error: " + err.Error()} +func (c BlastRADIUS) Run(ctx context.Context, t Target) Result { + // The shared exchange (a PAP-shaped request with a throwaway password) is + // enough: we only inspect whether the reply is signed, not whether auth + // succeeds — Access-Accept and Access-Reject are both fine. + b := c.Base + if b == nil || !b.done { + b = &BaseExchange{} + b.run(t) } - p.AddString(radius.AttrUserName, "authhound-probe") - // A throwaway password: we only inspect whether the reply is signed, not - // whether auth succeeds — Access-Accept and Access-Reject are both fine. - p.SetUserPassword("authhound-probe-blastradius-check", t.Secret) - addCommon(p, t) - - reqAuth := p.Authenticator - _, raw, _, err := radius.Exchange(t.Address, t.Secret, p, t.Timeout, t.LocalAddr) - if err != nil { - if errors.Is(err, radius.ErrTimeout) { - return markTimeout(Result{ - Check: "blastradius-posture", Status: StatusSkip, - Summary: "Could not check reply signing — no reply; resolve reachability first", - }) - } - return Result{Check: "blastradius-posture", Status: StatusSkip, Summary: "Could not check reply signing: " + err.Error()} + if b.timedOut { + return markTimeout(Result{ + Check: "blastradius-posture", Status: StatusSkip, + Summary: "Could not check reply signing — no reply; resolve reachability first", + }) + } + if b.err != nil { + return Result{Check: "blastradius-posture", Status: StatusSkip, Summary: "Could not check reply signing: " + b.err.Error()} } - present, valid := radius.VerifyMessageAuthenticator(raw, reqAuth, t.Secret) + present, valid := radius.VerifyMessageAuthenticator(b.raw, b.reqAuth, t.Secret) switch { case present && valid: return Result{ diff --git a/internal/check/certanalysis.go b/internal/check/certanalysis.go index f34a39a..2227622 100644 --- a/internal/check/certanalysis.go +++ b/internal/check/certanalysis.go @@ -3,6 +3,7 @@ package check import ( "crypto/x509" "fmt" + "regexp" "strconv" "strings" "time" @@ -11,9 +12,10 @@ import ( const certExpiryWarnDays = 21 // analyzeCert turns a captured server-certificate chain into a Result: it flags -// expiry (the classic "Wi-Fi died overnight" outage), an incomplete intermediate -// chain, a name mismatch against serverName, and reports the negotiated TLS -// version. Shared by the EAP server-cert check and RadSec. +// expiry (the classic "Wi-Fi died overnight" outage) of the leaf and of any +// intermediate that was sent, a missing intermediate, a name mismatch against +// serverName, and reports the negotiated TLS version. Shared by the EAP +// server-cert check and RadSec. // // serverName is the name the caller expects the certificate to be valid for // (from --server-name). Empty means the operator asserted nothing, so name @@ -34,19 +36,39 @@ func analyzeCert(checkName string, chain []*x509.Certificate, tlsVersion uint16, fields["san"] = strings.Join(sans, ", ") } - nameErr := error(nil) + // Name validation: "match" (a SAN matches), "cn-only" (no SAN at all, but the + // legacy Common Name matches — Windows accepts this, modern clients do not), + // "mismatch", or "skipped". + name := "skipped" switch { case serverName == "": - fields["name_validation"] = "skipped" case leaf.VerifyHostname(serverName) == nil: - fields["name_validation"] = "match" + name = "match" + case len(leaf.DNSNames) == 0 && len(leaf.IPAddresses) == 0 && strings.EqualFold(leaf.Subject.CommonName, serverName): + name = "cn-only" default: - nameErr = leaf.VerifyHostname(serverName) - fields["name_validation"] = "mismatch" + name = "mismatch" } + fields["name_validation"] = name + + ch := assessChain(chain) + fields["chain"] = ch.kind now := time.Now() daysLeft := int(leaf.NotAfter.Sub(now).Hours() / 24) + tlsv := tlsVersionName(tlsVersion) + + // CA certificates the server sent are part of what clients validate: an + // expired one breaks the handshake exactly like an expired leaf. + var expiredInter, interSoon *x509.Certificate + for _, c := range chain[1:] { + if expiredInter == nil && now.After(c.NotAfter) { + expiredInter = c + } + if interSoon == nil && !now.After(c.NotAfter) && int(c.NotAfter.Sub(now).Hours()/24) <= certExpiryWarnDays { + interSoon = c + } + } switch { case now.After(leaf.NotAfter): @@ -63,7 +85,15 @@ func analyzeCert(checkName string, chain []*x509.Certificate, tlsVersion uint16, Summary: fmt.Sprintf("Server certificate is not yet valid (starts %s)", leaf.NotBefore.UTC().Format("2006-01-02")), Detail: "The certificate's validity hasn't started — check the clock on the server and clients.", } - case nameErr != nil: + case expiredInter != nil: + return Result{ + Check: checkName, Status: StatusFail, Fields: fields, + Summary: fmt.Sprintf("Intermediate certificate %q EXPIRED on %s", expiredInter.Subject.CommonName, expiredInter.NotAfter.UTC().Format("2006-01-02")), + Detail: "The server certificate itself is still valid, but a CA certificate the server " + + "sends with it has expired, so clients cannot build a valid chain. Install the " + + "renewed CA certificate on the RADIUS server (certificate_file / the NPS machine store).", + } + case name == "mismatch": return Result{ Check: checkName, Status: StatusFail, Fields: fields, Summary: fmt.Sprintf("Server certificate does not match the expected name %q", serverName), @@ -72,13 +102,15 @@ func analyzeCert(checkName string, chain []*x509.Certificate, tlsVersion uint16, "Either the wrong certificate is selected on the server, or the expected name " + "(--server-name) is out of date.", } - case chainLooksIncomplete(chain): + case ch.kind == "leaf-only" && ch.intermediateLikely: return Result{ Check: checkName, Status: StatusWarn, Fields: fields, Summary: "Server sent only its leaf certificate — the intermediate chain looks incomplete", - Detail: "Clients that don't already trust/cache the issuing intermediate will fail " + - "with an 'unknown CA' error. Configure the server to present the full chain " + - "(leaf + intermediates).", + Detail: fmt.Sprintf("The certificate was issued by %q, which looks like an intermediate CA "+ + "(it is not in the chain the server sent, and the certificate points at an issuer "+ + "download URL). Clients that don't already trust/cache that intermediate fail with "+ + "an 'unknown CA' error. Configure the server to present the full chain (leaf + "+ + "intermediates).", ch.issuer), } case daysLeft <= certExpiryWarnDays: return Result{ @@ -87,23 +119,90 @@ func analyzeCert(checkName string, chain []*x509.Certificate, tlsVersion uint16, Detail: "Renew before it lapses. Continuous certificate-expiry alerting across every " + "server is part of AuthHound's monitoring tier.", } + case interSoon != nil: + return Result{ + Check: checkName, Status: StatusWarn, Fields: fields, + Summary: fmt.Sprintf("Intermediate certificate %q expires in %d days (%s)", interSoon.Subject.CommonName, + int(interSoon.NotAfter.Sub(now).Hours()/24), interSoon.NotAfter.UTC().Format("2006-01-02")), + Detail: "The server certificate is fine, but the intermediate CA certificate sent with it " + + "lapses soon and would break every client's chain validation. Install the renewed intermediate.", + } + case name == "cn-only": + return Result{ + Check: checkName, Status: StatusWarn, Fields: fields, + Summary: fmt.Sprintf("Server certificate matches %q by Common Name only — it has no Subject Alternative Name", serverName), + Detail: "Windows supplicants accept a CN match, so this may work today; Android 11+, " + + "iOS/macOS and most modern clients require the name in a DNS SAN and reject the " + + "handshake. Reissue the certificate with subjectAltName = DNS:" + serverName + ".", + } case serverName == "": return Result{ Check: checkName, Status: StatusWarn, Fields: fields, - Summary: fmt.Sprintf("Certificate captured — expiry and chain OK, but name validation was SKIPPED (%s)", - tlsVersionName(tlsVersion)), + Summary: fmt.Sprintf("Certificate captured — expiry and chain OK, but name validation was SKIPPED (%s)", tlsv), Detail: "No --server-name was given, so the probe could not check that this is the " + "certificate your clients expect. Names it is valid for: " + certNames(leaf) + ". " + "Re-run with --server-name to validate it — real clients do, and a name " + - "mismatch fails them even when expiry and chain are fine.", + "mismatch fails them even when expiry and chain are fine." + ch.note(), } default: return Result{ Check: checkName, Status: StatusPass, Fields: fields, - Summary: fmt.Sprintf("Server certificate valid for %d more days, name matches %q, chain looks complete (%s)", - daysLeft, serverName, tlsVersionName(tlsVersion)), + Summary: fmt.Sprintf("Server certificate valid for %d more days, name matches %q, %s (%s)", + daysLeft, serverName, ch.phrase(), tlsv), + Detail: strings.TrimSpace(ch.note()), + } + } +} + +// chainAssessment describes what the server sent alongside its leaf. +type chainAssessment struct { + kind string // "self-signed", "complete", "leaf-only" + issuer string // leaf issuer CN (leaf-only) + intermediateLikely bool // leaf-only AND the missing issuer looks like an intermediate +} + +// intermediateNameRe matches issuer names that are conventionally intermediate +// (issuing) CAs rather than roots. Belt and braces next to the AIA check. +var intermediateNameRe = regexp.MustCompile(`(?i)\b(intermediate|issuing|sub[- ]?ca|subordinate)\b`) + +// assessChain classifies the chain. A lone leaf issued directly by a private +// root that clients already trust is the most common enterprise setup (single- +// tier AD CS, FreeRADIUS's own CA); nothing is missing there, so it must NOT be +// reported as an incomplete chain. Only when the leaf carries an AIA "CA +// Issuers" URL (every two-tier AD CS and public CA leaf does) or the issuer is +// named like an intermediate do we call the missing issuer a real gap. +func assessChain(chain []*x509.Certificate) chainAssessment { + leaf := chain[0] + if leaf.CheckSignatureFrom(leaf) == nil { + return chainAssessment{kind: "self-signed"} + } + for _, c := range chain[1:] { + if c.Subject.String() == leaf.Issuer.String() { + return chainAssessment{kind: "complete"} } } + a := chainAssessment{kind: "leaf-only", issuer: leaf.Issuer.CommonName} + a.intermediateLikely = len(leaf.IssuingCertificateURL) > 0 || intermediateNameRe.MatchString(leaf.Issuer.String()) + return a +} + +// phrase is the chain fragment of the PASS summary line. +func (a chainAssessment) phrase() string { + if a.kind == "leaf-only" { + return fmt.Sprintf("issuer %q not sent (fine if it is a root your clients trust)", a.issuer) + } + return "chain looks complete" +} + +// note is the chain sentence appended to a detail when the leaf was sent alone +// but nothing indicates a missing intermediate. +func (a chainAssessment) note() string { + if a.kind != "leaf-only" || a.intermediateLikely { + return "" + } + return fmt.Sprintf(" Chain: only the server certificate was sent; its issuer %q is not included. "+ + "That is fine when %q is a root CA your clients already trust (single-tier PKI); if it "+ + "is an intermediate, configure the server to send it too.", a.issuer, a.issuer) } // certNames renders the names a certificate is actually valid for, for humans: @@ -119,20 +218,14 @@ func certNames(leaf *x509.Certificate) string { return "(none — the certificate carries no DNS name at all)" } +// chainLooksIncomplete is kept for callers/tests that only need the boolean +// view: true when the leaf's issuer is missing and looks like an intermediate. func chainLooksIncomplete(chain []*x509.Certificate) bool { if len(chain) == 0 { return false } - leaf := chain[0] - if leaf.CheckSignatureFrom(leaf) == nil { - return false // self-signed leaf: unusual but not an "incomplete chain" - } - for _, c := range chain[1:] { - if c.Subject.String() == leaf.Issuer.String() { - return false - } - } - return true + a := assessChain(chain) + return a.kind == "leaf-only" && a.intermediateLikely } func tlsVersionName(v uint16) string { diff --git a/internal/check/certanalysis_test.go b/internal/check/certanalysis_test.go index fc123da..68c6677 100644 --- a/internal/check/certanalysis_test.go +++ b/internal/check/certanalysis_test.go @@ -86,16 +86,17 @@ func TestAnalyzeCertNameMismatch(t *testing.T) { } } -// A certificate with no SAN must mismatch: modern clients (and Go) do not fall -// back to the Common Name, and the detail should say so. +// A certificate with no SAN whose Common Name matches is a WARN (Windows +// supplicants accept a CN match; Android/iOS and modern clients do not), and +// the detail must say the SAN is missing. func TestAnalyzeCertCNOnlyMismatch(t *testing.T) { chain := []*x509.Certificate{healthyCert(t /* no SANs */)} r := analyzeCert("server-cert", chain, tls.VersionTLS12, "radius.corp.com") - if r.Status != StatusFail { - t.Fatalf("CN-only: got %s (%s), want fail", r.Status, r.Summary) + if r.Status != StatusWarn || r.Fields["name_validation"] != "cn-only" { + t.Fatalf("CN-only: got %s/%s (%s), want warn/cn-only", r.Status, r.Fields["name_validation"], r.Summary) } - if !strings.Contains(r.Detail, "CN only") { - t.Errorf("detail should flag the missing SAN: %q", r.Detail) + if !strings.Contains(r.Detail, "Subject Alternative Name") && !strings.Contains(r.Summary, "Subject Alternative Name") { + t.Errorf("output should flag the missing SAN: %q", r.Summary) } } @@ -143,3 +144,83 @@ func TestAnalyzeCertMismatchOutranksExpiringSoon(t *testing.T) { t.Fatalf("mismatch+expiring: got %s (%s), want name-mismatch fail", r.Status, r.Summary) } } + +// issuedCert builds a leaf signed by a separate CA (not self-signed), with an +// optional AIA CA-Issuers URL — the shape real servers present. +func issuedCert(t *testing.T, cn string, dnsNames []string, issuerCN string, aia bool) *x509.Certificate { + t.Helper() + caKey, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + caTmpl := &x509.Certificate{SerialNumber: big.NewInt(10), Subject: pkix.Name{CommonName: issuerCN}, + NotBefore: time.Now().Add(-time.Hour), NotAfter: time.Now().Add(400 * 24 * time.Hour), + IsCA: true, BasicConstraintsValid: true, KeyUsage: x509.KeyUsageCertSign} + caDER, err := x509.CreateCertificate(rand.Reader, caTmpl, caTmpl, &caKey.PublicKey, caKey) + if err != nil { + t.Fatal(err) + } + ca, _ := x509.ParseCertificate(caDER) + key, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + tmpl := &x509.Certificate{SerialNumber: big.NewInt(11), Subject: pkix.Name{CommonName: cn}, DNSNames: dnsNames, + NotBefore: time.Now().Add(-time.Hour), NotAfter: time.Now().Add(365 * 24 * time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}} + if aia { + tmpl.IssuingCertificateURL = []string{"http://pki.corp.example/issuing.crt"} + } + der, err := x509.CreateCertificate(rand.Reader, tmpl, ca, &key.PublicKey, caKey) + if err != nil { + t.Fatal(err) + } + leaf, _ := x509.ParseCertificate(der) + return leaf +} + +// A leaf issued directly by a private root (single-tier PKI, no AIA) is the +// most common enterprise setup and must NOT be reported as an incomplete chain. +func TestAnalyzeCertLeafOnlyPrivateRootIsNotIncomplete(t *testing.T) { + leaf := issuedCert(t, "radius.corp.com", []string{"radius.corp.com"}, "Corp Root CA", false) + r := analyzeCert("server-cert", []*x509.Certificate{leaf}, tls.VersionTLS12, "radius.corp.com") + if r.Status != StatusPass { + t.Fatalf("got %s (%s), want pass", r.Status, r.Summary) + } + if r.Fields["chain"] != "leaf-only" || !strings.Contains(r.Summary, "not sent") { + t.Errorf("chain reporting: fields=%v summary=%q", r.Fields, r.Summary) + } +} + +// A leaf whose issuer advertises a download URL (AIA) or is named like an +// intermediate IS a missing-intermediate WARN. +func TestAnalyzeCertMissingIntermediateWarns(t *testing.T) { + for _, tc := range []struct { + issuer string + aia bool + }{{"Corp Root CA", true}, {"Corp Issuing CA 1", false}} { + leaf := issuedCert(t, "radius.corp.com", []string{"radius.corp.com"}, tc.issuer, tc.aia) + r := analyzeCert("server-cert", []*x509.Certificate{leaf}, tls.VersionTLS12, "radius.corp.com") + if r.Status != StatusWarn || !strings.Contains(r.Summary, "incomplete") { + t.Errorf("issuer=%q aia=%v: got %s (%s), want incomplete-chain warn", tc.issuer, tc.aia, r.Status, r.Summary) + } + } +} + +// A CN-only certificate (no SAN) whose CN matches is a WARN, not a FAIL: +// Windows accepts it, modern clients do not. +func TestAnalyzeCertCNOnlyWarns(t *testing.T) { + leaf := issuedCert(t, "radius.corp.com", nil, "Corp Root CA", false) + r := analyzeCert("server-cert", []*x509.Certificate{leaf}, tls.VersionTLS12, "radius.corp.com") + if r.Status != StatusWarn || r.Fields["name_validation"] != "cn-only" { + t.Errorf("got %s name_validation=%q (%s), want warn/cn-only", r.Status, r.Fields["name_validation"], r.Summary) + } + // A different name is still a hard mismatch. + if r := analyzeCert("server-cert", []*x509.Certificate{leaf}, tls.VersionTLS12, "other.corp.com"); r.Status != StatusFail { + t.Errorf("mismatch: got %s, want fail", r.Status) + } +} + +// An expired intermediate breaks clients like an expired leaf: FAIL. +func TestAnalyzeCertExpiredIntermediateFails(t *testing.T) { + leaf := healthyCert(t, "radius.corp.com") + expiredInter := testCert(t, "Corp Issuing CA", nil, time.Now().Add(-48*time.Hour), time.Now().Add(-24*time.Hour)) + r := analyzeCert("server-cert", []*x509.Certificate{leaf, expiredInter}, tls.VersionTLS12, "radius.corp.com") + if r.Status != StatusFail || !strings.Contains(r.Summary, "Intermediate") { + t.Errorf("got %s (%s), want intermediate-expired fail", r.Status, r.Summary) + } +} diff --git a/internal/check/check_test.go b/internal/check/check_test.go index 99b434c..a1aa4b0 100644 --- a/internal/check/check_test.go +++ b/internal/check/check_test.go @@ -7,6 +7,7 @@ import ( "encoding/binary" "net" "strings" + "sync" "testing" "time" ) @@ -23,6 +24,16 @@ type fakeServer struct { signMsgAuth bool // if true, sign replies with a Message-Authenticator (BlastRADIUS-hardened) acceptAttrs []byte // raw attribute bytes appended to an Access-Accept conn *net.UDPConn + + mu sync.Mutex + requests int +} + +// requestCount reports how many datagrams the server has received. +func (fs *fakeServer) requestCount() int { + fs.mu.Lock() + defer fs.mu.Unlock() + return fs.requests } func startFakeServer(t *testing.T, fs *fakeServer) string { @@ -48,6 +59,9 @@ func (fs *fakeServer) serve() { if err != nil { return } + fs.mu.Lock() + fs.requests++ + fs.mu.Unlock() if fs.silent { continue } @@ -259,3 +273,67 @@ func TestHMACSanity(t *testing.T) { t.Fatal("unexpected hmac-md5 size") } } + +// TestBaseExchangeSharedAcrossChecks: reachability, shared-secret and posture +// must ride on ONE request (one rejected login in the server log), and a +// silent server must make the dependent checks skip at once. +func TestBaseExchangeSharedAcrossChecks(t *testing.T) { + fs := &fakeServer{secret: "s3cret", signMsgAuth: true} + addr := startFakeServer(t, fs) + ctx := context.Background() + tgt := target(addr, "s3cret") + base := &BaseExchange{} + + if r := (Reachability{Base: base}).Run(ctx, tgt); r.Status != StatusPass { + t.Fatalf("reachability: got %s (%s)", r.Status, r.Summary) + } + if r := (SharedSecret{Base: base}).Run(ctx, tgt); r.Status != StatusPass { + t.Errorf("shared-secret: got %s (%s)", r.Status, r.Summary) + } + if r := (BlastRADIUS{Base: base}).Run(ctx, tgt); r.Status != StatusPass { + t.Errorf("posture: got %s (%s)", r.Status, r.Summary) + } + if got := fs.requestCount(); got != 1 { + t.Errorf("server saw %d requests for three checks, want 1", got) + } + + silent := startFakeServer(t, &fakeServer{secret: "s3cret", silent: true}) + stgt := target(silent, "s3cret") + stgt.Timeout = 300 * time.Millisecond + sbase := &BaseExchange{} + if r := (Reachability{Base: sbase}).Run(ctx, stgt); r.Status != StatusFail { + t.Fatalf("silent reachability: got %s", r.Status) + } + start := time.Now() + r := (PAP{User: "alice", Pass: "pw", Base: sbase}).Run(ctx, stgt) + if r.Status != StatusSkip || r.Fields[TimeoutField] != "true" { + t.Errorf("pap after unreachable: got %s fields=%v, want skip+timeout", r.Status, r.Fields) + } + if time.Since(start) > 100*time.Millisecond { + t.Errorf("dependent check waited for the network instead of skipping") + } +} + +// TestReachabilityUsesStatusServerRTT: when Status-Server answered, the +// reachability latency comes from it (an Access-Reject is delayed by +// FreeRADIUS's reject_delay, a Status-Server reply is not). +func TestReachabilityUsesStatusServerRTT(t *testing.T) { + base := &BaseExchange{statusOK: true, statusRTT: 7 * time.Millisecond} + addr := startFakeServer(t, &fakeServer{secret: "s3cret"}) + r := (Reachability{Base: base}).Run(context.Background(), target(addr, "s3cret")) + if r.Fields["rtt_source"] != "status-server" || r.Fields["rtt_ms"] != "7" { + t.Errorf("fields = %v, want rtt from status-server (7ms)", r.Fields) + } + if _, ok := r.Fields["auth_rtt_ms"]; !ok { + t.Errorf("auth_rtt_ms missing") + } +} + +// TestPAPRejectIsMarkedCredentialRejected: the marker repeat mode stops on. +func TestPAPRejectIsMarkedCredentialRejected(t *testing.T) { + addr := startFakeServer(t, &fakeServer{secret: "s3cret", goodUser: "alice", goodPass: "pw"}) + r := (PAP{User: "alice", Pass: "wrong"}).Run(context.Background(), target(addr, "s3cret")) + if r.Fields[CredentialRejectedField] != "true" { + t.Errorf("reject not marked: %v", r.Fields) + } +} diff --git a/internal/check/common.go b/internal/check/common.go index 5c174dc..4a2c80b 100644 --- a/internal/check/common.go +++ b/internal/check/common.go @@ -34,6 +34,11 @@ func addCommon(p *radius.Packet, t Target) { } } +// CredentialRejectedField marks an auth Result where the server processed the +// credentials and said no. Repeat mode stops on it: re-sending a rejected +// password N times is how test accounts get locked out. +const CredentialRejectedField = "credential_rejected" + // TimeoutField marks a Result whose underlying request got no reply, so // aggregate reporting (--count) can count lost requests separately from // processed rejections. Additive within schema major "1". diff --git a/internal/check/compare.go b/internal/check/compare.go index add919f..6c01faf 100644 --- a/internal/check/compare.go +++ b/internal/check/compare.go @@ -65,11 +65,12 @@ func CompareServers(runs []ServerRun) Comparison { return c } -// serverResponded reports whether the reachability check passed for a run. +// serverResponded reports whether the reachability check got a reply for a +// run: PASS, or WARN (a reply that needed a retransmit is still a reply). func serverResponded(results []Result) bool { for _, r := range results { if r.Check == "reachability" { - return r.Status == StatusPass + return r.Status == StatusPass || r.Status == StatusWarn } } return false diff --git a/internal/check/eaptls.go b/internal/check/eaptls.go index df7ef7b..de94b9c 100644 --- a/internal/check/eaptls.go +++ b/internal/check/eaptls.go @@ -18,11 +18,15 @@ import ( // and stops before sending any credential or client certificate. type ServerCert struct { ServerName string + Base *BaseExchange // reachability outcome; nil = always attempt } func (ServerCert) Name() string { return "server-cert" } func (c ServerCert) Run(ctx context.Context, t Target) Result { + if r, skip := c.Base.skipIfUnreachable("server-cert"); skip { + return r + } sess := &radius.EAPSession{ Addr: t.Address, Secret: t.Secret, diff --git a/internal/check/eaptls_auth.go b/internal/check/eaptls_auth.go index 588332a..d7d49c5 100644 --- a/internal/check/eaptls_auth.go +++ b/internal/check/eaptls_auth.go @@ -21,6 +21,7 @@ type EAPTLS struct { CertFile string KeyFile string ServerName string + Base *BaseExchange // reachability outcome; nil = always attempt } func (EAPTLS) Name() string { return "eap-tls" } @@ -33,6 +34,9 @@ func (c EAPTLS) Run(ctx context.Context, t Target) Result { } } + if r, skip := c.Base.skipIfUnreachable("eap-tls"); skip { + return r + } cert, err := tls.LoadX509KeyPair(c.CertFile, c.KeyFile) if err != nil { return Result{ diff --git a/internal/check/mtu.go b/internal/check/mtu.go index 795aac7..4ed9626 100644 --- a/internal/check/mtu.go +++ b/internal/check/mtu.go @@ -18,6 +18,7 @@ import ( // server-side log looks clean. Opt-in via --mtu (it sends a handful of probes). type MTUProbe struct { Enabled bool + Base *BaseExchange // reachability outcome; nil = always attempt } func (MTUProbe) Name() string { return "path-mtu" } @@ -33,6 +34,9 @@ func (c MTUProbe) Run(ctx context.Context, t Target) Result { if !c.Enabled { return Result{Check: "path-mtu", Status: StatusSkip, Summary: "Path-MTU probe not run (pass --mtu to enable)"} } + if r, skip := c.Base.skipIfUnreachable("path-mtu"); skip { + return r + } attrs := commonAttrs(t) // Baseline: a small packet must round-trip, or there's nothing to measure. diff --git a/internal/check/pap.go b/internal/check/pap.go index 9fbe22e..e4b7e9e 100644 --- a/internal/check/pap.go +++ b/internal/check/pap.go @@ -18,6 +18,7 @@ import ( type PAP struct { User string Pass string + Base *BaseExchange // reachability outcome; nil = always attempt } func (PAP) Name() string { return "pap-auth" } @@ -30,6 +31,9 @@ func (c PAP) Run(ctx context.Context, t Target) Result { } } + if r, skip := c.Base.skipIfUnreachable("pap-auth"); skip { + return r + } p, err := radius.NewAccessRequest(3) if err != nil { return Result{Check: "pap-auth", Status: StatusFail, Summary: "internal error: " + err.Error()} @@ -53,7 +57,7 @@ func (c PAP) Run(ctx context.Context, t Target) Result { reply, t) case radius.AccessReject: return Result{ - Check: "pap-auth", Status: StatusFail, + Check: "pap-auth", Status: StatusFail, Fields: map[string]string{CredentialRejectedField: "true"}, Summary: "PAP authentication rejected for " + c.User, Detail: "The server processed the login and said no. Causes: wrong password; " + "the account can't be checked via PAP (e.g. the backend only stores an " + diff --git a/internal/check/peap.go b/internal/check/peap.go index 8fc2102..26fc1f3 100644 --- a/internal/check/peap.go +++ b/internal/check/peap.go @@ -20,6 +20,7 @@ type PEAPMSCHAPv2 struct { User string Pass string ServerName string + Base *BaseExchange // reachability outcome; nil = always attempt } func (PEAPMSCHAPv2) Name() string { return "peap-mschapv2" } @@ -32,6 +33,9 @@ func (c PEAPMSCHAPv2) Run(ctx context.Context, t Target) Result { } } + if r, skip := c.Base.skipIfUnreachable("peap-mschapv2"); skip { + return r + } sess := &radius.EAPSession{ Addr: t.Address, Secret: t.Secret, @@ -50,6 +54,12 @@ func (c PEAPMSCHAPv2) Run(ctx context.Context, t Target) Result { ". If reachability/secret above failed, fix those first; otherwise the server " + "may not offer PEAP-MSCHAPv2.", } + if errors.Is(err, radius.ErrEAPFailure) { + r.Detail = "The server ended the PEAP session with EAP-Failure before the password " + + "was checked. Usually the identity is rejected up front: the account does not " + + "exist, a connection-request/network policy denies it, or the server does not " + + "offer PEAP for this client. Check the server's log for identity " + c.User + "." + } if errors.Is(err, radius.ErrTimeout) { r = markTimeout(r) } @@ -57,6 +67,9 @@ func (c PEAPMSCHAPv2) Run(ctx context.Context, t Target) Result { } fields := map[string]string{} + if sess.Retransmits > 0 { + fields["retransmits"] = strconv.Itoa(sess.Retransmits) + } if res.Cert != nil && len(res.Cert.Chain) > 0 { fields["server_cert"] = res.Cert.Chain[0].Subject.CommonName } @@ -77,6 +90,7 @@ func (c PEAPMSCHAPv2) Run(ctx context.Context, t Target) Result { if res.ErrorCode > 0 { fields["mschap_error"] = strconv.Itoa(res.ErrorCode) } + fields[CredentialRejectedField] = "true" return Result{ Check: "peap-mschapv2", Status: StatusFail, Fields: fields, Summary: fmt.Sprintf("PEAP-MSCHAPv2 rejected %s", c.User), diff --git a/internal/check/reachability.go b/internal/check/reachability.go index 609d337..434ea70 100644 --- a/internal/check/reachability.go +++ b/internal/check/reachability.go @@ -9,44 +9,71 @@ import ( "github.com/authhound/probe/internal/radius" ) -// Reachability sends a minimal Access-Request and reports whether the server -// answered at all, plus the round-trip time. Any validly-formed reply — even -// an Access-Reject — proves reachability; RADIUS servers reject unknown users -// but still answer, whereas an unknown *client* (unwhitelisted probe) gets -// silence. -type Reachability struct{} +// Reachability sends the shared credential-shaped Access-Request (see +// BaseExchange) and reports whether the server answered at all, plus the +// round-trip time. Any validly-formed reply — even an Access-Reject — proves +// reachability; RADIUS servers reject unknown users but still answer, whereas +// an unknown *client* (unwhitelisted probe) gets silence. +// +// The datagram is retransmitted on a NAS-like schedule before "no reply" is +// declared, so a single lost packet reads as a WARN about loss, not as "server +// down". Latency is taken from the Status-Server reply when the server answered +// one (never delayed); otherwise from the rejected test login, which FreeRADIUS +// delays by reject_delay (1 s default) — the output says which. +type Reachability struct { + Base *BaseExchange // shared with SharedSecret/BlastRADIUS; nil = standalone +} func (Reachability) Name() string { return "reachability" } -func (Reachability) Run(ctx context.Context, t Target) Result { - p, err := radius.NewAccessRequest(1) - if err != nil { - return Result{Check: "reachability", Status: StatusFail, Summary: "internal error building request: " + err.Error()} +func (c Reachability) Run(ctx context.Context, t Target) Result { + b := c.Base + if b == nil { + b = &BaseExchange{} } - p.AddString(radius.AttrUserName, "authhound-probe") - addCommon(p, t) + b.resetForIteration() + b.run(t) - _, _, rtt, err := radius.Exchange(t.Address, t.Secret, p, t.Timeout, t.LocalAddr) - fields := map[string]string{"rtt_ms": strconv.FormatInt(rtt.Milliseconds(), 10)} + fields := map[string]string{"rtt_ms": strconv.FormatInt(b.rtt.Milliseconds(), 10)} + if b.retransmits > 0 { + fields["retransmits"] = strconv.Itoa(b.retransmits) + } - if err == nil { - return Result{ - Check: "reachability", Status: StatusPass, - Summary: fmt.Sprintf("RADIUS server answered in %dms", rtt.Milliseconds()), - Fields: fields, + if b.err == nil { + r := Result{Check: "reachability", Status: StatusPass, Fields: fields} + if b.statusOK { + fields["rtt_ms"] = strconv.FormatInt(b.statusRTT.Milliseconds(), 10) + fields["rtt_source"] = "status-server" + fields["auth_rtt_ms"] = strconv.FormatInt(b.rtt.Milliseconds(), 10) + r.Summary = fmt.Sprintf("RADIUS server answered in %dms", b.statusRTT.Milliseconds()) + } else { + fields["rtt_source"] = "access-reject" + r.Summary = fmt.Sprintf("RADIUS server answered in %dms", b.rtt.Milliseconds()) + r.Detail = "Measured on a rejected test login. FreeRADIUS delays every Access-Reject " + + "by reject_delay (1 s by default), so this figure usually includes that second; " + + "enable Status-Server on the server to get an undelayed number." } + if b.retransmits > 0 { + r.Status = StatusWarn + r.Summary += fmt.Sprintf(" — after %d retransmit(s)", b.retransmits) + r.Detail = joinDetail(r.Detail, fmt.Sprintf( + "The first datagram got no reply and had to be re-sent %d time(s): packets are "+ + "being lost between this host and the server. Real clients retransmit too, but a "+ + "lossy path is exactly what produces intermittent 802.1X failures; run with --count "+ + "to measure the loss rate.", b.retransmits)) + } + return r } - if errors.Is(err, radius.ErrTimeout) { + if b.timedOut { fields[TimeoutField] = "true" srcIP := "" - var te *radius.TimeoutError - if errors.As(err, &te) && te.LocalIP != "" { - srcIP = te.LocalIP - fields["source_ip"] = te.LocalIP + if b.localIP != "" { + srcIP = b.localIP + fields["source_ip"] = b.localIP } return Result{ Check: "reachability", Status: StatusFail, - Summary: fmt.Sprintf("No reply from %s within %s", t.Address, t.Timeout), + Summary: fmt.Sprintf("No reply from %s within %s (sent %d times)", t.Address, t.Timeout, 1+b.retransmits), Detail: "Silence has three causes, and the first two look identical from " + "here — a server drops the request without replying in both cases. In " + "order of likelihood: 1) this probe isn't registered as a RADIUS client " + @@ -60,9 +87,12 @@ func (Reachability) Run(ctx context.Context, t Target) Result { Fields: fields, } } + if errors.Is(b.err, radius.ErrTimeout) { // defensive: timedOut should already be set + fields[TimeoutField] = "true" + } return Result{ Check: "reachability", Status: StatusFail, - Summary: "Could not reach the server: " + err.Error(), + Summary: "Could not reach the server: " + b.err.Error(), Fields: fields, } } diff --git a/internal/check/repeat.go b/internal/check/repeat.go index 4904dbb..112ce6e 100644 --- a/internal/check/repeat.go +++ b/internal/check/repeat.go @@ -46,6 +46,10 @@ type RepeatRun struct { RequestedInterval time.Duration Interval time.Duration // interval actually used (>= safety floor) Stretched bool // requested interval was below the floor + // StoppedReason is set when the loop ended before Count on purpose: the + // server rejected the test credentials, and repeating a rejected password + // is how accounts get locked out (see CredentialRejectedField). + StoppedReason string } // EffectiveInterval returns the between-iteration interval a repeat run will @@ -85,10 +89,26 @@ func RunRepeated(ctx context.Context, r *Runner, plan Plan, opts RepeatOptions) if opts.OnIteration != nil { opts.OnIteration(i+1, results) } + if check := credentialRejected(results); check != "" { + run.StoppedReason = fmt.Sprintf("stopped after iteration %d: the server rejected the credentials for %s; "+ + "repeating a rejected password would lock the account out. Fix the credentials and re-run.", i+1, check) + return run, nil + } } return run, nil } +// credentialRejected returns the name of the first auth check in results that +// the server answered with a credential reject, or "". +func credentialRejected(results []Result) string { + for _, r := range results { + if r.Fields[CredentialRejectedField] == "true" { + return r.Check + } + } + return "" +} + // CheckStats aggregates one check's results across all iterations. // // A "success" is any iteration the server answered and processed (pass, warn, diff --git a/internal/check/repeat_test.go b/internal/check/repeat_test.go index 9d6197a..6f22cc7 100644 --- a/internal/check/repeat_test.go +++ b/internal/check/repeat_test.go @@ -175,3 +175,20 @@ func TestAggregateVerdictWording(t *testing.T) { } } } + +// TestRunRepeatedStopsOnCredentialReject: a rejected password must not be +// re-sent N times (that is how test accounts get locked out). +func TestRunRepeatedStopsOnCredentialReject(t *testing.T) { + rej := Result{Check: "pap-auth", Status: StatusFail, Fields: map[string]string{CredentialRejectedField: "true"}} + plan := Plan{Checks: []Check{&stubCheck{name: "pap-auth", results: []Result{rej, rej, rej}}}} + run, err := RunRepeated(context.Background(), &Runner{}, plan, RepeatOptions{Count: 3, Interval: time.Millisecond}) + if err != nil { + t.Fatal(err) + } + if len(run.Iterations) != 1 { + t.Errorf("iterations = %d, want 1 (stop after the first reject)", len(run.Iterations)) + } + if !strings.Contains(run.StoppedReason, "pap-auth") || !strings.Contains(run.StoppedReason, "lock") { + t.Errorf("StoppedReason = %q", run.StoppedReason) + } +} diff --git a/internal/check/secret.go b/internal/check/secret.go index 83a045e..72f6924 100644 --- a/internal/check/secret.go +++ b/internal/check/secret.go @@ -2,51 +2,46 @@ package check import ( "context" - "errors" "github.com/authhound/probe/internal/radius" ) -// SharedSecret sends an Access-Request and verifies the Response Authenticator -// on the reply. A verifying reply cryptographically proves the server holds the -// SAME shared secret this probe used — the single most common source of -// "it's rejecting everyone" tickets that turn out to be a secret mismatch. +// SharedSecret verifies the Response Authenticator on the reply to the shared +// exchange (see BaseExchange). A verifying reply cryptographically proves the +// server holds the SAME shared secret this probe used — the single most common +// source of "it's rejecting everyone" tickets that turn out to be a secret +// mismatch. // // It disambiguates three states an admin usually can't tell apart: // - reply verifies -> secret is correct // - reply doesn't verify -> secret mismatch (rare; most servers just drop) // - no reply (timeout) -> unreachable OR probe not whitelisted as a client -type SharedSecret struct{} +type SharedSecret struct { + Base *BaseExchange // shared with Reachability; nil = send its own request +} func (SharedSecret) Name() string { return "shared-secret" } -func (SharedSecret) Run(ctx context.Context, t Target) Result { - p, err := radius.NewAccessRequest(2) - if err != nil { - return Result{Check: "shared-secret", Status: StatusFail, Summary: "internal error: " + err.Error()} +func (c SharedSecret) Run(ctx context.Context, t Target) Result { + b := c.Base + if b == nil || !b.done { + b = &BaseExchange{} + b.run(t) } - p.AddString(radius.AttrUserName, "authhound-probe") - // A wrong password is fine here — we only care whether the reply is signed - // with our secret, not whether auth succeeds. - p.SetUserPassword("authhound-probe-secret-check", t.Secret) - addCommon(p, t) - - reqAuth := p.Authenticator - _, raw, _, err := radius.Exchange(t.Address, t.Secret, p, t.Timeout, t.LocalAddr) - if err != nil { - if errors.Is(err, radius.ErrTimeout) { - return markTimeout(Result{ - Check: "shared-secret", Status: StatusSkip, - Summary: "Could not verify the shared secret — no reply", - Detail: "Most RADIUS servers silently drop requests when the secret is " + - "wrong OR when the client isn't whitelisted, so a timeout alone can't " + - "tell them apart. Fix reachability/whitelisting first, then re-run.", - }) - } - return Result{Check: "shared-secret", Status: StatusSkip, Summary: "Could not verify the shared secret: " + err.Error()} + if b.timedOut { + return markTimeout(Result{ + Check: "shared-secret", Status: StatusSkip, + Summary: "Could not verify the shared secret — no reply", + Detail: "Most RADIUS servers silently drop requests when the secret is " + + "wrong OR when the client isn't whitelisted, so a timeout alone can't " + + "tell them apart. Fix reachability/whitelisting first, then re-run.", + }) + } + if b.err != nil { + return Result{Check: "shared-secret", Status: StatusSkip, Summary: "Could not verify the shared secret: " + b.err.Error()} } - if radius.VerifyResponse(raw, reqAuth, t.Secret) { + if radius.VerifyResponse(b.raw, b.reqAuth, t.Secret) { return Result{ Check: "shared-secret", Status: StatusPass, Summary: "Shared secret is correct (reply signature verified)", diff --git a/internal/check/statusserver.go b/internal/check/statusserver.go index e7b310f..89ced88 100644 --- a/internal/check/statusserver.go +++ b/internal/check/statusserver.go @@ -18,11 +18,16 @@ import ( // any RADIUS request — an unregistered client is dropped silently, so a timeout // here is ambiguous. It therefore never FAILs: a reply is a PASS, and silence is // an INFO that defers to the reachability check below rather than guessing. -type StatusServer struct{} +type StatusServer struct { + Base *BaseExchange // records the undelayed RTT for the reachability check; may be nil +} func (StatusServer) Name() string { return "status-server" } -func (StatusServer) Run(ctx context.Context, t Target) Result { +func (c StatusServer) Run(ctx context.Context, t Target) Result { + if c.Base != nil { + c.Base.statusOK, c.Base.statusRTT = false, 0 + } p, err := radius.NewStatusServer(1) if err != nil { return Result{Check: "status-server", Status: StatusInfo, Summary: "internal error building Status-Server request: " + err.Error()} @@ -35,6 +40,9 @@ func (StatusServer) Run(ctx context.Context, t Target) Result { _, _, rtt, err := radius.Exchange(t.Address, t.Secret, p, t.Timeout, t.LocalAddr) if err == nil { + if c.Base != nil { + c.Base.statusOK, c.Base.statusRTT = true, rtt + } return Result{ Check: "status-server", Status: StatusPass, Summary: fmt.Sprintf("Server answered Status-Server in %dms (live; no auth attempt consumed)", rtt.Milliseconds()), diff --git a/internal/check/ttls.go b/internal/check/ttls.go index ef59dc8..d2535cc 100644 --- a/internal/check/ttls.go +++ b/internal/check/ttls.go @@ -16,6 +16,7 @@ type EAPTTLS struct { User string Pass string ServerName string + Base *BaseExchange // reachability outcome; nil = always attempt } func (EAPTTLS) Name() string { return "eap-ttls" } @@ -28,6 +29,9 @@ func (c EAPTTLS) Run(ctx context.Context, t Target) Result { } } + if r, skip := c.Base.skipIfUnreachable("eap-ttls"); skip { + return r + } sess := &radius.EAPSession{ Addr: t.Address, Secret: t.Secret, @@ -61,6 +65,7 @@ func (c EAPTTLS) Run(ctx context.Context, t Target) Result { Summary: "EAP-TTLS (inner PAP) authentication succeeded for " + c.User, }, res.Accept, t) } + fields[CredentialRejectedField] = "true" return Result{ Check: "eap-ttls", Status: StatusFail, Fields: fields, Summary: "EAP-TTLS (inner PAP) authentication failed for " + c.User, diff --git a/internal/radius/authattr.go b/internal/radius/authattr.go index e5f2041..b9d2cb2 100644 --- a/internal/radius/authattr.go +++ b/internal/radius/authattr.go @@ -133,12 +133,14 @@ func decodeVSA(v []byte) []AuthAttr { vendor := binary.BigEndian.Uint32(v[0:4]) rest := v[4:] var out []AuthAttr + skippedKeys := false for len(rest) >= 2 { vtype := rest[0] vlen := int(rest[1]) if isKeyingVSA(vendor, vtype) { // MS-MPPE-Send-Key / Recv-Key carry encrypted session key material, // not authorization. Never surface key bytes (trust: no secrets out). + skippedKeys = true if vlen >= 2 && vlen <= len(rest) { rest = rest[vlen:] continue @@ -165,7 +167,7 @@ func decodeVSA(v []byte) []AuthAttr { }) rest = rest[vlen:] } - if out == nil { + if out == nil && !skippedKeys { out = append(out, AuthAttr{Name: fmt.Sprintf("Vendor-%d", vendor), Value: printableOrHex(rest), Raw: hex.EncodeToString(rest), Vendor: int(vendor)}) } return out diff --git a/internal/radius/client.go b/internal/radius/client.go index 18f76e2..23adc85 100644 --- a/internal/radius/client.go +++ b/internal/radius/client.go @@ -7,58 +7,131 @@ import ( "time" ) +// ExchangeResult is what one Access-Request round trip produced. +type ExchangeResult struct { + Reply *Packet + Raw []byte // exact bytes received (for Response-Authenticator verification) + RTT time.Duration // first send -> reply (or -> deadline on timeout) + Retransmits int // how many times the datagram had to be re-sent before a reply +} + +// retransmitSchedule returns the offsets from the first send at which the same +// datagram is re-sent when nothing has arrived yet (RFC 5080 §2.2.1: a NAS +// retransmits the identical packet, same Identifier and Request Authenticator). +// One lost datagram therefore no longer reads as "server down". +// +// No retransmit fires before 1.5 s: FreeRADIUS delays every Access-Reject by +// reject_delay (1 s by default), and a retransmit inside that window is a +// duplicate the server discards while the real reply is still on its way — it +// would be reported as loss that never happened. Offsets are 40% and 70% of +// the timeout (never below that floor), and only when they leave at least +// 300 ms before the deadline for the reply; a timeout too short to fit a +// retry gets none. +func retransmitSchedule(timeout time.Duration) []time.Duration { + const earliest = 1500 * time.Millisecond + const margin = 300 * time.Millisecond + var out []time.Duration + for _, off := range []time.Duration{timeout * 4 / 10, timeout * 7 / 10} { + if off < earliest { + off = earliest + } + if off > timeout-margin { + break + } + if len(out) > 0 && off <= out[len(out)-1] { + continue + } + out = append(out, off) + } + return out +} + // Exchange sends one Access-Request and waits for a reply. It returns the // decoded reply, the raw reply bytes (for Response-Authenticator verification), // and the round-trip time. A Message-Authenticator is always included, which // is both good hygiene and required by servers hardened against BlastRADIUS -// (CVE-2024-3596). +// (CVE-2024-3596). See ExchangeR for the retransmit count. +func Exchange(addr string, secret string, p *Packet, timeout time.Duration, localAddr net.Addr) (reply *Packet, raw []byte, rtt time.Duration, err error) { + res, err := ExchangeR(addr, secret, p, timeout, localAddr) + if res == nil { + return nil, nil, 0, err + } + return res.Reply, res.Raw, res.RTT, err +} + +// ExchangeR is Exchange with the full result, including how many retransmits +// were needed. The datagram is re-sent on the retransmitSchedule from the same +// socket (same source port, Identifier and Request Authenticator), so the +// server's duplicate detection works exactly as it does for a real NAS. // // localAddr, when non-nil, is the source address the UDP socket binds to // (the --bind flag) — the way to pin the outgoing interface on a multi-homed // host. The chosen source IP is what the RADIUS server sees, and it is exactly // what TimeoutError.LocalIP reports back so the registration hint stays correct. -func Exchange(addr string, secret string, p *Packet, timeout time.Duration, localAddr net.Addr) (reply *Packet, raw []byte, rtt time.Duration, err error) { +func ExchangeR(addr string, secret string, p *Packet, timeout time.Duration, localAddr net.Addr) (*ExchangeResult, error) { wire, err := p.encode(secret) if err != nil { - return nil, nil, 0, err + return nil, err } dialer := net.Dialer{LocalAddr: localAddr} conn, err := dialer.Dial("udp", addr) if err != nil { - return nil, nil, 0, err + return nil, err } defer conn.Close() - if err := conn.SetDeadline(time.Now().Add(timeout)); err != nil { - return nil, nil, 0, err - } - + res := &ExchangeResult{} start := time.Now() + deadline := start.Add(timeout) + sched := retransmitSchedule(timeout) + if _, err := conn.Write(wire); err != nil { - return nil, nil, 0, err + return res, err } buf := make([]byte, 4096) - n, err := conn.Read(buf) - rtt = time.Since(start) - if err != nil { - if ne, ok := err.(net.Error); ok && ne.Timeout() { - return nil, nil, rtt, &TimeoutError{LocalIP: localIP(conn)} + for { + next := deadline + if len(sched) > 0 { + if t := start.Add(sched[0]); t.Before(next) { + next = t + } } - return nil, nil, rtt, err - } - - raw = make([]byte, n) - copy(raw, buf[:n]) - reply, err = decode(raw) - if err != nil { - return nil, raw, rtt, fmt.Errorf("decode reply: %w", err) - } - if reply.Identifier != p.Identifier { - return reply, raw, rtt, errors.New("reply identifier mismatch") + if err := conn.SetReadDeadline(next); err != nil { + return res, err + } + n, err := conn.Read(buf) + if err != nil { + if ne, ok := err.(net.Error); ok && ne.Timeout() { + if len(sched) > 0 && time.Now().Before(deadline) { + sched = sched[1:] + if _, werr := conn.Write(wire); werr != nil { + return res, werr + } + res.Retransmits++ + continue + } + res.RTT = time.Since(start) + return res, &TimeoutError{LocalIP: localIP(conn)} + } + return res, err + } + res.RTT = time.Since(start) + raw := make([]byte, n) + copy(raw, buf[:n]) + reply, err := decode(raw) + if err != nil { + res.Raw = raw + return res, fmt.Errorf("decode reply: %w", err) + } + if reply.Identifier != p.Identifier { + // A stale or stray datagram on our port: not our reply, keep waiting. + continue + } + res.Raw, res.Reply = raw, reply + return res, nil } - return reply, raw, rtt, nil } // ErrTimeout means no reply arrived before the deadline — the server is diff --git a/internal/radius/client_test.go b/internal/radius/client_test.go index 565f505..2663a05 100644 --- a/internal/radius/client_test.go +++ b/internal/radius/client_test.go @@ -34,3 +34,81 @@ func TestExchangeTimeoutCarriesLocalIP(t *testing.T) { t.Errorf("LocalIP: got %q, want 127.0.0.1", te.LocalIP) } } + +// TestExchangeRetransmits verifies that a datagram lost on the way is re-sent +// (same socket, same Identifier) and the eventual reply is accepted, with the +// retransmit counted — a single drop must not read as "server down". +func TestExchangeRetransmits(t *testing.T) { + pc, err := net.ListenPacket("udp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + defer pc.Close() + // Drop the first datagram, answer the second with a minimal Access-Reject. + go func() { + buf := make([]byte, 4096) + seen := 0 + for { + n, addr, err := pc.ReadFrom(buf) + if err != nil { + return + } + seen++ + if seen < 2 || n < 20 { + continue + } + reply := make([]byte, 20) + reply[0], reply[1] = 3, buf[1] + reply[3] = 20 + _, _ = pc.WriteTo(reply, addr) + } + }() + + p, err := NewAccessRequest(7) + if err != nil { + t.Fatal(err) + } + res, err := ExchangeR(pc.LocalAddr().String(), "s3cret", p, 2500*time.Millisecond, nil) + if err != nil { + t.Fatalf("expected a reply after retransmit, got %v", err) + } + if res.Retransmits != 1 { + t.Errorf("retransmits: got %d, want 1", res.Retransmits) + } + if res.Reply == nil || res.Reply.Identifier != 7 { + t.Errorf("reply not matched to the request") + } +} + +// TestRetransmitScheduleFitsTimeout pins the schedule: no retries when the +// timeout cannot fit one; otherwise two re-sends inside the deadline. +func TestRetransmitScheduleFitsTimeout(t *testing.T) { + if s := retransmitSchedule(time.Second); s != nil { + t.Errorf("1s: want no retransmits, got %v", s) + } + // 2s: one retry, and only after FreeRADIUS's 1s reject_delay window. + if s := retransmitSchedule(2 * time.Second); len(s) != 1 || s[0] != 1500*time.Millisecond { + t.Errorf("2s: got %v, want [1.5s]", s) + } + s := retransmitSchedule(5 * time.Second) + if len(s) != 2 || s[0] != 2*time.Second || s[1] != 3500*time.Millisecond { + t.Errorf("5s: got %v, want [2s 3.5s]", s) + } +} + +// TestResultTLVOnlyEchoesStatus: the PEAP Result-TLV acknowledgement must carry +// a Result TLV and nothing else, whatever the server packed next to it (Windows +// NPS adds a Crypto-Binding TLV; echoing it back is invalid). +func TestResultTLVOnlyEchoesStatus(t *testing.T) { + server := []byte{0x80, 0x03, 0x00, 0x02, 0x00, 0x01, // Result TLV: success + 0x80, 0x0c, 0x00, 0x38} // start of a Crypto-Binding TLV (type 12) + server = append(server, make([]byte, 56)...) + got := resultTLV(server) + want := []byte{0x80, 0x03, 0x00, 0x02, 0x00, 0x01} + if string(got) != string(want) { + t.Errorf("ack = %x, want %x", got, want) + } + if f := resultTLV([]byte{0x80, 0x03, 0x00, 0x02, 0x00, 0x02}); f[5] != 2 { + t.Errorf("failure status not preserved: %x", f) + } +} diff --git a/internal/radius/eapsession.go b/internal/radius/eapsession.go index c066113..ca0cba7 100644 --- a/internal/radius/eapsession.go +++ b/internal/radius/eapsession.go @@ -29,6 +29,10 @@ type EAPSession struct { Attrs []Attribute // common NAS attributes added to every request LocalAddr net.Addr // source address to bind (--bind); nil = OS default + // Retransmits counts datagrams that had to be re-sent during this session + // (see radius.ExchangeR): a non-zero value means packet loss on the path. + Retransmits int + radiusID byte state []byte // RADIUS State attribute to echo back rounds int @@ -59,10 +63,14 @@ func (s *EAPSession) send(eap []byte) (*EAPPacket, Code, error) { p.Add(AttrState, s.state) } - reply, _, _, err := Exchange(s.Addr, s.Secret, p, s.Timeout, s.LocalAddr) + res, err := ExchangeR(s.Addr, s.Secret, p, s.Timeout, s.LocalAddr) + if res != nil { + s.Retransmits += res.Retransmits + } if err != nil { return nil, 0, err } + reply := res.Reply s.lastReply = reply if st := reply.Get(AttrState); st != nil { s.state = st @@ -117,6 +125,11 @@ type CapturedCert struct { var errCertCaptured = errors.New("certificate captured") +// ErrEAPFailure is returned when the server ends the EAP conversation with an +// EAP-Failure (normally inside an Access-Reject). Callers use errors.Is to tell +// "the server said no" apart from transport or protocol errors. +var ErrEAPFailure = errors.New("server sent EAP-Failure") + // tlsConfig builds the PEAP outer-tunnel TLS config. The server certificate is // always captured for reporting. When abort is true, the handshake stops right // after the certificate is received (used by the read-only cert check); when @@ -197,6 +210,10 @@ func (s *EAPSession) AuthPEAPMSCHAPv2(ctx context.Context, userName, password, s res.Cert = captured if res.Success { res.Accept = s.driveToAccept(conn) + } else if len(conn.outBuf) > 0 { + // Ship the buffered failure acknowledgement so the server closes the + // session cleanly instead of waiting for it (best-effort). + _, _ = conn.exchangeAppData() } return res, nil } @@ -249,10 +266,21 @@ func (s *EAPSession) AuthEAPTLS(ctx context.Context, clientCert tls.Certificate, // doesn't include its issuer — so an untrusted cert produces a clear trust // error instead of silently sending none. cc := clientCert - conf.GetClientCertificate = func(*tls.CertificateRequestInfo) (*tls.Certificate, error) { return &cc, nil } + certRequested := false + conf.GetClientCertificate = func(*tls.CertificateRequestInfo) (*tls.Certificate, error) { + certRequested = true + return &cc, nil + } tc := tls.Client(conn, conf) if hsErr := tc.HandshakeContext(ctx); hsErr != nil { + if !certRequested && isEAPFailure(hsErr) { + // The server gave up before ever asking for our certificate, so the + // certificate cannot be the reason: no common TLS version or cipher + // suite, or a policy that rejects this identity up front. + return &EAPTLSResult{Success: false, Cert: captured, + Reason: "the server aborted the TLS handshake before requesting a client certificate, so the certificate is not the cause — most often no common TLS version or cipher suite (old server, restricted cipher_list), or a policy that rejects this identity before EAP-TLS starts. Check the server's TLS settings and its log for this identity."}, nil + } return &EAPTLSResult{Success: false, Reason: explainTLSError(hsErr), Cert: captured}, nil } @@ -296,6 +324,13 @@ func (c *eapTLSConn) exchangeAppData() (Code, error) { return code, err } +// isEAPFailure reports whether err is (or wraps, or textually carries) the +// EAP-Failure sentinel; crypto/tls returns the transport's error as-is, but the +// string check keeps this robust to wrapping. +func isEAPFailure(err error) bool { + return errors.Is(err, ErrEAPFailure) || strings.Contains(strings.ToLower(err.Error()), "eap-failure") +} + // explainTLSError turns a Go TLS handshake error into a cause an admin can act // on. Most EAP-TLS failures are the server rejecting the client certificate. func explainTLSError(err error) string { @@ -380,6 +415,14 @@ func runInnerMSCHAPv2(tc innerConn, userName, password string) (*PEAPResult, err if res.ErrorCode != 0 || res.Success { return res, nil // we already have a verdict; a torn-down tail is fine } + if isEAPFailure(err) { + // The server answered our credentials with an outer Access-Reject / + // EAP-Failure and no inner reason. That is a reject, not a broken + // exchange (FreeRADIUS sends a Result-TLV first; other servers don't). + res.ErrorCode = -1 + res.ErrorCause = "the server rejected the credentials and gave no inner reason (it ended the session with EAP-Failure): wrong password, or the account/policy denies this user." + return res, nil + } return nil, fmt.Errorf("reading MSCHAPv2 result: %w", err) } dbg("inner result type=%d datalen=%d", typ, len(data)) @@ -406,7 +449,10 @@ func runInnerMSCHAPv2(tc innerConn, userName, password string) (*PEAPResult, err // PEAPv0 carries the Result-TLV (and its ack) with a full EAP header; // a header-stripped echo makes the server reject the tunnel completion, // so the final Access-Accept (with the VLAN attributes) never arrives. - ack := (&EAPPacket{Code: EAPResponse, ID: innerID, Type: EAPTypeTLV, Data: data}).Marshal() + // The response carries ONLY a Result TLV. Windows NPS sends a + // Crypto-Binding TLV alongside the Result TLV; echoing the server's own + // binding back would be an invalid response (NPS reason code 301). + ack := (&EAPPacket{Code: EAPResponse, ID: innerID, Type: EAPTypeTLV, Data: resultTLV(data)}).Marshal() _, _ = tc.Write(ack) if success { res.Success = true @@ -425,6 +471,18 @@ func runInnerMSCHAPv2(tc innerConn, userName, password string) (*PEAPResult, err return nil, errors.New("no PEAP result after the MSCHAPv2 response") } +// resultTLV builds the client's Result TLV (draft-josefsson-pppext-eap-tls-eap +// §4.2 / MS-PEAP): mandatory bit, type 3, length 2, and the same status the +// server sent (1 = success, 2 = failure). Any other TLVs in the server's packet +// (Crypto-Binding, SoH) are deliberately not echoed. +func resultTLV(serverData []byte) []byte { + status := byte(2) + if len(serverData) >= 6 && serverData[5] == 1 { + status = 1 + } + return []byte{0x80, 0x03, 0x00, 0x02, 0x00, status} +} + // innerConn is the subset of net.Conn / tls.Conn that the inner exchange needs. type innerConn interface { Read([]byte) (int, error) @@ -558,7 +616,7 @@ func (c *eapTLSConn) receive(req *EAPPacket, code Code) ([]byte, error) { return server, nil } if req.Code == EAPFailure { - return nil, errors.New("server sent EAP-Failure during TLS handshake") + return nil, ErrEAPFailure } if req.Type != c.eapType || len(req.Data) < 1 { return nil, fmt.Errorf("unexpected EAP packet (type %d) during handshake", req.Type) diff --git a/internal/radius/ttls.go b/internal/radius/ttls.go index 1027080..845ed69 100644 --- a/internal/radius/ttls.go +++ b/internal/radius/ttls.go @@ -60,9 +60,15 @@ func (s *EAPSession) AuthEAPTTLS(ctx context.Context, userName, password, server // ttlsInnerPAP builds the inner Diameter AVPs for TTLS-PAP: User-Name (code 1) // and User-Password (code 2), both mandatory, cleartext (the tunnel protects -// them). RFC 5281 / RFC 6733 AVP framing. +// them). RFC 5281 / RFC 6733 AVP framing. The password is NUL-padded to a +// multiple of 16 octets as RFC 5281 §11.2.5 specifies and wpa_supplicant does; +// servers strip the padding (FreeRADIUS: rlm_eap_ttls uses strlen). func ttlsInnerPAP(userName, password string) []byte { - return append(diameterAVP(1, []byte(userName)), diameterAVP(2, []byte(password))...) + pw := []byte(password) + if pad := (16 - len(pw)%16) % 16; pad > 0 { + pw = append(pw, make([]byte, pad)...) + } + return append(diameterAVP(1, []byte(userName)), diameterAVP(2, pw)...) } func diameterAVP(code uint32, data []byte) []byte { diff --git a/internal/report/repeat.go b/internal/report/repeat.go index 234bac0..87579c8 100644 --- a/internal/report/repeat.go +++ b/internal/report/repeat.go @@ -22,6 +22,10 @@ type repeatDoc struct { IntervalStretched bool `json:"interval_stretched"` Iterations []iterationDoc `json:"iterations"` Aggregate []aggregateDoc `json:"aggregate"` + // StoppedEarly (additive) explains a run that ended before `count` on + // purpose — the credentials were rejected, so repeating would lock the + // account. Absent otherwise. + StoppedEarly string `json:"stopped_early,omitempty"` } type iterationDoc struct { @@ -57,6 +61,7 @@ func (s *JSONSink) SetRepeat(count int, run check.RepeatRun, stats []check.Check IntervalStretched: run.Stretched, Iterations: []iterationDoc{}, Aggregate: []aggregateDoc{}, + StoppedEarly: run.StoppedReason, } for _, iter := range run.Iterations { doc.Iterations = append(doc.Iterations, iterationDoc{Results: iter}) diff --git a/test/freeradius-smoke.sh b/test/freeradius-smoke.sh index 4ea6109..dc918c2 100755 --- a/test/freeradius-smoke.sh +++ b/test/freeradius-smoke.sh @@ -102,12 +102,17 @@ echo "== starting FreeRADIUS (debug, threaded for RadSec) ==" # request's NAS-Port-Type reaches the inner tunnel AND the inner reply's VLAN is # copied out to the outer Access-Accept — exactly how real 802.1X VLAN assignment # is configured, and what lets the probe read the VLAN over EAP. +# The image's bundled test certificates are only valid for 60 days from the +# image build (raddb/certs default_days), so they are regenerated at start; +# otherwise EAP-TLS and the server-cert check fail for a reason that has +# nothing to do with the probe. +REGEN_CERTS='cd /etc/raddb/certs && rm -f ca.pem ca.der ca.key ca.crt server.pem server.crt server.csr server.key client.pem client.crt client.csr client.key client.p12 index.txt* serial* && make >/dev/null 2>&1 && cd /' docker run -d --rm --name ah-freeradius --network host \ -v "$work/authorize:/etc/raddb/mods-config/files/authorize:ro" \ -v "$work/radsec:/etc/raddb/sites-enabled/radsec:ro" \ -v "$work/server-short.pem:/etc/raddb/certs/server-short.pem:ro" \ --entrypoint sh freeradius/freeradius-server:latest -c \ - "sed -i -E 's/(use_tunneled_reply|copy_request_to_tunnel) = no/\1 = yes/' /etc/raddb/mods-available/eap && exec freeradius -fxx -l stdout" >/dev/null + "$REGEN_CERTS && sed -i -E 's/(use_tunneled_reply|copy_request_to_tunnel) = no/\1 = yes/' /etc/raddb/mods-available/eap && exec freeradius -fxx -l stdout" >/dev/null # Wait for it to be listening. for i in $(seq 1 30); do @@ -355,7 +360,7 @@ EOF docker run -d --rm --name ah-secondary -p 127.0.0.1:11814:1812/udp \ -v "$work/authorize:/etc/raddb/mods-config/files/authorize:ro" \ -v "$work/clients-secondary:/etc/raddb/clients.conf:ro" \ - freeradius/freeradius-server:latest -fxx -l stdout >/dev/null + --entrypoint sh freeradius/freeradius-server:latest -c "$REGEN_CERTS && exec freeradius -fxx -l stdout" >/dev/null for i in $(seq 1 30); do if docker logs ah-secondary 2>&1 | grep -q "Ready to process requests"; then break; fi sleep 0.5 @@ -403,7 +408,7 @@ client dummy { EOF docker run -d --rm --name ah-freeradius-nc --network host \ -v "$work/clients-none:/etc/raddb/clients.conf:ro" \ - freeradius/freeradius-server:latest -fxx -l stdout >/dev/null + --entrypoint sh freeradius/freeradius-server:latest -c "$REGEN_CERTS && exec freeradius -fxx -l stdout" >/dev/null for i in $(seq 1 30); do if docker logs ah-freeradius-nc 2>&1 | grep -q "Ready to process requests"; then break; fi sleep 0.5 @@ -440,7 +445,7 @@ EOF docker run -d --rm --name ah-freeradius-flaky -p 127.0.0.1:11812:1812/udp \ -v "$work/authorize:/etc/raddb/mods-config/files/authorize:ro" \ -v "$work/clients-flaky:/etc/raddb/clients.conf:ro" \ - freeradius/freeradius-server:latest -fxx -l stdout >/dev/null + --entrypoint sh freeradius/freeradius-server:latest -c "$REGEN_CERTS && exec freeradius -fxx -l stdout" >/dev/null docker run -d --rm --name ah-netem --network "container:ah-freeradius-flaky" \ --cap-add NET_ADMIN alpine:3 sh -c \ "apk add --no-cache iproute2 >/dev/null && tc qdisc replace dev eth0 root netem loss 25% delay 30ms 20ms && sleep infinity" >/dev/null