Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HTTP client.c: fix OSSL_HTTP_proxy_connect() for HTTPS proxy use #15796

Closed
wants to merge 5 commits into from
Closed

HTTP client.c: fix OSSL_HTTP_proxy_connect() for HTTPS proxy use #15796

wants to merge 5 commits into from

Conversation

DDvO
Copy link
Contributor

@DDvO DDvO commented Jun 17, 2021 •
edited
Loading

I was not happy that many TODOs also for CMP code were removed (during my vacation) in PR #15539 without actually solving them.
I've started tracing those TODOs for which I feel responsible.

I just found out that the answer on the TODO is the server host name correct for TLS via proxy?, which was removed from apps/cmp.c, is "yes",
but while experimenting on this I encountered a bug that leads to connection failure (with the misleading error: received wrong http version) when the HTTP client tries to CONNECT using TLS via a proxy. This is fixed by the present PR.

@DDvO DDvO added approval: otc review pending triaged: bug The issue/pr is/fixes a bug labels Jun 17, 2021
@DDvO DDvO added this to the 3.0.0 milestone Jun 17, 2021
@DDvO DDvO mentioned this pull request Jun 17, 2021
Closed
2 tasks
@paulidale
Copy link
Contributor

Apologies for removing them. Most didn't seem all that relevant...

@DDvO DDvO force-pushed the fix_OSSL_HTTP_proxy_connect branch from cc2cd32 to a5bea2f Compare June 17, 2021 06:25
@t8m t8m added approval: done This pull request has the required number of approvals and removed approval: otc review pending labels Jun 17, 2021
paulidale
paulidale previously approved these changes Jun 17, 2021
View reviewed changes
@DDvO
Copy link
Contributor Author

DDvO commented Jun 17, 2021

Argh, doing the fix I introduced another bug - fixed now.

@t8m t8m dismissed paulidale’s stale review June 17, 2021 14:10

Substantial changes

@t8m t8m added approval: done This pull request has the required number of approvals and removed approval: done This pull request has the required number of approvals labels Jun 17, 2021
openssl-machine pushed a commit that referenced this pull request Jun 18, 2021
@DDvO
http_client.c: fix OSSL_HTTP_proxy_connect() for HTTPS proxy use
8df299d 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #15796)
openssl-machine pushed a commit that referenced this pull request Jun 18, 2021
@DDvO
http_client.c: fix error reporting (a char was missing; improve style)
552aeae 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #15796)
openssl-machine pushed a commit that referenced this pull request Jun 18, 2021
@DDvO
http_client.c: make prefix checking more readable and more efficient
b6fec96 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #15796)
openssl-machine pushed a commit that referenced this pull request Jun 18, 2021
@DDvO
http_client.c: make HTTP_LINE1_MINLEN more efficient
765860a 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #15796)
openssl-machine pushed a commit that referenced this pull request Jun 18, 2021
@DDvO
http_client.c: fix HTTP_VERSION_STR_LEN and make it more efficient
47bb597 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #15796)
@DDvO
Copy link
Contributor Author

DDvO commented Jun 18, 2021

Merged - thanks @t8m and @paulidale for your reviews.

@DDvO
Copy link
Contributor Author

DDvO commented Jun 18, 2021 •
edited
Loading

BTW, since the pattern for checking if a string is prefixed by a string literal, such as

strncmp(algo, "ecdsa", 5) == 0

is pretty common also elsewhere in OpenSSL code, how about moving

+#define HAS_PREFIX(str, prefix) (strncmp(str, prefix, sizeof(prefix) - 1) == 0)

which I introduced locally in http_client.c, to macros.h and making use of it globally?

This would make the code more readable and

  • safer, in case the length constant is determined manually
  • more efficient, in case it is calculated using strlen()
  • more clear and less error-prone, in case sizeof is used.

@paulidale
Copy link
Contributor

Adding that macro more widely seems like a reasonable idea. I'd modify it slightly:

#define HAS_PREFIX(str, prefix) (strncmp(str, prefix "", sizeof(prefix) - 1) == 0)

this will error if prefix isn't a string constant. Otherwise badness could happen sizeof(char *) - 1 could walk past the end of a string and will almost certainly not be the desired length.

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@DDvO DDvO mentioned this pull request Jun 21, 2021
Closed
@t8m
Copy link
Member

t8m commented Jun 23, 2021

Closing as this was merged already.

@t8m t8m closed this Jun 23, 2021
devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021
@DDvO @devnexen
http_client.c: fix OSSL_HTTP_proxy_connect() for HTTPS proxy use
b0357d0 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#15796)
devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021
@DDvO @devnexen
http_client.c: fix error reporting (a char was missing; improve style)
5de642d 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#15796)
devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021
@DDvO @devnexen
http_client.c: make prefix checking more readable and more efficient
421401e 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#15796)
devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021
@DDvO @devnexen
http_client.c: make HTTP_LINE1_MINLEN more efficient
00bdb4f 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#15796)
devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021
@DDvO @devnexen
http_client.c: fix HTTP_VERSION_STR_LEN and make it more efficient
8de5c11 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#15796)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@paulidale paulidale paulidale approved these changes

Assignees
No one assigned
Labels
approval: done This pull request has the required number of approvals triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@DDvO @paulidale @openssl-machine @t8m