feat: v0.2 - multi-crate, sigstore support, challenge-response, docker image #8
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRestructures the project from a monolithic library into a Rust workspace with specialized crates (core, cli, gpg, sigstore, wasm), adds Sigstore keyless signing alongside OpenPGP, and migrates CLI, core primitives, and cryptographic backends into modular, feature-gated components with expanded challenge-response workflows and WASM bindings. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Core
participant GPG
participant Sigstore
participant Agent
participant OIDC
participant Rekor
Note over User,CLI: User requests signing
User->>CLI: sign --backend=gpg|sigstore input.pdf
CLI->>Core: split_pdf(input)
Core-->>CLI: clean_pdf, suffix_data
alt backend == gpg
CLI->>GPG: sign_gpg(clean_pdf, key_spec,...)
GPG->>Agent: request keypair (gpg-agent)
Agent-->>GPG: keypair
GPG-->>CLI: SignResult {fingerprint, uids, signature}
else backend == sigstore
CLI->>Sigstore: sign_sigstore(clean_pdf, endpoints,...)
Sigstore->>OIDC: obtain_identity_token()
OIDC-->>Sigstore: identity_token
Sigstore->>Rekor: submit bundle
Rekor-->>Sigstore: log_index
Sigstore-->>CLI: SignResult {certificate_identity, issuer, log_index}
end
CLI->>Core: encode_suffix_block(SignResult)
Core-->>CLI: encoded_block
CLI->>CLI: append encoded_block to clean_pdf + suffix_data
CLI-->>User: signed PDF output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
0x77dev
changed the title
0x77dev
changed the title
0x77dev
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (28)
scripts/e2e.sh (2)
49-63: Add error handling for invalid Sigstore token.The conditional block runs Sigstore operations only if
SIGSTORE_IDENTITY_TOKENis set, but if the token is invalid or expired at runtime, thesignandverifycommands may fail with unclear errors. Theset -ewill catch stderr, but consider adding explicit validation or error messaging.# Sigstore tests require network and OIDC token if [[ -n "${SIGSTORE_IDENTITY_TOKEN:-}" ]]; then + # Validate token is not empty after expansion + if [[ -z "$SIGSTORE_IDENTITY_TOKEN" ]]; then + echo "ERROR: SIGSTORE_IDENTITY_TOKEN is set but empty" >&2 + exit 1 + fi + echo "==> Test 3: Sigstore sign + verify" - sigstore_signed="$("$PDF_SIGN" sign --backend sigstore input.pdf --identity-token "$SIGSTORE_IDENTITY_TOKEN")" + sigstore_signed="$("$PDF_SIGN" sign --backend sigstore input.pdf --identity-token "$SIGSTORE_IDENTITY_TOKEN")" || { + echo "ERROR: Sigstore sign failed (token may be invalid or expired)" >&2 + exit 1 + } "$PDF_SIGN" verify "$sigstore_signed" | grep -x OK >/dev/null
1-13: Add validation for required binary before first use.The script assumes
$PDF_SIGNis available but does not check if the binary exists or is executable. This can lead to unclear errors later.#!/usr/bin/env bash # End-to-end tests for pdf-sign. # Runs in Nix sandbox (offline) for GPG tests, and optionally with network for Sigstore. set -euo pipefail + +# Verify pdf-sign binary is available +if [[ -z "${PDF_SIGN:-}" ]] || ! command -v "$PDF_SIGN" &>/dev/null; then + echo "ERROR: PDF_SIGN binary not found or not in PATH" >&2 + exit 1 +fiscripts/autocast.yaml (1)
54-61: Consider making tamper offset more defensive or documented.The fixed byte offset (4096) and value ('X') may not reliably corrupt the signature if the PDF structure changes or varies. For a demo, consider either:
- Adding a comment explaining why offset 4096 is safe, or
- Using a more robust tampering approach (e.g., truncating the file or modifying a later offset in the signature area)
This is a low-priority refinement since the demo's primary goal is illustrative rather than exhaustive.
scripts/autocast.sh (1)
48-48: Improve error diagnostics for PDF download failure.The curl command suppresses all output (
>/dev/null 2>&1), which means if the download fails, the user receives no helpful error message when the script exits. Consider capturing curl's stderr for visibility into network/download failures.-curl -fsSL -o "${_PDF_SIGN_DEMO_DIR}/bitcoin.pdf" https://bitcoin.org/bitcoin.pdf >/dev/null 2>&1 +curl -fSL -o "${_PDF_SIGN_DEMO_DIR}/bitcoin.pdf" https://bitcoin.org/bitcoin.pdf \ + || { echo "Failed to download bitcoin.pdf from https://bitcoin.org/bitcoin.pdf" >&2; exit 1; }This change:
- Removes the silent
-sflag to show progress/errors from curl- Keeps
-Sto show errors even with-s- Adds explicit error message with exit code
- Directs error to stderr (>&2)
README.md (1)
188-191: Clarify GPG agent requirement and OpenPGP/Sigstore block parsing.Minor clarity suggestions:
- Line 188: "All private key operations delegated to
gpg-agent" — consider adding that this requiresgpg-agentto be running and accessible via$GNUPGHOME(documented on line 283, but could be mentioned earlier).- Lines 237–244: The PGP block example shows standard ASCII-armored format. Confirm the tool correctly skips/validates existing
-----BEGIN PGP SIGNATURE-----blocks and appends new ones.- Lines 248–255: The Sigstore block format uses a custom header (
-----BEGIN PDF-SIGN SIGSTORE-----). Confirm parsing correctly distinguishes this from PGP blocks.No breaking issues, but early clarity prevents user confusion.
Consider adding a note like:
GPG Signing Requirements: Ensure
gpg-agentis running (gpg-agent --daemon) and your private key is in the agent. See Configuring GPG Agent forGNUPGHOME.This bridges lines 188 and 283.
Also applies to: 237-244, 248-255
.cargo/config.toml (1)
2-5: Clarify or update the comment about hardening flags.The comment states "Disable hardening flags that don't work with WASM", but the configuration only sets a stack size and doesn't explicitly disable any hardening flags. Either update the comment to accurately describe the stack size configuration, or add the relevant hardening flag disablement if needed.
-# Disable hardening flags that don't work with WASM +# Configure WASM target with appropriate stack size rustflags = [ "-C", "link-arg=-zstack-size=4194304", ]tsconfig.json (1)
24-27: Consider enablingnoPropertyAccessFromIndexSignaturefor type safety.While the current configuration disables some stricter checks, enabling
noPropertyAccessFromIndexSignaturewould prevent accessing potentially undefined properties on indexed types, reducing runtime errors in WASM bindings.// Some stricter flags (disabled by default) "noUnusedLocals": false, "noUnusedParameters": false, - "noPropertyAccessFromIndexSignature": false + "noPropertyAccessFromIndexSignature": truecrates/gpg/src/keybox.rs (2)
55-72: Silent error skipping may hide important failures.Line 56 uses
filter_map(|r| r.ok())which silently discards keybox record parsing errors. If individual records fail to parse, this could silently return fewer certificates than expected without any indication to the user.Consider logging skipped errors:
let certs = match kbx - .filter_map(|r| r.ok()) + .filter_map(|r| match r { + Ok(record) => Some(record), + Err(e) => { + tracing::warn!("Skipping malformed keybox record: {}", e); + None + } + }) .filter_map(|r| match r {
133-139: Add context to file read error.The
std::fs::read(path)?error propagates without indicating which operation failed.- return Cert::from_bytes(&std::fs::read(path)?) - .with_context(|| format!("Failed to load certificate from file: {}", path.display())); + let bytes = std::fs::read(path) + .with_context(|| format!("Failed to read certificate file: {}", path.display()))?; + return Cert::from_bytes(&bytes) + .with_context(|| format!("Failed to parse certificate from file: {}", path.display()));crates/cli/src/util.rs (1)
3-15: Document the rounding threshold rationale.The threshold value
1_048_525appears carefully chosen to prevent displaying "1024.0 KB", but the derivation isn't immediately obvious to future maintainers.Consider adding a comment explaining the threshold:
const KB: f64 = 1024.0; const MB: f64 = KB * 1024.0; + // Threshold chosen to avoid displaying "1024.0 KB" due to rounding. + // At 1_048_524 bytes (1023.99 KB), we still show KB. + // At 1_048_525 bytes and above, we switch to MB display. const KB_TO_MB_ROUNDING_THRESHOLD: usize = 1_048_525;crates/wasm/Cargo.toml (1)
37-37: Inconsistent dependency version specification.Line 37 uses an explicit version
"0.1.7"forconsole_error_panic_hookwhile all other dependencies use workspace resolution. Consider moving this to the workspace dependencies for consistency, or document why an explicit version is required.If no specific version requirement exists, apply this change:
-console_error_panic_hook = { version = "0.1.7", optional = true } +console_error_panic_hook = { workspace = true, optional = true }And add it to the workspace dependencies in the root
Cargo.toml.crates/wasm/tests/web.rs (1)
45-63: Consider consolidating duplicate serialization tests.A nearly identical
test_challenge_serializationappears in bothcrates/wasm/src/lib.rs(lines 319-334) and this file. While having both unit and integration tests is valid, consider whether both are necessary or if one provides sufficient coverage.Cargo.toml (1)
60-61: Consider documenting tokio feature expectations.
tokiois declared withdefault-features = falseand no features at the workspace level. Ensure each crate that depends on tokio specifies the required features (e.g.,rt,rt-multi-thread,macros).crates/core/src/pdf.rs (1)
26-48: Consider adding an error case test.The tests cover happy paths well. Consider adding a test for the error case when no
%%EOFmarker exists to verify the error message.#[test] fn errors_when_no_eof_marker() { let invalid = b"%%PDF-1.4\n...content..."; assert!(find_eof_offset(invalid).is_err()); }flake.nix (1)
56-75: Consider deduplicating theautocastimport.The
autocastderivation is imported twice from./nix/demo.nix— once in thepackagesblock (lines 58-61) and again indevShells.default(lines 72-75). While Nix's lazy evaluation prevents redundant computation, this duplicates the import expression.You could lift the
let autocast = ...binding outside both blocks or reference it from the packages set:devShells.default = import ./nix/shell.nix { inherit pkgs; pdfSign = package.pdfSign; - autocast = import ./nix/demo.nix { - inherit pkgs craneLib; - lib = pkgs.lib; - }; + inherit (self.packages.${system}) autocast; pre-commit-check = import ./nix/git-hooks.nix { inherit git-hooks system pkgs; src = ./.; }; };Alternatively, move
autocastto a sharedletbinding at the system level.crates/core/src/digest.rs (1)
99-104: Consider verifying the actual digest value in the test.The test only asserts the output length. Adding a check against a known SHA-512 hash of
"hello"would catch potential algorithm implementation issues.#[test] fn sha512_computes_correctly() { let data = b"hello"; let digest = compute_digest(DigestAlgorithm::Sha512, data); assert_eq!(digest.len(), 64); + // Known SHA-512 of "hello" + let expected_hex = "9b71d224bd62f3785d96d46ad3ea3d73319bfbc2890caadae2dff72519673ca72323c3d99ba5c11d7c7acc6e14b8c5da0c4663475c2e5c3adef46f73bcdec043"; + let expected: Vec<u8> = (0..64).map(|i| u8::from_str_radix(&expected_hex[i*2..i*2+2], 16).unwrap()).collect(); + assert_eq!(digest, expected); }crates/cli/src/sign.rs (1)
170-171: Consider refactoring to reduce parameter count.The
sign_sigstorefunction has 10 parameters. While#[allow(clippy::too_many_arguments)]suppresses the lint, grouping related parameters into a struct would improve maintainability and make the API cleaner.You could create a
SigstoreSignCommandstruct to bundle the endpoint-related options and pass it instead of individual parameters.nix/package.nix (1)
60-77: Docker image configuration looks solid.The layered image setup with volumes for
/gnupgand/datais appropriate for GPG key management and PDF processing. Usinglib.getExeensures the correct binary path.One minor consideration: the
tag = "latest"is hardcoded. For reproducible builds and proper versioning, consider using theversionvariable.image = pkgs.dockerTools.buildLayeredImage { name = "ghcr.io/0x77dev/pdf-sign"; - tag = "latest"; + tag = version;crates/core/src/suffix.rs (1)
123-131: Consider extractingfind_subsliceto avoid duplication.This helper function is identical to the one in
crates/gpg/src/verify.rs(lines 225-233). Consider moving it topdf-sign-coreand re-exporting it for both crates to use.crates/sigstore/Cargo.toml (2)
9-12: Feature flags appear unused.The
nativeandwasmfeatures are defined but don't currently gate any dependencies or conditional compilation. If these are placeholders for future work, consider adding a TODO comment or removing them until needed to avoid confusion.
28-31: Consider scoping Tokio features.
tokio = { workspace = true, features = ["full"] }includes all Tokio features. If only specific features are needed (e.g.,rt-multi-thread,macros,time), narrowing the feature set can reduce compile times and binary size.crates/cli/src/cli.rs (2)
72-74: Consider validating digest_algorithm at parse time.The
digest_algorithmis aStringwith a default of"sha512". Based on the codebase context (DigestAlgorithm::Sha512), only SHA-512 appears supported. Consider using aclap::ValueEnumto restrict valid values at parse time rather than runtime.+#[derive(Debug, Clone, Copy, PartialEq, Eq, clap::ValueEnum)] +pub enum DigestAlgorithmArg { + Sha512, +} /// Digest algorithm (Sigstore backend, default: sha512) - #[arg(long, default_value = "sha512")] - digest_algorithm: String, + #[arg(long, value_enum, default_value = "sha512")] + digest_algorithm: DigestAlgorithmArg,
110-126: Challenge command is GPG-specific but not explicitly documented.The
Challengesubcommand requires--keyand--embed_uid, which are GPG-specific options. The help text mentions "GPG signing" but consider adding a note that this is OpenPGP-only, or add backend support in the future for consistency withSign.crates/gpg/src/challenge.rs (1)
184-205: Test coverage is minimal.The serialization test is useful, but consider adding integration tests for the challenge-response flow with actual GPG signatures to ensure cryptographic correctness.
crates/sigstore/src/verify.rs (1)
190-265: Significant code duplication betweenextract_identity_from_bundleandextract_bundle_info.Both functions parse the certificate, extract SAN, and extract the OIDC issuer using nearly identical code. The only differences are minor:
extract_identity_from_bundleerrors on missing identity whileextract_bundle_inforeturns "unknown", andextract_identity_from_bundlehas slightly different whitespace handling.Consider extracting common certificate parsing logic into a shared helper:
struct CertInfo { identity: Option<String>, issuer: Option<String>, } fn extract_cert_info(bundle: &sigstore::bundle::Bundle) -> Result<CertInfo> { // Common certificate parsing, SAN extraction, and issuer extraction... } fn extract_identity_from_bundle(bundle: &sigstore::bundle::Bundle) -> Result<(String, String)> { let info = extract_cert_info(bundle)?; let identity = info.identity.ok_or_else(|| anyhow::anyhow!("No email or URI found in certificate SAN"))?; let issuer = info.issuer.unwrap_or_else(|| "unknown".to_string()); Ok((identity, issuer)) } fn extract_bundle_info(bundle: &sigstore::bundle::Bundle) -> Result<(String, String)> { let info = extract_cert_info(bundle)?; Ok(( info.identity.unwrap_or_else(|| "unknown".to_string()), info.issuer.unwrap_or_else(|| "unknown".to_string()), )) }Also applies to: 267-337
crates/wasm/src/lib.rs (1)
41-48:VerificationResultuses GPG-centric field naming for Sigstore verification.The
gpg_signaturesfield name is misleading when used for Sigstore verification results. The struct also lacks Sigstore-specific information likecertificate_identityandrekor_log_index.Consider a more generic structure or separate result types:
#[derive(Tsify, Serialize, Deserialize)] #[tsify(into_wasm_abi, from_wasm_abi)] #[serde(rename_all = "camelCase")] pub struct VerificationResult { pub valid: bool, - pub gpg_signatures: Vec<SignatureInfo>, + pub signatures: Vec<SignatureInfo>, + #[serde(skip_serializing_if = "Option::is_none")] + pub sigstore_identity: Option<String>, + #[serde(skip_serializing_if = "Option::is_none")] + pub sigstore_issuer: Option<String>, }Also applies to: 249-304
crates/sigstore/src/sign.rs (2)
107-111: Consider defensive handling forlog_indexcast.The cast
entry.log_index as u64could produce unexpected results iflog_indexis ever negative (though unlikely in practice). A defensive check ortry_into()would be safer.- .map(|entry| entry.log_index as u64); + .and_then(|entry| u64::try_from(entry.log_index).ok());
139-205: Code duplication withverify.rs::extract_bundle_info.This function is nearly identical to
extract_bundle_infoincrates/sigstore/src/verify.rs. Consider extracting the shared certificate parsing logic into a common helper in this crate to reduce duplication.Note a subtle behavioral difference: this version errors if no email/URI is found in SAN (line 190), while verify.rs falls back to
"unknown". Ensure this stricter behavior during signing is intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockbun.lockis excluded by!**/*.lock
📒 Files selected for processing (60)
.cargo/config.toml(1 hunks).envrc(1 hunks).github/workflows/ci.yml(2 hunks).gitignore(1 hunks).vscode/settings.json(1 hunks)Cargo.toml(1 hunks)LICENSE(1 hunks)README.md(1 hunks)crates/cli/Cargo.toml(1 hunks)crates/cli/src/app.rs(1 hunks)crates/cli/src/cli.rs(1 hunks)crates/cli/src/commands.rs(1 hunks)crates/cli/src/json.rs(1 hunks)crates/cli/src/main.rs(1 hunks)crates/cli/src/sign.rs(1 hunks)crates/cli/src/util.rs(1 hunks)crates/core/Cargo.toml(1 hunks)crates/core/src/digest.rs(1 hunks)crates/core/src/lib.rs(1 hunks)crates/core/src/pdf.rs(1 hunks)crates/core/src/suffix.rs(1 hunks)crates/core/src/types.rs(1 hunks)crates/gpg/Cargo.toml(1 hunks)crates/gpg/src/challenge.rs(1 hunks)crates/gpg/src/keybox.rs(1 hunks)crates/gpg/src/lib.rs(1 hunks)crates/gpg/src/sign.rs(1 hunks)crates/gpg/src/verify.rs(1 hunks)crates/gpg/tests/challenge_response.rs(1 hunks)crates/sigstore/Cargo.toml(1 hunks)crates/sigstore/src/lib.rs(1 hunks)crates/sigstore/src/sign.rs(1 hunks)crates/sigstore/src/verify.rs(1 hunks)crates/wasm/.gitignore(1 hunks)crates/wasm/Cargo.toml(1 hunks)crates/wasm/LICENSE(1 hunks)crates/wasm/src/lib.rs(1 hunks)crates/wasm/tests/web.rs(1 hunks)flake.nix(2 hunks)nix/checks.nix(2 hunks)nix/demo.nix(1 hunks)nix/git-hooks.nix(1 hunks)nix/package.nix(2 hunks)nix/shell.nix(2 hunks)package.json(1 hunks)rustfmt.toml(1 hunks)scripts/autocast.sh(1 hunks)scripts/autocast.yaml(1 hunks)scripts/e2e.sh(2 hunks)src/app.rs(0 hunks)src/cli.rs(0 hunks)src/json.rs(0 hunks)src/keybox.rs(0 hunks)src/lib.rs(0 hunks)src/main.rs(0 hunks)src/pdf.rs(0 hunks)src/sign.rs(0 hunks)src/util.rs(0 hunks)src/verify.rs(0 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (10)
- src/util.rs
- src/main.rs
- src/cli.rs
- src/app.rs
- src/sign.rs
- src/keybox.rs
- src/verify.rs
- src/pdf.rs
- src/lib.rs
- src/json.rs
🧰 Additional context used
🧬 Code graph analysis (14)
crates/cli/src/main.rs (1)
crates/cli/src/app.rs (1)
run(6-99)
crates/gpg/tests/challenge_response.rs (1)
crates/gpg/src/challenge.rs (2)
prepare_challenge(51-84)validate_response(146-182)
crates/cli/src/commands.rs (7)
crates/core/src/pdf.rs (1)
split_pdf(21-24)crates/core/src/suffix.rs (2)
encode_suffix_block(100-121)parse_suffix_blocks(39-96)crates/gpg/src/keybox.rs (1)
load_cert(133-160)crates/sigstore/src/verify.rs (1)
verify_blob(51-133)crates/gpg/src/verify.rs (1)
verify_signatures(128-195)crates/gpg/src/challenge.rs (2)
prepare_challenge(51-84)apply_response(91-139)crates/wasm/src/lib.rs (2)
prepare_challenge(52-92)apply_response(96-135)
crates/cli/src/app.rs (2)
crates/cli/src/sign.rs (2)
sign_gpg(30-168)sign_sigstore(171-317)crates/cli/src/commands.rs (3)
verify_pdf(25-240)prepare_challenge(243-353)apply_response(356-503)
crates/core/src/lib.rs (3)
crates/core/src/digest.rs (3)
compute_digest(51-59)encode_sri(62-69)parse_sri(74-93)crates/core/src/pdf.rs (2)
find_eof_offset(9-15)split_pdf(21-24)crates/core/src/suffix.rs (2)
encode_suffix_block(100-121)parse_suffix_blocks(39-96)
crates/core/src/suffix.rs (1)
crates/gpg/src/verify.rs (1)
find_subslice(226-234)
crates/wasm/tests/web.rs (2)
crates/gpg/tests/challenge_response.rs (1)
test_challenge_serialization(43-58)crates/wasm/src/lib.rs (1)
test_challenge_serialization(320-335)
crates/sigstore/src/lib.rs (2)
crates/sigstore/src/sign.rs (1)
sign_blob(68-137)crates/sigstore/src/verify.rs (1)
verify_blob(51-133)
crates/wasm/src/lib.rs (5)
crates/core/src/pdf.rs (1)
split_pdf(21-24)crates/core/src/suffix.rs (2)
encode_suffix_block(100-121)parse_suffix_blocks(39-96)crates/gpg/src/verify.rs (1)
verify_signatures(128-195)crates/sigstore/src/verify.rs (1)
verify_blob(51-133)crates/gpg/tests/challenge_response.rs (1)
test_challenge_serialization(43-58)
crates/gpg/src/challenge.rs (3)
crates/cli/src/commands.rs (2)
prepare_challenge(243-353)apply_response(356-503)crates/wasm/src/lib.rs (2)
prepare_challenge(52-92)apply_response(96-135)crates/gpg/src/verify.rs (2)
get_certs(24-67)check(69-90)
crates/cli/src/cli.rs (1)
crates/core/src/digest.rs (1)
name(16-20)
crates/sigstore/src/verify.rs (2)
crates/core/src/digest.rs (3)
compute_digest(51-59)try_from(41-46)name(16-20)crates/sigstore/src/sign.rs (1)
extract_bundle_info(140-205)
crates/gpg/src/verify.rs (1)
crates/gpg/src/keybox.rs (2)
find_certs_in_keybox(98-127)load_keybox_certs(23-84)
crates/sigstore/src/sign.rs (2)
crates/core/src/digest.rs (2)
compute_digest(51-59)name(16-20)crates/sigstore/src/verify.rs (1)
extract_bundle_info(268-337)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Dev Shell (macos-arm64)
- GitHub Check: Build (linux-amd64)
- GitHub Check: Build (linux-arm64)
- GitHub Check: Build (macos-arm64)
- GitHub Check: Build (macos-arm64)
- GitHub Check: Build (linux-arm64)
- GitHub Check: Dev Shell (macos-arm64)
- GitHub Check: Build (linux-amd64)
🔇 Additional comments (83)
rustfmt.toml (2)
4-9: Formatting configuration is well-aligned.The spacing (2-space indentation), tab settings, and Unix newline convention are appropriate and consistent with the referenced
.editorconfigfile. This should provide consistent formatting across the workspace.
2-2: No issues found. Edition "2024" is a valid and stable Rust edition, released on February 20, 2025 with Rust 1.85. The configuration is correct.scripts/e2e.sh (1)
41-46: The test correctly validates multi-signature PDFs. The implementation confirms all three assumptions:
- PDF format supports multiple signatures appended sequentially in suffix blocks
- The verify command with multiple --cert flags validates all loaded certificates against signatures found in the PDF, returning an error if any validation fails
- The output format matches the expected pattern—"OK" is printed to stdout when piped
The test will correctly pass only when both signatures are successfully added and validate against their respective certificates.
scripts/autocast.yaml (2)
6-30: LGTM on settings and environment configuration.The terminal settings, custom shell configuration with stable prompt, and environment variables are well-suited for reproducible demo capture. The 900-second timeout appropriately accounts for GPG key generation and network operations.
41-41: All CLI flags used in the demo are correctly implemented.The flags
--key,--backend sigstore, and--outputin the Sign command, as well as the Verify command, are all properly defined incrates/cli/src/cli.rsand match their usage in the demo script.scripts/autocast.sh (4)
13-19: LGTM on cleanup implementation.The cleanup function is well-protected with nested error suppression, and the unconditional trap at exit ensures temporary files are removed regardless of how the script terminates.
Also applies to: 50-50
30-32: LGTM on GPG home setup.The isolated GNUPGHOME with strict 700 permissions follows GPG best practices and ensures the demo uses an ephemeral, secure keyring.
43-45: LGTM on ephemeral GPG key generation.The non-interactive key generation with empty passphrase is appropriate for a demo. The batch mode with loopback pinentry-mode ensures automation-friendly behavior.
55-55: The autocast invocation on line 55 correctly uses documented flags for autocast v0.1.0. The-d 88msflag is the short form of--type-speed(sets delay between key presses to 88ms), and--overwriteis a valid flag to overwrite the output file. No issues found.README.md (7)
213-217: Verify multi-signer signature isolation and ordering.Line 216 states "Multiple signatures (GPG + Sigstore) can coexist on one PDF." The README example (lines 329–340) shows sequential signing (Alice GPG, then Bob Sigstore).
Confirm:
- PDF parsing correctly isolates and verifies each signature independently
- Verification reports which signature(s) passed/failed
- No cross-contamination if a later signer corrupts the PDF
- The order of verification does not affect security guarantees
This is especially important for legal/audit workflows where signature provenance matters.
345-362: Air-gapped challenge-response workflow is clear; verify digest stability.Lines 345–362 document a realistic air-gapped signing workflow. The steps are:
- Prepare challenge (compute digest)
- Transfer challenge JSON
- Sign on isolated machine
- Transfer signature back
- Apply signature
- Verify
Confirm:
- The digest algorithm (SHA-512 by default, line 97) is locked in the challenge JSON and cannot be changed during response
- The
signed_range_len(line 260) ensures the PDF bytes haven't drifted between challenge and apply-response- If the PDF is modified between challenge and apply-response, the operation clearly fails
These guarantees prevent subtle signing errors in high-security workflows.
155-164:and
70-70: All hardcoded Sigstore and OIDC endpoints in README.md are verified as current production URLs (as of 2024–2025): Fulcio (https://fulcio.sigstore.dev), Rekor (https://rekor.sigstore.dev), Sigstore OIDC issuer (https://oauth2.sigstore.dev/auth), Google OIDC (https://accounts.google.com), and GitHub Actions OIDC (https://token.actions.githubusercontent.com) are all correct and match official documentation. No action needed.
35-35: No changes needed. The README CLI documentation is accurate and consistent with the implementation incrates/cli/src/cli.rs. All documented command names, options, backend identifiers (gpg,sigstore), and flags match the actual Clap CLI definitions.
5-5: Recording is publicly accessible; verify it demonstrates both signing backends.The asciinema recording URL is publicly accessible and available for download as both .cast and .txt formats on asciinema.org. However, confirm the recording demonstrates both the GPG and Sigstore signing workflows, as both are core features described in the README's quickstart. If the recording shows only one backend, consider whether it should be re-recorded to showcase both sign and verify operations across both backends.
9-19: Verification reveals the implementation differs from the stated enforcement model.The CLI does not enforce identity/issuer constraints as described. When users omit
--certificate-identityand--certificate-oidc-issuer, verification succeeds by extracting and using the embedded identity/issuer from the Sigstore bundle itself (see commands.rs lines 139–147). The README's claim on line 230 that "Verification fails unless expected identity/issuer provided" is misleading—it only fails if constraints are explicitly provided and don't match, not if the user provides nothing.Additionally, identity and issuer values are logged via
tracing::info!()in verify.rs (line 118), contradicting the README's assertion that the library "never logs tokens or identity material." While OIDC tokens are correctly not logged, identity material is included in logs.The security model is less strict than documented: pinned verification (user-provided constraints) is optional rather than required, with fallback to bundle-embedded values.
Likely an incorrect or invalid review comment.
.gitignore (1)
7-9: LGTM!Standard ignore patterns for Node.js dependencies and build artifacts, appropriate for the WASM and TypeScript integration introduced in this PR.
.envrc (1)
5-8: LGTM!The watch directives ensure the development environment reloads when Nix configuration changes, and the
--accept-flake-configflag aligns with the CI workflow configuration..vscode/settings.json (1)
1-6: LGTM!Appropriate Rust analyzer configuration for the new multi-crate workspace, enabling comprehensive IDE support across all targets and features.
.github/workflows/ci.yml (3)
13-16: LGTM!Appropriate permissions for artifact attestation generation, aligning with the Sigstore support introduced in this PR.
57-60: LGTM!Artifact attestation step properly generates build provenance for all artifacts in the dist directory, providing supply chain security for the built binaries.
68-100: LGTM!The Dev Shell job ensures the development environment works correctly across all supported platforms, complementing the build validation.
crates/core/src/types.rs (2)
5-11: LGTM!The
VerifiedSignaturestructure appropriately captures OpenPGP verification results with key fingerprint, user IDs, and source information.
13-19: LGTM!The
VerifiedSigstoreSignaturestructure correctly represents Sigstore verification results, including certificate identity, OIDC issuer, and optional Rekor log index.nix/checks.nix (1)
25-27: LGTM!Setting
CARGO_BUILD_TARGETand--workspace --all-featuresensures comprehensive testing across all workspace members with full feature coverage.crates/gpg/src/keybox.rs (2)
152-159: LGTM - Safe unwrap with appropriate warning.The
unwrap()on line 159 is safe because it's guarded by theis_empty()check on line 145. The warning for multiple matches is helpful for user awareness.
9-15: LGTM - Standard GnuPG home resolution.Correctly prioritizes
GNUPGHOMEenvironment variable before falling back to~/.gnupg, following GnuPG conventions.crates/wasm/.gitignore (1)
1-5: LGTM!Standard gitignore entries for wasm-pack projects - ignoring the
pkg/output directory and compiled.wasmbinaries.crates/core/Cargo.toml (1)
1-17: LGTM - Clean workspace-based crate manifest.Good use of workspace inheritance for metadata and dependencies. This ensures consistent versioning across the multi-crate workspace.
nix/shell.nix (1)
21-35: LGTM - Well-organized tooling additions.Good separation of concerns with clear comments. The WASM tooling (wasm-pack, wasm-bindgen-cli, binaryen, lld), web development tools (bun, nodejs_24), and demo tools align well with the PR's goals.
crates/cli/src/util.rs (1)
17-38: LGTM!The test coverage is thorough, validating both the rounding behavior and exact boundary transitions.
crates/gpg/Cargo.toml (4)
1-7: LGTM!Package metadata follows workspace conventions correctly.
9-12: LGTM!Feature flags are well-structured. The empty
challengefeature serves as a marker for conditional compilation, which is a standard Rust pattern.
28-32: LGTM!Platform-specific Tokio feature configuration is appropriate. Restricting WASM builds to
syncandmacrosavoids incompatible runtime features.
14-26: LGTM!Workspace dependency declarations are correct, with appropriate optional gating for
sequoia-gpg-agent.crates/cli/src/main.rs (1)
11-26: LGTM!The CLI entry point follows best practices: tracing to stderr, clean separation of concerns, and proper error handling. The error display logic in
app::runensures users see detailed error messages before the process exits.crates/wasm/Cargo.toml (2)
1-11: LGTM!Package configuration is correct. The dual crate types (
cdylibandrlib) properly support both WASM output and workspace integration.
20-23: LGTM!Disabling
wasm-optis a reasonable workaround for the bulk-memory operations issue. The comment clearly explains the trade-off between size optimization and build reproducibility.crates/cli/Cargo.toml (1)
1-26: LGTM!The CLI manifest is well-configured with appropriate dependencies and feature flags for both signing backends.
crates/gpg/tests/challenge_response.rs (4)
17-29: LGTM!Basic happy path coverage for challenge preparation is appropriate for an integration test.
31-40: LGTM!Test validates that the
embed_uidoption propagates correctly through the challenge preparation flow.
42-58: LGTM!Serialization round-trip test provides good coverage of the serde implementation for the Challenge type.
60-75: LGTM!Error path testing for invalid signatures is appropriate and well-structured.
crates/wasm/tests/web.rs (2)
12-26: LGTM!Test appropriately validates the WASM-specific Challenge structure, which uses base64-encoded strings instead of raw bytes.
28-43: LGTM!Test validates the VerificationResult structure correctly.
crates/cli/src/app.rs (3)
1-4: LGTM!Imports are clean and appropriate for the CLI orchestration layer.
76-98: LGTM!Error handling is well-structured with proper separation between JSON and human-readable output modes. The cause chain extraction correctly skips the top-level error.
24-27: No changes needed. The--keyrequirement is properly enforced by clap'srequired_if_eq("backend", "gpg")attribute (line 40 ofcli.rs). The.expect()at line 25 is safe and will not panic during normal operation.Cargo.toml (2)
69-73: LGTM!Release profile settings are well-suited for a production CLI binary, balancing binary size and performance.
13-13: The Rust 2024 edition is stable.The
edition = "2024"stabilized in Rust 1.85 (released February 20, 2025) and does not require a nightly compiler. Ensure your CI and build environments use Rust 1.85 or later.Likely an incorrect or invalid review comment.
crates/core/src/lib.rs (1)
1-13: LGTM!Clean module organization with appropriate re-exports. The core crate properly establishes a dependency-free foundation for the workspace.
crates/core/src/pdf.rs (1)
8-15: LGTM!The implementation correctly finds the last
%%EOFmarker usingwindows(5).rposition(). This properly handles PDFs with multiple EOF markers from incremental saves.crates/gpg/src/sign.rs (4)
36-47: LGTM!Key selection logic correctly applies the standard policy to find alive, non-revoked, signing-capable keys. The
.clone()on line 47 ensures the key outlives the iterator.
55-62: LGTM!The conditional UID embedding correctly takes only the first UID when
embed_uidis enabled.
64-75: LGTM!GPG agent connection and keypair retrieval are properly implemented with descriptive error context.
77-105: LGTM!Signature creation correctly uses the sequoia-openpgp API with proper finalization order (signer before armor writer).
crates/sigstore/src/lib.rs (1)
1-13: LGTM! Clean module structure and well-organized public API.The crate root properly exposes the sign/verify modules with appropriate re-exports and documents the default Sigstore public endpoints. The API surface is minimal and well-scoped.
crates/core/src/digest.rs (1)
1-93: LGTM! Well-designed digest abstraction with SRI support.The implementation correctly handles algorithm agility, SRI encoding/decoding with both padded and unpadded base64, and proper length validation. The tracing instrumentation on
compute_digestis a nice touch for observability.crates/cli/src/sign.rs (2)
61-67: Asymmetric block preservation between GPG and Sigstore signing.
sign_gpg(lines 61-67) only preserves existing OpenPGP signature blocks, dropping any Sigstore bundles. In contrast,sign_sigstore(lines 280-283) preserves all existing blocks regardless of type.If a PDF was previously signed with Sigstore and then signed with GPG, the Sigstore signature would be lost. This may be intentional, but if not, consider aligning the behavior.
Is this asymmetry intentional? If mixed-signature PDFs should be supported,
sign_gpgshould preserve all block types:- let existing_pgp_sigs: Vec<_> = existing_blocks - .iter() - .filter_map(|b| match b { - SuffixBlock::OpenPgpSig(data) => Some(data.clone()), - _ => None, - }) - .collect(); + // Preserve all existing blocks + let existing_blocks_to_preserve = existing_blocks.clone();And update the write loop accordingly.
Also applies to: 280-283
19-28: LGTM! Good helper function with solid test coverage.The
default_signed_output_pathfunction handles edge cases well, and the tests cover normal paths, dotfiles, and error cases appropriately.Also applies to: 319-348
crates/gpg/src/lib.rs (1)
1-27: LGTM! Well-structured feature-gated module organization.The feature gates are appropriately applied:
verifyis unconditional, enabling verification in all environments (including WASM)nativegateskeyboxandsignwhich require GPG agent accesschallengegates the challenge-response workflow separatelyThis structure supports both native CLI usage and cross-platform verification scenarios cleanly.
nix/package.nix (1)
7-18: LGTM! Workspace-aware source filtering.The filter correctly includes Rust sources, Cargo files, the crates directory, and falls back to crane's cargo source filtering. This properly supports the new multi-crate workspace structure.
crates/core/src/suffix.rs (2)
137-168: Good test coverage for round-trip encoding.The tests verify both PGP block parsing and Sigstore block round-tripping. This ensures the encode/decode cycle is consistent.
43-61: Let-chains syntax requires Rust 1.88+.The
if let ... && let ...syntax (let-chains) is used here. This was stabilized in Rust 1.88. Ensure the project's minimum supported Rust version supports this feature, as edition 2024 alone only requires Rust 1.80+.Likely an incorrect or invalid review comment.
crates/cli/src/json.rs (1)
5-56: Well-structured JSON output types.The structs are appropriately designed for CLI output serialization. Using lifetime parameters for constant strings (
status,command) avoids unnecessary allocations.crates/cli/src/cli.rs (1)
4-21: Clean CLI structure with well-documented arguments.The global flags and subcommand pattern are well-organized. The use of
global = truefor--jsonand--verboseensures they work with all subcommands.crates/gpg/src/challenge.rs (1)
51-84: LGTM!The
prepare_challengefunction correctly validates that the certificate has a signing-capable key usingStandardPolicy, extracts the fingerprint, and constructs a well-formed challenge with proper metadata.crates/cli/src/commands.rs (3)
25-240: LGTM!The
verify_pdffunction correctly handles dual-backend verification (OpenPGP and Sigstore), with proper policy construction for Sigstore and appropriate JSON/terminal output modes. The Tokio runtime usage is acceptable for CLI context.
243-353: LGTM!The
prepare_challengefunction correctly loads the certificate, prepares the challenge via the GPG module, and provides helpful output including the executable GPG command for air-gapped signing workflows.
356-503: LGTM!The
apply_responsefunction correctly reconstructs the challenge, validates the signature via the GPG module, preserves existing signature blocks, and writes the signed PDF. The output path defaulting logic is sensible.crates/sigstore/src/verify.rs (2)
50-133: LGTM!The
verify_blobfunction implements a robust verification flow: version validation, signed range length check, digest validation, and proper integration with the Sigstore verifier. The pre-extraction of identity for regex matching before invoking the verifier is a sound approach.
135-188: LGTM!The
build_identity_policyfunction correctly enforces the strict policy requirement, validates matchers (including regex compilation), and creates an exactIdentitypolicy with the actual certificate values after validation passes.crates/wasm/src/lib.rs (3)
51-92: LGTM!The
prepare_challengefunction correctly parses the armored certificate, splits the PDF, and delegates to the GPG challenge module. The helper GPG command generation is useful for users.
137-192: LGTM!The
verify_gpgfunction correctly handles the case of no signatures (returningvalid: false), parses optional certificates, and properly maps verification results to the WASM-friendlySignatureInfostruct.
194-247: LGTM!The
sign_sigstorefunction correctly handles the feature gate, prepares signing options, and properly constructs the signed PDF with existing blocks preserved and the new Sigstore bundle appended.crates/gpg/src/verify.rs (4)
61-63: Returning all keybox certificates when no specific match is found may be overly permissive.When
idsdon't match any certificates in the keybox, the code returns the entire keybox. This allows verification to succeed against any key in the user's keybox, which may not be the intended security posture.Is this fallback intentional? If the signature's key ID doesn't match any known certificate, it might be more secure to fail:
if out.is_empty() { - return Ok(keybox.clone()); + // No matching certificate found for the signature's key IDs + return Err(openpgp::Error::InvalidOperation( + "No matching certificate found in keybox".into() + ).into()); }Alternatively, if the intent is to allow any trusted key, consider documenting this behavior.
128-195: LGTM!The
verify_signaturesfunction correctly handles multiple signature blocks, properly initializes the verification helper per signature, and extracts signer information. The cert_source tracking is useful for reporting.
199-224: LGTM!The
extract_pgp_signaturesfunction correctly handles both CRLF and LF line endings and extracts multiple signature blocks. The accompanying tests provide good coverage for these edge cases.
236-263: LGTM!The tests adequately cover the CRLF and LF line ending handling in
extract_pgp_signatures.crates/sigstore/src/sign.rs (4)
9-28: LGTM!The endpoint configuration struct with sensible defaults from crate-level constants is well-structured. Empty default for
oidc_client_secretis appropriate for public OAuth client flows.
30-47: LGTM!Clean options struct with good documentation on the
identity_tokenfield explaining the CI vs interactive flow distinction.
49-56: LGTM!Result struct is well-defined with appropriate optionality for
rekor_log_index.
58-67: Good documentation and instrumentation.The doc comments clearly describe the signing workflow steps, and the tracing instrumentation with
skip(data, options)and field extraction is well done for observability.
| let signing_ctx = sigstore::bundle::sign::SigningContext::async_production() | ||
| .await | ||
| .context("Failed to initialize Sigstore signing context")?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the sign.rs file around the mentioned lines
cat -n crates/sigstore/src/sign.rs | head -150 | tail -100Repository: 0x77dev/pdf-sign
Length of output: 4159
🏁 Script executed:
# Search for how SigningContext is implemented and if it supports custom endpoints
fd "\.rs" | xargs grep -l "SigningContext" | head -5Repository: 0x77dev/pdf-sign
Length of output: 87
🏁 Script executed:
# Look for any custom endpoint configuration in the sign.rs file
rg "endpoints|fulcio_url|rekor_url" crates/sigstore/src/sign.rs -A 2 -B 2Repository: 0x77dev/pdf-sign
Length of output: 1734
🏁 Script executed:
# Search for SigningContext implementation to see if there's a way to specify custom endpoints
rg "SigningContext" --type rust -A 5 -B 2 | head -100Repository: 0x77dev/pdf-sign
Length of output: 594
🏁 Script executed:
# Check if there's documentation or usage of sigstore-rs library
fd "Cargo.toml" | xargs grep -l sigstoreRepository: 0x77dev/pdf-sign
Length of output: 142
🏁 Script executed:
# Look for any mention of custom endpoints or alternative initialization methods
rg "async_production|SigningContext" crates/ -A 3 -B 1Repository: 0x77dev/pdf-sign
Length of output: 412
🏁 Script executed:
# Check the sigstore-rs crate dependency version
cat crates/sigstore/Cargo.toml | grep -A 5 "sigstore"Repository: 0x77dev/pdf-sign
Length of output: 583
🏁 Script executed:
# Search for any documentation or comments about endpoint configuration
rg "production|staging|custom|endpoint" crates/sigstore/ -iRepository: 0x77dev/pdf-sign
Length of output: 1230
🏁 Script executed:
# Check if there are any other SigningContext methods available by searching for imports
rg "use.*sigstore" crates/sigstore/src/sign.rs -B 2 -A 10Repository: 0x77dev/pdf-sign
Length of output: 42
🏁 Script executed:
# Check main Cargo.toml for sigstore version
rg "sigstore" Cargo.toml -A 2 -B 2Repository: 0x77dev/pdf-sign
Length of output: 515
🏁 Script executed:
# Let's search for any alternative methods to create signing context
rg "SigningContext::" crates/ -oRepository: 0x77dev/pdf-sign
Length of output: 104
🏁 Script executed:
# Check if there's documentation in comments about why async_production is used
rg "async_production|SigningContext" crates/sigstore/src/ -B 5 -A 5Repository: 0x77dev/pdf-sign
Length of output: 891
🌐 Web query:
sigstore-rs 0.13 SigningContext custom endpoints configuration
💡 Result:
Yes — sigstore-rs lets you configure custom endpoints in the SigningContext via a SigningConfig (or by loading a ClientTrustConfig / trusted-root). You can either:
- Construct a SigningConfig with explicit endpoints (fulcio, rekor, TSA, OIDC) and build a signer from it; or
- Load a trusted root / signing config (TUF trust bundle) and create a SigningContext from that, which encapsulates the service endpoints and keys.
Minimal example (illustrative — adjust types/async as needed):
- Create a custom SigningConfig with custom URLs (fulcio/rekor/…).
- Make a SigningContext (or Signer) from that config and use it to sign.
Rust-style pseudo-code:
let cfg = SigningConfig {
fulcio_url: "https://fulcio.example.com".into(),
rekor_url: "https://rekor.example.com".into(),
/* set tsa/oidc etc as needed */
};
let ctx = SigningContext::from(cfg);
let signer = ctx.signer(identity_token);
let bundle = signer.sign(artifact).await?;
See the crate docs for field names and constructors:
- sigstore-sign (SigningConfig / SigningContext examples). [1]
- sigstore-trust-root (signing config & trusted-root support). [2]
- Sigstore system docs on custom components / using custom endpoints or TUF trusted roots. [3]
References:
[1] sigstore-sign crate docs (SigningConfig / SigningContext).
[2] sigstore-trust-root crate docs (Signing Config / trusted root).
[3] Sigstore docs — Configuring Cosign / custom components (custom Fulcio/Rekor/TSA via TUF or env vars).
🏁 Script executed:
# Let's also check the verify.rs to see if it uses production()
cat crates/sigstore/src/verify.rs | grep -A 5 -B 5 "production"Repository: 0x77dev/pdf-sign
Length of output: 447
🏁 Script executed:
# Check if there are any TODOs or FIXMEs related to endpoints
rg "TODO|FIXME|XXX" crates/sigstore/src/sign.rsRepository: 0x77dev/pdf-sign
Length of output: 42
🌐 Web query:
sigstore-rs 0.13.0 SigningConfig API fulcio_url rekor_url
💡 Result:
Short answer: You can configure Fulcio and Rekor endpoints with SigningConfig — the crate exposes helpers (SigningConfig::production(), ::staging()) and also a plain struct form where you set fulcio_url and rekor_url directly (e.g. SigningConfig { fulcio_url: "...".into(), rekor_url: "...".into(), ... }). See the sigstore-sign docs and the trust-root/signing-config descriptions. [1][2]
Sources:
- sigstore-sign (SigningConfig examples showing fulcio_url / rekor_url). [1]
- sigstore-trust-root / SigningConfig description (service endpoints, CA / Rekor URLs). [2]
🏁 Script executed:
# Check if SigningConfig is imported or used anywhere in the codebase
rg "SigningConfig|ClientTrustConfig" crates/Repository: 0x77dev/pdf-sign
Length of output: 42
🏁 Script executed:
# Let's look at the full sign.rs file structure to see if there's any setup
wc -l crates/sigstore/src/sign.rsRepository: 0x77dev/pdf-sign
Length of output: 91
🏁 Script executed:
# Check the imports at the top of sign.rs
head -30 crates/sigstore/src/sign.rsRepository: 0x77dev/pdf-sign
Length of output: 938
Configured endpoints are ignored; signing always uses production Sigstore.
SigningContext::async_production() hardcodes production Fulcio/Rekor URLs, making options.endpoints.fulcio_url and options.endpoints.rekor_url ineffective. The SigstoreEndpoints configuration is only used for the OIDC token flow, not for the signing context initialization. This prevents using custom or staging Sigstore instances.
Use SigningConfig with custom endpoints instead: SigningConfig { fulcio_url: options.endpoints.fulcio_url, rekor_url: options.endpoints.rekor_url, ... } to respect the configured endpoints.
🤖 Prompt for AI Agents
In crates/sigstore/src/sign.rs around lines 72 to 74, the code calls
SigningContext::async_production(), which forces production Fulcio/Rekor
endpoints and ignores options.endpoints; replace this by constructing a
SigningConfig that uses options.endpoints.fulcio_url and
options.endpoints.rekor_url (plus any other required fields from options), then
initialize the SigningContext from that config (the async variant) so the
configured endpoints are honored; ensure proper await and error context
propagation when creating the signing context.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flake.nix (1)
62-86: Remove duplicatedautocastimport.The
autocastderivation is imported twice: once inpackages(lines 64-67) and again indevShells.default(lines 78-81). This causes the same derivation to be evaluated twice unnecessarily.Refactor to import
autocastonce at theletbinding level:in { checks = import ./nix/checks.nix { inherit pkgs craneLib package git-hooks system ; }; - packages = - let - autocast = import ./nix/demo.nix { - inherit pkgs craneLib; - lib = pkgs.lib; - }; - in - { + packages = { default = package.pdfSign; pdf-sign = package.pdfSign; inherit autocast; }; devShells.default = import ./nix/shell.nix { inherit pkgs; pdfSign = package.pdfSign; - autocast = import ./nix/demo.nix { - inherit pkgs craneLib; - lib = pkgs.lib; - }; + inherit autocast; pre-commit-check = import ./nix/git-hooks.nix { inherit git-hooks system pkgs; src = ./.; }; }; }And move
autocastto the outerletblock alongsidepackage:let pkgs = import nixpkgs { ... }; craneLib = crane.mkLib pkgs; package = import ./nix/package.nix { ... }; autocast = import ./nix/demo.nix { inherit pkgs craneLib; lib = pkgs.lib; }; in
♻️ Duplicate comments (2)
Cargo.toml (1)
32-33: Bilrost versions are now valid.The versions have been updated to
0.1010.2, which exists on crates.io, addressing the previous review feedback.crates/cli/src/commands.rs (1)
120-136: Previous issue has been addressed.The conflicting identity/issuer options now correctly bail with clear error messages when both exact and regexp variants are provided. This prevents the silent ignore behavior flagged in the previous review.
🧹 Nitpick comments (10)
scripts/e2e.sh (2)
54-59: Remove redundant null check of SIGSTORE_IDENTITY_TOKEN.Line 55 already checks that
SIGSTORE_IDENTITY_TOKENis non-empty; the second check at line 56 is redundant.if [[ -n "${SIGSTORE_IDENTITY_TOKEN:-}" ]]; then - if [[ -z "$SIGSTORE_IDENTITY_TOKEN" ]]; then - echo "ERROR: SIGSTORE_IDENTITY_TOKEN is set but empty" >&2 - exit 1 - fi - echo "==> Test 3: Sigstore sign + verify"
69-77: Test multi-backend verification incompletely.The multi-backend test (Test 4) adds a Sigstore signature to a GPG-signed PDF but only verifies the GPG signature. To ensure both backends function in the combined scenario, verify the Sigstore signature as well.
"$PDF_SIGN" verify "$multi_backend" --cert cert1.asc | grep -x OK >/dev/null + # Also verify Sigstore signature is present + "$PDF_SIGN" verify "$multi_backend" | grep -x OK >/dev/null echo " [OK] Multi-backend signing passed"Confirm whether the intent is to verify both GPG and Sigstore signatures in the multi-backend test, or only ensure backward compatibility.
nix/package.nix (1)
60-82: Consider using the version for the Docker image tag instead of"latest".Using
tag = "latest"makes it difficult to track which version of the application is deployed. Consider using the version variable for reproducibility and traceability.image = pkgs.dockerTools.buildLayeredImage { name = "ghcr.io/0x77dev/pdf-sign"; - tag = "latest"; + tag = version;Alternatively, expose both tags if you need
latestfor convenience:passthru.imageLatest = image; passthru.imageVersioned = pkgs.dockerTools.buildLayeredImage { name = "ghcr.io/0x77dev/pdf-sign"; tag = version; # ... rest of config };crates/gpg/tests/challenge_response.rs (1)
57-73: Consider adding a test for valid signature acceptance.The test suite validates that invalid signatures are rejected, but there's no test verifying that a correctly signed response is accepted. Adding a positive test case would improve coverage of the happy path.
Would you like me to help generate a test that creates a valid signature using the test certificate's secret key and verifies it passes
validate_response? This would require using sequoia's signing capabilities to produce a detached signature over the challenge data.crates/cli/src/cli.rs (1)
38-76: Consider addingconflicts_withfor backend-specific options.The Sign command mixes GPG-specific options (
key,embed_uid) with Sigstore-specific options (oidc_issuer,fulcio_url, etc.). While the required_if_eq constraint ensures--keyis provided for GPG, there's no validation preventing users from passing GPG-specific options when using Sigstore backend (or vice versa). These would be silently ignored.Consider adding
conflicts_withattributes to prevent confusion:/// Key specification: file path (.asc), fingerprint, key ID, or email (GPG backend only) - #[arg(short, long, required_if_eq("backend", "gpg"))] + #[arg(short, long, required_if_eq("backend", "gpg"), conflicts_with_all = ["oidc_issuer", "fulcio_url", "rekor_url", "oidc_client_id", "oidc_client_secret", "identity_token"])] key: Option<String>, /// Embed signer UID into the OpenPGP signature (GPG backend only) - #[arg(long)] + #[arg(long, conflicts_with_all = ["oidc_issuer", "fulcio_url", "rekor_url", "oidc_client_id", "oidc_client_secret", "identity_token"])] embed_uid: bool,Alternatively, this could be documented as acceptable behavior where unused options are simply ignored.
crates/cli/src/commands.rs (2)
145-162: Consider warning users when auto-trusting embedded identity.When no explicit identity constraints are provided, the code extracts the identity from the Sigstore bundle itself and verifies against it (lines 152-157). This means verification succeeds for any valid Sigstore signature, regardless of the signer's identity.
While this behavior is documented in the README (which requires explicit identity policy), a CLI warning when falling back to auto-trust would help prevent accidental approval of unexpected signers.
let options = if has_sigstore_constraints { let policy = policy.clone(); SigstoreVerifyOptions { policy, offline } } else { + eprintln!( + "{}", + style(" Warning: No identity policy provided - trusting embedded signer identity") + .yellow() + ); let (id, iss) = pdf_sign_sigstore::verify::extract_identity_from_block(bundle_block)?; let policy = VerifyPolicy { certificate_identity: Some(CertificateIdentityMatcher::Exact(id)), certificate_oidc_issuer: Some(OidcIssuerMatcher::Exact(iss)), }; SigstoreVerifyOptions { policy, offline } };
304-312: Consider extractingChallengeJsonto a shared module.The
ChallengeJsonstruct is defined twice - once for serialization inprepare_challenge(withgpg_command) and once for deserialization inapply_response(without it). While the current approach works, extracting tocrates/cli/src/json.rswould improve consistency and maintainability.Also applies to: 398-405
crates/cli/src/sign.rs (1)
83-96: Consider reusing the spinner instance.A new
ProgressBarspinner is created at lines 83-90 before connecting to GPG agent, but the previous spinner (lines 39-45) was already finished. This is fine but creates slightly repetitive code. The same pattern appears at lines 108-114 and 252-258 for writing the output file.This is a minor stylistic observation - the current approach is clear and works correctly.
crates/gpg/src/challenge.rs (1)
204-225: Test coverage is minimal but adequate for the core serialization contract.Consider adding tests for
prepare_challengeandvalidate_responseedge cases (invalid certs, version mismatch, fingerprint mismatch) in follow-up work.crates/wasm/src/lib.rs (1)
261-324: Sigstore verification works but has minor API polish issues.
TODO on line 307: Only the first Sigstore signature is verified; multiple signature support is deferred.
Confusing result structure: The
gpg_signaturesfield is always empty for Sigstore verification (line 315). Consider adding asigstore_signaturesfield or a more genericsignaturesfield toVerificationResultfor consistency across backends.Would you like me to propose a unified
VerificationResultstructure that works for both GPG and Sigstore signatures?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.cargo/config.toml(1 hunks)Cargo.toml(1 hunks)README.md(1 hunks)crates/cli/src/cli.rs(1 hunks)crates/cli/src/commands.rs(1 hunks)crates/cli/src/sign.rs(1 hunks)crates/cli/src/util.rs(1 hunks)crates/core/src/digest.rs(1 hunks)crates/core/src/pdf.rs(1 hunks)crates/gpg/src/challenge.rs(1 hunks)crates/gpg/src/keybox.rs(1 hunks)crates/gpg/tests/challenge_response.rs(1 hunks)crates/sigstore/src/sign.rs(1 hunks)crates/wasm/src/lib.rs(1 hunks)flake.nix(4 hunks)nix/checks.nix(2 hunks)nix/package.nix(2 hunks)scripts/e2e.sh(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/cli/src/util.rs
- crates/gpg/src/keybox.rs
- crates/core/src/pdf.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-15T05:48:24.869Z
Learnt from: 0x77dev
Repo: 0x77dev/pdf-sign PR: 8
File: nix/git-hooks.nix:17-17
Timestamp: 2025-12-15T05:48:24.869Z
Learning: In Nix configurations, prefer using pkgs.prek as a drop-in replacement for pkgs.pre-commit when available in nixpkgs. prek is a Rust-based reimplementation of pre-commit designed for better performance (single binary, no Python runtime). Update references in Nix files (e.g., nix/git-hooks.nix) to use pkgs.prek instead of pkgs.pre-commit, and verify that prek is present in your nixpkgs version before making the switch.
Applied to files:
nix/package.nixnix/checks.nixflake.nix
🧬 Code graph analysis (7)
crates/gpg/tests/challenge_response.rs (1)
crates/gpg/src/challenge.rs (2)
prepare_challenge(51-84)validate_response(146-202)
scripts/e2e.sh (1)
crates/sigstore/src/sign.rs (2)
default(20-28)default(41-47)
crates/cli/src/cli.rs (1)
crates/core/src/digest.rs (1)
name(16-20)
crates/gpg/src/challenge.rs (1)
crates/gpg/src/verify.rs (2)
get_certs(24-67)check(69-90)
crates/cli/src/sign.rs (7)
crates/core/src/digest.rs (3)
encode_sri(62-69)name(16-20)from_name(23-28)crates/core/src/pdf.rs (1)
split_pdf(21-24)crates/core/src/suffix.rs (2)
encode_suffix_block(100-121)parse_suffix_blocks(39-96)crates/cli/src/util.rs (1)
format_bytes(3-18)crates/gpg/src/keybox.rs (1)
load_cert(139-168)crates/gpg/src/sign.rs (1)
create_signature(31-106)crates/wasm/src/lib.rs (1)
sign_sigstore(208-259)
crates/sigstore/src/sign.rs (2)
crates/core/src/digest.rs (3)
compute_digest(51-59)try_from(41-46)name(16-20)crates/sigstore/src/verify.rs (1)
extract_bundle_info(268-337)
crates/wasm/src/lib.rs (7)
crates/cli/src/commands.rs (2)
prepare_challenge(249-359)apply_response(362-509)crates/gpg/src/challenge.rs (2)
prepare_challenge(51-84)apply_response(91-139)crates/core/src/pdf.rs (1)
split_pdf(21-24)crates/core/src/suffix.rs (2)
encode_suffix_block(100-121)parse_suffix_blocks(39-96)crates/gpg/src/verify.rs (1)
verify_signatures(128-195)crates/cli/src/sign.rs (1)
sign_sigstore(162-308)crates/sigstore/src/sign.rs (1)
sign_blob(69-156)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build (linux-amd64)
- GitHub Check: Dev Shell (macos-arm64)
- GitHub Check: Build (macos-arm64)
- GitHub Check: Build (linux-arm64)
🔇 Additional comments (34)
scripts/e2e.sh (2)
61-67: Clarify Sigstore verification behavior.The Sigstore verify call (line 66) does not pass a
--certflag, unlike the GPG verify calls. If Sigstore certs are embedded in the signature bundle and verified automatically, this is correct. If a cert is required, add--certto line 66 for consistency and explicit validation.Confirm whether
verifyon a Sigstore-signed PDF requires an explicit cert flag or validates using embedded credentials.
1-40: Script structure and setup are solid.The binary validation, GPG environment configuration, and dual-key generation are well-handled. The previous review's concern about CLI argument ordering has been resolved—all invocations now correctly place positional arguments before flags.
nix/package.nix (1)
7-18: LGTM! Comprehensive source filtering for workspace builds.The filter correctly includes Rust sources, Cargo files, the crates directory, and delegates to
craneLib.filterCargoSourcesfor additional Cargo-related files.Cargo.toml (1)
1-9: LGTM! Well-structured workspace configuration.The workspace is properly configured with resolver "2" and clear member organization separating core, backend-specific (gpg, sigstore), CLI, and WASM crates.
flake.nix (1)
20-23: LGTM! Proper rust-overlay integration.The
rust-overlayinput correctly followsnixpkgsto avoid dependency conflicts, and is properly wired into the overlays..cargo/config.toml (1)
1-5: LGTM! Appropriate WASM stack size configuration.The 4MB stack size is reasonable for WASM builds that include cryptographic operations (OpenPGP/Sigstore), which can have deep call stacks.
crates/gpg/tests/challenge_response.rs (2)
7-12: LGTM! Clean test certificate helper.The
make_test_certhelper appropriately generates a general-purpose certificate for testing purposes.
14-26: LGTM! Comprehensive test for challenge preparation.The test properly validates all key fields: version, data_to_sign content, non-empty fingerprint, and the embed_uid option.
README.md (1)
1-375: LGTM!The documentation is comprehensive and well-structured, covering both GPG and Sigstore backends, challenge-response workflows, security considerations, and practical examples. The organization with clear sections for commands, features, and examples makes it easy to navigate.
crates/core/src/digest.rs (1)
1-131: LGTM!The digest abstraction is well-designed with:
- Clear enum representation with
repr(u8)for efficient serialization- Flexible parsing that accepts common algorithm name variants
- Proper validation of digest length in
parse_sri- Good test coverage including edge cases like unpadded base64
The SRI format provides a standard, interoperable way to represent digests with algorithm agility.
crates/cli/src/cli.rs (1)
1-154: LGTM for overall CLI structure.The Clap-based CLI is well-organized with clear subcommands and appropriate argument definitions. The use of
value_enumforSigningBackend, global flags for--jsonand--verbose, and proper documentation strings make for a good user experience.crates/cli/src/commands.rs (1)
1-509: LGTM for the overall implementation.The verify, challenge, and apply-response flows are well-implemented with proper error handling, progress indication, and JSON output support. The code correctly preserves existing signature blocks when applying new signatures.
crates/cli/src/sign.rs (2)
310-339: Good test coverage for edge cases.The tests properly validate the default output path logic including the edge case where a root path has no filename. This ensures robust error handling.
1-339: LGTM for the overall sign module.The implementation cleanly separates GPG and Sigstore signing paths while maintaining consistent structure. Key features:
- Proper preservation of existing signature blocks for multi-signer support
- Good error context with
anyhow- JSON output option for automation
- Sensible defaults for Sigstore endpoints
crates/gpg/src/challenge.rs (4)
17-44: Well-structured data types for the challenge-response workflow.The
Challenge,ChallengeOptions, andSignResultstructs are appropriately designed with clear serialization support and documentation. The separation of concerns is clean.
46-84: LGTM!The
prepare_challengefunction correctly validates that the certificate has a signing-capable key before creating the challenge. The instrumentation and error handling are appropriate.
86-139: Previous cryptographic verification issue has been addressed.The function now correctly passes the certificate to
validate_response(line 120), enabling proper cryptographic verification of the signature against the expected key. The version and fingerprint validation logic is sound.
146-202: Cryptographic verification is now properly implemented.The
Helperimplementation correctly provides the certificate viaget_certsand validates signature results incheck(). This aligns with the pattern used incrates/gpg/src/verify.rsand addresses the previous review concern about missing cryptographic verification.crates/sigstore/src/sign.rs (6)
9-48: Well-structured configuration types with sensible defaults.The
SigstoreEndpointsandSignOptionsstructs provide good configurability. Default implementations use production constants appropriately.
72-93: Good documentation of sigstore-rs limitation.The code now explicitly documents that
SigningContext::async_production()hardcodes production endpoints, and logs a warning when custom endpoints are configured. This is an acceptable mitigation until sigstore-rs exposes a public API for custom endpoint configuration.
95-105: Clean token acquisition flow.The dual-path approach (provided token for CI, interactive OIDC otherwise) is well-designed and documented.
107-156: LGTM!The signing session creation, data signing, and bundle construction are implemented correctly. The digest computation aligns with the core crate's
DigestAlgorithmimplementation.
158-224: Certificate parsing implementation is sound.The extraction of SAN identity and OIDC issuer from the Fulcio certificate follows the expected Sigstore certificate structure. The fallback to "unknown" for missing issuer is a reasonable default.
232-276: Port allocation and timeout issues from previous review are addressed.Dynamic port allocation (line 237) and the 5-minute timeout (lines 270-276) resolve the previous concerns.
Minor TOCTOU race on port availability: The pattern of binding, extracting the port, then dropping the listener (line 245) before sigstore's
RedirectListener::newcreates a small window where another process could claim the port. This is low risk in practice and an inherent limitation of the sigstore-rs API.crates/wasm/src/lib.rs (7)
11-17: Good initialization pattern for WASM.The
#[wasm_bindgen(start)]hook with conditional panic hook setup is appropriate for browser environments.
19-48: TypeScript-friendly data structures are well-designed.The use of
tsifywithinto_wasm_abiandfrom_wasm_abienables seamless TypeScript integration. ThecamelCaserename convention aligns with JavaScript conventions.
50-92: LGTM!The
prepare_challengefunction correctly bridges the GPG challenge API to WASM. The generatedgpg_commanduses the certificate's fingerprint which is a hex string, making it safe for shell interpolation.
94-147: Previous concern about ignored challenge parameter is now addressed.The function now validates that
challenge.data_base64matches the PDF bytes being signed (lines 117-123), preventing accidental application of signatures to wrong documents. The validation and signature appending logic is correct.
149-204: LGTM!The GPG verification correctly handles optional certificate provision and maps verification results to the WASM-friendly
VerificationResultstructure.
206-259: Well-implemented feature-gated Sigstore signing.The function correctly preserves existing signature blocks while appending the new Sigstore bundle. The feature gate with explicit error message for disabled builds is appropriate.
326-348: Basic test coverage is present.The serialization test validates the core
Challengestructure. Consider adding tests for error paths (invalid cert, malformed PDF) in follow-up work.nix/checks.nix (3)
26-27: LGTM! Comprehensive workspace testing.The
--workspace --all-featuresflags ensure thorough testing across all crates in the new multi-crate structure, which aligns well with the workspace refactoring mentioned in the PR objectives.
25-25: The code is correct as-is.CARGO_BUILD_TARGETis appropriately set only forcargo-test(line 25), not forpre-commit-check, because the environment variable is only relevant for Rust cargo operations, not for git hooks. ThecargoTargetvariable ensures consistent cross-platform Rust builds in CI contexts, which applies to the cargo test invocation but not to pre-commit hooks that run linters and other checks.Likely an incorrect or invalid review comment.
8-14: Architecture hardcoding issue resolved.The implementation correctly uses platform-native target detection (
rust.rustcTargetwith fallback tohostPlatform.config), which properly handles Intel Macs, ARM Linux, and other platform combinations that the previous hardcoded approach failed on.Since your project uses
nixos-unstable, therust.rustcTargetattribute is available and this approach will work across all supported platforms without issues.
| let spinner = ProgressBar::new_spinner(); | ||
| spinner.set_style( | ||
| ProgressStyle::default_spinner() | ||
| .template("{spinner:.cyan} {msg}") | ||
| .unwrap(), | ||
| ); | ||
| spinner.enable_steady_tick(Duration::from_millis(80)); | ||
|
|
||
| spinner.set_message(format!("Reading PDF {}", style(input.display()).cyan())); | ||
| let mut pdf_data = Vec::new(); | ||
| let mut file = BufReader::new( | ||
| File::open(&input).with_context(|| format!("Failed to open PDF: {}", input.display()))?, | ||
| ); | ||
| file.read_to_end(&mut pdf_data)?; | ||
| spinner.finish_with_message(format!( | ||
| "[OK] Read PDF ({})", | ||
| style(format_bytes(pdf_data.len())).cyan() | ||
| )); | ||
|
|
||
| let (clean_pdf, suffix) = split_pdf(&pdf_data)?; | ||
| let existing_blocks = parse_suffix_blocks(suffix)?; | ||
|
|
||
| let digest_alg = DigestAlgorithm::from_name(&digest_algorithm)?; | ||
|
|
||
| let endpoints = pdf_sign_sigstore::SigstoreEndpoints { | ||
| oidc_issuer: oidc_issuer.unwrap_or_else(|| pdf_sign_sigstore::DEFAULT_OIDC_ISSUER.to_string()), | ||
| fulcio_url: fulcio_url.unwrap_or_else(|| pdf_sign_sigstore::DEFAULT_FULCIO_URL.to_string()), | ||
| rekor_url: rekor_url.unwrap_or_else(|| pdf_sign_sigstore::DEFAULT_REKOR_URL.to_string()), | ||
| oidc_client_id: oidc_client_id | ||
| .unwrap_or_else(|| pdf_sign_sigstore::DEFAULT_OIDC_CLIENT_ID.to_string()), | ||
| oidc_client_secret: oidc_client_secret.unwrap_or_default(), | ||
| }; | ||
|
|
||
| eprintln!(" OIDC Issuer: {}", style(&endpoints.oidc_issuer).dim()); | ||
| eprintln!(" Fulcio: {}", style(&endpoints.fulcio_url).dim()); | ||
| eprintln!(" Rekor: {}", style(&endpoints.rekor_url).dim()); | ||
|
|
||
| if identity_token.is_none() { | ||
| eprintln!( | ||
| "\n{}", | ||
| style("Your browser will open for OIDC authentication...").yellow() | ||
| ); | ||
| } | ||
|
|
||
| spinner.set_message("Performing keyless signing..."); | ||
|
|
||
| let rt = tokio::runtime::Runtime::new()?; | ||
| let sign_options = pdf_sign_sigstore::sign::SignOptions { | ||
| endpoints, | ||
| digest_algorithm: digest_alg, | ||
| identity_token, | ||
| }; | ||
|
|
||
| let sign_result = rt.block_on(pdf_sign_sigstore::sign::sign_blob(clean_pdf, &sign_options))?; |
There was a problem hiding this comment.
Spinner reuse after finish_with_message may not work as expected.
The spinner created at line 181 is finished at line 195-198 with finish_with_message. At line 225, spinner.set_message is called on the same finished spinner before signing. A finished ProgressBar may not display new messages.
Consider creating a new spinner for the signing phase:
spinner.finish_with_message(format!(
"[OK] Read PDF ({})",
style(format_bytes(pdf_data.len())).cyan()
));
// ... endpoint configuration ...
if identity_token.is_none() {
eprintln!(
"\n{}",
style("Your browser will open for OIDC authentication...").yellow()
);
}
- spinner.set_message("Performing keyless signing...");
+ let spinner = ProgressBar::new_spinner();
+ spinner.set_style(
+ ProgressStyle::default_spinner()
+ .template("{spinner:.cyan} {msg}")
+ .unwrap(),
+ );
+ spinner.enable_steady_tick(Duration::from_millis(80));
+ spinner.set_message("Performing keyless signing...");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let spinner = ProgressBar::new_spinner(); | |
| spinner.set_style( | |
| ProgressStyle::default_spinner() | |
| .template("{spinner:.cyan} {msg}") | |
| .unwrap(), | |
| ); | |
| spinner.enable_steady_tick(Duration::from_millis(80)); | |
| spinner.set_message(format!("Reading PDF {}", style(input.display()).cyan())); | |
| let mut pdf_data = Vec::new(); | |
| let mut file = BufReader::new( | |
| File::open(&input).with_context(|| format!("Failed to open PDF: {}", input.display()))?, | |
| ); | |
| file.read_to_end(&mut pdf_data)?; | |
| spinner.finish_with_message(format!( | |
| "[OK] Read PDF ({})", | |
| style(format_bytes(pdf_data.len())).cyan() | |
| )); | |
| let (clean_pdf, suffix) = split_pdf(&pdf_data)?; | |
| let existing_blocks = parse_suffix_blocks(suffix)?; | |
| let digest_alg = DigestAlgorithm::from_name(&digest_algorithm)?; | |
| let endpoints = pdf_sign_sigstore::SigstoreEndpoints { | |
| oidc_issuer: oidc_issuer.unwrap_or_else(|| pdf_sign_sigstore::DEFAULT_OIDC_ISSUER.to_string()), | |
| fulcio_url: fulcio_url.unwrap_or_else(|| pdf_sign_sigstore::DEFAULT_FULCIO_URL.to_string()), | |
| rekor_url: rekor_url.unwrap_or_else(|| pdf_sign_sigstore::DEFAULT_REKOR_URL.to_string()), | |
| oidc_client_id: oidc_client_id | |
| .unwrap_or_else(|| pdf_sign_sigstore::DEFAULT_OIDC_CLIENT_ID.to_string()), | |
| oidc_client_secret: oidc_client_secret.unwrap_or_default(), | |
| }; | |
| eprintln!(" OIDC Issuer: {}", style(&endpoints.oidc_issuer).dim()); | |
| eprintln!(" Fulcio: {}", style(&endpoints.fulcio_url).dim()); | |
| eprintln!(" Rekor: {}", style(&endpoints.rekor_url).dim()); | |
| if identity_token.is_none() { | |
| eprintln!( | |
| "\n{}", | |
| style("Your browser will open for OIDC authentication...").yellow() | |
| ); | |
| } | |
| spinner.set_message("Performing keyless signing..."); | |
| let rt = tokio::runtime::Runtime::new()?; | |
| let sign_options = pdf_sign_sigstore::sign::SignOptions { | |
| endpoints, | |
| digest_algorithm: digest_alg, | |
| identity_token, | |
| }; | |
| let sign_result = rt.block_on(pdf_sign_sigstore::sign::sign_blob(clean_pdf, &sign_options))?; | |
| let spinner = ProgressBar::new_spinner(); | |
| spinner.set_style( | |
| ProgressStyle::default_spinner() | |
| .template("{spinner:.cyan} {msg}") | |
| .unwrap(), | |
| ); | |
| spinner.enable_steady_tick(Duration::from_millis(80)); | |
| spinner.set_message(format!("Reading PDF {}", style(input.display()).cyan())); | |
| let mut pdf_data = Vec::new(); | |
| let mut file = BufReader::new( | |
| File::open(&input).with_context(|| format!("Failed to open PDF: {}", input.display()))?, | |
| ); | |
| file.read_to_end(&mut pdf_data)?; | |
| spinner.finish_with_message(format!( | |
| "[OK] Read PDF ({})", | |
| style(format_bytes(pdf_data.len())).cyan() | |
| )); | |
| let (clean_pdf, suffix) = split_pdf(&pdf_data)?; | |
| let existing_blocks = parse_suffix_blocks(suffix)?; | |
| let digest_alg = DigestAlgorithm::from_name(&digest_algorithm)?; | |
| let endpoints = pdf_sign_sigstore::SigstoreEndpoints { | |
| oidc_issuer: oidc_issuer.unwrap_or_else(|| pdf_sign_sigstore::DEFAULT_OIDC_ISSUER.to_string()), | |
| fulcio_url: fulcio_url.unwrap_or_else(|| pdf_sign_sigstore::DEFAULT_FULCIO_URL.to_string()), | |
| rekor_url: rekor_url.unwrap_or_else(|| pdf_sign_sigstore::DEFAULT_REKOR_URL.to_string()), | |
| oidc_client_id: oidc_client_id | |
| .unwrap_or_else(|| pdf_sign_sigstore::DEFAULT_OIDC_CLIENT_ID.to_string()), | |
| oidc_client_secret: oidc_client_secret.unwrap_or_default(), | |
| }; | |
| eprintln!(" OIDC Issuer: {}", style(&endpoints.oidc_issuer).dim()); | |
| eprintln!(" Fulcio: {}", style(&endpoints.fulcio_url).dim()); | |
| eprintln!(" Rekor: {}", style(&endpoints.rekor_url).dim()); | |
| if identity_token.is_none() { | |
| eprintln!( | |
| "\n{}", | |
| style("Your browser will open for OIDC authentication...").yellow() | |
| ); | |
| } | |
| let spinner = ProgressBar::new_spinner(); | |
| spinner.set_style( | |
| ProgressStyle::default_spinner() | |
| .template("{spinner:.cyan} {msg}") | |
| .unwrap(), | |
| ); | |
| spinner.enable_steady_tick(Duration::from_millis(80)); | |
| spinner.set_message("Performing keyless signing..."); | |
| let rt = tokio::runtime::Runtime::new()?; | |
| let sign_options = pdf_sign_sigstore::sign::SignOptions { | |
| endpoints, | |
| digest_algorithm: digest_alg, | |
| identity_token, | |
| }; | |
| let sign_result = rt.block_on(pdf_sign_sigstore::sign::sign_blob(clean_pdf, &sign_options))?; |
🤖 Prompt for AI Agents
In crates/cli/src/sign.rs around lines 181 to 234, the ProgressBar created at
the top is finished at the PDF-read section (finish_with_message) but then
reused at line ~225 via spinner.set_message("Performing keyless signing..."),
which may not update because a finished ProgressBar should not be reused; create
a new ProgressBar for the signing phase instead of reusing the finished one:
after spinner.finish_with_message(...) keep that spinner done, then instantiate
a new ProgressBar (apply the same ProgressStyle, enable_steady_tick) and set its
message to "Performing keyless signing..." before starting the signing runtime,
and finally finish that new spinner (e.g., finish_with_message) after sign_blob
completes.
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
Enhancements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.