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

dev/core#2726 - Set ssl option when using DSN with SSL #20974

Merged
merged 1 commit into from
Jul 30, 2021
Merged

dev/core#2726 - Set ssl option when using DSN with SSL #20974

merged 1 commit into from
Jul 30, 2021

Conversation

erawat
Copy link
Contributor

@erawat erawat commented Jul 29, 2021

Overview

Refer to issue https://lab.civicrm.org/dev/core/-/issues/2726.

DB::connect() function requires an options array that define ssl=true for mysqli to connect to the CiviCRM database over ssl connection.

This PR adds logic to check if DSN is using SSL then pass the option ssl=true

Before

Exception was thrown when using DSN with SSL to connect the database.

Cannot open mysqli://user:password@local:3306/civicrm_db?new_link=true&ca=%2Fetc%2Fmysql%2Fdatabase-certificate.pem: DB Error: connect failed

After

No error is thrown when using DSN with SSL.

@civicrm-builder
Copy link

Can one of the admins verify this patch?

@civibot
Copy link

civibot bot commented Jul 29, 2021

(Standard links)

@civibot civibot bot added the master label Jul 29, 2021
@seamuslee001
Copy link
Contributor

Jenkins add to white list

@seamuslee001
Copy link
Contributor

Jenkins add to whitelist

@seamuslee001
Copy link
Contributor

this seems fine to me @demeritcowboy do you think this will work in this situation or do we need code more like https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/DAO.php#L202

@demeritcowboy
Copy link
Contributor

This is probably fine I was just going to test it since it's been a year since I last looked at this. In my previous WIP on this I had written a blurb as to why I think this is different from the DAO and why it should NOT do something like the pear static: https://github.com/civicrm/civicrm-core/pull/18125/files

@erawat
Copy link
Contributor Author

erawat commented Jul 30, 2021

@seamuslee001 @demeritcowboy Is there any other things I could do to get this PR merged?

@demeritcowboy
Copy link
Contributor

Thanks for kickstarting this again. Normally I'd suggest writing a unit test but since the test nodes don't use SSL that won't work. As an alternative you could review someone else's PR.

@demeritcowboy
Copy link
Contributor

  • General standards
    • [r-explain] Comment
      • I was a little confused because the lab ticket talks about civicrm-setup and links to an outdated github repo, whereas civi installs fine with mysql ssl if using either cv or for example in drupal enabling from the modules page. Where the problem comes up would most commonly be seen during civi upgrade.
    • [r-user] PASS
    • [r-doc] PASS
    • [r-run] PASS
  • Developer standards
    • [r-tech] PASS
    • [r-code] PASS
    • [r-maint] N/A
    • [r-test] PASS

@demeritcowboy demeritcowboy merged commit a3b19ec into civicrm:master Jul 30, 2021
@jamienovick
Copy link

Thanks so much for reviewing @demeritcowboy

@erawat
Copy link
Contributor Author

erawat commented Jul 30, 2021

  • General standards

    • [r-explain] Comment

      • I was a little confused because the lab ticket talks about civicrm-setup and links to an outdated github repo, whereas civi installs fine with mysql ssl if using either cv or for example in drupal enabling from the modules page. Where the problem comes up would most commonly be seen during civi upgrade.
    • [r-user] PASS

    • [r-doc] PASS

    • [r-run] PASS

  • Developer standards

    • [r-tech] PASS
    • [r-code] PASS
    • [r-maint] N/A
    • [r-test] PASS

@demeritcowboy Thank you so much for reviewing and merging this.

just to clarify and for reference on how we could produce the issue with civicrm-setup, as we did not use cv core:install command nor enable civicrm from the drupal page. We programmatically install CiviCRM using Civi/Setup class and we got the issue.

See below a snippet how the installation was done.

  $setup = Setup::instance();
    $model = $setup->getModel();
    $model->cmsBaseUrl = $this->getBaseUrl();
    $model->db = [
      'server' => "{$civiDbSettings['host']}:{$civiDbSettings['port']}",
      'username' => $civiDbSettings['username'],
      'password' => $civiDbSettings['password'],
      'database' => $civiDbSettings['database'],
    ];
   $model->db['ssl_params'] = $sslParams;
   $model->cmsDb['ssl_params'] = $sslParams;
   $setup->installFiles();
   $setup->installDatabase();

@demeritcowboy
Copy link
Contributor

Ok thanks - that's interesting because that's very similar to what cv core:install does. Hmm.

erawat added a commit to compucorp/civicrm-core that referenced this pull request Jul 30, 2021
erawat added a commit to compucorp/civicrm-core that referenced this pull request Jul 30, 2021
@erawat erawat deleted the core#2762 branch August 3, 2021 19:21
erawat added a commit to compucorp/civicrm-core that referenced this pull request Sep 14, 2021
erawat added a commit to compucorp/civicrm-core that referenced this pull request Sep 14, 2021
erawat added a commit to compucorp/civicrm-core that referenced this pull request Sep 14, 2021
erawat added a commit to compucorp/civicrm-core that referenced this pull request Sep 14, 2021
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.

5 participants