Feat/native xades signer#35
Conversation
Replace xmlsec1 CLI dependency with Native Elixir implementation using OTP's :crypto and :public_key for ECDSA-SHA256 signing. The xmlsec1 binary is broken on macOS (KEY-NOT-FOUND for all key types). - Add KsefHub.XadesSigner.Native implementing the Behaviour callback - Add AuthTokenRequest.build_body/2 for canonical body XML (digest input) - Switch default signer config from Xmlsec1 to Native - Produces XAdES-BES with QualifyingProperties (SigningTime, SigningCertificate) - Uses openssl CLI only for PKCS12 extraction (existing dependency) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Generate ECDSA P-256 test keypair and PKCS12 in setup - Verify signed XML structure (declaration, challenge, NIP, algorithms) - Verify XAdES QualifyingProperties presence (SigningTime, SigningCertificate) - Verify ECDSA signature against embedded certificate public key - Test error cases: invalid PKCS12 data, wrong password Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Handle PrivateKeyInfo (PKCS8) PEM type from openssl output - Extract raw private key binary for :crypto.sign compatibility - Replace escaped-quote strings with ~s() sigils (credo compliance) - Fix signature verification test to extract ECPoint binary Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
KSeF rejected the signature with error 9105: "Invalid digest value for reference pointing to element '#SignedProps-1'." The issue: Exclusive C14N only renders xmlns:ds on elements that visibly utilize the ds: prefix (i.e., elements named ds:*). The xades:SignedProperties element uses the xades: prefix, so xmlns:ds must NOT appear on it. Instead, each ds:* child element (DigestMethod, DigestValue, X509IssuerName, X509SerialNumber) gets its own xmlns:ds declaration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
KSeF rejected signature with error 9105: "Podpis zweryfikowany negatywnie"
(Signature verified negatively).
:crypto.sign returns DER-encoded ASN.1 SEQUENCE {INTEGER r, INTEGER s},
but XML Digital Signature 1.1 (and KSeF's Java verifier) expects raw r||s
format (IEEE P1363): each value zero-padded to 32 bytes for P-256 (64 bytes
total).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughReplaces the xmlsec1 signer with a new native XAdES-BES signer and canonical AuthTokenRequest body; removes xmlsec1 implementation and Docker xmlsec1 package; updates invoice parsing for namespace-agnostic XPath; extends sync/fetch APIs to propagate failed counts; adds tests for native signer and adjusted sync behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant NativeSigner as Native Signer
participant OpenSSL
participant CryptoAPI as OTP Crypto/PublicKey
participant XMLBuilder as XML Builder
Client->>NativeSigner: sign_challenge(challenge, nip, pkcs12, password)
NativeSigner->>OpenSSL: extract private key & cert from PKCS#12
OpenSSL-->>NativeSigner: PEM key, DER cert
NativeSigner->>CryptoAPI: parse cert metadata, compute digests
NativeSigner->>NativeSigner: build body & SignedProperties digests
NativeSigner->>CryptoAPI: sign SignedInfo (ECDSA-SHA256)
CryptoAPI-->>NativeSigner: DER signature
NativeSigner->>NativeSigner: der -> raw r||s, base64 encode
NativeSigner->>XMLBuilder: assemble ds:Signature + xades:QualifyingProperties
XMLBuilder-->>NativeSigner: signed AuthTokenRequest XML
NativeSigner-->>Client: {:ok, signed_xml}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/ksef_hub/xades_signer/native.ex`:
- Around line 33-36: Add a `@doc` block for the public function sign_challenge/4
describing its purpose, the meanings of the parameters (challenge, nip,
certificate_data, certificate_password), the return values ({:ok, String.t()} |
{:error, term()}), and a short usage example if applicable; place the `@doc`
immediately above the `@spec/def` for sign_challenge so it documents the public
API and satisfies the module's documentation guidelines.
🧹 Nitpick comments (3)
test/ksef_hub/xades_signer/native_test.exs (2)
165-217: Test helper passes password as CLI argument.The
generate_test_pkcs12/0helper passes the password directly as a CLI argument (-passout pass:#{password}) on line 207. While this is test-only code with a hardcoded test password, it's worth noting this pattern differs from the production code which uses file-based password passing per security guidelines.For test code with non-sensitive hardcoded passwords, this is acceptable, but consider using the same pattern as production (
-passout file:...) for consistency if you want tests to exercise the same code paths.
9-13: Consider usingsetup_allfor PKCS12 generation.The
setupcallback regenerates the PKCS12 bundle for every test case. Since all tests use the same test certificate, usingsetup_allwould reduce redundant OpenSSL invocations and speed up the test suite.Suggested optimization
describe "sign_challenge/4" do - setup do + setup_all do {p12_data, password} = generate_test_pkcs12() %{p12_data: p12_data, password: password} endlib/ksef_hub/xades_signer/native.ex (1)
360-368: Duplicatedescape_xml/1helper.This function is identical to the one in
AuthTokenRequest(lines 70-78). Consider extracting to a shared utility module to follow DRY principles.
InvoiceFetcher.fetch_all now returns {ok, count, max_ts, failed_count}
so the sync worker can surface download failures in Oban job metadata.
Previously, individual download errors were silently swallowed, making
completed syncs with 0/0 invoices appear successful in the UI.
Also adds temporary debug logging to capture KSeF v2 response field
names for fixing the ksef_number field mapping.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
KSeF v2 API uses "ksefNumber" (not "ksefReferenceNumber") and "acquisitionDate" (not "acquisitionTimestamp"). This was causing all invoice downloads to 404 because the ksef_number was nil. Also removes temporary debug logging that was added to diagnose the field name mismatch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Some KSeF invoices use namespace prefixes (e.g. n0:Faktura, n0:P_2) instead of bare element names. XPath queries like //P_2 don't match <n0:P_2>. Switch all XPaths to use local-name() function to match elements regardless of namespace prefix. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Delete XadesSigner.Xmlsec1 module (replaced by Native) - Remove build/2 signature template from AuthTokenRequest - Remove xmlsec1 from Dockerfile runtime dependencies - Update fallback default in Auth to Native - Update docs and test error tuples Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/ksef_hub/invoices/parser.ex (2)
13-36: 🛠️ Refactor suggestion | 🟠 MajorMissing
@specforparse/1function.As per coding guidelines, every function must have an
@spectype specification. The publicparse/1function is missing its typespec.📝 Proposed fix
+ `@spec` parse(binary()) :: {:ok, map()} | {:error, {:invalid_xml, String.t()}} def parse(xml_string) when is_binary(xml_string) do
42-48: 🛠️ Refactor suggestion | 🟠 MajorMissing
@specfordetermine_type/2function.As per coding guidelines, every function must have an
@spectype specification.📝 Proposed fix
+ `@spec` determine_type(map(), String.t()) :: String.t() def determine_type(parsed_invoice, our_nip) do
🤖 Fix all issues with AI agents
In `@lib/ksef_hub/invoices/parser.ex`:
- Around line 52-63: Add `@spec` type specifications for each private function
listed: extract_name/2, parse_line_items/1, parse_date/1, parse_decimal/1,
parse_integer/1, and default_currency/1. For example, declare extract_name/2 as
returning String.t() and taking SweetXml.xmlElement() and String.t();
parse_line_items/1 as returning list() or [map()]; parse_date/1 as returning
Date.t() | nil; parse_decimal/1 as Decimal.t() | nil; parse_integer/1 as
integer() | nil; and default_currency/1 as String.t(). Place each `@spec`
immediately above its defp so Dialyzer/Type checks pick up the contracts and
ensure the empty-string clause signatures match (e.g., parse_date("") -> nil,
parse_decimal("") -> nil, parse_integer("") -> nil, default_currency("") ->
"PLN").
- Around line 59-61: The three-expression construction using xpath and
String.trim in lib/ksef_hub/invoices/parser.ex is a single long line that fails
mix format; split it into multiple lines so formatting is clean—keep the xpath
calls for first and last as-is (variables first and last), then combine and trim
using a separate expression (for example pipe the interpolated string into
String.trim/1 or assign to an intermediate variable) so the code around
xpath(...), first, last and String.trim is broken across lines and passes mix
format.
In `@lib/ksef_hub/sync/invoice_fetcher.ex`:
- Around line 30-54: The private functions do_fetch/6, handle_next_action/7, and
process_invoices/5 are missing `@specs`; add appropriate `@spec` annotations above
each function matching their parameters and return shapes (e.g., context
struct/type, from/date, integer page_offset, count/failure accumulators, max_ts
timestamps) and the returned tuple ({:ok, count, max_ts, failed} or equivalent)
to satisfy the project's requirement that every function has a type spec; ensure
the specs align with the actual types used in
ksef_client().query_invoice_metadata results and the tuples returned by these
functions and add similar `@specs` for the other modified private functions noted
at lines 74-94 and 96-109 (use the same style and concrete types as surrounding
module specs).
In `@lib/ksef_hub/sync/sync_worker.ex`:
- Around line 94-115: Add `@spec` annotations for the changed private functions
sync_all_types/4 and sync_type/4 (and any other modified private functions
around the indicated region) to satisfy the lib/**/*.ex spec requirement: locate
the function definitions for sync_all_types/4 and sync_type/4 and add
appropriate `@spec` lines above each (matching their argument and return types
used in the implementation — e.g., types for company_id, job/message structs or
maps, options, and the return tuple such as {:ok, ...} | {:error, ...}); ensure
private functions use `@spec` with when clauses if needed and that the specs
reflect the actual return shapes used in functions called like store_meta/2 and
broadcast_sync_completed/2.
🧹 Nitpick comments (1)
lib/ksef_hub/sync/sync_worker.ex (1)
118-142: Propagate failed-download counts on partial success.The successful side’s failed count (
_if/_ef) is currently dropped, which loses useful failure info in logs/meta.Suggested tweak
- {{:ok, ic, _if}, {:error, reason}} -> + {{:ok, ic, if_failed}, {:error, reason}} -> Logger.error( "Expense sync failed for company #{company_id}: #{inspect(reason)} (#{ic} income invoices synced)" ) store_meta(job, %{ "income_count" => ic, + "income_failed" => if_failed, "error" => inspect(reason), "failed_type" => "expense" }) - {:ok, :partial, %{succeeded: :income, failed: {:expense, reason}}} + {:ok, :partial, %{succeeded: :income, failed: {:expense, reason}, income_failed: if_failed}} - {{:error, reason}, {:ok, ec, _ef}} -> + {{:error, reason}, {:ok, ec, ef_failed}} -> Logger.error( "Income sync failed for company #{company_id}: #{inspect(reason)} (#{ec} expense invoices synced)" ) store_meta(job, %{ "expense_count" => ec, + "expense_failed" => ef_failed, "error" => inspect(reason), "failed_type" => "income" }) - {:ok, :partial, %{succeeded: :expense, failed: {:income, reason}}} + {:ok, :partial, %{succeeded: :expense, failed: {:income, reason}, expense_failed: ef_failed}}
- Use setup_all for PKCS12 generation (runs once, not per test) - Use file-based password passing in test helper (matches production) - Extract shared escape_xml/1 to AuthTokenRequest, defdelegate in Native - Add @SPEC to Parser.parse/1 and Parser.determine_type/2 - Propagate failed-download counts on partial sync success Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Satisfies project guideline requiring @SPEC on every function. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ksef_hub/sync/sync_worker.ex (1)
199-209:⚠️ Potential issue | 🟠 MajorAvoid advancing checkpoints when
failed> 0.If downloads fail but the checkpoint advances to
max_timestamp, invoices older than the overlap window can be skipped permanently. Consider short‑circuiting to an error or holding the checkpoint whenfailed > 0to ensure retry.💡 Suggested guard
- {:ok, count, max_timestamp, failed} -> + {:ok, count, max_timestamp, failed} when failed > 0 -> + {:error, {:download_failed, failed}} + + {:ok, count, max_timestamp, failed} -> case Checkpoints.advance(type, company_id, max_timestamp) do {:ok, _checkpoint} -> {:ok, count, failed}
🤖 Fix all issues with AI agents
In `@lib/ksef_hub/sync/sync_worker.ex`:
- Around line 101-123: The code treats any non‑zero failed downloads as a full
sync by returning {:ok, :full}; change the return to indicate a partial sync
when failures exist so callers (like update_last_sync/1) can react: after
computing total_failed and storing meta (Map.put "error" when total_failed>0),
return {:ok, :partial} instead of {:ok, :full} when total_failed > 0, otherwise
keep {:ok, :full}; keep calls to store_meta(job, meta) and
broadcast_sync_completed(company_id, %{income: ic, expense: ec}) unchanged.
In `@lib/ksef_hub/xades_signer/native.ex`:
- Around line 139-163: The PKCS8 branch in parse_ec_private_key currently passes
the decoded :PrivateKeyInfo record straight to extract_ec_private_key_binary/1
which only matches {:ECPrivateKey,...}; update parse_ec_private_key/1 so that
when it matches [{:PrivateKeyInfo, der, :not_encrypted} | _] you first decode
the PrivateKeyInfo via :public_key.der_decode(:PrivateKeyInfo, der), extract the
contained privateKey DER binary (the privateKey field of the PrivateKeyInfo
tuple), then der_decode that privateKey as an :ECPrivateKey (via
:public_key.der_decode(:ECPrivateKey, private_key_der)) and finally call
extract_ec_private_key_binary/1 with the resulting ECPrivateKey tuple; retain
the existing error branches for unsupported types and missing keys.
In `@test/ksef_hub/xades_signer/native_test.exs`:
- Around line 1-225: This file (module KsefHub.XadesSigner.NativeTest) failed
mix format; run mix format on the project or on this file and commit the changes
so the tests and helpers (e.g., der_integer/1, generate_test_pkcs12/0, and the
describe "sign_challenge/4" tests) are properly formatted; after formatting,
re-run mix format --check-formatted to confirm the CI error is resolved.
🧹 Nitpick comments (1)
test/ksef_hub/xades_signer/native_test.exs (1)
218-223: Consider secure cleanup for sensitive temp files.While this is test code, the
afterblock deletes sensitive files (private key, password) without overwriting them with zeros first. For consistency with production practices and to establish good habits:♻️ Suggested secure cleanup
after + # Overwrite sensitive files before deletion + for path <- [key_path, pass_path] do + if File.exists?(path) do + size = File.stat!(path).size + File.write!(path, :binary.copy(<<0>>, size)) + end + end + File.rm(key_path) File.rm(cert_path) File.rm(p12_path) File.rm(pass_path) endBased on learnings: "overwrite temp files with zeros before deletion" for files containing sensitive data like passwords and private keys.
| {{:ok, ic, if_}, {:ok, ec, ef}} -> | ||
| total_failed = if_ + ef | ||
|
|
||
| if total_failed > 0 do | ||
| Logger.warning( | ||
| "Sync complete for company #{company_id}: #{ic} income, #{ec} expense invoices (#{total_failed} failed downloads)" | ||
| ) | ||
| else | ||
| Logger.info( | ||
| "Sync complete for company #{company_id}: #{ic} income, #{ec} expense invoices" | ||
| ) | ||
| end | ||
|
|
||
| meta = %{"income_count" => ic, "expense_count" => ec} | ||
|
|
||
| store_meta(job, %{"income_count" => ic, "expense_count" => ec}) | ||
| meta = | ||
| if total_failed > 0, | ||
| do: Map.put(meta, "error", "#{total_failed} invoice downloads failed"), | ||
| else: meta | ||
|
|
||
| store_meta(job, meta) | ||
| broadcast_sync_completed(company_id, %{income: ic, expense: ec}) | ||
| {:ok, :full} |
There was a problem hiding this comment.
Treat non‑zero failed downloads as a partial sync.
total_failed > 0 still returns {:ok, :full}, which triggers update_last_sync/1 and can mask partial failures. Consider returning :partial when failures exist so callers can react.
💡 Suggested adjustment
- store_meta(job, meta)
- broadcast_sync_completed(company_id, %{income: ic, expense: ec})
- {:ok, :full}
+ store_meta(job, meta)
+ broadcast_sync_completed(company_id, %{income: ic, expense: ec})
+ if total_failed > 0 do
+ {:ok, :partial, %{failed: total_failed}}
+ else
+ {:ok, :full}
+ end📝 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.
| {{:ok, ic, if_}, {:ok, ec, ef}} -> | |
| total_failed = if_ + ef | |
| if total_failed > 0 do | |
| Logger.warning( | |
| "Sync complete for company #{company_id}: #{ic} income, #{ec} expense invoices (#{total_failed} failed downloads)" | |
| ) | |
| else | |
| Logger.info( | |
| "Sync complete for company #{company_id}: #{ic} income, #{ec} expense invoices" | |
| ) | |
| end | |
| meta = %{"income_count" => ic, "expense_count" => ec} | |
| store_meta(job, %{"income_count" => ic, "expense_count" => ec}) | |
| meta = | |
| if total_failed > 0, | |
| do: Map.put(meta, "error", "#{total_failed} invoice downloads failed"), | |
| else: meta | |
| store_meta(job, meta) | |
| broadcast_sync_completed(company_id, %{income: ic, expense: ec}) | |
| {:ok, :full} | |
| {{:ok, ic, if_}, {:ok, ec, ef}} -> | |
| total_failed = if_ + ef | |
| if total_failed > 0 do | |
| Logger.warning( | |
| "Sync complete for company #{company_id}: #{ic} income, #{ec} expense invoices (#{total_failed} failed downloads)" | |
| ) | |
| else | |
| Logger.info( | |
| "Sync complete for company #{company_id}: #{ic} income, #{ec} expense invoices" | |
| ) | |
| end | |
| meta = %{"income_count" => ic, "expense_count" => ec} | |
| meta = | |
| if total_failed > 0, | |
| do: Map.put(meta, "error", "#{total_failed} invoice downloads failed"), | |
| else: meta | |
| store_meta(job, meta) | |
| broadcast_sync_completed(company_id, %{income: ic, expense: ec}) | |
| if total_failed > 0 do | |
| {:ok, :partial, %{failed: total_failed}} | |
| else | |
| {:ok, :full} | |
| end |
🤖 Prompt for AI Agents
In `@lib/ksef_hub/sync/sync_worker.ex` around lines 101 - 123, The code treats any
non‑zero failed downloads as a full sync by returning {:ok, :full}; change the
return to indicate a partial sync when failures exist so callers (like
update_last_sync/1) can react: after computing total_failed and storing meta
(Map.put "error" when total_failed>0), return {:ok, :partial} instead of {:ok,
:full} when total_failed > 0, otherwise keep {:ok, :full}; keep calls to
store_meta(job, meta) and broadcast_sync_completed(company_id, %{income: ic,
expense: ec}) unchanged.
| @spec parse_ec_private_key(String.t()) :: {:ok, binary()} | {:error, term()} | ||
| defp parse_ec_private_key(pem) do | ||
| case :public_key.pem_decode(pem) do | ||
| [{:ECPrivateKey, der, :not_encrypted} | _] -> | ||
| extract_ec_private_key_binary(:public_key.der_decode(:ECPrivateKey, der)) | ||
|
|
||
| [{:PrivateKeyInfo, der, :not_encrypted} | _] -> | ||
| # PKCS8-wrapped key (openssl outputs this for EC keys) | ||
| extract_ec_private_key_binary(:public_key.der_decode(:PrivateKeyInfo, der)) | ||
|
|
||
| [{type, _der, _enc} | _] -> | ||
| {:error, {:unsupported_key_type, type}} | ||
|
|
||
| _ -> | ||
| {:error, :no_private_key_found} | ||
| end | ||
| end | ||
|
|
||
| # :crypto.sign requires the raw private key binary, not the ECPrivateKey record | ||
| @spec extract_ec_private_key_binary(term()) :: {:ok, binary()} | {:error, term()} | ||
| defp extract_ec_private_key_binary({:ECPrivateKey, _, priv_key_bin, _, _, _}) do | ||
| {:ok, priv_key_bin} | ||
| end | ||
|
|
||
| defp extract_ec_private_key_binary(_), do: {:error, :not_ec_key} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Erlang OTP public_key der_decode PrivateKeyInfo PKCS8 EC private key
💡 Result:
In Erlang/OTP, a PKCS#8 EC private key is typically a DER PrivateKeyInfo (or the newer OneAsymmetricKey) that wraps an inner DER ECPrivateKey.
Decode flow (DER PKCS#8 → #'ECPrivateKey'{})
-include_lib("public_key/include/public_key.hrl").
%% Der = binary() containing PKCS#8 DER (PrivateKeyInfo)
P8 = public_key:der_decode('PrivateKeyInfo', Der),
%% PrivateKeyInfo.privateKey is an OCTET STRING that contains (for EC) a DER ECPrivateKey
EC = public_key:der_decode('ECPrivateKey', P8#'PrivateKeyInfo'.privateKey).der_decode/2 supports ASN.1 types including 'PrivateKeyInfo', 'ECPrivateKey', and 'OneAsymmetricKey'. [1]
The decoded EC key is the record #'ECPrivateKey'{version, privateKey, parameters, publicKey}. [2]
If you also need the curve (namedCurve OID) from PKCS#8
The curve is in PrivateKeyInfo.privateKeyAlgorithm.parameters (encoded), which you can decode as 'EcpkParameters':
Alg = P8#'PrivateKeyInfo'.privateKeyAlgorithm,
Params = public_key:der_decode('EcpkParameters', Alg#'AlgorithmIdentifier'.parameters).
%% Params is typically {namedCurve, Oid}Practical note (PEM)
If your input is PEM (-----BEGIN PRIVATE KEY-----), you can do:
public_key:pem_decode/1 then public_key:pem_entry_decode/1 to get a decoded structure. [1]
Sources:
[1] Erlang/OTP public_key docs (types + der_decode/2, PEM decode APIs). (erlang.org)
[2] Erlang/OTP Public-Key Records (#'ECPrivateKey'{...} fields). (erlang.org)
Citations:
- 1: https://www.erlang.org/doc/apps/public_key/public_key.html
- 2: https://www.erlang.org/doc/apps/public_key/public_key_records
🏁 Script executed:
# Check how public_key module is used in the codebase and verify record structures
rg "public_key\.der_decode|PrivateKeyInfo" lib/ksef_hub/xades_signer/ -A 2 -B 2Repository: appunite/appunite-ksef-ex
Length of output: 803
🏁 Script executed:
# Check if there are tests for this functionality
fd -e "test.exs" | xargs rg "parse_ec_private_key|PrivateKeyInfo" -lRepository: appunite/appunite-ksef-ex
Length of output: 104
PKCS8 PrivateKeyInfo decoding fails because the structure doesn't match extract_ec_private_key_binary/1's pattern.
When pem_decode returns {:PrivateKeyInfo, ...}, the code passes the decoded PrivateKeyInfo record directly to extract_ec_private_key_binary/1, which only matches {:ECPrivateKey, ...} tuples. Per Erlang/OTP documentation, the PrivateKeyInfo record contains the EC key in its privateKey field as a DER-encoded binary, requiring an additional decode step. This causes the function to return {:error, :not_ec_key} for all PKCS8-formatted keys (BEGIN PRIVATE KEY), breaking certificate signing.
Suggested fix
[{:PrivateKeyInfo, der, :not_encrypted} | _] ->
# PKCS8-wrapped key (openssl outputs this for EC keys)
- extract_ec_private_key_binary(:public_key.der_decode(:PrivateKeyInfo, der))
+ case :public_key.der_decode(:PrivateKeyInfo, der) do
+ {:PrivateKeyInfo, _v, _alg, priv_der, _attrs} ->
+ extract_ec_private_key_binary(:public_key.der_decode(:ECPrivateKey, priv_der))
+ _ ->
+ {:error, :unsupported_key_type}
+ end📝 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.
| @spec parse_ec_private_key(String.t()) :: {:ok, binary()} | {:error, term()} | |
| defp parse_ec_private_key(pem) do | |
| case :public_key.pem_decode(pem) do | |
| [{:ECPrivateKey, der, :not_encrypted} | _] -> | |
| extract_ec_private_key_binary(:public_key.der_decode(:ECPrivateKey, der)) | |
| [{:PrivateKeyInfo, der, :not_encrypted} | _] -> | |
| # PKCS8-wrapped key (openssl outputs this for EC keys) | |
| extract_ec_private_key_binary(:public_key.der_decode(:PrivateKeyInfo, der)) | |
| [{type, _der, _enc} | _] -> | |
| {:error, {:unsupported_key_type, type}} | |
| _ -> | |
| {:error, :no_private_key_found} | |
| end | |
| end | |
| # :crypto.sign requires the raw private key binary, not the ECPrivateKey record | |
| @spec extract_ec_private_key_binary(term()) :: {:ok, binary()} | {:error, term()} | |
| defp extract_ec_private_key_binary({:ECPrivateKey, _, priv_key_bin, _, _, _}) do | |
| {:ok, priv_key_bin} | |
| end | |
| defp extract_ec_private_key_binary(_), do: {:error, :not_ec_key} | |
| `@spec` parse_ec_private_key(String.t()) :: {:ok, binary()} | {:error, term()} | |
| defp parse_ec_private_key(pem) do | |
| case :public_key.pem_decode(pem) do | |
| [{:ECPrivateKey, der, :not_encrypted} | _] -> | |
| extract_ec_private_key_binary(:public_key.der_decode(:ECPrivateKey, der)) | |
| [{:PrivateKeyInfo, der, :not_encrypted} | _] -> | |
| # PKCS8-wrapped key (openssl outputs this for EC keys) | |
| case :public_key.der_decode(:PrivateKeyInfo, der) do | |
| {:PrivateKeyInfo, _v, _alg, priv_der, _attrs} -> | |
| extract_ec_private_key_binary(:public_key.der_decode(:ECPrivateKey, priv_der)) | |
| _ -> | |
| {:error, :unsupported_key_type} | |
| end | |
| [{type, _der, _enc} | _] -> | |
| {:error, {:unsupported_key_type, type}} | |
| _ -> | |
| {:error, :no_private_key_found} | |
| end | |
| end | |
| # :crypto.sign requires the raw private key binary, not the ECPrivateKey record | |
| `@spec` extract_ec_private_key_binary(term()) :: {:ok, binary()} | {:error, term()} | |
| defp extract_ec_private_key_binary({:ECPrivateKey, _, priv_key_bin, _, _, _}) do | |
| {:ok, priv_key_bin} | |
| end | |
| defp extract_ec_private_key_binary(_), do: {:error, :not_ec_key} |
🤖 Prompt for AI Agents
In `@lib/ksef_hub/xades_signer/native.ex` around lines 139 - 163, The PKCS8 branch
in parse_ec_private_key currently passes the decoded :PrivateKeyInfo record
straight to extract_ec_private_key_binary/1 which only matches
{:ECPrivateKey,...}; update parse_ec_private_key/1 so that when it matches
[{:PrivateKeyInfo, der, :not_encrypted} | _] you first decode the PrivateKeyInfo
via :public_key.der_decode(:PrivateKeyInfo, der), extract the contained
privateKey DER binary (the privateKey field of the PrivateKeyInfo tuple), then
der_decode that privateKey as an :ECPrivateKey (via
:public_key.der_decode(:ECPrivateKey, private_key_der)) and finally call
extract_ec_private_key_binary/1 with the resulting ECPrivateKey tuple; retain
the existing error branches for unsupported types and missing keys.
| defmodule KsefHub.XadesSigner.NativeTest do | ||
| use ExUnit.Case, async: true | ||
|
|
||
| alias KsefHub.XadesSigner.Native | ||
|
|
||
| @challenge "20250211-test-challenge-abc123" | ||
| @nip "1234567890" | ||
|
|
||
| setup_all do | ||
| {p12_data, password} = generate_test_pkcs12() | ||
| %{p12_data: p12_data, password: password} | ||
| end | ||
|
|
||
| describe "sign_challenge/4" do | ||
|
|
||
| test "returns {:ok, signed_xml} with valid PKCS12", %{p12_data: p12_data, password: password} do | ||
| assert {:ok, signed_xml} = Native.sign_challenge(@challenge, @nip, p12_data, password) | ||
| assert is_binary(signed_xml) | ||
| end | ||
|
|
||
| test "signed XML contains XML declaration", %{p12_data: p12_data, password: password} do | ||
| {:ok, signed_xml} = Native.sign_challenge(@challenge, @nip, p12_data, password) | ||
| assert String.starts_with?(signed_xml, "<?xml version=\"1.0\" encoding=\"UTF-8\"?>") | ||
| end | ||
|
|
||
| test "signed XML contains AuthTokenRequest with challenge and NIP", %{ | ||
| p12_data: p12_data, | ||
| password: password | ||
| } do | ||
| {:ok, signed_xml} = Native.sign_challenge(@challenge, @nip, p12_data, password) | ||
| assert signed_xml =~ "<Challenge>#{@challenge}</Challenge>" | ||
| assert signed_xml =~ "<Nip>#{@nip}</Nip>" | ||
| assert signed_xml =~ "<SubjectIdentifierType>certificateSubject</SubjectIdentifierType>" | ||
| end | ||
|
|
||
| test "signed XML contains ds:Signature with SignatureValue", %{ | ||
| p12_data: p12_data, | ||
| password: password | ||
| } do | ||
| {:ok, signed_xml} = Native.sign_challenge(@challenge, @nip, p12_data, password) | ||
| assert signed_xml =~ "<ds:Signature" | ||
| assert signed_xml =~ "<ds:SignatureValue>" | ||
| assert signed_xml =~ "</ds:SignatureValue>" | ||
|
|
||
| # SignatureValue should be non-empty Base64 | ||
| [_, sig_value] = Regex.run(~r/<ds:SignatureValue>([^<]+)</, signed_xml) | ||
| assert {:ok, _} = Base.decode64(sig_value) | ||
| assert byte_size(sig_value) > 0 | ||
| end | ||
|
|
||
| test "signed XML contains X509Certificate", %{p12_data: p12_data, password: password} do | ||
| {:ok, signed_xml} = Native.sign_challenge(@challenge, @nip, p12_data, password) | ||
| assert signed_xml =~ "<ds:X509Certificate>" | ||
|
|
||
| [_, cert_b64] = Regex.run(~r/<ds:X509Certificate>([^<]+)</, signed_xml) | ||
| assert {:ok, cert_der} = Base.decode64(cert_b64) | ||
| # Should be a valid DER certificate | ||
| assert {:OTPCertificate, _, _, _} = :public_key.pkix_decode_cert(cert_der, :otp) | ||
| end | ||
|
|
||
| test "signed XML contains XAdES QualifyingProperties", %{ | ||
| p12_data: p12_data, | ||
| password: password | ||
| } do | ||
| {:ok, signed_xml} = Native.sign_challenge(@challenge, @nip, p12_data, password) | ||
| assert signed_xml =~ "<xades:QualifyingProperties" | ||
| assert signed_xml =~ "<xades:SignedProperties" | ||
| assert signed_xml =~ "Id=\"SignedProps-1\"" | ||
| assert signed_xml =~ "<xades:SigningTime>" | ||
| assert signed_xml =~ "<xades:SigningCertificate>" | ||
| assert signed_xml =~ "<xades:CertDigest>" | ||
| assert signed_xml =~ "<xades:IssuerSerial>" | ||
| end | ||
|
|
||
| test "signed XML contains correct algorithm URIs", %{ | ||
| p12_data: p12_data, | ||
| password: password | ||
| } do | ||
| {:ok, signed_xml} = Native.sign_challenge(@challenge, @nip, p12_data, password) | ||
|
|
||
| # Exclusive C14N | ||
| assert signed_xml =~ "http://www.w3.org/2001/10/xml-exc-c14n#" | ||
| # ECDSA-SHA256 | ||
| assert signed_xml =~ "http://www.w3.org/2001/04/xmldsig-more#ecdsa-sha256" | ||
| # SHA-256 digest | ||
| assert signed_xml =~ "http://www.w3.org/2001/04/xmlenc#sha256" | ||
| # Enveloped signature transform | ||
| assert signed_xml =~ "http://www.w3.org/2000/09/xmldsig#enveloped-signature" | ||
| end | ||
|
|
||
| test "signed XML has two references in SignedInfo", %{ | ||
| p12_data: p12_data, | ||
| password: password | ||
| } do | ||
| {:ok, signed_xml} = Native.sign_challenge(@challenge, @nip, p12_data, password) | ||
|
|
||
| # Reference URI="" (document) | ||
| assert signed_xml =~ "<ds:Reference URI=\"\">" | ||
| # Reference URI="#SignedProps-1" (signed properties) | ||
| assert signed_xml =~ "URI=\"#SignedProps-1\"" | ||
| end | ||
|
|
||
| test "signature is verifiable with the embedded certificate", %{ | ||
| p12_data: p12_data, | ||
| password: password | ||
| } do | ||
| {:ok, signed_xml} = Native.sign_challenge(@challenge, @nip, p12_data, password) | ||
|
|
||
| # Extract SignatureValue (raw r||s format, 64 bytes for P-256) | ||
| [_, sig_b64] = Regex.run(~r/<ds:SignatureValue>([^<]+)</, signed_xml) | ||
| signature_raw = Base.decode64!(sig_b64) | ||
| assert byte_size(signature_raw) == 64 | ||
|
|
||
| # Convert r||s back to DER for :crypto.verify | ||
| <<r::binary-size(32), s::binary-size(32)>> = signature_raw | ||
| r_der = der_integer(r) | ||
| s_der = der_integer(s) | ||
| seq_content = r_der <> s_der | ||
| signature_der = <<0x30, byte_size(seq_content)::8>> <> seq_content | ||
|
|
||
| # Extract certificate and get public key point (uncompressed EC point) | ||
| [_, cert_b64] = Regex.run(~r/<ds:X509Certificate>([^<]+)</, signed_xml) | ||
| cert_der = Base.decode64!(cert_b64) | ||
| cert = :public_key.pkix_decode_cert(cert_der, :otp) | ||
|
|
||
| {:OTPCertificate, | ||
| {:OTPTBSCertificate, _, _, _, _, _, _, | ||
| {:OTPSubjectPublicKeyInfo, _, {:ECPoint, ec_point_bin}}, _, _, _}, _, _} = cert | ||
|
|
||
| # Reconstruct canonical SignedInfo (with xmlns:ds on element) | ||
| [_, signed_info_content] = | ||
| Regex.run(~r/<ds:SignedInfo>(.*?)<\/ds:SignedInfo>/s, signed_xml) | ||
|
|
||
| signed_info_xml = | ||
| "<ds:SignedInfo xmlns:ds=\"http://www.w3.org/2000/09/xmldsig#\">" <> | ||
| signed_info_content <> "</ds:SignedInfo>" | ||
|
|
||
| # :crypto.verify expects [ec_point_binary, :secp256r1] for ECDSA | ||
| assert :crypto.verify(:ecdsa, :sha256, signed_info_xml, signature_der, [ | ||
| ec_point_bin, | ||
| :secp256r1 | ||
| ]) | ||
| end | ||
|
|
||
| test "returns error for invalid PKCS12 data" do | ||
| assert {:error, _} = Native.sign_challenge(@challenge, @nip, "not-a-pkcs12", "password") | ||
| end | ||
|
|
||
| test "returns error for wrong password", %{p12_data: p12_data} do | ||
| assert {:error, _} = Native.sign_challenge(@challenge, @nip, p12_data, "wrong-password") | ||
| end | ||
| end | ||
|
|
||
| # Encode a raw big-endian integer as ASN.1 DER INTEGER | ||
| @spec der_integer(binary()) :: binary() | ||
| defp der_integer(bytes) do | ||
| # Strip leading zeros | ||
| trimmed = String.trim_leading(bytes, <<0>>) | ||
| trimmed = if trimmed == "", do: <<0>>, else: trimmed | ||
|
|
||
| # Add leading 0x00 if high bit is set (ASN.1 positive integer) | ||
| padded = if :binary.first(trimmed) >= 128, do: <<0>> <> trimmed, else: trimmed | ||
| <<0x02, byte_size(padded)::8>> <> padded | ||
| end | ||
|
|
||
| # Generate a self-signed ECDSA P-256 certificate and PKCS12 bundle for testing | ||
| @spec generate_test_pkcs12() :: {binary(), String.t()} | ||
| defp generate_test_pkcs12 do | ||
| password = "test-password-123" | ||
| tmp_dir = System.tmp_dir!() | ||
| rand = :rand.uniform(999_999) | ||
| key_path = Path.join(tmp_dir, "ksef_test_#{rand}_key.pem") | ||
| cert_path = Path.join(tmp_dir, "ksef_test_#{rand}_cert.pem") | ||
| p12_path = Path.join(tmp_dir, "ksef_test_#{rand}_cert.p12") | ||
| pass_path = Path.join(tmp_dir, "ksef_test_#{rand}_pass.txt") | ||
|
|
||
| try do | ||
| File.write!(pass_path, password) | ||
| File.chmod!(pass_path, 0o600) | ||
|
|
||
| # Generate EC P-256 private key | ||
| {_, 0} = | ||
| System.cmd("openssl", ["ecparam", "-genkey", "-name", "prime256v1", "-out", key_path]) | ||
|
|
||
| # Generate self-signed certificate | ||
| {_, 0} = | ||
| System.cmd("openssl", [ | ||
| "req", | ||
| "-new", | ||
| "-x509", | ||
| "-key", | ||
| key_path, | ||
| "-out", | ||
| cert_path, | ||
| "-days", | ||
| "1", | ||
| "-subj", | ||
| "/CN=Test KSeF/O=Test Org/C=PL" | ||
| ]) | ||
|
|
||
| # Create PKCS12 bundle (file-based password, consistent with production) | ||
| {_, 0} = | ||
| System.cmd("openssl", [ | ||
| "pkcs12", | ||
| "-export", | ||
| "-in", | ||
| cert_path, | ||
| "-inkey", | ||
| key_path, | ||
| "-out", | ||
| p12_path, | ||
| "-passout", | ||
| "file:#{pass_path}" | ||
| ]) | ||
|
|
||
| p12_data = File.read!(p12_path) | ||
| {p12_data, password} | ||
| after | ||
| File.rm(key_path) | ||
| File.rm(cert_path) | ||
| File.rm(p12_path) | ||
| File.rm(pass_path) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Fix formatting issues flagged by CI.
The pipeline indicates this file failed the mix format --check-formatted check. Run mix format to fix the formatting issues before merging.
🧰 Tools
🪛 GitHub Actions: CI
[error] 13-16: Mix format failed: --check-formatted. The following file is not formatted.
🤖 Prompt for AI Agents
In `@test/ksef_hub/xades_signer/native_test.exs` around lines 1 - 225, This file
(module KsefHub.XadesSigner.NativeTest) failed mix format; run mix format on the
project or on this file and commit the changes so the tests and helpers (e.g.,
der_integer/1, generate_test_pkcs12/0, and the describe "sign_challenge/4"
tests) are properly formatted; after formatting, re-run mix format
--check-formatted to confirm the CI error is resolved.
- sync_type: skip checkpoint advance when failed > 0 to prevent
permanently skipping invoices outside the overlap window on retry
- sync_all_types: return {:ok, :partial} when downloads fail so
handle_sync_result doesn't update last_sync_at
- Extract handle_both_succeeded/6 to reduce cyclomatic complexity
- Test helper: zero-overwrite sensitive temp files before deletion
- Clarify PrivateKeyInfo comment (der_decode returns ECPrivateKey directly)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary by CodeRabbit
New Features
Configuration
Improvements
Tests
Chores