Skip to content

feat(cli): group the certificate commands under a certs topic - #162

Merged
paolomainardi merged 6 commits into
mainfrom
feat/160-certs-topic
Sep 1, 2026
Merged

feat(cli): group the certificate commands under a certs topic#162
paolomainardi merged 6 commits into
mainfrom
feat/160-certs-topic

Conversation

@paolomainardi

@paolomainardi paolomainardi commented Sep 1, 2026

Copy link
Copy Markdown
Member

User description

🤖 This was written by an AI agent on behalf of @paolomainardi.

Refs #160 (the certs half; hosts describe stays open for a follow-up).

What changes

The three certificate commands become one topic, spark-http-proxy certs, in bin/lib/certs.sh:

  • certs list prints a table: the directory once, then one row per certificate with the file holding it and the file holding its key, or missing. Works with Docker stopped.
  • certs describe <domain> reads one certificate with openssl and reports what it covers, its validity, its issuer, and whether the running proxy serves it. Given a hostname with no certificate of its own it finds the certificate that covers it, or explains that a wildcard matches one label only and names the certificate to generate.
  • certs generate and certs delete keep the bodies of generate-mkcert and remove-cert unchanged.
  • generate-mkcert, list-certs, remove-cert still run, warn on stderr with their replacement, and leave the help and the completion.

Output on a real machine

$ spark-http-proxy certs list
Certificates in ~/.local/spark/http-proxy/certs

DOMAIN                                      CERTIFICATE                                             KEY
*.asyncops.sparkfabrik.sparkfabrik.loc      _wildcard_.asyncops.sparkfabrik.sparkfabrik.loc.pem     _wildcard_.asyncops.sparkfabrik.sparkfabrik.loc-key.pem
*.firestarter.sparkfabrik.sparkfabrik.loc   _wildcard_.firestarter.sparkfabrik.sparkfabrik.loc.pem  _wildcard_.firestarter.sparkfabrik.sparkfabrik.loc-key.pem
paolomainardi.loc                           paolomainardi.loc.pem                                   paolomainardi.loc-key.pem

3 certificates. Remove one with: spark-http-proxy certs delete 'paolomainardi.loc'

$ spark-http-proxy certs describe a.b.spark.loc
❌ No certificate covers a.b.spark.loc
ℹ  *.spark.loc does not cover it: a wildcard matches one label only
ℹ  Generate one with: spark-http-proxy certs generate '*.b.spark.loc'
ℹ  List installed certificates with: spark-http-proxy certs list

openssl

describe is the only command that reads a certificate. It uses x509 options present in both OpenSSL 3 and Apple's LibreSSL (-text, -startdate, -enddate, -issuer, -checkend), probes the read once before printing, and stops with the reason when openssl is missing or the file cannot be read. Verified on p620 (OpenSSL 3.6.3) and on a Mac with both /opt/homebrew/bin/openssl and /usr/bin/openssl (LibreSSL 3.3.6): identical records.

