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

configure: check for the capath by default #11987

Closed
wants to merge 2 commits into from
Closed

configure: check for the capath by default #11987

wants to merge 2 commits into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Sep 29, 2023

... and adapt how wolfSSL uses it.

@michael-o
Copy link
Contributor

michael-o commented Sep 29, 2023 •
edited

Is this an extract of my open PR? Why not merge the PR then? This one misses some bits.

@bagder
Copy link
Member Author

bagder commented Sep 29, 2023

How so? I did not read anything about this in your PR?

@michael-o
Copy link
Contributor

How so? I did not read anything about this in your PR?

My PR introduces CA path support for wolfSSL for the detection, lib and the CLI tool. I have also supplied test case (output) for this.

@bagder
Copy link
Member Author

bagder commented Sep 29, 2023

Ah yes, now I see that you did a partial of this indeed. Sorry, it was not intended. I just fell over this when double-checking how the capath is (not) used when I merged your #11985

@michael-o
Copy link
Contributor

michael-o commented Sep 29, 2023 •
edited

Ah yes, now I see that you did a partial of this indeed. Sorry, it was not intended. I just fell over this when double-checking how the capath is (not) used when I merged your #11985

Maybe we can conclude my PR #11934 first and then you can do additional improvements?

@bagder
Copy link
Member Author

bagder commented Sep 29, 2023

Maybe we can conclude my PR first and then you can do additional improvements?

I believe my fix is a little different and the more complete ca path (check by default) fix, whilst yours is a partial.

@michael-o
Copy link
Contributor

Maybe we can conclude my PR first and then you can do additional improvements?

I believe my fix is a little different and the more complete ca path (check by default) fix, whilst yours is a partial.

They intersect, but your does not cover:

@bagder
Copy link
Member Author

bagder commented Sep 29, 2023

your wolfssl.c change is not the correct one though, see my change

@michael-o
Copy link
Contributor

your wolfssl.c change is not the correct one though, see my change

You mean wolfSSL_CTX_load_verify_locations()? I intentionally did not touch for wolfSSL_CTX_load_verify_locations_ex(() because this changes behavior and didn't move to an rc variable to keep the diff small.

@bagder
Copy link
Member Author

bagder commented Sep 29, 2023

You mean wolfSSL_CTX_load_verify_locations()? I intentionally did not touch for wolfSSL_CTX_load_verify_locations_ex(() because this changes behavior and didn't move to an rc variable to keep the diff small.

It is actually necessary for not changing the behavior. Without that change, wolfSSL can't run on a vanilla Debian that has a capath.

michael-o
michael-o reviewed Sep 29, 2023
View reviewed changes
Copy link
Contributor

@michael-o michael-o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable now for me. I can rebase my PR on top of this when merged into master.

@michael-o
Copy link
Contributor

You mean wolfSSL_CTX_load_verify_locations()? I intentionally did not touch for wolfSSL_CTX_load_verify_locations_ex(() because this changes behavior and didn't move to an rc variable to keep the diff small.

It is actually necessary for not changing the behavior. Without that change, wolfSSL can't run on a vanilla Debian that has a capath.

Not only on Debian, but everywhere where you have expired or invalid CA cers. Noticed this one as well.

@bagder
Copy link
Member Author

bagder commented Sep 29, 2023

Not only on Debian, but everywhere where you have expired or invalid CA cers. Noticed this one as well.

Sure, I meant "in widely used setups". I just happened to test on Debian.

@vszakats

This comment was marked as resolved.

Sign in to view
@bagder
wolfssl: ignore errors in CA path
3f235be 
The default wolfSSL_CTX_load_verify_locations() function is quite picky
with the certificates it loads and will for example return error if just
one of the certs has expired.

With the *_ex() function and its WOLFSSL_LOAD_FLAG_IGNORE_ERR flag, it
behaves more similar to what OpenSSL does by default.

Even the set of default certs on my Debian unstable has several expired
ones.

Assisted-by: Juliusz Sosinowicz
Assisted-by: Michael Osipov

Closes #11987
@bagder
configure: check for the capath by default
ed81eb2 
... if the chosen TLS backend supports it: OpenSSL, GnuTLS, mbedTLS or wolfSSL

cmake: synced

Assisted-by: Viktor Szakats
Closes #11987
@bagder
Copy link
Member Author

bagder commented Sep 30, 2023

Thanks @vszakats and @michael-o for your help on this, I have you both credited in the commits as well as @julek-wolfssl.

@bagder bagder closed this in 463528b Sep 30, 2023
bagder added a commit that referenced this pull request Sep 30, 2023
@bagder
configure: check for the capath by default
849bd50 
... if the chosen TLS backend supports it: OpenSSL, GnuTLS, mbedTLS or wolfSSL

cmake: synced

Assisted-by: Viktor Szakats
Closes #11987
@bagder bagder deleted the bagder/configure-capath branch September 30, 2023 09:20
@michael-o
Copy link
Contributor

Thanks @vszakats and @michael-o for your help on this, I have you both credited in the commits as well as @julek-wolfssl.

Thanks, I will rebase and rephrase my PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael-o michael-o michael-o left review comments

Assignees
No one assigned
Labels
build TLS
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bagder @michael-o @vszakats