CMP: fix flag indicating the use of SSL/TLS for client-side HTTP connections #21176

DDvO · 2023-06-11T16:24:27Z

CMP: fix OSSL_CMP_MSG_http_perform() by adding option OSSL_CMP_OPT_USE_TLS
OSSL_CMP_CTX_new.pod: remove overlap with OSSL_HTTP_transfer.pod; improve the latter
apps/cmp.c: -tls_used may be implied by -server https:...; improve related checks and doc

Fixes #21120

Checklist

documentation is added or updated
tests are added or updated

DDvO · 2023-06-12T06:33:09Z

The two CI failures are unrelated.

t8m · 2023-06-12T06:39:10Z

The two CI failures are unrelated.

Although unrelated the CMS failure looks really suspicious.

…lated checks and doc

DDvO · 2023-06-12T07:57:12Z

The two CI failures are unrelated.

Although unrelated the CMS failure looks really suspicious.

I agree the failing CMS consistency tests on buildbot/master:unix-ubuntu-aarch64 looked suspicious,
but luckily this disappeared after I rebased to the latest master half an hour ago.

doc/man1/openssl-cmp.pod.in

t8m · 2023-06-12T13:12:30Z

I do not think this is a bug fix. Also what if anyone is already depending on the existing behavior of having the callback implying TLS?

DDvO · 2023-06-12T13:28:19Z

I do not think this is a bug fix.

As witnessed by #21120, the existing code has at least unexpected and undocumented behavior.
I see it as a bug because the use of the callback and its optional argument is needlessly limited to adding TLS (to the given CMP HTTP transfer).
If it is used in the CMP context for anything else, as can be done for the general (non-CMP) HTTP client, things go wrong as it is falsely assumed that TLS is being used (if the argument is non-NULL) or not being used (if the arg is NULL).

Also what if anyone is already depending on the existing behavior of having the callback implying TLS?

This is indeed a problem, but I'd say the risk is small because it's an advanced feature and the existing behavior is weird and not documented.

DDvO · 2023-06-12T13:31:44Z

Also what if anyone is already depending on the existing behavior of having the callback implying TLS?

With this argument, we couldn't do any bug fixes because there might be someone who - for whatever strange reason - started relying on wrong behavior.

tmshort

Just a clarification needed on server vs client use of OSSL_CMP_OPT_USE_TLS.

tmshort · 2023-06-12T13:26:01Z

crypto/cmp/cmp_local.h

