From b1f3948d1dca8ea9b9462993963283a6b6294fe6 Mon Sep 17 00:00:00 2001 From: Pavel Merdin Date: Feb 13 2026 20:06:20 +0000 Subject: Transition to NEED_GUIDANCE on certread subprocess failure When the certread subprocess fails (e.g. NSS database missing or corrupt), it exits with a non-zero status without writing data to the pipe. The CM_READING_CERT handler ignored the exit status and unconditionally proceeded through to MONITORING with stale certificate data, causing infinite renewal loops. Add a finished_reading method to the certread vtable (following the keyiread pattern) that checks the subprocess exit status. When the subprocess reports failure, transition to NEED_GUIDANCE instead of continuing to MONITORING. Also fix certread-o to propagate the actual exit status instead of unconditionally calling _exit(0). Fixes: https://pagure.io/certmonger/issue/302 Assisted-by: Claude Code Signed-off-by: Pavel Merdin --- diff --git a/src/certread-int.h b/src/certread-int.h index b7fbd39..7045652 100644 --- a/src/certread-int.h +++ b/src/certread-int.h @@ -22,6 +22,8 @@ struct cm_certread_state_pvt { /* Check if something changed, for example we finished reading the * cert. */ int (*ready)(struct cm_certread_state *state); + /* Check if we successfully read the cert. */ + int (*finished_reading)(struct cm_certread_state *state); /* Get a selectable-for-read descriptor we can poll for status changes. * */ int (*get_fd)(struct cm_certread_state *state); diff --git a/src/certread-n.c b/src/certread-n.c index 47617f3..cabdb7e 100644 --- a/src/certread-n.c +++ b/src/certread-n.c @@ -451,6 +451,18 @@ cm_certread_n_ready(struct cm_certread_state *state) return cm_subproc_ready(state->subproc); } +/* Check if we were able to successfully read the certificate. */ +static int +cm_certread_n_finished_reading(struct cm_certread_state *state) +{ + int status; + status = cm_subproc_get_exitstatus(state->subproc); + if (WIFEXITED(status) && (WEXITSTATUS(status) == 0)) { + return 0; + } + return -1; +} + /* Get a selectable-for-read descriptor we can poll for status changes. */ static int cm_certread_n_get_fd(struct cm_certread_state *state) @@ -489,6 +501,7 @@ cm_certread_n_start(struct cm_store_entry *entry) if (state != NULL) { memset(state, 0, sizeof(*state)); state->pvt.ready = cm_certread_n_ready; + state->pvt.finished_reading = cm_certread_n_finished_reading; state->pvt.get_fd= cm_certread_n_get_fd; state->pvt.done= cm_certread_n_done; state->entry = entry; diff --git a/src/certread-o.c b/src/certread-o.c index a06391b..26e7698 100644 --- a/src/certread-o.c +++ b/src/certread-o.c @@ -108,7 +108,7 @@ cm_certread_o_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, } } fclose(fp); - _exit(0); + _exit(status); } /* Check if something changed, for example we finished reading the data we need @@ -119,6 +119,18 @@ cm_certread_o_ready(struct cm_certread_state *state) return cm_subproc_ready(state->subproc); } +/* Check if we were able to successfully read the certificate. */ +static int +cm_certread_o_finished_reading(struct cm_certread_state *state) +{ + int status; + status = cm_subproc_get_exitstatus(state->subproc); + if (WIFEXITED(status) && (WEXITSTATUS(status) == 0)) { + return 0; + } + return -1; +} + /* Get a selectable-for-read descriptor we can poll for status changes. */ static int cm_certread_o_get_fd(struct cm_certread_state *state) @@ -153,6 +165,7 @@ cm_certread_o_start(struct cm_store_entry *entry) if (state != NULL) { memset(state, 0, sizeof(*state)); state->pvt.ready = cm_certread_o_ready; + state->pvt.finished_reading = cm_certread_o_finished_reading; state->pvt.get_fd= cm_certread_o_get_fd; state->pvt.done= cm_certread_o_done; state->entry = entry; diff --git a/src/certread.c b/src/certread.c index fb02996..f6a6c7e 100644 --- a/src/certread.c +++ b/src/certread.c @@ -69,6 +69,16 @@ cm_certread_ready(struct cm_certread_state *state) return pvt->ready(state); } +/* Check if we finished reading the certificate. */ +int +cm_certread_finished_reading(struct cm_certread_state *state) +{ + struct cm_certread_state_pvt *pvt; + + pvt = (struct cm_certread_state_pvt *) state; + return pvt->finished_reading(state); +} + /* Get a selectable-for-read descriptor we can poll for status changes. */ int cm_certread_get_fd(struct cm_certread_state *state) diff --git a/src/certread.h b/src/certread.h index a54d700..f015a7d 100644 --- a/src/certread.h +++ b/src/certread.h @@ -28,6 +28,8 @@ struct cm_certread_state *cm_certread_n_start(struct cm_store_entry *entry); struct cm_certread_state *cm_certread_o_start(struct cm_store_entry *entry); /* Check if something changed, for example we finished reading the cert. */ int cm_certread_ready(struct cm_certread_state *state); +/* Check if we were able to read the certificate. */ +int cm_certread_finished_reading(struct cm_certread_state *state); /* Get a selectable-for-read descriptor we can poll for status changes. */ int cm_certread_get_fd(struct cm_certread_state *state); /* Clean up after reading the certificate. */ diff --git a/src/iterate.c b/src/iterate.c index b855950..3f6e2cb 100644 --- a/src/iterate.c +++ b/src/iterate.c @@ -1308,34 +1308,42 @@ cm_iterate_entry(struct cm_store_entry *entry, struct cm_store_ca *ca, case CM_READING_CERT: if (cm_certread_ready(state->cm_certread_state) == 0) { - /* Finished reloading certificate. */ - cm_certread_done(state->cm_certread_state); - state->cm_certread_state = NULL; - if (emit_entry_saved_cert != NULL) { - (*emit_entry_saved_cert)(context, entry); - } - /* Start the post-save hoook, if there is one. */ - state->cm_hook_state = cm_hook_start_postsave(entry, - context, - get_ca_by_index, - get_n_cas, - get_entry_by_index, - get_n_entries); - if (state->cm_hook_state != NULL) { - /* Note that we're doing the post-save. */ - entry->cm_state = CM_POST_SAVED_CERT; - /* Wait for status update, or poll. */ - *readfd = cm_hook_get_fd(state->cm_hook_state); - if (*readfd == -1) { - *when = cm_time_soon; + if (cm_certread_finished_reading(state->cm_certread_state) != 0) { + /* Certread subprocess failed. */ + cm_certread_done(state->cm_certread_state); + state->cm_certread_state = NULL; + entry->cm_state = CM_NEED_GUIDANCE; + *when = cm_time_now; + } else { + /* Finished reloading certificate. */ + cm_certread_done(state->cm_certread_state); + state->cm_certread_state = NULL; + if (emit_entry_saved_cert != NULL) { + (*emit_entry_saved_cert)(context, entry); + } + /* Start the post-save hoook, if there is one. */ + state->cm_hook_state = cm_hook_start_postsave(entry, + context, + get_ca_by_index, + get_n_cas, + get_entry_by_index, + get_n_entries); + if (state->cm_hook_state != NULL) { + /* Note that we're doing the post-save. */ + entry->cm_state = CM_POST_SAVED_CERT; + /* Wait for status update, or poll. */ + *readfd = cm_hook_get_fd(state->cm_hook_state); + if (*readfd == -1) { + *when = cm_time_soon; + } else { + *when = cm_time_no_time; + } } else { - *when = cm_time_no_time; + /* Failed to start the post-save, or nothing to do; + * skip it. */ + entry->cm_state = CM_NEED_TO_NOTIFY_ISSUED_SAVED; + *when = cm_time_now; } - } else { - /* Failed to start the post-save, or nothing to do; - * skip it. */ - entry->cm_state = CM_NEED_TO_NOTIFY_ISSUED_SAVED; - *when = cm_time_now; } } else { /* Wait for status update, or poll. */ diff --git a/tests/010-iterate/expected.out b/tests/010-iterate/expected.out index 77add1c..2e1c9b1 100644 --- a/tests/010-iterate/expected.out +++ b/tests/010-iterate/expected.out @@ -1214,4 +1214,11 @@ The sky is falling: Certificate in file "$tmpdir/certfile10" issued by CA and sa Request ID: Test Notification type: issued_and_saved +[Certread failure for NSS init failure (bug demo).] +NEED_TO_READ_CERT +-START- +READING_CERT +NEED_GUIDANCE +-STOP- + Test complete. diff --git a/tests/010-iterate/run.sh b/tests/010-iterate/run.sh index 3ca6ec9..bd57014 100755 --- a/tests/010-iterate/run.sh +++ b/tests/010-iterate/run.sh @@ -955,4 +955,34 @@ cat $tmpdir/notification.txt | sed 's@'"$tmpdir"'@$tmpdir@g' CERTMONGER_CONFIG_DIR="$SAVED_CONFIG_DIR" echo +echo '[Certread failure for NSS init failure (bug demo).]' +mkdir -p $tmpdir/bad_nssdb +cat > ca << EOF +id=SelfSign +ca_type=INTERNAL:SELF +EOF +cat > entry_nss << EOF +id=Test +ca_name=SelfSign +state=NEED_TO_READ_CERT +key_storage_type=FILE +key_storage_location=$tmpdir/keyfile +cert_storage_type=NSSDB +cert_storage_location=$tmpdir/bad_nssdb +cert_nickname=testcert +notification_method=STDOUT +EOF +# When NSS_InitContext() fails (no database files in directory), certread-n +# calls _exit(CM_SUB_STATUS_ERROR_INITIALIZING) without writing to the pipe. +# The parent currently ignores the failure and unconditionally proceeds to +# MONITORING with stale cert data. After fixing the bug, this should reach +# NEED_GUIDANCE instead. +$toolsdir/iterate ca entry_nss NEED_TO_READ_CERT,READING_CERT +if test "`grep ^state entry_nss`" != state=NEED_GUIDANCE ; then + echo Certread failure test: unexpected state. + grep ^state entry_nss + exit 1 +fi + +echo echo Test complete.