Standardise log levels (#223) - #263
Open
AlfioEmanueleFresta wants to merge 12 commits into
Open
AlfioEmanueleFresta wants to merge 12 commits into
AlfioEmanueleFresta wants to merge 12 commits into
Conversation
Defines tracing level semantics (error for library faults only, warn for unexpected device behaviour, info for sparse lifecycle, debug for protocol flow and lengths, trace for raw bytes), the sensitive-data rule, and the static-message-with-structured-fields form. Closes the policy half of #223.
A logging_lints dylint library enforcing the mechanical parts of the logging policy: - tracing_message_interpolation: messages must be static string literals - print_macro_in_library: no print/println/eprint/eprintln - log_crate_macro: no log crate macros - sensitive_field_above_debug: best-effort denylist at info and above Registered in the workspace metadata and run in CI, where the three deterministic lints fail the build. The heuristic stays a warning.
Downgrade peer, transport, IO and decode failures from error! to warn!, keeping error! only for in-memory crypto invariants. Reduce byte-array dumps to lengths at debug! with full values at trace!, and stop the trial-decrypt span from recording the EID key.
Wire-framing and packet-validation failures are device behaviour, so they move from error! to warn!. Demote the device-blink notices to debug! and use static messages with structured fields.
Downgrade connect, GATT, read/write and pairing failures from error! to warn!, demote the device-cancelled notice to debug!, and reduce the service-data dump to a length at debug!.
NFC/PC-SC transport faults move from error! to warn! and the stray println! becomes a warn!. U2F APDU non-success status words are an expected, polled part of the flow, so they log at debug! rather than error!. Raw APDU buffers stay at trace! with static messages.
Malformed or unknown device responses move from error! to warn!. The preflight credential dumps were logging credential IDs at info!, which is sensitive: keep only a count at debug! and move the full lists to trace!. Replace interpolated messages with structured fields.
Stop logging AES key material and ciphertext: log a length instead. Peer public-key and decrypt failures move from error! to warn!, while genuine crypto invariants (HMAC/HKDF/cipher build on fixed inputs) stay at error!. Replace interpolated messages with structured fields.
Malformed Client PIN responses and device faults move from error! to warn!, keeping error! for in-memory invariants. Reduce the pinUvAuthToken and HMAC-secret outputs to lengths, and replace interpolated messages with structured fields.
Replace interpolated messages with structured fields so the virt test harness satisfies the logging lints.
AlfioEmanueleFresta
force-pushed
the
chore/log-levels
branch
from
September 13, 2026 19:25
67baeb1 to
95f13ef
Compare
AlfioEmanueleFresta
marked this pull request as ready for review
September 13, 2026 19:25
msirringhaus
approved these changes
Sep 15, 2026
msirringhaus
left a comment
msirringhaus
left a comment
Collaborator
There was a problem hiding this comment.
I really like this! Some minor questions inline, but overall very nice!
| trace!(?apdu, "Received APDU"); | ||
| ``` | ||
|
|
||
| Prefer a span over repeating the same field on every event. A ceremony or a |
Collaborator
There was a problem hiding this comment.
Maybe add an example here?
| - large-blob plaintext | ||
| - private keys | ||
| - credential IDs | ||
| - user handles and user names |
Collaborator
There was a problem hiding this comment.
How do we feel about hardware specific things (e.g. USB device address)?
| Some(modality) => Ok(modality), | ||
| None => { | ||
| warn!("Channel did not return modality."); | ||
| warn!("Channel did not return modality"); |
Collaborator
There was a problem hiding this comment.
Is the absence of a full stop at the end of the line enforced? Should this be mentioned in logging.md in the "how to write"-section?
| } else { | ||
| error!("Failed to split HMAC Secret outputs. Unexpected output length: {}. Skipping HMAC extension", output.len()); | ||
| warn!( | ||
| output_len = output.len(), |
Collaborator
There was a problem hiding this comment.
Isn't this violating the rules?
These are sensitive and must never be logged above
debug!:
- HMAC salts and outputs, and PRF values
...
Atdebug!log only a length or a presence flag.
Should such a situation be dealt with by having two loggings at the same place, with different levels of information?
| warn!( | ||
| total = out.len(), | ||
| "largeBlobArray exceeded {LARGE_BLOB_MAX_ARRAY_BYTES}, aborting" | ||
| { total = out.len(), cap = LARGE_BLOB_MAX_ARRAY_BYTES }, |
Collaborator
There was a problem hiding this comment.
Same here, as mentioned above. Logging the lenght in warn()?
| } | ||
|
|
||
| #[instrument] | ||
| #[instrument(level = Level::DEBUG, skip_all)] |
Collaborator
There was a problem hiding this comment.
Should we add some sentence or two about these in logging.md as well?
| frame_len = decrypted_frame.len(), | ||
| padding_len, "Padding length exceeds frame length" | ||
| warn!( | ||
| { frame_len = decrypted_frame.len(), padding_len }, |
Collaborator
There was a problem hiding this comment.
Is this fine to log at warn? Not entirely sure right now, what the decrypted frame contains.
| [target.'cfg(all())'] | ||
| rustflags = ["-C", "linker=dylint-link"] | ||
|
|
||
| # For Rust versions 1.74.0 and onward, the following alternative can be used |
Collaborator
There was a problem hiding this comment.
Aren't you pinning the nightly rust further down for this crate? Should this then be already available?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #223.
Adds
docs/logging.md, a logging policy for the library:It then brings the whole library in line with the policy. Most messages logged at error were really device, transport or decoding failures and now log at warn, while genuine library invariants stay at error. Expected polled conditions such as U2F user presence drop to debug. A few places were logging secrets or full credential lists above debug, including key material and the preflight list. Those now log a length or move to trace. All interpolated messages and one stray println become structured tracing calls.
A custom dylint lint in
lints/enforces the mechanical rules (static messages, no print family, no log crate, and a best-effort sensitive-name heuristic) and runs in CI.