Verification

  • shellcheck -x bin/spark-http-proxy bin/lib/*.sh: same four pre-existing findings as main, nothing new.
  • certs list, describe (named, covered, one-label miss, unknown, unreadable file, missing openssl), help, unknown subcommand, the three deprecated names, generate without a domain, delete without a domain, unknown domain, and no terminal: run by hand on p620 (31 certificates) and on the Mac (3).
  • test/test.sh moves its certificate tests to the new names and adds assertions for the deprecated alias, the table, and the two describe refusals. The suite runs in CI.

Out of scope

build/traefik/entrypoint.sh filters keys with grep -v "\-key", a substring match on the path, so a certificate named like my-keycloak.spark.loc.pem is never served while certs list shows it installed. Container side, separate issue.


PR Type

Enhancement, Tests, Documentation


Description

  • Group certificate operations under certs subcommands

  • Describe coverage, validity, issuer, and serving status

  • Preserve deprecated commands with replacement warnings

  • Update completion, tests, and documentation


Diagram Walkthrough

flowchart LR
  cli["certs command"]
  list["List certificate files"]
  describe["Describe certificate details"]
  mutate["Generate or delete certificates"]
  openssl["OpenSSL certificate inspection"]
  proxy["Running proxy certificate state"]
  legacy["Deprecated top-level commands"]
  cli -- "dispatches" --> list
  cli -- "dispatches" --> describe
  cli -- "dispatches" --> mutate
  describe -- "reads metadata with" --> openssl
  describe -- "checks serving status" --> proxy
  mutate -- "applies changes to" --> proxy
  legacy -- "warns and forwards" --> cli
Loading

File Walkthrough

Relevant files
Enhancement
2 files
certs.sh
Add unified certificate command topic and inspection         
+450/-0 
spark-http-proxy
Integrate certs routing, completion, and prerequisite handling
+39/-267
Refactoring
1 files
hosts.sh
Reuse shared home-directory path abbreviation helper         
+2/-13   
Tests
1 files
test.sh
Test certificate subcommands, compatibility, and failure handling
+71/-23 
Documentation
3 files
AGENTS.md
Update certificate command safety classifications               
+3/-3     
CHANGELOG.md
Record certificate topic and deprecated commands                 
+5/-0     
README.md
Document certificate listing, description, generation, and deletion
+55/-21 


Assisted-by: pr-agent/gpt-5.6-sol

The three certificate commands move into bin/lib/certs.sh as one topic:
certs list, certs describe, certs generate and certs delete. The old
names generate-mkcert, list-certs and remove-cert still run, warn on
stderr with their replacement, and leave the help and the completion.

certs list is a table: the directory once, then one row per certificate
with the file holding it and the file holding its key, or "missing".
It needs neither Docker nor the proxy.

certs describe reads one certificate with openssl and reports what it
covers, its validity, its issuer, and whether the running proxy serves
it. Given a hostname with no certificate of its own it finds the one
that covers it, or explains that a wildcard matches one label only and
names the certificate to generate. It uses only x509 options present
in both OpenSSL and LibreSSL, probes the read once, and stops with the
reason when openssl is missing or the file cannot be read. The other
certs commands do not depend on openssl.

generate and delete keep their bodies. hosts_abbreviate becomes the
shared abbreviate_home so both libraries print ~ paths the same way.

Refs: #160
Assisted-by: claude-code/claude-fable-5-1
… with printf

The record printer no longer probes openssl itself; the two entry points
in certs_describe do, so a covered hostname is probed once rather than
twice. The validity label is aligned with printf instead of a padded
string.

Refs: #160
Assisted-by: claude-code/claude-fable-5-1
@sparkfabrik-ai-bot

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

160 - Partially compliant

Compliant requirements:

  • Group certificate operations under the certs topic.
  • Keep the three legacy certificate commands as deprecated aliases.
  • Warn on stderr and route legacy commands to replacements.
  • Hide legacy commands from help and completion.
  • Keep certs list working without Docker.
  • Explain one-label wildcard coverage in certs describe.
  • Move certificate commands into bin/lib/certs.sh.
  • Keep hosts list state-file based.
  • Leave remote-host probing behavior unchanged.
  • Do not change the peer protocol or Traefik configuration.

Non-compliant requirements:

  • Show expiration and trust columns in certs list.
  • Include expired-certificate information in the list summary.
  • Degrade certs list to a domain-only view without OpenSSL and explain the missing metadata.
  • Report whether a described certificate is trusted by this machine's CA.
  • Enhance hosts describe with live Docker container details.
  • Redact secrets in the container command shown by hosts describe.

Requires further human verification:

  • Verify generated completion scripts contain no legacy command names.
  • Verify output formatting and alignment with a large real certificate collection.
  • Verify compatibility across supported OpenSSL and LibreSSL versions.
  • Verify the deprecated aliases preserve existing scripts in supported environments.
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Missing Metadata

certs list derives domains only from filenames and reports certificate/key filenames, but never reads expiration or trust information. Consequently, an expired certificate is indistinguishable from a valid one and the required expired-certificate summary cannot be produced. When OpenSSL is unavailable, the output also cannot explain that metadata was omitted because that metadata is never attempted.

certs_list() {
  local cert_file domain cert_name key_name rows=() row fields
  local found=0 missing=0 w_domain=6 w_cert=11

  while IFS= read -r cert_file; do
    [[ -z "${cert_file}" ]] && continue
    domain="$(cert_domain_from_file "${cert_file}")"
    cert_name="$(basename "${cert_file}")"
    key_name="${cert_name%.pem}-key.pem"
    if [[ ! -f "${CERT_DIR}/${key_name}" ]]; then
      key_name="missing"
      missing=$((missing + 1))
    fi
    rows+=("${domain}"$'\t'"${cert_name}"$'\t'"${key_name}")
    [[ "${#domain}" -gt "${w_domain}" ]] && w_domain="${#domain}"
    [[ "${#cert_name}" -gt "${w_cert}" ]] && w_cert="${#cert_name}"
    found=$((found + 1))
  done < <(cert_files)

  if [[ "${found}" -eq 0 ]]; then
    log_warning "No certificates found. Generate one with: $(basename "${0}") certs generate '*.spark.loc'"
    return 0
  fi

  echo "Certificates in $(abbreviate_home "${CERT_DIR}")"
  echo ""
  printf '%-*s  %-*s  %s\n' "${w_domain}" "DOMAIN" "${w_cert}" "CERTIFICATE" "KEY"
  for row in "${rows[@]}"; do
    IFS=$'\t' read -r -a fields <<<"${row}"
    printf '%-*s  %-*s  %s\n' "${w_domain}" "${fields[0]}" "${w_cert}" "${fields[1]}" "${fields[2]}"
  done
  echo ""

  local summary
  summary="$(count_noun "${found}" certificate)"
  [[ "${missing}" -gt 0 ]] && summary="${summary}, ${missing} without a private key"
  echo "${summary}. Remove one with: $(basename "${0}") certs delete '${domain}'"
Wrong Status

certs describe reports whether the running proxy references the certificate instead of whether it is signed by this machine's trusted CA. A certificate can be served while untrusted, or trusted while the proxy is stopped, so the served field does not answer the required trust question.

# The only field that needs the proxy, so it degrades instead of failing.
if docker info >/dev/null 2>&1 && is_running http-proxy; then
  if docker exec http-proxy grep -qF "certFile: /traefik/certs/$(basename "${cert_file}")" /traefik/dynamic/auto-tls.yml 2>/dev/null; then
    echo "  served         yes, by the running proxy"
  else
    echo "  served         no, the proxy has not applied it. Apply with: $(basename "${0}") restart"
  fi
else
  echo "  served         unknown, the proxy is not running"
fi

Assisted-by: pr-agent/gpt-5.6-sol

@sparkfabrik-ai-bot

Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle certificate generation failures

Check the certificate-generation result before reporting success or applying the
files. Without this guard, a failed mkcert invocation can produce a successful
command and leave the proxy serving an old or incomplete certificate pair.

bin/lib/certs.sh [146-150]

-mkcert -cert-file "${CERT_DIR}/${safe_filename}.pem" \
+if ! mkcert -cert-file "${CERT_DIR}/${safe_filename}.pem" \
   -key-file "${CERT_DIR}/${safe_filename}-key.pem" \
-  "${domain}"
+  "${domain}"; then
+  log_error "Failed to generate certificates for: ${domain}"
+  return 1
+fi
 
 log_success "Certificates generated successfully:"
Suggestion importance[1-10]: 8

__

Why: An unsuccessful mkcert invocation currently still triggers a success message and apply_certificates, potentially leaving incomplete files while the command appears successful. The proposed change correctly stops and reports the failure.

Medium
Propagate root CA installation failures

Propagate failures from mkcert -install instead of unconditionally returning
success. Otherwise certificate generation continues even when the local CA could not
be installed.

bin/lib/certs.sh [6-10]

 if command -v mkcert >/dev/null 2>&1; then
   log_info "Running mkcert -install to ensure root CA is installed..."
-  mkcert -install
-  return 0
+  if mkcert -install; then
+    return 0
+  fi
+  log_error "Failed to install the mkcert root CA"
+  return 1
 fi
Suggestion importance[1-10]: 7

__

Why: The current branch returns success even if mkcert -install fails, allowing certificate generation to continue without a trusted CA. The proposed guard correctly propagates that failure.

Medium
Validate every scanned certificate

Validate each cert_file before reading its SANs rather than validating only the
first file returned. A corrupt later certificate is currently treated as having no
names, so certs describe can incorrectly report that no certificate covers the
requested hostname.

bin/lib/certs.sh [312-319]

-local first
-first="$(cert_files | head -n 1)"
-if [[ -n "${first}" ]]; then
-  require_openssl "${first}" || return 1
-fi
 while IFS= read -r cert_file; do
   [[ -z "${cert_file}" ]] && continue
+  require_openssl "${cert_file}" || return 1
   while IFS= read -r name; do
Suggestion importance[1-10]: 6

__

Why: Validating only the first certificate allows unreadable later files to be silently interpreted as having no SANs, producing a misleading definitive result. Moving require_openssl into the loop accurately validates every certificate before inspecting it.

Low

Assisted-by: pr-agent/gpt-5.6-sol

… self-contained

The integration suite sources bin/spark-http-proxy up to the first
top-level `case "$1" in` and sources bin/lib/hosts.sh on its own. The
prerequisite check becomes a function so it no longer introduces an
earlier case, and hosts.sh keeps hosts_abbreviate; certs.sh carries its
own copy. The two certificate tests that read function bodies and the
capability probe now look in bin/lib/certs.sh, where they moved.

Refs: #160
Assisted-by: claude-code/claude-fable-5-1
Refs: #160
Assisted-by: claude-code/claude-fable-5-1
… it as empty

Refs: #160
Assisted-by: claude-code/claude-fable-5-1
… when it is named

Refs: #160
Assisted-by: claude-code/claude-fable-5-1
@paolomainardi
paolomainardi merged commit b2de1c7 into main Sep 1, 2026
14 checks passed
@paolomainardi
paolomainardi deleted the feat/160-certs-topic branch September 1, 2026 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant