p11_child: Add timeout parameter #7890

thalman · 2025-03-20T17:53:40Z

p11_child communication with OCSP server may take a long time because of network issues. Then p11_child is killed after p11_child_timeout and the authentication fails.

This is not desirable when certificate_verification is set to soft_ocsp. This update will pass the timeout to the child process so it can cancel the OCSP verification before it is terminated.

Resolves: #6601

alexey-tikhonov · 2025-03-20T18:54:26Z

RHBZ: https://issues.redhat.com/browse/RHEL-4981

sumit-bose · 2025-03-21T10:37:59Z

src/p11_child/p11_child_openssl.c

@@ -582,6 +597,7 @@ errno_t init_p11_ctx(TALLOC_CTX *mem_ctx, const char *ca_db,
        return ENOMEM;
    }

+    ctx->start_timestamp = time(NULL);


Hi,

why don't you put timeout into struct p11_ctx as well (it would fit into struct cert_verify_opts as well) ? This would help to avoid adding timeout into all the function headers.

bye,
Sumit

I was thinking of that too, I have no preference here. But before proceeding with changes I want to do some testing to see if this solves the issue correctly. This is still draft PR.

I would put this into cert_verify_opts as it would reduce number of changes (this struct is populated already in main()). But I don't insist.

alexey-tikhonov · 2025-04-09T14:29:58Z

@thalman, could you please rebase?

p11_child communication with OCSP server may take a long time because of network issues. Then p11_child is killed after `p11_child_timeout` and the authentication fails. This is not desirable when `certificate_verification` is set to `soft_ocsp`. This update will pass the timeout to the child process so it can cancel the OCSP verification before it is terminated. Resolves: SSSD#6601

thalman · 2025-04-10T13:06:04Z

@thalman, could you please rebase?

Rebased

alexey-tikhonov · 2025-04-12T09:21:47Z

src/p11_child/p11_child.h

@@ -59,7 +60,8 @@ enum pin_mode {
 };

 errno_t init_p11_ctx(TALLOC_CTX *mem_ctx, const char *ca_db,
-                     bool wait_for_card, struct p11_ctx **p11_ctx);
+                     bool wait_for_card, time_t timeout,


Any reason to use time_t specifically and not plain int or unsigned?
This arg is just a period (number of seconds?), not a time point.

Also code parses it as POPT_ARG_LONG, it's better to match types strictly to avoid surprises (especially on non-x86 platforms).

for parsing I use long int but the rest of the code expects int. I did not have particular reason for using time_t I can change that to int and add check that this parameter fits into it.

Would it be possible to simply use int everywhere (including parsing)?

And of course because we use time() to calculate the timeout.

That's a rabbit hole. Looks like there is no portable way to increment time_t directly.

Currently it is implicitly cast-ed to time_t from long when provided to do_work(). Maybe that's ok.

src/p11_child/p11_child_common.c

alexey-tikhonov · 2025-04-12T09:35:39Z

Ha... It appears query_responder() already supports timeout argument, it just wasn't properly provided...

alexey-tikhonov · 2025-04-12T09:36:11Z

@spoore1, would you like a test build before we merge this?

alexey-tikhonov · 2025-04-12T09:41:37Z

src/p11_child/p11_child_openssl.c

