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

[TLS] Add support for platform-specific CA bundles. #76836

Merged
merged 1 commit into from
May 12, 2023

Conversation

Faless
Copy link
Collaborator

@Faless Faless commented May 8, 2023

Adds a new OS::get_system_ca_certs method which can be implemented by platforms to retrieve the list of trusted CA certificates using OS specific APIs.

The function should return the certificates in PEM format, and is currently implemented for Windows/macOS/LinuxBSD(*)/Android.

mbedTLS will fall back to bundled certificates when the OS returns no certificates.

(*) LinuxBSD does not have a standardized certificates store location. The current implementation will test for common locations and may return an empty string on some distributions (falling back to the bundled certificates).

Implements godotengine/godot-proposals#2970

@cunyx
Copy link

cunyx commented May 9, 2023

Thank you for working on this!

May I ask to check for "_SYSTEM_CERTS_PATH" in function
"OS_LinuxBSD::get_system_ca_certificates"
of file
"platform/linuxbsd/os_linuxbsd.cpp"
before testing distro specific paths?

This would allow to set a path at compile time with higher priority,
regardless of the distribution.
Additionally this gives environments not explicitly listed here a chance to
be covered as well.

Is it possible, to get the path
"/var/lib/ca-certificates/ca-bundle.pem"
included for openSUSE?

Thanks!

@Faless Faless force-pushed the tls/system_certs branch 2 times, most recently from b8e9588 to 8c9f831 Compare May 10, 2023 13:54
@Faless Faless requested a review from m4gr3d May 10, 2023 13:55
@Faless
Copy link
Collaborator Author

Faless commented May 10, 2023

@m4gr3d thanks for the speedy review 🥳 ! I updated the PR with your suggestions.

Is it possible, to get the path "/var/lib/ca-certificates/ca-bundle.pem" included for openSUSE?

@cunyx done, thanks 🥳 !

May I ask to check for "_SYSTEM_CERTS_PATH" in function [...] to set a path at compile time with higher priority [and] give environments not explicitly listed here a chance to be covered as well.

@cunyx I am unsure about this, I see the value for package maintainers but I'm not really sure it's a good solution at large, as it would create an export template only working with a specific distro ( CC @akien-mga , opinions?).

@akien-mga
Copy link
Member

akien-mga commented May 10, 2023

May I ask to check for "_SYSTEM_CERTS_PATH" in function [...] to set a path at compile time with higher priority [and] give environments not explicitly listed here a chance to be covered as well.

@cunyx I am unsure about this, I see the value for package maintainers but I'm not really sure it's a good solution at large, as it would create an export template only working with a specific distro ( CC @akien-mga , opinions?).

It's not portable indeed, but might still be worth checking for these use cases:

  • Distro packages of the editor on distros not covered by the current hardcoded list
  • Distro packages of the runtime on distros not covered by the current hardcoded list (e.g. to run open source Godot games packaged in the distro which would use TLS)
  • Custom builds of Godot for games shipped on controller environments (embedded, arcade or gambling machines) where a custom cert location is wanted

Personally I don't use the feature in my Fedora and Mageia packages for Godot: https://src.fedoraproject.org/rpms/godot/blob/rawhide/f/godot.spec

That being said, the _SYSTEM_CERTS_PATH compile time logic was added specifically as a stopgap solution for the lack of the feature implemented here, so I'm not sure it's really going to be used much. But I see the point about making it possible to inject custom paths in our hardcoded list.

As a reminder, _SYSTEM_CERTS_PATH is used to set the default value for the editor setting network/tls/editor_tls_certificates.
We also have network/tls/certificate_bundle_override with an empty string as default value for the runtime.

@akien-mga
Copy link
Member

BTW when it comes to locating the certs bundle, my distro (Mageia) has these symlinks, I wonder if they're a widespread convention on some other distros so we wouldn't need to hardcode all the distro-specific paths they resolve to?

$ ls -l /etc/pki/tls/cert.pem
lrwxrwxrwx 1 root root 57 May  9 16:22 /etc/pki/tls/cert.pem -> ../../../etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem
$ ls -l /etc/pki/tls/certs/ca-bundle.*
lrwxrwxrwx 1 root root 60 May  9 16:22 /etc/pki/tls/certs/ca-bundle.crt -> ../../../../etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem
lrwxrwxrwx 1 root root 66 May  9 16:22 /etc/pki/tls/certs/ca-bundle.trust.crt -> ../../../../etc/pki/ca-trust/extracted/openssl/ca-bundle.trust.crt

@Calinou
Copy link
Member

