Skip to content

ssl: ability to add raw X509 certs to the cert store #6877

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

Merged
merged 8 commits into from
Oct 2, 2024

Conversation

yerseg
Copy link
Contributor

@yerseg yerseg commented Sep 6, 2024

There are custom cert stores on macOS and Windows (Apple Keychain, Windows Credentials Manager). In our product we enumerate certs in the stores and add them to SSL cert store. It seems like useful feature at all.

@ethomson
Copy link
Member

ethomson commented Sep 6, 2024

Just to be clear - this adds it to the in-memory certificate store, this doesn't mutate any files on-disk. In other words, when I start some session, it gets the default on-disk information; and you can add to and enrich this information for only the lifetime of the application.

This makes sense to me as a useful feature. Questions:

  1. Can you enhance the documentation to illustrate that this is not saved to disk and that the certificate is now valid for the lifetime of the application? (There's no way to "clear" the certificate store, and I don't think that we need such functionality; it would be good to be a bit more explicit since certs could represent a lot of trust.)
  2. Are you intending to add similar support for macOS and Windows, or only OpenSSL?

@yerseg
Copy link
Contributor Author

yerseg commented Sep 9, 2024

Thank you for review!

  1. I've enhanced the doc. Seems like it is enough information about a cert lifetime.
  2. I've not fully understood your point :( What did you mean? This feature only allows you to add raw certificates. The responsibility for obtaining the certificates, however, lies with the client code and not the library.

@ethomson
Copy link
Member

ethomson commented Sep 9, 2024

Thanks; I think that's a helpful improvement!

I've not fully understood your point :( What did you mean? This feature only allows you to add raw certificates. The responsibility for obtaining the certificates, however, lies with the client code and not the library.

In your initial comment, you mentioned that "there are custom cert stores on macOS and Windows"; but I don't understand what that comment has to do with this PR.

In your application, are you using those custom cert stores on macOS and Windows directly, and then libgit2 makes use of them just because that's the architecture of those libraries? But there's no real way to do this on OpenSSL, which is why you're creating this PR?

Or are there custom stores on those systems, but this PR doesn't address those?

This is just my ignorance about how these custom certificate systems work on all those platforms, I'm hoping to get some more of this knowledge about how you're working.

Thanks!

@yerseg
Copy link
Contributor Author

yerseg commented Sep 9, 2024

Sorry for the misunderstanding. I mentioned certificate stores to provide context for the feature. Let me explain further:

On macOS, there's the Apple Keychain, and on Windows, the Windows Credential Manager. Both offer APIs to list certificates in the store and retrieve them in X509 format, which our application does.

Once we retrieve the certificates, we need to add them to the SSL backend in libgit2. My changes make this possible.

@ethomson
Copy link
Member

ethomson commented Sep 9, 2024

Oh! You're building with OpenSSL on all platforms and you're pulling carts out of the various platform specific subsystems and then feeding them into OpenSSL. Is that roughly accurate?

I had assumed that you were building with OpenSSL on Linux (and using the win32 and Mac TLS stacks on windows and Mac, respectively).

That all makes sense; like I said, just curious about your use case and how / or if we should think about this setting for Mac and windows.

@ethomson
Copy link
Member

ethomson commented Sep 9, 2024

(No need to apologize btw, I was making assumptions.)

@yerseg
Copy link
Contributor Author

yerseg commented Sep 9, 2024

Yes, you’re right. We’re building with openssl on all platforms (win, mac, linux) because of our app specific. We don’t like the OS specific ssl stacks for less transparency.
Also when building libgit2 we use patched cmake-scripts with customized openssl target because of static linking.
I don’t know is it possible to provide full support of certificates managing with openssl on mac and win. It will lead usage of the additional platform APIs and so I am not expecting that kind of support in the libgit.

@ethomson
Copy link
Member

Question: does this compile with cmake -DUSE_HTTPS=OpenSSL-Dynamic? I think that we might need to load X509_STORE_add_cert symbol there.

@yerseg
Copy link
Contributor Author

yerseg commented Sep 30, 2024

Question: does this compile with cmake -DUSE_HTTPS=OpenSSL-Dynamic? I think that we might need to load X509_STORE_add_cert symbol there.

Fixed in commit below.

@yerseg
Copy link
Contributor Author

yerseg commented Sep 30, 2024

I have also added a test that is similar to others in the online::customcert test suite. However, I need assistance setting up an additional test server hosted at https://test.libgit2.org:3443/ and generating a test cert. This cert must replace the template self-signed.pem.raw I've already placed in the repo.

We provide functionality for callers to mutate the SSL context. We want
to test these various mutations, but OpenSSL does not provide a
mechanism to _reset_ these mutations (short of burning down the SSL
context). Provide an internal mechanism to reset the state for our own
tests.
Update the custom cert tests to test various custom certificates.
The OpenSSL certificate setting functions _may_ interact; try to
document that a bit better.
@ethomson
Copy link
Member

ethomson commented Oct 1, 2024

I needed the cert and the key, so I created a new one for test.libgit2.org and updated the PR. I also noticed that the custom cert tests would fail unless you cleared the context between runs, so I added a (currently) internal function to do that.

This passes the CI runs - please take a look and make sure that I didn't break anything?

@yerseg
Copy link
Contributor Author

yerseg commented Oct 2, 2024

Your changes look fine. Thanks for help!

@ethomson ethomson merged commit 751c68f into libgit2:main Oct 2, 2024
34 checks passed
@ethomson
Copy link
Member

ethomson commented Oct 2, 2024

Thanks @yerseg !

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

Successfully merging this pull request may close these issues.

2 participants