@@ -49,6 +49,7 @@ struct ossl_cmp_ctx_st {
    int keep_alive; /* persistent connection: 0=no, 1=prefer, 2=require */
    int msg_timeout; /* max seconds to wait for each CMP message round trip */
    int total_timeout; /* max number of seconds an enrollment may take, incl. */
+    int tls_used; /* whether to use TLS for client-side HTTP connections */


Presumably, this is not just for clients? In apps/cmp.c the flag is set regardless of opt_server

So far, tls_used is implemented for the client side only.
And as now stated in openssl-cmp.h.in:

It is ignored if the B<-server> option is not given

tmshort · 2023-06-12T13:28:11Z

apps/cmp.c

+    if (opt_tls_used)
+        (void)OSSL_CMP_CTX_set_option(ctx, OSSL_CMP_OPT_USE_TLS, 1);


If this option is for clients only, should there be a && opt_server == NULL here?

We could do that, but the option is anyway ignored in this case (with a warning).

If/when we later add TLS support to the server side, we would have to revert adding && opt_server == NULL.

t8m · 2023-06-12T14:18:50Z

Also what if anyone is already depending on the existing behavior of having the callback implying TLS?

With this argument, we couldn't do any bug fixes because there might be someone who - for whatever strange reason - started relying on wrong behavior.

The app code already did this, what if someone just mimicked it? Can you please try harder to make this backwards compatible?

For example this could be a new feature for 3.2 where the OP is OSSL_CMP_OPT_NO_TLS and it switches off the tls if the callback is in use.

DDvO · 2023-06-12T14:36:44Z

Also what if anyone is already depending on the existing behavior of having the callback implying TLS?

With this argument, we couldn't do any bug fixes because there might be someone who - for whatever strange reason - started relying on wrong behavior.

The app code already did this, what if someone just mimicked it? Can you please try harder to make this backwards compatible?

For example this could be a new feature for 3.2 where the OP is OSSL_CMP_OPT_NO_TLS and it switches off the tls if the callback is in use.

Hmm, such a OSSL_CMP_OPT_NO_TLS option would only solve half of the problem.

Here is my proposal for a backward compatible solution:
define for the new OSSL_CMP_OPT_USE_TLS option a third value, namely -1, which is assumed by the default and means the hitherto behavior, where the callback function arg being non-NULL determines whether to use TLS.
When the OSSL_CMP_OPT_USE_TLS option is used and explicitly set to 0, TLS is not used, and when set to 1, TLS is used.

This way, the old behavior is by default, and explicitly setting the TLS leads to the expected new behavior.
Ok?

t8m · 2023-06-12T14:56:51Z

OK, but for master branch only.

DDvO · 2023-06-12T15:03:33Z

OK, but for master branch only.

Good, but why not also for at least 3.0?
As mentioned, the hitherto behavior is weird and not documented.
Doing the backport with the new solution does not harm existing code
and gives users of the 3.0 LTS version both a to-the-point doc and the chance to do the TLS (de-)selection in a proper way.

t8m · 2023-06-12T15:18:48Z

I would not block merging it to 3.1 and 3.0 if someone else approves it, but in my opinion this is clearly a new feature, not a bug fix.

…rove related checks and doc

…rove the latter

…E_TLS Fixes openssl#21120

…_OPT_USE_TLS

tmshort

I'm ok with a back port to 3.1/3.0. Should this also be in master (not labeled for master, but the PR is on master)?

DDvO · 2023-06-20T15:51:17Z

I'm ok with a back port to 3.1/3.0. Should this also be in master (not labeled for master, but the PR is on master)?

Pleased to hear.
I simply forgot to tick the master label - done now.

openssl-machine · 2023-07-21T00:09:09Z

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

DDvO · 2023-08-13T10:42:01Z

Ping @openssl/committers for 2nd review

openssl-machine · 2023-09-13T00:09:53Z

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

DDvO · 2023-10-07T14:58:20Z

Pinging again @openssl/committers for 2nd review.

t8m

OK for the master branch only.

openssl-machine · 2023-10-10T16:00:29Z

This pull request is ready to merge

t8m · 2023-10-10T18:36:54Z

Merged to the master branch. Thank you for your contribution.

…lated checks and doc Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21176)

…rove the latter Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21176)

…E_TLS Fixes #21120 Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21176)

…lated checks and doc Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl/openssl#21176) Signed-off-by: fly2x <fly2x@hitls.org>

…rove the latter Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl/openssl#21176) Signed-off-by: fly2x <fly2x@hitls.org>

…E_TLS Fixes #21120 Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl/openssl#21176) Signed-off-by: fly2x <fly2x@hitls.org>

DDvO added this to the 3.2.0 milestone Jun 11, 2023

apps/cmp.c: -tls_used may be implied by -server https:...; improve re…

0d382e3

…lated checks and doc

DDvO force-pushed the fix_CMP_indication_HTTPS branch from 6f1df38 to 05d5e91 Compare June 12, 2023 07:26

mattcaswell requested changes Jun 12, 2023

View reviewed changes

doc/man1/openssl-cmp.pod.in Outdated Show resolved Hide resolved

DDvO requested a review from mattcaswell June 12, 2023 08:31

mattcaswell approved these changes Jun 12, 2023

View reviewed changes

mattcaswell removed the approval: otc review pending This pull request needs review by an OTC member label Jun 12, 2023

tmshort requested changes Jun 12, 2023

View reviewed changes

DDvO added 4 commits June 12, 2023 17:52

fixup! apps/cmp.c: -tls_used may be implied by -server https:...; imp…

57e6c11

…rove related checks and doc

OSSL_CMP_CTX_new.pod: remove overlap with OSSL_HTTP_transfer.pod; imp…

1add447

…rove the latter

CMP: fix OSSL_CMP_MSG_http_perform() by adding option OSSL_CMP_OPT_US…

ed9c761

…E_TLS Fixes openssl#21120

fixup! CMP: fix OSSL_CMP_MSG_http_perform() by adding option OSSL_CMP…

3eb5224

…_OPT_USE_TLS

DDvO requested review from tmshort and mattcaswell June 12, 2023 15:57

DDvO added the approval: otc review pending This pull request needs review by an OTC member label Jun 12, 2023

DDvO changed the title ~~CMP: fix flag requiring the use of SSL/TLS for client-side HTTP connections~~ CMP: fix flag indicating the use of SSL/TLS for client-side HTTP connections Jun 12, 2023