Calinou commented May 10, 2023

In the long term, would this PR allow for no longer bundling a certificate bundle on most platforms? This would decrease binary size by a non-negligible amount, while also making binaries more future-proof.

Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

The Android section looks good!

@cunyx
Copy link

cunyx commented May 10, 2023

Thanks for the feedback!

I am unsure about this, I see the value for package maintainers but I'm not really sure it's a good solution at large, as it would create an export template only working with a specific distro).

I would argue, those who want the compiled binary to be portable cross distro,
should not and will not set the build option "system_certs_path".

If "_SYSTEM_CERTS_PATH" is set to the default ("\n" i think) the check in this
function should return "false" and the other hardcoded files will be probed.

Wouldn't this keep portability except builder explicitly opted out by setting
build option "system_certs_path"?

The name of this option seems to somehow imply,
that this will be "system" specific and not portable.

Personally I don't use the feature in my Fedora and Mageia packages for Godot:

It seems you do not unbundle "certs".
Doing this on openSUSE, we are (currently) thankful for the possibility to set a path with a build option.

This is for example needed to load the asset library.

Another use case for checking "_SYSTEM_CERTS_PATH" with highest priority
maybe the wish to override the (here listed) distro default file
with a stricter set of CAs or some self signed root certificate
without having to think about the possibility,
that the distribution provided defaults are still
somehow used as a fallback via this function.

BTW when it comes to locating the certs bundle, my distro (Mageia) has these symlinks, I wonder if they're a widespread convention on some other distros so we wouldn't need to hardcode all the distro-specific paths they resolve to?

In (an older) openSUSE i see a link from
/etc/ssl/ca-bundle.pem to /var/lib/ca-certificates/ca-bundle.pem
but no .pem or .crt bundles in /etc/pki
and no directories /etc/pki/tls and /etc/pki/ca-trust
but a directory /etc/pki/trust (without a ca bundle)

@Faless Faless force-pushed the tls/system_certs branch from 8c9f831 to b88526b Compare May 11, 2023 05:08
@Faless Faless requested a review from a team as a code owner May 11, 2023 05:08
@Faless Faless force-pushed the tls/system_certs branch from b88526b to 4f5d1f0 Compare May 11, 2023 05:43
@Faless
Copy link
Collaborator Author

Faless commented May 11, 2023

@akien-mga @cunyx I added the _SYSTEM_CERTS_PATH check, let me know if I understood correctly the desired behavior.


In the long term, would this PR allow for no longer bundling a certificate bundle on most platforms?

@Calinou Yes, I think Windows/macOS/Android could already unbundle them.

On linuxbsd it's a bit trickier due to each distro having its own paths so we should probably evaluate if we can cover 99% of the users I guess.

The only missing platforms are Web (where we don't need bundling), and iOS/iPadOS/tvOS which as far as I understand does not give developers access to the anchor certificates.

@cunyx
Copy link

cunyx commented May 11, 2023

@Faless
Really nice that you are open and so fast to adapt the code!

I added the _SYSTEM_CERTS_PATH check, let me know if I understood correctly the desired behavior.

As your coding knowledge is much more superior, i can not offer
useful technical comments for your implementation.

If i understand the code logic correctly,
if "_SYSTEM_CERTS_PATH" is non-empty but opening the file fails,
the empty string "" is returned immediately.

I'm not sure if in this case it might be better to probe the remaining explicitly listed files nevertheless.

This will make it perhaps more robust and portable.
"_SYSTEM_CERTS_PATH" would only take priority if it is set at compile time
and pointing to an existing readable file.

But this might be more complex and of course I'm fine with your current solution as well.

Thanks!

Adds a new OS::get_system_ca_certs method which can be implemented by
platforms to retrieve the list of trusted CA certificates using OS
specific APIs.

The function should return the certificates in PEM format, and is
currently implemented for Windows/macOS/LinuxBSD(*)/Android.

mbedTLS will fall back to bundled certificates when the OS returns no
certificates.

(*) LinuxBSD does not have a standardized certificates store location.
    The current implementation will test for common locations and may
    return an empty string on some distributions (falling back to the
    bundled certificates).
@Faless Faless force-pushed the tls/system_certs branch from 4f5d1f0 to 6fd9982 Compare May 12, 2023 07:59
@Faless
Copy link
Collaborator Author

Faless commented May 12, 2023

I'm not sure if in this case it might be better to probe the remaining explicitly listed files nevertheless.

Ah, I see, no problem, done :).

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks great!

@akien-mga akien-mga merged commit 258fabd into godotengine:master May 12, 2023
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

5 participants