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

curl: support embedding a CA bundle #14059

Closed
wants to merge 20 commits into from
Closed

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Jun 29, 2024

Add the ability to embed a CA bundle into the curl binary. It is used
when no other runtime or build-time option set one.

This helps curl-for-win macOS and Linux builds to run standalone, and
also helps Windows builds to avoid picking up the CA bundle from an
arbitrary (possibly world-writable) location (though this behaviour is
not currently disablable).

Usage:

  • cmake: -DCURL_CA_EMBED=/path/to/curl-ca-bundle.crt
  • autotools: --with-ca-embed=/path/to/curl-ca-bundle.crt
  • Makefile.mk: CURL_CA_EMBED=/path/to/curl-ca-bundle.crt

Also add new command-line option --dump-ca-embed to dump the embedded
CA bundle to standard output.

Closes #14059


TODO:

Ideas:

  • gzipped embedding? (230KB → 130KB)
    with reusable unzipper function inside src/tool_something.c
  • move hugehelp to use that too? (ATM the unzip code is part of the generated source)
  • move up cacert embedding to libcurl?
  • strip CR bytes before embedding for better reproducibility?
  • generate the dummy tool_ca_embed.c also in CMake/Makefile.mk to keep binary output in sync? (though the dummy code is likely discarded by the linker in autotools builds.)
  • build-time option to disable FindWin32CACert() feature? → curl: add options for safe/no CA bundle search (Windows) #14582

@vszakats vszakats added TLS cmdline tool feature-window A merge of this requires an open feature window labels Jun 29, 2024
@vszakats vszakats marked this pull request as draft June 29, 2024 17:21
@vszakats vszakats marked this pull request as ready for review June 29, 2024 17:43
@vszakats vszakats force-pushed the ca-embed branch 2 times, most recently from 6fe2b3c to 778f760 Compare June 29, 2024 20:14
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

It might make sense to display this as a feature in curl -V so that a user can tell if it has a cacert bundle built-in or not.

src/tool_operate.c Show resolved Hide resolved
@vszakats vszakats force-pushed the ca-embed branch 3 times, most recently from 3a3db00 to 21808ed Compare June 29, 2024 23:09
@vszakats

This comment was marked as resolved.

vszakats added a commit to curl/curl-for-win that referenced this pull request Jul 1, 2024
@bagder
Copy link
Member

bagder commented Jul 1, 2024

Do we want to fall back to the old malloc-less feature display in case malloc() fails?

When malloc fails (which is basically not possible on most modern systems) we should bail out. Not try something else. IMHO.

@vszakats
Copy link
Member Author

vszakats commented Jul 1, 2024

Thanks, I agree, leaving it as-is.

On another question: Do we want a --dump-embedded-cacert option (which would write the embedded CA bundle to stdout and quit)? Any preference for the option name?

It'd be useful for transparency, i.e. to see the trusted roots without peeking into the binary with a capable viewer (or strings). → added as --dump-ca-embed.

@bagder
Copy link
Member

bagder commented Jul 1, 2024

Do we want a --dump-embedded-cacert option (which would write the embedded CA bundle to stdout and quit)? Any preference for the option name?

I think such a command line option makes a lot of sense, yes. I don't have any better name suggestion!

@vszakats
Copy link
Member Author

vszakats commented Jul 1, 2024

I went with --dump-ca-embed. Rhymes with the rest of new symbols added. Not too long.

@vszakats vszakats force-pushed the ca-embed branch 2 times, most recently from b85cd0b to c36e110 Compare July 1, 2024 23:10
@vszakats vszakats changed the title curl: add support for embedding a CA bundle curl: support embedding a CA bundle Jul 2, 2024
vszakats added a commit to curl/curl-for-win that referenced this pull request Jul 2, 2024
And use it when available.

It enables this feature in the active PR:
curl/curl#14059
@vszakats
Copy link
Member Author

vszakats commented Jul 2, 2024

Patch that allows disabling the Windows-specific PATH-search for a CA bundle:

--- a/src/tool_doswin.c
+++ b/src/tool_doswin.c
@@ -621,7 +621,7 @@ CURLcode FindWin32CACert(struct OperationConfig *config,
 {
   CURLcode result = CURLE_OK;
 
-#ifdef CURL_WINDOWS_APP
+#if defined(CURL_WINDOWS_APP) || defined(CURL_WINDOWS_SKIP_SEARCHPATH_CACERT)
   (void)config;
   (void)backend;
   (void)bundle_file;

This option becomes useful when combined with embedded bundles, as it prevents curl from picking one up from a "random" disk location, including world-writable ones. It still allows to specify one manually either via the documented envs or command-line / .curlrc options, overriding the embedded default.

Does this make sense? Does it raise any concern?

@vszakats
Copy link
Member Author

vszakats commented Jul 3, 2024

Embedded certs is now tested via curl-for-win. Also in this PR, after this commit and re-running the workflow: curl/curl-for-win@b4945dd

@bagder
Copy link
Member

bagder commented Jul 5, 2024

gzipped embedding? (230KB → 130KB)

A downside with compressing this:

Since the TLS libraries cannot use the blob compressed, this would then have to use 230KB allocated memory to store the cacert bundle in, in addition to the 130KB compressed read-only memory already used for the compressed image. Thus spending more total memory in run-time than doing it completely uncompressed will need...

TODO:
- gzipped embedding
  with static unzipper inside src/tool_something.c
  move hugehelp to use that too?
- move up to libcurl?
cmake: skip embed when not building the curl tool

cmake fixup

cmake: cleanup

cmake: add option
VS2008:
```
>src/tool_help.c(221) : warning C4090: 'function' : different 'const' qualifiers
>src/tool_help.c(227) : warning C4090: 'function' : different 'const' qualifiers
>src/tool_help.c(232) : warning C4090: 'function' : different 'const' qualifiers
```
Ref: https://ci.appveyor.com/project/curlorg/curl/builds/50120824/job/d0pxatoxsifm4tll#L130

VS2010:
```
src/tool_help.c(221): error C2220: warning treated as error - no 'object' file generated [_bld\src\curl.vcxproj]
src/tool_help.c(221): warning C4090: 'function' : different 'const' qualifiers [_bld\src\curl.vcxproj]
src/tool_help.c(227): warning C4090: 'function' : different 'const' qualifiers [_bld\src\curl.vcxproj]
src/tool_help.c(232): warning C4090: 'function' : different 'const' qualifiers [_bld\src\curl.vcxproj]
```
Ref: https://ci.appveyor.com/project/curlorg/curl/builds/50120824/job/3i9kby4p0n7mgl6d#L106
@vszakats vszakats closed this in 8a3740b Aug 3, 2024
@vszakats vszakats deleted the ca-embed branch August 3, 2024 07:25
@gvanem
Copy link
Contributor

gvanem commented Aug 3, 2024

Compiling with -DCURL_CA_EMBED and a curl.exe --dump-ca-embed works fine. But
compiling w/o -DCURL_CA_EMBED and the same command prints nothing.

Is this intentional? Why not a "--dump-ca-embed option was disabled at build-time" (as for --manual)

@vszakats
Copy link
Member Author

vszakats commented Aug 3, 2024

Thanks for your feedback.

My thinking is that an empty output is a clear sign that there is no embedded CA. Unlike --manual, I expect this output to be more often used in scripts or redirected, instead of being read by humans on the terminal. For those uses an stderr output might be more of a burden than useful (though an errorlevel might help?). All in all, no strong opinion here.

This feature is brand new and I guess time/feedback/contributions will hint what would yet be useful to tweak around it.

vszakats added a commit to curl/curl-for-win that referenced this pull request Aug 6, 2024
vszakats added a commit that referenced this pull request Sep 12, 2024
Add missing rule dependency on the user-specified CA bundle. This fixes
including it when using the curl distro tarball, and other cases.

Also:
- fix the internal name of the CA bundle to avoid nested quotes.
  It broke broke the rule dependency for the make tool.
- exclude the generated (empty) `tool_ca_embed.c` file from the distro
  tarball.
  Patch-by: Daniel Stenberg

Follow-up to 8a3740b #14059
Reported-by: rampageX on github
Fixes #14879
Closes #14882
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmdline tool enhancement feature-window A merge of this requires an open feature window TLS
Development

Successfully merging this pull request may close these issues.

4 participants