tmshort approved these changes Jun 20, 2023

View reviewed changes

tmshort removed the approval: review pending This pull request needs review by a committer label Jun 20, 2023

DDvO added the branch: master Merge to master branch label Jun 20, 2023

DDvO mentioned this pull request Jul 6, 2023

Why is http_cb_arg used to determine whether to use tls in the CMP? #21120

Closed

hlandau removed this from the 3.2.0 milestone Aug 31, 2023

t8m approved these changes Oct 9, 2023

View reviewed changes

t8m added approval: done This pull request has the required number of approvals and removed approval: otc review pending This pull request needs review by an OTC member branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Oct 9, 2023

openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Oct 10, 2023

t8m closed this Oct 10, 2023

openssl-machine pushed a commit that referenced this pull request Oct 10, 2023

apps/cmp.c: -tls_used may be implied by -server https:...; improve re…

4a9299a

…lated checks and doc Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21176)

openssl-machine pushed a commit that referenced this pull request Oct 10, 2023

OSSL_CMP_CTX_new.pod: remove overlap with OSSL_HTTP_transfer.pod; imp…

2f76888

…rove the latter Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21176)

openssl-machine pushed a commit that referenced this pull request Oct 10, 2023

CMP: fix OSSL_CMP_MSG_http_perform() by adding option OSSL_CMP_OPT_US…

ac0677b

…E_TLS Fixes #21120 Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21176)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMP: fix flag indicating the use of SSL/TLS for client-side HTTP connections #21176

CMP: fix flag indicating the use of SSL/TLS for client-side HTTP connections #21176

DDvO commented Jun 11, 2023 •

edited

DDvO commented Jun 12, 2023

t8m commented Jun 12, 2023

DDvO commented Jun 12, 2023

t8m commented Jun 12, 2023

DDvO commented Jun 12, 2023 •

edited

DDvO commented Jun 12, 2023 •

edited

tmshort left a comment

tmshort Jun 12, 2023

DDvO Jun 12, 2023

tmshort Jun 12, 2023

DDvO Jun 12, 2023

t8m commented Jun 12, 2023

DDvO commented Jun 12, 2023 •

edited

t8m commented Jun 12, 2023

DDvO commented Jun 12, 2023

t8m commented Jun 12, 2023

tmshort left a comment •

edited

DDvO commented Jun 20, 2023

openssl-machine commented Jul 21, 2023

DDvO commented Aug 13, 2023

openssl-machine commented Sep 13, 2023

DDvO commented Oct 7, 2023

t8m left a comment

openssl-machine commented Oct 10, 2023

t8m commented Oct 10, 2023

		if (opt_tls_used)
		(void)OSSL_CMP_CTX_set_option(ctx, OSSL_CMP_OPT_USE_TLS, 1);

CMP: fix flag indicating the use of SSL/TLS for client-side HTTP connections #21176

CMP: fix flag indicating the use of SSL/TLS for client-side HTTP connections #21176

Conversation

DDvO commented Jun 11, 2023 • edited

Checklist

DDvO commented Jun 12, 2023

t8m commented Jun 12, 2023

DDvO commented Jun 12, 2023

t8m commented Jun 12, 2023

DDvO commented Jun 12, 2023 • edited

DDvO commented Jun 12, 2023 • edited

tmshort left a comment

Choose a reason for hiding this comment

tmshort Jun 12, 2023

Choose a reason for hiding this comment

DDvO Jun 12, 2023

Choose a reason for hiding this comment

tmshort Jun 12, 2023

Choose a reason for hiding this comment

DDvO Jun 12, 2023

Choose a reason for hiding this comment

t8m commented Jun 12, 2023

DDvO commented Jun 12, 2023 • edited

t8m commented Jun 12, 2023

DDvO commented Jun 12, 2023

t8m commented Jun 12, 2023

tmshort left a comment • edited

Choose a reason for hiding this comment

DDvO commented Jun 20, 2023

openssl-machine commented Jul 21, 2023

DDvO commented Aug 13, 2023

openssl-machine commented Sep 13, 2023

DDvO commented Oct 7, 2023

t8m left a comment

Choose a reason for hiding this comment

openssl-machine commented Oct 10, 2023

t8m commented Oct 10, 2023

DDvO commented Jun 11, 2023 •

edited

DDvO commented Jun 12, 2023 •

edited

DDvO commented Jun 12, 2023 •

edited

DDvO commented Jun 12, 2023 •

edited

tmshort left a comment •

edited