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.

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

Link mbedTLS libraries in when SHA1_BACKEND == "mbedTLS" #4678

Merged
merged 3 commits into from
Jun 15, 2018

Conversation

staticfloat
Copy link
Contributor

Over at Julia, we really enjoy the ability to build libgit2 against mbedTLS and completely elide dependency on libssl for cryptographic operations. We noticed that with the release of v0.27.1 there were a few holes in the buildsystem for mbedTLS:

  • The shim routines for sha1 calculation do not actually get added to the list of files to be compiled.

  • If SHA1_BACKEND is set to mbedTLS, but USE_HTTPS is not set to mbedTLS, libmbedtls doesn't get linked properly.

I have taken the liberty of editing the travis build configuration to do more complete mbedTLS testing.

@carlosmn
Copy link
Member

and completely elide dependency on libssl for cryptographic operations

Note that we do not use libssl for SHA-1 by default but instead use a vendored version of SHA-1 which can detect the SHAttered attack.

While I guess this is fine for completeness, are you sure you want to use a library that cannot detect the attack?

@simonbyrne
Copy link
Contributor

I should also clarify: we were using v0.27.1 + the changes from #4173

@staticfloat
Copy link
Contributor Author

Note that we do not use libssl for SHA-1 by default but instead use a vendored version of SHA-1 which can detect the SHAttered attack.

Ah, I did not notice this, as I was last using v0.26.0, which I do not believe did this by default. That's neat! I agree with you; we should be using the collision detection version by default.

For code coverage purposes, I think the .travis.yml changes within this PR still make sense, however.

@pks-t
Copy link
Member

pks-t commented Jun 15, 2018

Yeah, it makes a lot of sense to put this into our existing mbedTLS builds. Thanks for your changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants