OPEN-85: General API refactoring to ensure consistent design and naming across all modules (#21)

- Introduced code review templates to standardize review practices.
- Provided architectural guidelines to ensure consistency in
implementation.
- Added constant time equals for secure key materials.
- Overall refactoring and alignments of the API.

Author: tobias-schwerdtfeger

AGENTS.md

@@ -0,0 +1,62 @@
+<!--
+SPDX-FileCopyrightText: Copyright 2025 gematik GmbH
+
+SPDX-License-Identifier: Apache-2.0
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+
+*******
+
+For additional notes and disclaimer from gematik and in case of changes by gematik,
+find details in the "Readme" file.
+-->
+
+# Repository Guidelines
+
+## Project Structure & Modules
+- Rust workspace root at `./`, member crates under `core-modules/` (`asn1`, `crypto`, `crypto-openssl-sys`, `healthcard`).
+- Kotlin/JVM bindings and examples live in `core-modules-kotlin/` (`healthcard`, `sample-app`).
+- Documentation is in `docs/` (see `docs/architecture/README.md` and `docs/interop/jvm.md` for deeper details).
+
+## Build, Test & Development
+- Build all Rust crates: `cargo build` (from the repository root).
+- Run Rust tests: `cargo test` (add `-p <crate>` to limit scope, e.g. `cargo test -p healthcard`).
+- Kotlin bindings and sample: from `core-modules-kotlin/`, use `./gradlew :healthcard:build` and `./gradlew :sample-app:run`.
+- Before pushing, ensure `cargo +nightly fmt` and `cargo clippy --all-targets --all-features` are clean.
+
+## Coding Style & Naming
+- Rust is formatted with `rustfmt` (see `rustfmt.toml`), 4-space indentation, max line width 120.
+- Use expressive, domain-oriented names and follow existing module layout (e.g. `exchange`, `command`, `identifier` in `healthcard`).
+- TODOs must reference a ticket: `// TODO OPEN-1234: Brief description`.
+- New files must carry SPDX headers and license annotations consistent with existing files.
+
+## Testing Guidelines
+- Prefer Rust unit tests close to the code (inline `#[cfg(test)] mod tests`).
+- For cryptography and parsing, use known-good test vectors where possible.
+- JVM code uses Kotlin test/JUnit; run with `./gradlew test` (or module-specific tasks such as `:sample-app:test`).
+- Aim to keep or improve test coverage for modified modules and cover edge cases relevant to healthcard flows.
+
+## Commit & Pull Request Guidelines
+- Use ticket-prefixed commit messages, e.g. `OPEN-123: Short, imperative summary`.
+- Each PR should include: a short description, motivation/linked issue, and notes on testing performed.
+- Call out breaking API changes and update release notes or changelogs where applicable.
+- Keep PRs focused and small; follow existing patterns rather than introducing new architectural concepts without prior discussion.
+
+## Security & Configuration
+- Review `SECURITY.md` for responsible disclosure and security expectations.
+- Be mindful of handling keys, secrets, and card data; never commit real credentials or production traces.
+- When in doubt about crypto or card-IO changes, seek review from someone familiar with the respective module.
+
+## Agent & AI Review Instructions
+- When using AI tools for refactoring or review, follow the crate-specific templates: `core-modules/asn1/AGENTS_REVIEW_TEMPLATE.md`, `core-modules/crypto/AGENTS_REVIEW_TEMPLATE.md`, and `core-modules/healthcard/AGENTS_REVIEW_TEMPLATE.md`.
+- Treat these templates as additional constraints on top of this document: keep changes incremental, behavior-preserving, and aligned with the security and architectural guidance they provide.

core-modules-kotlin/sample-app/src/main/kotlin/de/gematik/openhealth/sample/PcscCardChannel.kt

@@ -23,10 +23,13 @@ package de.gematik.openhealth.sample
 
 import de.gematik.openhealth.healthcard.CardChannel
 import de.gematik.openhealth.healthcard.CardChannelException
-import de.gematik.openhealth.healthcard.TrustedChannelException
+import de.gematik.openhealth.healthcard.CommandApdu
+import de.gematik.openhealth.healthcard.HealthCardResponseStatus
+import de.gematik.openhealth.healthcard.ResponseApdu
 import javax.smartcardio.CardChannel as PcscChannel
 import javax.smartcardio.CardException
 import javax.smartcardio.CommandAPDU
+import kotlin.collections.joinToString
 
 /**
  * Adapts the javax.smartcardio channel so it can be consumed by the UniFFI generated API.
@@ -40,16 +43,16 @@ class PcscCardChannel(
         return historicalBytes != null && historicalBytes.size > 15
     }
 
-    override fun transmit(command: ByteArray): ByteArray {
+    override fun transmit(command: CommandApdu): ResponseApdu {
         try {
-            val response = delegate.transmit(CommandAPDU(command))
-            return response.bytes
+            val response = delegate.transmit(CommandAPDU(command.toBytes()))
+            val sw = response.sw.toUShort()
+            val status = if (sw.toInt() == 0x9000) HealthCardResponseStatus.SUCCESS else HealthCardResponseStatus.UNKNOWN_STATUS
+            return ResponseApdu(sw = sw, status = status, data = response.data)
         } catch (ex: CardException) {
             throw CardChannelException.Transport(
-                TrustedChannelException.Transport(
-                    code = 0u,
-                    reason = ex.message ?: "PC/SC transmit failed",
-                )
+                code = 0u,
+                reason = ex.message ?: "PC/SC transmit failed",
             )
         }
     }

core-modules-kotlin/sample-app/src/main/kotlin/de/gematik/openhealth/sample/TrustedChannelCli.kt

@@ -21,10 +21,15 @@
 
 package de.gematik.openhealth.sample
 
+import de.gematik.openhealth.healthcard.ApduException
+import de.gematik.openhealth.healthcard.CardAccessNumber
+import de.gematik.openhealth.healthcard.CommandApdu
+import de.gematik.openhealth.healthcard.TrustedChannelException
 import de.gematik.openhealth.healthcard.establishTrustedChannel
 import javax.smartcardio.Card
 import javax.smartcardio.CardTerminals
 import javax.smartcardio.TerminalFactory
+import kotlin.collections.joinToString
 
 /**
  * Minimal demonstration that wires the trusted channel implementation into the PC/SC stack.
@@ -37,13 +42,24 @@ fun main() {
         ?: System.getenv("CARD_ACCESS_NUMBER")
         ?: error("Provide CARD_ACCESS_NUMBER env variable or -DcardAccessNumber=XXXXXX")
     val apdu = byteArrayOf(0x00, 0x84.toByte(), 0x00, 0x00, 0x08) // GET CHALLENGE default
+    val can = try {
+        CardAccessNumber.fromDigits(cardAccessNumber)
+    } catch (ex: TrustedChannelException) {
+        error("Invalid CAN: ${ex.message}")
+    }
 
     val card = openPcscCard()
     try {
-        val trustedChannel = establishTrustedChannel(PcscCardChannel(card.basicChannel), cardAccessNumber)
+        val trustedChannel = establishTrustedChannel(PcscCardChannel(card.basicChannel), can)
         println("Trusted channel established.")
-        val response = trustedChannel.transmit(apdu)
-        println("Trusted channel response: ${response.toHexString()}")
+        val command = try {
+            CommandApdu.fromBytes(apdu)
+        } catch (ex: ApduException) {
+            error("Failed to build command APDU: ${ex.message}")
+        }
+        val response = trustedChannel.transmit(command)
+        val sw = "%04X".format(response.sw.toInt())
+        println("Trusted channel response: SW=$sw, data=${response.data.toHexString()}, status=${response.status}")
     } finally {
         try {
             card.disconnect(false)

core-modules/asn1/AGENTS_REVIEW_TEMPLATE.md

@@ -0,0 +1,150 @@
+<!--
+SPDX-FileCopyrightText: Copyright 2025 gematik GmbH
+
+SPDX-License-Identifier: Apache-2.0
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+
+*******
+
+For additional notes and disclaimer from gematik and in case of changes by gematik,
+find details in the "Readme" file.
+-->
+
+# AI Code Review & Refactoring Template – `asn1` Crate
+
+You are an expert Rust engineer with strong background in ASN.1, DER/BER encoding, parsing security, and safe library design. You are reviewing and (optionally) refactoring the `asn1` crate of the OpenHealth Core project.
+
+The primary goal is to improve safety, correctness, clarity, and consistency without changing the externally visible behavior or published API contracts, unless explicitly noted.
+
+When in doubt, favor **small, well-justified, incremental improvements** over large sweeping rewrites.
+
+---
+
+## 1. Context & Goals
+
+- The `asn1` crate provides **ASN.1 primitives and utilities** for other crates (`crypto`, `healthcard`, …).
+- It deals with:
+  - Tags, OIDs, and basic ASN.1 value types
+  - Encoding and decoding logic (likely DER-like)
+  - Date/time handling
+  - Error types for parsing/encoding issues
+- **Goal:** A small, well-structured, predictable core with **robust parsing**, **clear errors**, and **no undefined behavior**.
+
+Before making changes:
+
+1. Scan `src/lib.rs` to understand the public API surface.
+2. Inspect modules such as `decoder.rs`, `encoder.rs`, `tag.rs`, `oid.rs`, `date_time.rs`, and `error.rs`.
+3. Identify which items are public and likely used by other crates.
+
+---
+
+## 2. General Review Checklist
+
+Work through this checklist and propose changes where they add clear value and preserve behavior.
+
+### 2.1 API & Structure
+
+- [ ] Is the module structure (`lib.rs` + submodules) clear and cohesive?
+- [ ] Are public types and functions named consistently and descriptively?
+- [ ] Are there obvious internal details accidentally exposed as `pub`?
+- [ ] Are public types and functions documented sufficiently for downstream crate authors?
+- [ ] Do re-exports and visibility align with how the crate is intended to be used?
+
+### 2.2 ASN.1 Correctness & Robustness
+
+- [ ] Are **length fields** and **tag values** validated rigorously (e.g., no unchecked overflows or negative lengths)?
+- [ ] Does decoding handle malformed or truncated input safely (no panics, no indexing out of bounds)?
+- [ ] Are unknown or unsupported tags handled in a predictable way (e.g., explicit error variants)?
+- [ ] Is there any custom ASN.1 logic that should be better documented or encapsulated?
+- [ ] Are ASN.1 date/time encodings correctly handled (e.g., UTCTime vs GeneralizedTime, timezone handling)?
+- [ ] Are any implicit assumptions (e.g., encoding rules) clearly documented and consistently applied?
+
+### 2.3 Error Handling
+
+- [ ] Are all failures represented with meaningful error variants (using `thiserror`) instead of generic strings?
+- [ ] Are errors **non-panicking** for invalid input (i.e., malformed ASN.1 should return `Result::Err`, not `panic!`)?
+- [ ] Are error variants documented and named consistently?
+- [ ] Is error propagation clear and idiomatic (`?` operator, contextual information where helpful)?
+
+### 2.4 Safety & Performance
+
+- [ ] Is all code safe Rust? If `unsafe` is present, is it minimal, necessary, and well-documented?
+- [ ] Are there any potential integer overflows or unchecked arithmetic in length/tag handling?
+- [ ] Are slices and indexing operations guarded by explicit bounds checks or safe helpers?
+- [ ] Are `regex` usages justified and efficient (or could simpler parsing logic suffice)?
+- [ ] Are allocations and copies kept reasonable given typical data sizes?
+
+### 2.5 Style & Consistency
+
+- [ ] Is the code formatted according to `rustfmt` and consistent across modules?
+- [ ] Are naming conventions consistent (snake_case, CamelCase, acronyms, etc.)?
+- [ ] Is there duplication that could be factored into helpers without harming clarity?
+- [ ] Are module/file responsibilities clear (e.g., tags in `tag.rs`, OIDs in `oid.rs`)?
+
+---
+
+## 3. Refactoring Guidelines
+
+When proposing refactors, follow these principles:
+
+- **Preserve behavior and public API** unless there is an obvious bug or unsafe behavior.
+- Prefer **small, composable helpers** over deeply nested, complex functions.
+- Avoid introducing new dependencies without a strong justification.
+- Keep ASN.1 logic **explicit and well-documented**; avoid “magic numbers” without explanation.
+- Where possible, make parsing and encoding **table-driven** or declarative, but only if it simplifies the code.
+
+### 3.1 Common Refactor Targets
+
+- [ ] Simplify complex decoding/encoding functions by extracting small helper functions.
+- [ ] Replace manual error construction with consistent, typed error variants.
+- [ ] Remove redundant conversions or allocations (e.g., unnecessary `to_vec`).
+- [ ] Clarify edge-case handling with explicit branches and comments (especially around length calculations).
+- [ ] Ensure public functions return clear, composable types (e.g., `Result<_, Asn1Error>`).
+
+---
+
+## 4. Security-Specific Checks
+
+Although `asn1` is not performing cryptography directly, it is **security-sensitive** as it parses inputs that may come from untrusted sources.
+
+- [ ] Ensure no panics can be triggered by arbitrary input.
+- [ ] Ensure all indexing into slices/arrays is bounds checked.
+- [ ] Avoid `unwrap`, `expect`, and `panic!` in parsing paths; use robust error handling instead.
+- [ ] Watch for integer truncation or sign errors when converting lengths or indices.
+- [ ] Consider denial-of-service vectors (e.g., extremely large lengths); ensure the design allows callers to constrain inputs as needed.
+
+If you find any potential vulnerability, describe:
+
+1. The problematic code,
+2. A minimal proof-of-concept scenario, and
+3. A concrete, minimal fix.
+
+---
+
+## 5. Output Format
+
+When you finish your review of the `asn1` crate, produce:
+
+1. **High-level summary**
+   - One paragraph summarizing overall code health, structure, and main risks.
+2. **Findings list**
+   - A bullet list with: *[Severity]* – short title – file:line – concise explanation.
+3. **Concrete refactor suggestions**
+   - For each suggestion, include:
+     - The goal (e.g., “clarify error handling in decoder”),
+     - A short rationale,
+     - A **small, focused patch** or pseudo-code showing the change.
+
+Avoid huge rewrites; prefer a sequence of small, reviewable improvements.
+