+        /* substract 1 so we finish before child is forcibly terminated */
+        req_timeout = p11_ctx->timeout - 1 -
+            (time(NULL) - p11_ctx->start_timestamp);
+        if (req_timeout < 0) {


Not needed if below condition changed to if (req_timeout <= 0)

With the one item (deadline) approach this changes a bit but the condition is still needed.

Only to have a dedicated debug message.
Otherwise instead of two:

if (req_timeout <= 0) { ... if (req_timeout != 0) {

there could be single

if (req_timeout > 0) { process_responder() }

src/p11_child/p11_child_openssl.c

alexey-tikhonov · 2025-04-12T09:53:30Z

src/responder/ifp/ifp_users.c

@@ -1128,7 +1128,7 @@ ifp_users_find_by_valid_cert_send(TALLOC_CTX *mem_ctx,
        goto done;
    }

-    state->extra_args = talloc_zero_array(state, const char *, 8);
+    state->extra_args = talloc_zero_array(state, const char *, 10);


Do you think it would be possible to factor out a common helper, that prepares p11_child args, out of ifp/pamsrv_p11/ssh to reduce code copy&paste?

I will take a look

spoore1 · 2025-04-13T21:31:05Z

@spoore1, would you like a test build before we merge this?

If it's ok if I can't test until Tuesday or Wednesday, yes. I can't guarantee I'll be able to test before that.

alexey-tikhonov · 2025-04-16T16:29:39Z

src/p11_child/p11_child_openssl.c

@@ -44,8 +44,7 @@ struct p11_ctx {
    const char *ca_db;
    bool wait_for_card;
    struct cert_verify_opts *cert_verify_opts;
-    time_t start_timestamp;
-    time_t timeout;
+    int ocsp_deadline;


Unlike timeout, this is a better fit for time_t, actually (see another comment).

alexey-tikhonov · 2025-04-16T16:31:31Z

src/p11_child/p11_child_openssl.c

-            (time(NULL) - p11_ctx->start_timestamp);
-        if (req_timeout < 0) {
+    if (p11_ctx->ocsp_deadline != -1  && p11_ctx->cert_verify_opts->soft_ocsp) {
+        req_timeout = p11_ctx->ocsp_deadline - time(NULL);


There is a difftime().
But it returns double.
I now tend to agree to keep everything as time_t and that's it.

alexey-tikhonov added Bugzilla branch: sssd-2-9 labels Mar 20, 2025

alexey-tikhonov self-requested a review March 20, 2025 18:54

alexey-tikhonov self-assigned this Mar 20, 2025

thalman force-pushed the p11_child_timeout branch 2 times, most recently from 56a725f to 220bbaa Compare March 21, 2025 08:30

sumit-bose reviewed Mar 21, 2025

View reviewed changes

thalman force-pushed the p11_child_timeout branch from 220bbaa to 1559bdf Compare April 2, 2025 11:29

thalman marked this pull request as ready for review April 2, 2025 12:22

andreboscatto assigned sumit-bose Apr 3, 2025

alexey-tikhonov added the Waiting for review label Apr 9, 2025

thalman force-pushed the p11_child_timeout branch from 1559bdf to d1aeb3c Compare April 10, 2025 12:54

alexey-tikhonov added the coverity Trigger a coverity scan label Apr 12, 2025

alexey-tikhonov reviewed Apr 12, 2025

View reviewed changes

src/p11_child/p11_child_common.c Outdated Show resolved Hide resolved

alexey-tikhonov reviewed Apr 12, 2025

View reviewed changes

src/p11_child/p11_child_openssl.c Outdated Show resolved Hide resolved

alexey-tikhonov added Changes requested and removed Waiting for review labels Apr 12, 2025

alexey-tikhonov reviewed Apr 12, 2025

View reviewed changes

use deadline in the p11_ctx instead two timeouts

ae9175a

alexey-tikhonov reviewed Apr 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

p11_child: Add timeout parameter #7890

p11_child: Add timeout parameter #7890

thalman commented Mar 20, 2025

alexey-tikhonov commented Mar 20, 2025

sumit-bose Mar 21, 2025

thalman Mar 21, 2025

alexey-tikhonov Apr 12, 2025

alexey-tikhonov commented Apr 9, 2025

thalman commented Apr 10, 2025

alexey-tikhonov Apr 12, 2025

alexey-tikhonov Apr 12, 2025

thalman Apr 16, 2025

alexey-tikhonov Apr 16, 2025

thalman Apr 16, 2025

alexey-tikhonov Apr 16, 2025

alexey-tikhonov Apr 16, 2025

alexey-tikhonov commented Apr 12, 2025

alexey-tikhonov commented Apr 12, 2025

alexey-tikhonov Apr 12, 2025

thalman Apr 16, 2025

alexey-tikhonov Apr 16, 2025 •

edited

Loading

alexey-tikhonov Apr 12, 2025 •

edited

Loading

thalman Apr 16, 2025

spoore1 commented Apr 13, 2025

alexey-tikhonov Apr 16, 2025

alexey-tikhonov Apr 16, 2025 •

edited

Loading

p11_child: Add timeout parameter #7890

Are you sure you want to change the base?

p11_child: Add timeout parameter #7890

Conversation

thalman commented Mar 20, 2025

alexey-tikhonov commented Mar 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexey-tikhonov commented Apr 9, 2025

thalman commented Apr 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexey-tikhonov commented Apr 12, 2025

alexey-tikhonov commented Apr 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexey-tikhonov Apr 16, 2025 • edited Loading

Choose a reason for hiding this comment

alexey-tikhonov Apr 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spoore1 commented Apr 13, 2025

Choose a reason for hiding this comment

alexey-tikhonov Apr 16, 2025 • edited Loading

Choose a reason for hiding this comment

alexey-tikhonov Apr 16, 2025 •

edited

Loading

alexey-tikhonov Apr 12, 2025 •

edited

Loading

alexey-tikhonov Apr 16, 2025 •

edited

Loading