Dev/rodpersky/p2 all changes - #231
Conversation
Complete P1.1 by introducing a typed, secret-free watch-backed health snapshot and publisher. This gives later gRPC, systemd, datastore, listener, and upstream integrations one maintainable source of startup, liveness, readiness, and degradation semantics.
Complete P1.2 by declaring datastore load and notification policies, retrying SONiC startup with cancellable jittered backoff, refreshing and resubscribing continuous sources, retaining known-good configuration after rejected candidates, and running one preferred recovery probe across zero-server reloads.
Complete P1.3 by centralizing SIGTERM and Ctrl-C handling, broadcasting shutdown only after health withdrawal, publishing listener binding/bound/stopped transitions with guards, awaiting sibling cancellation on listener failure, preserving request draining, and cleaning Unix sockets before return.
Complete P1.4 by sequencing aggregate connection observations per server-set generation, reporting available after any successful warm-up or request bind, reporting unavailable only after full candidate exhaustion, ignoring preferred-probe failures, and resetting to unknown after material reloads without changing readiness.
Complete P1.5 by adding tonic-health 0.14.6 beside the existing Client API, mapping stable startup, liveness, readiness, overall, and business-service names from the common runtime snapshot, supporting Check and Watch, and withdrawing every status during coordinated shutdown without changing the application proto.
Complete P1.6 by sharing UDS and TCP channel construction, adding a typed standard health client, packaging tacacsrs-agent-health, defining stable serving/invocation/check exit codes, enforcing total timeouts, redacting diagnostics, and testing the built probe over TCP and Unix sockets.
Complete P1.7 by adding auto, none, and strict systemd modes, consuming common health snapshots, sending READY once and STOPPING during drain, publishing sanitized degraded status, injecting command execution for tests, preserving auto warn-and-continue behavior, and making strict notification failures fatal.
Complete P1.8 by proving health Watch closure, readiness withdrawal before active-request drain, retry-after-refresh-failure ordering, and redaction of snapshots, public errors, systemd arguments, and probe diagnostics. The existing matrix also covers listener races, failover, host modes, probes, and dynamic reload behavior.
Complete P1.9 with a reusable WSL/Linux process test that starts before Redis, recovers readiness after ConfigDB appears, validates UDS and proxy listeners, replaces stale sockets, drains an active proxy connection on SIGTERM, releases endpoints, and exercises none, auto, and strict systemd modes.
Complete P1.10 by documenting standard health names and probe exits, none/auto/systemd behavior, Redis startup and resubscription supervision, coordinated drain, container guidance, and the independent downstream proxy secret model. The SONiC unit now permits supervised database recovery and requests strict systemd integration.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Complete P2.1 by pinning YangModels to an immutable detached revision, isolating Windows and Linux sparse caches, recording canonical source/input/output hashes, and adding manifest-backed two-run regeneration verification before central credential features are enabled.
Complete P2.2 by enabling only RFC central keystore and truststore support, updating the pinned input hash, and locking the entire optional-feature map so unrelated raw-key, encrypted-key, hidden-key, CSR, hello, and notification capabilities cannot drift.
Complete P2.3 by regenerating from pinned RFC inputs so direct and bundled certificate, EPSK, CA, and EE choices expose their central references and generated mandatory-choice metadata. Existing inline builders explicitly select no central reference, preserving their behavior.
Complete P2.4 by applying generated inline-or-central choice metadata to direct identities and all client/server bundles, adding missing server-credential validation, and keeping inline-only derivation and key checks separate from unresolved central material.
Complete P2.5 by proving config-local enumeration clears only credentials-reference while preserving structured certificate, EPSK, CA, and EE central references and all EPSK metadata exactly. External central IDs remain separate from aggregated local bundle validation.
Complete P2.6 with deterministic borrowed certificate, EPSK, CA, and EE slots, typed opaque references, redacted debug output, and a sanitized enumerate-first error. The config crate remains provider-neutral and preserves incomplete RFC containers for planning.
Introduce deterministic owned plans, an async resolver contract, typed secret/public material, closed result matching, sanitized failures, and a test fake. This keeps central references opaque and defers SONiC provider and runtime projection behavior to P3 as required by the P2 boundary.
Add compile-time negative trait assertions and runtime redaction and explicit-borrow tests for secrets and secret-bearing aggregates. Record zeroize 1.9.0 as a direct dependency so P2 secret ownership is deliberate and cannot silently regress into cloneable, serializable, or revealing APIs.
Prove exact direct and bundled central-reference round trips and all four request variants after local bundle enumeration. Keep downstream inline EPSK fixtures explicit about the absent central source so the full workspace all-feature gate verifies existing networking behavior against the regenerated model.
Document the generated RFC 9950 central shapes, opaque generic validation boundary, secret-safety guarantees, and P3 SONiC provider/runtime projection handoff. Add a runnable fake-resolver example so consumers can follow parse, enumerate, plan, resolve, and explicit secret borrowing without provider-specific code.
There was a problem hiding this comment.
Pull request overview
This PR extends the workspace’s TACACS+ YANG config pipeline to support RFC 9950 “central keystore/truststore” reference shapes end-to-end (parse/validate/enumerate/round-trip), and introduces a new provider-neutral crate (tacacsrs-credential-resolution) that plans and validates secret resolution without performing provider I/O.
Changes:
- Add central credential reference inspection in
tacacsrs-config(inspect_central_references) and update validation/enumeration/tests to preserve central reference values as opaque strings. - Introduce
tacacsrs-credential-resolutionwith deterministic request planning, an async resolver trait, secret-safe material types, and a closed request/result matcher (plus example + tests). - Pin and verify deterministic YANG generation inputs/outputs (manifest + verification scripts), and update docs/README to reflect the new layering.
Reviewed changes
Copilot reviewed 38 out of 39 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents the new crate and updated config/resolution layering. |
| libraries/tacacsrs_networking/src/transport/tls_psk/from_server.rs | Updates test server construction for new generated EPSK fields. |
| libraries/tacacsrs_networking/src/transport/tls_psk/context.rs | Updates EPSK test construction for new generated fields. |
| libraries/tacacsrs_networking/src/transport/tls_psk/config.rs | Updates EPSK test construction for new generated fields. |
| libraries/tacacsrs_credential_resolution/tests/resolution.rs | Adds end-to-end planning/resolution/result-set validation tests (including redaction expectations). |
| libraries/tacacsrs_credential_resolution/tests/material_safety.rs | Adds compile-time and runtime checks ensuring secret-bearing types don’t implement risky traits and redact debug output. |
| libraries/tacacsrs_credential_resolution/src/result_set.rs | Implements the closed result-set validator and redacted debug output. |
| libraries/tacacsrs_credential_resolution/src/resolver.rs | Defines the async resolver trait and plan execution helper. |
| libraries/tacacsrs_credential_resolution/src/request.rs | Implements deterministic planning and typed request/reference abstractions. |
| libraries/tacacsrs_credential_resolution/src/material.rs | Adds secret-safe material containers (SecretBytes, PublicBytes) and resolved credential variants. |
| libraries/tacacsrs_credential_resolution/src/lib.rs | Crate facade and re-exports. |
| libraries/tacacsrs_credential_resolution/src/fake.rs | Deterministic fake resolver for tests/examples. |
| libraries/tacacsrs_credential_resolution/src/error.rs | Typed, sanitized resolution errors. |
| libraries/tacacsrs_credential_resolution/README.md | Documents the provider-neutral contract and safety/redaction guarantees. |
| libraries/tacacsrs_credential_resolution/examples/central_resolution.rs | Runnable example showing planning + fake resolution for a central-reference config. |
| libraries/tacacsrs_credential_resolution/Cargo.toml | Adds the new crate and its dependencies. |
| libraries/tacacsrs_config/yang/verify_generated.py | Adds deterministic generation verification against a pinned manifest. |
| libraries/tacacsrs_config/yang/test_expand_yang_tree.py | Adds unit tests for cache revision pinning + manifest verification. |
| libraries/tacacsrs_config/yang/generation-manifest.json | Records pinned upstream commit + SHA-256s for inputs/outputs. |
| libraries/tacacsrs_config/yang/feature-flags.ini | Enables central-keystore/truststore-related model feature flags. |
| libraries/tacacsrs_config/yang/expanded-tree.txt | Updates the pinned expanded YANG tree output to include central reference paths. |
| libraries/tacacsrs_config/yang/expand_yang_tree.py | Pins upstream revision, enforces detached-HEAD cache, and improves cache cleaning. |
| libraries/tacacsrs_config/tests/parse_validation.rs | Updates expected validation messages for new choice cases and constraints. |
| libraries/tacacsrs_config/tests/generated_types.rs | Adds tests asserting generated shapes and choice metadata for central references. |
| libraries/tacacsrs_config/tests/enumeration_integration.rs | Adds integration tests ensuring enumeration preserves nested central references/metadata. |
| libraries/tacacsrs_config/tests/central_validation.rs | Adds validation tests covering central-only, dual-choice rejection, and mandatory choice enforcement. |
| libraries/tacacsrs_config/tests/central_roundtrip.rs | Adds round-trip tests ensuring central reference values serialize/parse exactly. |
| libraries/tacacsrs_config/tests/central_references.rs | Adds tests for deterministic inspection slots and redacted debug behavior. |
| libraries/tacacsrs_config/src/validation.rs | Extends choice validation for central keystore/truststore cases and validates server-credential bundles. |
| libraries/tacacsrs_config/src/lib.rs | Exposes the new central reference inspection API at top-level and under runtime. |
| libraries/tacacsrs_config/src/generated.rs | Updates generated types to include central-keystore/truststore reference fields and choice metadata. |
| libraries/tacacsrs_config/src/central_references.rs | Adds provider-neutral inspection of central references as deterministic typed slots. |
| libraries/tacacsrs_config/src/builders.rs | Updates builders to populate new generated fields with None. |
| libraries/tacacsrs_config/README.md | Updates crate docs to describe central inspection and the new resolution boundary. |
| docs/yang-config-guide.md | Updates guide to include central inspection and the credential-resolution workflow. |
| DEVELOPMENT.md | Documents pinned YANG generation inputs/outputs, determinism verification, and layering boundaries. |
| Cargo.toml | Adds tacacsrs-credential-resolution to the workspace members. |
| Cargo.lock | Adds lock entries for the new crate and new dev dependency (static_assertions). |
| .gitattributes | Ensures LF normalization for generated/config generation artifacts. |
Comments suppressed due to low confidence (1)
libraries/tacacsrs_config/src/validation.rs:390
validate_choicerelies oncase_presencehaving the same number of elements aschoice_cases, but it never checks that invariant. Now that central-keystore/truststore adds extra choice cases, a future caller bug (wrong boolean slice length) could silently misvalidate config and produce misleading errors.
Consider adding an explicit length check and failing fast with an internal error message.
fn validate_choice(
server_name: &str,
field_path: &str,
choice_cases: &[(&str, &[&str])],
mandatory: bool,
Store expected responses and validated entries by RequestSlot instead of relying on plan iteration order matching vector position. Add a regression that reverses request iteration and proves credential and context lookup remain associated with their original slots.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 39 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
libraries/tacacsrs_config/src/validation.rs:444
- Same issue as above for client-credentials bundles: gating
reject_unsupported_epsk_derivation()oninline_definition.is_some()meanscontext/target-*fields are accepted forcentral-keystore-referenceEPSKs even though the runtime transport currently doesn't act on them.
Unless the central-resolution path already supports these derivation fields, keep rejecting them for all EPSK configs to avoid silently accepting unsupported settings.
) -> anyhow::Result<()> {
if let Some(ref epsk) = creds.tls13_epsk {
if epsk.inline_definition.is_some() {
reject_unsupported_epsk_derivation(context, epsk)?;
}
}
libraries/tacacsrs_config/src/validation.rs:431
reject_unsupported_epsk_derivation()is now only applied whenepsk.inline_definition.is_some(). That allows configs usingcentral-keystore-referenceto setcontext,target-protocol, ortarget-kdfwithout being rejected, but the TLS-PSK transport currently doesn't consume those fields at all (only identity/hash/groups + key bytes), so they'd be silently ignored if/when central-key resolution is wired up.
If these fields are still unsupported end-to-end, reject them regardless of whether the key is inline or central to avoid accepting configuration that the runtime cannot honor.
This issue also appears on line 439 of the same file.
if let Some(ref epsk) = ci.tls13_epsk {
if epsk.inline_definition.is_some() {
reject_unsupported_epsk_derivation(context, epsk)?;
}
}
No description provided.