Skip to content
Snippets Groups Projects

lib/system/certs.c: Add support for SSL_CERT_DIR, SSL_CERT_FILE

Open lib/system/certs.c: Add support for SSL_CERT_DIR, SSL_CERT_FILE
sundbry/gnutls:ssl-cert-dir into master
5 unresolved threads
Open Ryan Sundberg requested to merge sundbry/gnutls:ssl-cert-dir into master
5 unresolved threads

Resolves issue #1279

Checklist

  • Commits have Signed-off-by: with name/author being identical to the commit author
  • Code modified for feature
  • Test suite updated with functionality tests
  • Test suite updated with negative tests
  • Documentation updated / NEWS entry present (for non-trivial changes)
  • CI timeout is 2h or higher (see Settings/CICD/General pipelines/Timeout)

Reviewer's checklist:

  • Any issues marked for closing are addressed
  • There is a test suite reasonably covering new functionality or modifications
  • Function naming, parameters, return values, types, etc., are consistent and according to CONTRIBUTION.md
  • This feature/change has adequate documentation added
  • No obvious mistakes in the code

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Daiki Ueno
    Daiki Ueno @dueno started a thread on the diff
    • lib/system/certs.c
    116 157 #endif
    117 158
    118 159 #if defined(ENABLE_PKCS11) && defined(DEFAULT_TRUST_STORE_PKCS11)
    119 ret =
    120 gnutls_x509_trust_list_add_trust_file(list,
    121 DEFAULT_TRUST_STORE_PKCS11,
    122 crl_file,
    123 GNUTLS_X509_FMT_DER,
    124 tl_flags, tl_vflags);
    125 if (ret > 0)
    126 r += ret;
    160 if (r == 0) {
    • Daiki Ueno
      Daiki Ueno @dueno
      Owner

      What is the intention behind this check? Besides the fact that r is always 0 here, the following code also has this check, so I wonder if it was to make the trust store implementations (PKCS#11, SSL_CERT_DIR, SSL_CERT_FILE, default trust dir) mutually exclusive. In that case do we need to calculate the sum?

    • Ryan Sundberg
      Ryan Sundberg @sundbry
      Author

      I was trying to be security conscious while merging the two functions together. My thought here was that if the user has some method explicitly configured for the root certs, it should short circuit on the first one and return there. For example, if the user has a PKCS11 module for the root certs, and some certs are loaded from there, the environment variable should not be read (it should ONLY use the pkcs11 certs). Or if they have the environment variable set, and some are loaded, it should bypass loading the default /etc/ssl/certs.

      I don't know how many installations are out there with both DEFAULT_TRUST_STORE_PKCS11 and DEFAULT_TRUST_STORE_FILE configured, if any, where this may affect them if they wanted to load certs from more than one default source concurrently.

    • Ryan Sundberg
      Ryan Sundberg @sundbry
      Author

      Should we make this as a configure option --enable-ssl-env? Without the flag enabled we can keep the existing behavior without the if (r == 0) checks.

      Edited by Ryan Sundberg
    • Please register or sign in to reply
  • Ryan Sundberg added 1 commit

    added 1 commit

    • 9f47634b - certs.c: reorganize ANDROID conditionals

    Compare with previous version

  • Alexander Sosedkin
    Alexander Sosedkin @asosedkin started a thread on the diff
    • lib/system/certs.c
    116 static int load_revoked_certs(gnutls_x509_trust_list_t list,
    117 const char *revoked_certs_dir, unsigned type)
    118 {
    119 DIR *dirp;
    120 struct dirent *d;
    121 int ret;
    122 int r = 0;
    123 char path[GNUTLS_PATH_MAX];
    124
    125 dirp = opendir(revoked_certs_dir);
    126 if (dirp != NULL) {
    127 do {
    128 d = readdir(dirp);
    129 if (d != NULL && d->d_type == DT_REG) {
    130 snprintf(path, sizeof(path),
    131 "/data/misc/keychain/cacerts-removed/%s",
  • Alexander Sosedkin
    Alexander Sosedkin @asosedkin started a thread on the diff
    • lib/system/certs.c
    114 # include <unistd.h>
    115
    116 static int load_revoked_certs(gnutls_x509_trust_list_t list,
    117 const char *revoked_certs_dir, unsigned type)
    118 {
    119 DIR *dirp;
    120 struct dirent *d;
    121 int ret;
    122 int r = 0;
    123 char path[GNUTLS_PATH_MAX];
    124
    125 dirp = opendir(revoked_certs_dir);
    126 if (dirp != NULL) {
    127 do {
    128 d = readdir(dirp);
    129 if (d != NULL && d->d_type == DT_REG) {
  • Alexander Sosedkin
    Alexander Sosedkin @asosedkin started a thread on the diff
    • lib/system/certs.c
    199 r += ret;
    200 }
    127 201 #endif
    128 202
    129 203 #ifdef DEFAULT_TRUST_STORE_FILE
    130 ret =
    131 gnutls_x509_trust_list_add_trust_file(list,
    132 DEFAULT_TRUST_STORE_FILE,
    133 crl_file,
    134 GNUTLS_X509_FMT_PEM,
    135 tl_flags, tl_vflags);
    136 if (ret > 0)
    137 r += ret;
    204 if (r == 0) {
    205 ret =
    206 gnutls_x509_trust_list_add_trust_file(list,
  • Alexander Sosedkin
    Alexander Sosedkin @asosedkin started a thread on the diff
    • lib/system/certs.c
    125 if (ret > 0)
    126 r += ret;
    162 if (r == 0) {
    163 ret =
    164 gnutls_x509_trust_list_add_trust_file(list,
    165 DEFAULT_TRUST_STORE_PKCS11,
    166 crl_file,
    167 GNUTLS_X509_FMT_DER,
    168 tl_flags, tl_vflags);
    169 if (ret > 0)
    170 r += ret;
    171 }
    172 #endif
    173
    174 if (r == 0) {
    175 const char *ssl_cert_dir = getenv("SSL_CERT_DIR");
    Please register or sign in to reply