Open
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the OpenSSL provider V2 initialization flow to load HSM credentials from on-disk files (instead of hardcoded values) and to parse provider configuration (key file paths + API revision) from openssl.cnf with environment-variable support for credential paths.
Changes:
- Add binary credential loading (ID/PIN) from configured file paths and cleanse credentials after use.
- Add provider config parsing for BMK/MUK/OBK paths and API revision, with defaults and
file:prefix handling. - Extend
AZIHSM_CONFIGwith credential path fields and API revision fields/constants.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| plugins/ossl_prov/src/azihsm_ossl_hsm.c | Load credentials from fixed-size binary files and use them when opening a session. |
| plugins/ossl_prov/src/azihsm_ossl_base.c | Parse config from core params + env vars; validate API revision before opening device/session. |
| plugins/ossl_prov/inc/azihsm_ossl_base.h.in | Add config constants/fields for credential paths and API revision (template header). |
| plugins/ossl_prov/inc/azihsm_ossl_base.h | Add config constants/fields for credential paths and API revision (generated header). |
jenstopp
force-pushed
the
feat/add-configuration-file-v2
branch
from
0836aa7 to
48a2b66
Compare
jenstopp
force-pushed
the
feat/add-configuration-file-v2
branch
from
de5414f to
7d174cf
Compare
jenstopp
force-pushed
the
feat/add-configuration-file-v2
branch
from
c38b655 to
7fc7b24
Compare
rajesh-gali
reviewed
Mar 6, 2026
jenstopp
force-pushed
the
feat/add-configuration-file-v2
branch
from
23b00d0 to
45cc4a6
Compare
- Add explicit <stdbool.h>, <stdint.h> to azihsm_ossl_base.h.in and <stdio.h> to azihsm_ossl_base.c (avoid relying on transitive includes) - Remove unused static core_get_params global (-Wunused-but-set-variable) - Harden load_credentials_from_file() with open()+fstat()+read() to ensure the path is a regular file of the expected size before reading - Add azihsm_path_is_safe() rejecting NULL, empty, and ".." credential paths from env vars; change parse_provider_config() to OSSL_STATUS to propagate failures to OSSL_provider_init() Signed-off-by: Jens Topp <jens.topp@9elements.com>
- Replace single `read()` call with a loop that retries on `EINTR` and accumulates partial reads - Split `fstat()` failure from the file-type/size check — each case now has a distinct error message - Fix formatting - Add missing headers - Validate all key-material paths (`bmk`, `muk`, `obk`, `pota_private_key`, `pota_public_key`) with `azihsm_path_is_safe` before use - Add missing `return OSSL_FAILURE` after invalid `api-revision` format error Signed-off-by: Jens Topp <jens.topp@9elements.com>
The previous code embedded a fixed 48-byte OBK (DEFAULT_OBK, the sequence 0x01..0x30) and a fixed P-384 key pair (POTA_PRIVATE_KEY_DER / POTA_PUBLIC_KEY_DER) directly in the source as static arrays. Replace the fallbacks with first-use generation: - OBK: generate_and_save_obk() fills 48 bytes with RAND_bytes() and writes them to the configured obk_path (mode 0600) on first start. Subsequent starts load the persisted file, producing the same MOBK on every EstablishCredential call. - POTA key pair: generate_and_save_pota_keypair() generates a fresh P-384 key pair with EVP_EC_gen(), encodes the private key as SEC1 DER (d2i_AutoPrivateKey-compatible) and the public key as SubjectPublicKeyInfo DER, and writes both files (mode 0600). Subsequent starts load the persisted pair and produce a valid ECDSA-SHA384 endorsement signature. The three static arrays, the three default_* boolean guards, and the conditional free_buffer() branches are all removed. All key buffers are now always heap-allocated, making the cleanup paths uniform. Signed-off-by: Jens Topp <jens.topp@9elements.com>
Signed-off-by: Jens Topp <jens.topp@9elements.com>
Five independent read-file implementations and four independent
write-file implementations existed across the provider sources.
The versions in azihsm_ossl_hsm.c had full ERR_raise_data
coverage.
Extract these two implementations into a new shared module:
azihsm_file_load(path, buffer)
- ENOENT treated as success (file not yet created)
- 64 KB size limit
- Full ERR_raise_data at every error path (errno + path in message)
azihsm_file_write(path, data, len)
- write() loop handles short writes and EINTR
- O_NOFOLLOW, mode 0600
- Unlinks partial file on failure
- Full ERR_raise_data at every error path
Callers updated:
- azihsm_ossl_hsm.c: remove load_file_to_buffer(); reduce
write_buffer_to_file() to a thin struct azihsm_buffer wrapper
- azihsm_ossl_mac.c, kdf.c, store.c: remove local read_file() /
read_key_file(); replace call sites with azihsm_file_load()
- azihsm_ossl_keymgmt_ec.c, keymgmt_rsa.c: replace all lines of
inline fopen/fseek/ftell/fread with azihsm_file_load(); replace
all inline open/fdopen/fwrite with
azihsm_ossl_write_masked_key_to_file()
- azihsm_ossl_masked_key.c: reduce write_masked_key_to_file() body
to a one-line delegate to azihsm_file_write()
Signed-off-by: Jens Topp <jens.topp@9elements.com>
Signed-off-by: Jens Topp <jens.topp@9elements.com>
Signed-off-by: Jens Topp <jens.topp@9elements.com>
Change all seven hardcoded /var/lib/azihsm/ default paths to CWD-relative equivalents (e.g. ./bmk.bin) so the provider works out of the box without any system-wide directory setup. Missing key files are still auto-generated on first use. Add a Configuration section to the README documenting the three-tier config priority (openssl.cnf → env vars → defaults), the full openssl.cnf structure required to load the provider and override key paths, all supported parameters with their new defaults, and both invocation styles (provider-path vs OPENSSL_CONF). Update the Installation section to replace the mandatory /var/lib/azihsm directory creation step with an optional note pointing to the Configuration section. Signed-off-by: Jens Topp <jens.topp@9elements.com>
Remove auto-generation of OBK and POTA keys on first use. These are security-sensitive files that should be managed explicitly, not created silently. The provider now returns a descriptive error when either is absent (caller source), including the expected format, size, and the openssl command needed to generate the missing material. Update the README configuration table with format details for OBK and POTA paths, and replace the "auto-generated on first use" note with explicit descriptions of which files are auto-managed (BMK, MUK) and which must be provided (OBK, POTA, credentials). Signed-off-by: Jens Topp <jens.topp@9elements.com>
Signed-off-by: Jens Topp <jens.topp@9elements.com>
Signed-off-by: Jens Topp <jens.topp@9elements.com>
Signed-off-by: Jens Topp <jens.topp@9elements.com>
Signed-off-by: Christian Walter <christian.walter@9elements.com>
Signed-off-by: Christian Walter <christian.walter@9elements.com>
walterchris
force-pushed
the
feat/add-configuration-file-v2
branch
from
0a3988c to
df75378
Compare
Signed-off-by: Christian Walter <christian.walter@9elements.com>
Collaborator
|
Credentials PIN should be read from the ENV vars and not as a file. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
You can also share your feedback on Copilot code review. Take the survey.
Signed-off-by: Christian Walter <christian.walter@9elements.com>
walterchris
force-pushed
the
feat/add-configuration-file-v2
branch
from
7d7df2f to
54802e2
Compare
Signed-off-by: Christian Walter <christian.walter@9elements.com>
Comment on lines
+399
to
+402
| if (!((pair[0] >= '0' && pair[0] <= '9') || (pair[0] >= 'a' && pair[0] <= 'f') || | ||
| (pair[0] >= 'A' && pair[0] <= 'F')) || | ||
| !((pair[1] >= '0' && pair[1] <= '9') || (pair[1] >= 'a' && pair[1] <= 'f') || | ||
| (pair[1] >= 'A' && pair[1] <= 'F'))) |
Check notice
Code scanning / CodeQL
Complex condition Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 minutes ago
In general, to fix this kind of problem, you extract the complex boolean expression into one or more helper constructs: a small static function (e.g., is_hex_digit), or intermediate boolean variables with descriptive names. Then the original if uses these helpers, reducing the number of chained logical operators and improving readability.
For this specific code, the best approach without changing functionality is to introduce a small static int is_hex_char(char c) helper in the same file and then replace the complex if condition with calls to this helper. This eliminates the repeated range logic, makes the intent obvious, and removes the complex condition entirely. The helper should return nonzero if c is a valid hex digit (0-9, a-f, A-F). Then the condition becomes if (!is_hex_char(pair[0]) || !is_hex_char(pair[1])), which is short and clear. No new headers or external libraries are needed; we just add a static function definition above hex_decode_credentials within plugins/ossl_prov/src/azihsm_ossl_base.c and change the if body starting around lines 398–403 accordingly.
Suggested changeset
1
plugins/ossl_prov/src/azihsm_ossl_base.c
| @@ -373,6 +373,13 @@ | ||
| * | ||
| * Returns OSSL_SUCCESS on success, OSSL_FAILURE on error. | ||
| */ | ||
| static int is_hex_char(char c) | ||
| { | ||
| return ((c >= '0' && c <= '9') || | ||
| (c >= 'a' && c <= 'f') || | ||
| (c >= 'A' && c <= 'F')); | ||
| } | ||
|
|
||
| static OSSL_STATUS hex_decode_credentials(const char *env_name, const char *hex_str, uint8_t *out) | ||
| { | ||
| size_t len = strlen(hex_str); | ||
| @@ -396,10 +403,7 @@ | ||
| char pair[3] = { hex_str[i * 2], hex_str[i * 2 + 1], '\0' }; | ||
|
|
||
| /* Validate both nibbles are hex digits */ | ||
| if (!((pair[0] >= '0' && pair[0] <= '9') || (pair[0] >= 'a' && pair[0] <= 'f') || | ||
| (pair[0] >= 'A' && pair[0] <= 'F')) || | ||
| !((pair[1] >= '0' && pair[1] <= '9') || (pair[1] >= 'a' && pair[1] <= 'f') || | ||
| (pair[1] >= 'A' && pair[1] <= 'F'))) | ||
| if (!is_hex_char(pair[0]) || !is_hex_char(pair[1])) | ||
| { | ||
| ERR_raise_data( | ||
| ERR_LIB_PROV, |
Copilot is powered by AI and may make mistakes. Always verify output.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a updated PR of #115 rebased and working on top of main.
Running the provider now requires having the credentials loaded via files; they are not hardcoded anymore:
Also make sure to remove any existing
bmk.bin/muk.binfiles from CWD as they will not work with the new OBK. Each of the files will be generated on first use.