Feat/key crt upload#27
Conversation
Document the decision to add server-side PKCS12 conversion from separate .key + .crt files, making certificate upload accessible to non-technical users. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move write, path, and delete helpers into KsefHub.SecureTemp module for reuse by the upcoming Pkcs12Converter. Remove unused :timeout option from System.cmd call (not supported in Elixir 1.18). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Define convert/3 callback for key+crt to PKCS12 conversion. Generate test certificates (unencrypted key, encrypted key, mismatched key) for converter tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Convert .key + .crt to .p12 via openssl CLI with secure temp files. Generates random 32-byte password for the output PKCS12 bundle. Passes passwords via file (not CLI args) to avoid ps exposure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add :pkcs12_converter config key with Openssl as default and Mock for tests. Define Mox mock for the Pkcs12Converter.Behaviour. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add upload_mode toggle (:p12 | :key_crt) with conditional form sections. In key_crt mode, upload separate .key and .crt files, call Pkcs12Converter, then reuse existing save_credential flow. Extract consume_single_upload/2 for shared upload consumption. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add upload_mode toggle (:p12 | :key_crt) with conditional form sections. In key_crt mode, upload separate .key and .crt files, call Pkcs12Converter, then reuse existing save_credential flow. Register all uploads in mount (accept :any for key/crt) and toggle visibility via upload_mode assign. Add tests for mode toggle, successful conversion, converter errors, missing files, and passphrase forwarding. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds server-side PKCS#12 conversion for separate .key + .crt uploads, a SecureTemp utility, a Pkcs12Converter behaviour plus an OpenSSL implementation, LiveView UI/flow changes to support dual upload modes, configuration and mocks, tests, and fixtures. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant LiveView as CertificateLive
participant Converter as Pkcs12Converter.Openssl
participant SecureTemp as SecureTemp
participant OpenSSL as OpenSSL
participant FS as File System
participant DB as Database
User->>LiveView: Upload key + crt files (key_crt mode)
LiveView->>LiveView: Read private key & certificate
LiveView->>Converter: convert(key_data, crt_data, passphrase)
Converter->>SecureTemp: write(key_data, ".key")
SecureTemp->>FS: Create file (0600 perms)
SecureTemp-->>Converter: key_path
Converter->>SecureTemp: write(crt_data, ".crt")
SecureTemp->>FS: Create file (0600 perms)
SecureTemp-->>Converter: crt_path
Converter->>SecureTemp: write(password, ".pass")
SecureTemp->>FS: Create file (0600 perms)
SecureTemp-->>Converter: pass_path
Converter->>OpenSSL: Execute pkcs12 -export (reads files, writes p12)
OpenSSL->>FS: Read key/crt/pass files
OpenSSL->>FS: Create p12 file
OpenSSL-->>Converter: Success
Converter->>FS: Read p12 file content
FS-->>Converter: p12 binary
Converter->>SecureTemp: delete(key_path)
Converter->>SecureTemp: delete(crt_path)
Converter->>SecureTemp: delete(pass_path)
Converter->>SecureTemp: delete(p12_path)
SecureTemp->>FS: Overwrite with zeros, remove files
Converter-->>LiveView: {:ok, %{p12_data, p12_password}}
LiveView->>DB: save_credential(p12_data, p12_password)
DB-->>LiveView: Success
LiveView-->>User: Credential saved
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/ksef_hub/xades_signer/xmlsec1.ex (1)
36-43:⚠️ Potential issue | 🟠 MajorMissing timeout on
System.cmdcall.The
timeoutoption was removed from theSystem.cmdcall. Per project learnings, xmlsec1 calls should have a 30-second timeout to prevent indefinite hangs.🐛 Proposed fix: restore timeout
- case System.cmd("xmlsec1", args, stderr_to_stdout: true) do + case System.cmd("xmlsec1", args, stderr_to_stdout: true, timeout: 30_000) doBased on learnings: "When calling xmlsec1 for XADES signing: ... apply 30-second timeout on System.cmd calls"
lib/ksef_hub_web/live/certificate_live.ex (2)
274-373:⚠️ Potential issue | 🟠 MajorUse
<.form for={@Form}>component instead of plain<form>element and add a unique DOM ID.The
@formis correctly assigned viato_form/2, but the template must use the<.form>Phoenix component with theidattribute for proper LiveView form binding and event handling.✅ Suggested fix
- <form phx-submit="save" phx-change="validate" class="space-y-4"> + <.form for={`@form`} phx-submit="save" phx-change="validate" class="space-y-4" id="certificate-upload"> ... - </form> + </.form>
14-63:⚠️ Potential issue | 🟠 MajorAdd
@docand@specfor public functions.The public LiveView callbacks
mount/3,handle_event/3, andrender/1lack required documentation and type specifications. Each must include@docdescribing the function's purpose and@specspecifying parameter and return types.✍️ Example pattern
+ `@doc` "Initializes the certificate management LiveView." + `@spec` mount(map(), map(), Phoenix.LiveView.Socket.t()) :: + {:ok, Phoenix.LiveView.Socket.t()} `@impl` true def mount(_params, _session, socket) do ... end + `@doc` "Handles LiveView events for certificate management." + `@spec` handle_event(String.t(), map(), Phoenix.LiveView.Socket.t()) :: + {:noreply, Phoenix.LiveView.Socket.t()} `@impl` true def handle_event("validate", %{"credential" => params}, socket) do ... end + `@doc` "Renders the certificate management view." + `@spec` render(map()) :: Phoenix.LiveView.Rendered.t() `@impl` true def render(assigns) do ... endPer coding guidelines: "Every public function must have
@docdocumentation" and "Every function (public and private) must have@spectype specification."
🤖 Fix all issues with AI agents
In `@lib/ksef_hub_web/live/certificate_live.ex`:
- Around line 195-196: Add a private type specification above the defp
pkcs12_converter function: declare `@spec` pkcs12_converter() :: module() and
place it directly above the defp pkcs12_converter do ... end definition so the
private function has an explicit spec indicating it returns a module.
- Around line 47-55: handle_event("toggle_upload_mode", ...) currently uses
String.to_existing_atom(mode) which can raise on unexpected input; replace that
conversion with an explicit whitelist/case that maps allowed string values to
atoms (e.g. "local" -> :local, "remote" -> :remote) and provide a safe fallback
(e.g. :default or keep current socket.assigns.upload_mode) so unexpected strings
do not crash the process; update the assignment to use the sanitized new_mode
when calling assign(upload_mode: new_mode) and preserve the existing form assign
code.
In `@lib/ksef_hub/credentials/pkcs12_converter/openssl.ex`:
- Around line 15-18: Add a `@doc` block for the public function convert/3
describing its purpose, parameters and return values; update the module in which
convert/3 is defined (the OpenSSL PKCS12 converter) to include a short
description, `@param`-style notes for key_data, crt_data and key_passphrase
semantics (nil allowed), and document the {:ok, %{p12_data: binary(),
p12_password: String.t()}} and {:error, term()} return shapes and possible error
conditions; keep the doc concise and placed immediately above the `@spec/def` so
it satisfies the project’s public-function documentation requirement.
- Around line 39-46: In the clause that matches {output, exit_code} (the OpenSSL
failure branch), stop logging or returning the raw OpenSSL output to avoid
leaking secrets: change Logger.error("openssl pkcs12 failed (exit #{exit_code}):
#{output}") to log only the non-sensitive exit_code (e.g. Logger.error("openssl
pkcs12 failed (exit #{exit_code})")) and change the returned error tuple from
{:error, {:openssl_failed, exit_code, output}} to a form that omits output (e.g.
{:error, {:openssl_failed, exit_code}}), and ensure you do not reference the
output variable elsewhere in that branch.
- Around line 33-46: The System.cmd call invoking "openssl" currently has no
timeout and can hang; update the call in the pkcs12 conversion block (the case
matching System.cmd("openssl", args, stderr_to_stdout: true)) to include a
30_000 ms timeout by adding timeout: 30_000 to the options map passed to
System.cmd so the call fails after 30 seconds and the existing error handling
(the {output, exit_code} and file read branches) continues to work unchanged.
In `@test/ksef_hub_web/live/certificate_live_test.exs`:
- Around line 39-42: Add an id to the template span and update the test to
assert presence via LiveView helpers: in the template locate the span with class
"label-text" that contains "Certificate File (.p12 / .pfx)" and add
id="p12-certificate-label" to it, and in the test "defaults to p12 upload mode"
(the test function shown) change the live/3 return binding to capture the live
view (e.g., {:ok, view, _html} = live(conn, ~p"/certificates")) and replace the
string assertion with assert has_element?(view, "#p12-certificate-label").
- Around line 4-13: The LiveView tests need Mox expectations propagated to the
LiveView process; add the setup callback setup :set_mox_from_context before the
existing setup :verify_on_exit! so the test process can set Mox expectations
used by LiveView. Locate the test module where setup :verify_on_exit! is
declared and insert setup :set_mox_from_context immediately above it (using the
same atom-style setup invocation).
- Around line 89-200: Replace raw string assertions using render(view) =~ with
selector-based assertions that check the flash elements: in the "converts and
saves credential" and "passes key passphrase to converter" tests (inside
describe "save with key_crt mode") replace assert render(view) =~ "Certificate
uploaded successfully." with assert has_element?(view, "#flash-info") (or
has_element?(view, "#flash-info", "Certificate uploaded successfully.") if you
want to assert text too); in the "shows error when converter fails" and "shows
error when files missing" tests replace assert render(view) =~ "Certificate
conversion failed" and assert render(view) =~ "Please upload both private key
and certificate files." with assert has_element?(view, "#flash-error")
(optionally including the message text in the selector check). Ensure you update
only the assertions and keep the surrounding setup
(file_input/render_upload/form/render_submit) and mocks
(KsefHub.Credentials.Pkcs12Converter.Mock expectations) unchanged.
🧹 Nitpick comments (2)
lib/ksef_hub/secure_temp.ex (2)
15-20: Race condition between file creation and permission setting.There's a brief window between
File.write!andFile.chmod!where the file exists with default permissions. For sensitive data like private keys, consider using:erlang.open_portwith{:spawn_executable, ...}and exclusive file creation, or accept this minor risk given the temp directory context.A safer approach would be to set umask or use atomic file creation:
♻️ Optional: atomic secure write
def write(content, suffix) do path = path(suffix) + # Create file with restrictive permissions atomically + {:ok, fd} = :file.open(path, [:write, :exclusive, {:mode, 0o600}]) + :ok = :file.write(fd, content) + :ok = :file.close(fd) - File.write!(path, content) - File.chmod!(path, 0o600) path end
39-55: Silent error swallowing indelete/1may hide issues.The blanket
rescue _ -> :okhides all errors, and the results ofFile.write/2(line 43) andFile.rm/1(line 49) are discarded. While this is acceptable for cleanup code, consider at minimum logging unexpected errors to aid debugging if secure deletion fails.♻️ Optional: log unexpected errors
+ require Logger + `@spec` delete(String.t()) :: :ok def delete(path) do if File.exists?(path) do case File.stat(path) do {:ok, %{size: size}} when size > 0 -> - File.write(path, :binary.copy(<<0>>, size)) + _ = File.write(path, :binary.copy(<<0>>, size)) _ -> :ok end - File.rm(path) + _ = File.rm(path) end :ok rescue - _ -> :ok + error -> + Logger.warning("SecureTemp.delete failed for #{path}: #{inspect(error)}") + :ok end
| test "defaults to p12 upload mode", %{conn: conn} do | ||
| {:ok, _view, html} = live(conn, ~p"/certificates") | ||
| assert html =~ "Certificate File (.p12 / .pfx)" | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the certificate live test file and examine it
find . -name "*certificate_live*" -type f | head -20Repository: appunite/appunite-ksef-ex
Length of output: 163
🏁 Script executed:
# Check the test file structure
head -50 test/ksef_hub_web/live/certificate_live_test.exsRepository: appunite/appunite-ksef-ex
Length of output: 1524
🏁 Script executed:
# Examine the certificate live template file
cat -n lib/ksef_hub_web/live/certificate_live.exRepository: appunite/appunite-ksef-ex
Length of output: 18722
🏁 Script executed:
# Look for any .heex files related to certificates
find . -name "*certificate*" -type f | grep -E "\.(heex|html\.heex)$"Repository: appunite/appunite-ksef-ex
Length of output: 51
Add element ID to template and use has_element?/2 for testing.
The template's label (line 278) lacks an id attribute. Add id="p12-certificate-label" to the span, then update the test to use assert has_element?(view, "#p12-certificate-label") instead of html =~ "...".
<span id="p12-certificate-label" class="label-text">Certificate File (.p12 / .pfx)</span>🤖 Prompt for AI Agents
In `@test/ksef_hub_web/live/certificate_live_test.exs` around lines 39 - 42, Add
an id to the template span and update the test to assert presence via LiveView
helpers: in the template locate the span with class "label-text" that contains
"Certificate File (.p12 / .pfx)" and add id="p12-certificate-label" to it, and
in the test "defaults to p12 upload mode" (the test function shown) change the
live/3 return binding to capture the live view (e.g., {:ok, view, _html} =
live(conn, ~p"/certificates")) and replace the string assertion with assert
has_element?(view, "#p12-certificate-label").
There was a problem hiding this comment.
Actionable comments posted: 1
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_web/live/certificate_live.ex (1)
285-299:⚠️ Potential issue | 🟡 MinorRun
mix formatto fix formatting.The CI pipeline indicates this file has formatting issues. Please run
mix formatto resolve.
🤖 Fix all issues with AI agents
In `@test/ksef_hub_web/live/certificate_live_test.exs`:
- Around line 165-171: This file has formatting issues; run the Elixir formatter
on the test file (e.g., run mix format) to reformat the test module around the
"passes key passphrase to converter" test and the surrounding assertions
(references: test block named "passes key passphrase to converter" and module
KsefHub.Credentials.Pkcs12Converter.Mock) so the spacing/line breaks match mix
format expectations and CI will pass.
🧹 Nitpick comments (2)
lib/ksef_hub_web/live/certificate_live.ex (1)
28-37: Consider restricting accepted file types for key/cert uploads.Using
accept: :anyallows any file type. While this provides flexibility for various PEM extensions (.key,.pem,.crt,.cer), it also allows users to accidentally upload unrelated files. Consider using a more specific accept list if you want early client-side filtering:accept: ~w(.key .pem .crt .cer application/x-pem-file)However, since PEM files have inconsistent MIME types across browsers,
accept: :anywith server-side validation is also a valid approach.test/ksef_hub_web/live/certificate_live_test.exs (1)
46-74: Consider using element IDs for content assertions.While the mode toggle tests work correctly, the assertions using
html =~ "Private Key File"etc. (lines 55-57, 72-73) test against raw HTML content. Per coding guidelines, prefer testing for element presence with unique IDs:# Instead of: assert html =~ "Private Key File (.key / .pem)" # Consider adding IDs to template and asserting: assert has_element?(view, "#private-key-label")This is a minor refinement since the current tests are functional.
| |> render_submit() | ||
|
|
||
| assert has_element?(view, "#flash-error", "Please upload both private key and certificate files.") | ||
| end | ||
|
|
||
| test "passes key passphrase to converter", %{conn: conn} do | ||
| KsefHub.Credentials.Pkcs12Converter.Mock |
There was a problem hiding this comment.
Run mix format to fix formatting.
The CI pipeline indicates this file has formatting issues around lines 165-171. Please run mix format to resolve.
🧰 Tools
🪛 GitHub Actions: CI
[error] 165-171: mix format check failed. The following file is not formatted: test/ksef_hub_web/live/certificate_live_test.exs. Please run 'mix format' to fix formatting.
🤖 Prompt for AI Agents
In `@test/ksef_hub_web/live/certificate_live_test.exs` around lines 165 - 171,
This file has formatting issues; run the Elixir formatter on the test file
(e.g., run mix format) to reformat the test module around the "passes key
passphrase to converter" test and the surrounding assertions (references: test
block named "passes key passphrase to converter" and module
KsefHub.Credentials.Pkcs12Converter.Mock) so the spacing/line breaks match mix
format expectations and CI will pass.
- Whitelist upload mode toggle input instead of String.to_existing_atom - Stop logging raw openssl output to avoid leaking secrets - Add 30s timeout (Task.yield) to openssl and xmlsec1 System.cmd calls - Switch <form> to <.form> component with DOM id - Add @doc/@SPEC on public LiveView callbacks and pkcs12_converter/0 - Use has_element? selectors in tests instead of raw string matching - Add setup :set_mox_from_context for proper Mox propagation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
emilwojtaszek
force-pushed
the
feat/key-crt-upload
branch
from
f29ead5 to
f8f72b0
Compare
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests