Skip to content

Allow libgit2 to be compiled with mbedtls3. #6759

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
Merged
merged 5 commits into from
Mar 16, 2024
Merged

Allow libgit2 to be compiled with mbedtls3. #6759

merged 5 commits into from
Mar 16, 2024

Conversation

adamharrison
Copy link
Contributor

OK; turns out there were relatively minimal changes required to have this happen.

Some background and reason for the changes:

  1. mbedtls/config.h has been replaced by mbedtls/build_info.h mbedtls3. In either event, this file does not need to be included directly, as it is included by default in all lower other mbedtls headers.
  2. The internals of the ssl_conf have been flagged as private. In theory, these shouldn't have been used directly in the first place, I think. However, given that we are using them, I decided to not change the system, and simply attach the appropriate prefix.

I think that's all we need. It compiles for me. Any thoughts?

@adamharrison adamharrison marked this pull request as draft March 13, 2024 21:36
@adamharrison
Copy link
Contributor Author

adamharrison commented Mar 14, 2024 •
edited
Loading

It looks like this will actually require an update to ntlmclient; as it stands, it depends on md4, which is removed from mbedtls3. We can probably include that algorithm statically in the worst case, copied from mbedtls2. I'll open a PR there too.

So, it does look like this library is optional. Would you like me to PR mbedtls3 changes there as well, or ignore? And, I guess, if I ignore, should I adjust the cmake file to automatically set USE_NTLMCLIENT to OFF, if not specified, in the case we are using mbedtls3?

ethomson added 3 commits March 16, 2024 14:31
@ethomson
cli: use libgit2 system includes
5940639 
libgit2 may adjust the system includes - for example, to locate a
dependency installed in a funny location. Ensure that the cli
understands those include paths as well.
@ethomson
build: update ntlmclient dependency
0798617 
The ntlmclient dependency now bundles an MD4 implementation for
compatibility with newer mbedTLS. Include that so that we can support
mbedTLS 3.
@ethomson
mbedtls: keep track of what we allocate, then free it
3266fc4 
Instead of trying to allocate something, hand it to mbedTLS, and then
peer into its private data structures to try to free that thing... we
could just keep track of it ourselves.

Once we've done that, we needn't do an allocation _anyway_, we can just
keep it on the stack.
@ethomson
Copy link
Member

Thanks for opening this! I agree that freeing things in the private data structure is not the right thing to do. Looking at it a bit closer, we don't even need to heap allocate that data. I moved it all to static data.

I also updated ntlmclient to cope with the lack of MD4 in mbedTLS.

I pushed up those changes on top of your PR branch. Thanks again!

@ethomson ethomson marked this pull request as ready for review March 16, 2024 18:09
@ethomson ethomson merged commit 47b7d72 into libgit2:main Mar 16, 2024
@ethomson
Copy link
Member

Thanks @adamharrison!

@ethomson ethomson added the build label Mar 16, 2024
@branchvincent branchvincent mentioned this pull request Mar 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
build
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@adamharrison @ethomson