Prototype API for fetching os cacerts #5853
Conversation
CT Test Results 3 files 66 suites 45m 19s ⏱️ Results for commit 5dc34cf. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
dgud
force-pushed
the
dgud/pub_key/cacerts/OTP-17798
branch
from
0e1a977 to
fcb3152
Compare
| load_darwin() -> | ||
| %% Temporary solution, should probably be re-written to use Keychain Access API | ||
| KeyChainFile = "/System/Library/Keychains/SystemRootCertificates.keychain", | ||
| TmpPemFile = "/tmp/all_certs.pem", |
There was a problem hiding this comment.
Would this be fixed before this change is merged?
Otherwise I might suggest to use some functions returning temporary file name (or temporary file dir)
There was a problem hiding this comment.
Probably not, this should be good enough, tm.
Please do, this will remain until someone with Obj-C knowledge decides that it is a much better solution,
and updates the nif, writes a configure file and ..
There was a problem hiding this comment.
Does the security export command's -o switch allow - as an argument to export to stdout, removing the use of a temp file? I'd check but I don't have access to OS X.
There was a problem hiding this comment.
Going through Security API would be ideal but in the meantime I think we can improve how we shell out. The default is to write to stdout so going through the temp file should be unnecessary:
> X = os:cmd("security export -t certs -f pemseq -k /System/Library/Keychains/SystemRootCertificates.keychain"), {lists:sublist(X,10), length(X)}.
{"-----BEGIN",249291}| %% Return cacerts | ||
| -spec get() -> [public_key:combined_cert()]. | ||
| get() -> | ||
| case persistent_term:get(?MODULE, not_loaded) of |
There was a problem hiding this comment.
If I understand correctly, this persistent_term would stay in memory forever (even after I unload public_key application).
Another observation, there is no automated "reload" operation (compared to SSL certificate cache that will detect changed files on file system). So long-running systems won't detect CA roots changed (and a manual operation would be needed).
One solution I was thinking of was to add an actual process (supervised by public_key, which will now turn from library with no processes to a library with a process). That would also allow to reuse code from SSL application that stores decoded PEM files (in an ETS table).
There was a problem hiding this comment.
If I understand correctly, this persistent_term would stay in memory forever
True, but do we really need to handle that case?
I.e. add a delete_cacerts() function to the API that will never be called, or do you see another solution.
There is no automated reload, which is what I wanted:
-
We don't want change the public_key from a library application
-
I think I implemented the original cache in ssl and Ingela have been fixing bugs in that since then.
So I want to keep it simple.
The user (should) know when the system is updated and can thus call load/[0|1]
|
About the testing, yeah that could work, will check some names atleast. |
|
This is great to see being added. My only thought is not really a concern for this specific PR but I think it'd be nice if In rebar3 we construct the ssl options for We'd still need to override in case the user specifies a specific cacertfile in the configuration, but for general usage it'd be nice to not have to configure this long list of options to make a secure HTTPS request. |
|
@max-au Extended the tests after your suggestions and added a cacerts_clear() for completion, @tsloughter Yeah that was my intention but should be done but outside of this PR. |
|
Not really an Erlang user myself, but I stumbled upon this patch from |
Perhaps worth mentioning that curl supports |
|
Thanks @dgud !
I also suspect that ssl_pem_cache is where all the certificates end up anyway (unless cache is disabled, but in this case performance degrades too much). One approach I can think of is adding a |
|
Supporting environment variables should be out of the scope of providing easy access to the system's trusted root certificates. |
|
Luke Bakken schreef op zo 10-04-2022 om 12:29 [-0700]:
Supporting environment variables should be out of the scope of
providing easy access to the system's trusted root certificates.
On my system, /etc/ssl/certs sometimes does not exist:
```
# imagine openssl was replaced by some erlang thing here
$ guix shell --container coreutils nss-certs openssl
[env]$ ls /etc
group hosts passwd
[env]$ ls /etc/ssl
ls: cannot access '/etc/ssl': No such file or directory
```
instead, to find the root certificates, you have to look in
SSL_CERT_DIR or SSL_CERT_FILE:
```
[env]$ echo $SSL_CERT_DIR
/gnu/store/3hbqsd3h44fl9ri8wsf4nn56dhh15c51-profile/etc/ssl/certs
[env]$ ls $SSL_CERT_DIR
[lots of certificates]
```
so hardcoding /etc/ssl/certs is not always sufficient.
|
|
@emixa-d thanks for pointing that out. It seems like it would be useful to investigate the following:
|
There are no nifs in ssl, they are in the crypto application.
Who knows what people will do?
No the cache is only used when the cert files are read by ssl, if you use the cacerts argument nothing is cached,
|
I do like that from a useability perspective, but we have had complaints before for that an erlang system The load/1 function can be used for loading an arbitrary file, so the user/application can easily read the environment So for now I don't think we should read it by default. |
|
Dan Gudmundsson schreef op di 12-04-2022 om 00:33 [-0700]:
> Not really an Erlang user myself, but I stumbled upon this patch
> from
> https://issues.guix.gnu.org/54796#52
> Could the SSL_CERT_FILE environment variable be supported?
> That way, non-privileged users could easily override which
> certificates
> to trust.
I do like that from a useability perspective, but we have had
complaints before for that an erlang system
default loads an '.erlang' file and other things like that and that
that can be an security issue that the user
it allowed to change the behavior by default.
I don't follow how this can be a security issue (assuming this is not
about setuid or setgid binaries or the like) -- if a user is malicious,
they could just run a patched erlang that recognises SSL_CERT_DIR.
E.g., not sure how the erlang VM is actually started, but on Guix it
would be something along the lines of:
$ SSL_CERT_FILE=$HOME/evil.crt guix shell erlang --with-source=$HOME/my-patches-erlang.tar.gz -- erlang evil.erlang
This can also be done with an unpatched erlang: just run erlang in a
a container that bind-mounts /etc/ssl/certs to $HOME/evil-certs.
The hypothetical malicious user could also modify which certificates are
used, without using a patched erlang or containers:
1. grab a copy of the source code of the erlang project
2. modify it to look in $HOME/evil-certs
3. compile & run
They could also do their maliciousity without Erlang, e.g. with C and gcc.
|
|
(GitHub seems to have lost this response) Luke Bakken schreef op zo 10-04-2022 om 15:48 [-0700]:
The utility curl looks in AFAIK, the library curl does not look in any environemn variables by
Guix (as in, some of the support libraries like guix/git.scm and I am not aware of any ‘competing’ standards. Git has its own environment variable 'nss' has its own mechanism (some database and not directly files --
If Java recognises any environment variables, I haven't found them. Firefox does not use I'm out of VMs, unless QEMU counts here.
On Guix System, it's /etc/ssl/certs, but only if not running in some On Debian systems, it's /etc/ssl/certs. Apparently, on at least one ArchLinux version, it is /etc/ssl/cert.pem OpenSUSE puts it in yet another location: /var/lib/ca- Apparently FreeBSD puts things in /etc/ssl/cert.pem: |
|
@emixa-d I believe the discussion about security comes from that if you download a binary and run it |
@dgud thanks. I agree, environment variable support should be left up to application developers. |
|
@max-au @lukebakken @wojtekmach Removed the temporary file and read from stdin instead. |
Works great! |
@max-au As a counter-argument: I think the scope of the @dgud Out of curiosity, why add a new NIF to |
|
@potatosalad Because crypto is "thin" wrapper towards "SSLs crypto-lib" and unrelated to this. |
|
(I previously sent this response GitHub by e-mail but it seems to have lost this response) Dan Gudmundsson schreef op di 12-04-2022 om 13:34 [+0000]:
If I, as user, download some binaries and run them, I expect them to If some attacker has compromised my shell process to set some Anyway, OK, but I expect it to be eventually patched downstream because
... AFAICT, often applications don't bother with opting-into |
API: load/0 load/1 and get/0 clear/0 Writes to persistent_term to cache the results, user can re-load cache with load/0 and load/1 functions. load/1 is made for users with an unsupported OS, or if they want to load the cache with data from another file.
And add a simple testcase.
dgud
force-pushed
the
dgud/pub_key/cacerts/OTP-17798
branch
from
7fead27 to
5dc34cf
Compare
API: load/0 load/1 and get/0
Writes to persistent_term to cache the results,
user can re-load cache with load/0 and load/1 functions.
load/1 is made for users with an unsupported OS, or if they
want to load the cache with data from another file.
Currently works on linux (ubuntu) and win32