When using this module with a Sentry server that has a self-signed certificate, there isn't a way to tell the Raven PHP library not to verify the SSL certificate of the server. This means that messages don't get submitted to Sentry, and fail silently (no SSL error is logged in PHP or Drupal watchdog).

I've created a patch which adds a checkbox option to the Sentry admin form allowing the verify_ssl option to be set. It defaults to "True" (SSL certificates must be verified).

Another avenue for exploration is to perform a test message when a Drupal status report is generated, which would provide a way of verifying that this module can successfully contact Sentry (rather than just indicating whether or not it's enabled/configured).

CommentFileSizeAuthor
#27 2653134-d8-27.patch3.59 KBmfb
#25 2653134-d8-25.patch5.34 KBmfb
#21 2653134-d8-3.patch3.65 KBdakku
#18 2653134-d8-2.patch3.59 KBdakku
#15 2653134-d8-1.patch3.72 KBdakku
#8 raven-2653134-add_connection_settings_fields.patch3.42 KBmfb
#5 raven-add_connection_settings_fields-7.x.png50.06 KBjustinstandring
#5 raven-add_connection_settings_fields-7.x.patch4.04 KBjustinstandring
raven-add_verify_ssl_option-7.x.patch1.27 KBjustinstandring

Comments

justinstandring created an issue. See original summary.

Status: Needs review » Needs work

The last submitted patch, raven-add_verify_ssl_option-7.x.patch, failed testing.

justinstandring’s picture

justinstandring commented
Status: Needs work » Needs review

Updating this to 'Needs Review' - the test-bot is currently throwing false results: https://www.drupal.org/node/2645590

mfb’s picture

mfb
they or he
commented

From a security/authenticity perspective it would actually be better to allow the site admin to provide a CA cert via the ca_cert option rather than disabling SSL verification.

justinstandring’s picture

justinstandring commented
StatusFileSize
new raven-add_connection_settings_fields-7.x.patch4.04 KB
new raven-add_connection_settings_fields-7.x.png50.06 KB

Good point. I've made a new patch that provides three options for SSL verification:

  1. Verify SSL
  2. Verify against a CA certificate
  3. Don't verify SSL (not recommended)

When 'Verify against a CA certificate' is selected, a textfield is shown to enter the path to the certificate file. I've grouped these options with 'Timeout' under a fieldset called 'Connection Settings'. I've attached a screenshot of this.

Again, the module defaults to verifying SSL by default (and if the certificate file is non-existent).

Status: Needs review » Needs work

The last submitted patch, 5: raven-add_connection_settings_fields-7.x.patch, failed testing.

mfb’s picture

mfb
they or he
commented
Status: Needs work » Needs review

ok I added some minimal tests so patches will be green now.

mfb’s picture

mfb
they or he
commented
StatusFileSize
new raven-2653134-add_connection_settings_fields.patch3.42 KB

a bit of refactoring, and make the connection settings collapsed by default.

mfb’s picture

mfb
they or he
commented

@justinstandring have you verified that the CA cert setting is working?

justinstandring’s picture

justinstandring commented

Yes - I've tested all three options

  • mfb committed 4f3119c on 7.x-1.x 
    Issue #2653134 by justinstandring, mfb: Add ca_cert and verify_ssl...
mfb’s picture

mfb
they or he
commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Needs review » Patch (to be ported)

  • mfb committed 4f3119c on 7.x-2.x 
    Issue #2653134 by justinstandring, mfb: Add ca_cert and verify_ssl...
mfb’s picture

mfb
they or he
commented
Version: 8.x-1.x-dev » 8.x-2.x-dev
dakku’s picture

dakku commented
StatusFileSize
new 2653134-d8-1.patch3.72 KB

Please see quick port to D8 branch.

dakku’s picture

dakku commented
Status: Patch (to be ported) » Needs review
mfb’s picture

mfb
they or he
commented
Status: Needs review » Needs work

We don't need raven_ - it's redundant as we're already in raven.settings

dakku’s picture

dakku commented
StatusFileSize
new 2653134-d8-2.patch3.59 KB

@mfb here is a re-rolled one..

dakku’s picture

dakku commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 18: 2653134-d8-2.patch, failed testing. View results

dakku’s picture

dakku commented
Status: Needs work » Needs review
StatusFileSize
new 2653134-d8-3.patch3.65 KB

re-rolled against latest dev..

mfb’s picture

mfb
they or he
commented

LGTM. have you tested all the options?

dakku’s picture

dakku commented

@mgf seemed good one my side :)
Do let me know if you come across something!

mfb’s picture

mfb
they or he
commented
Status: Needs review » Needs work

Can you use dependency injection for \Drupal::service('file_system')?

FYI to catch this you can run phpcs --standard=DrupalPractice .

(yes there is actually one other place in RavenConfigForm.php where \Drupal slipped in..)

mfb’s picture

mfb
they or he
commented
Status: Needs work » Needs review
StatusFileSize
new 2653134-d8-25.patch5.34 KB

Status: Needs review » Needs work

The last submitted patch, 25: 2653134-d8-25.patch, failed testing. View results

mfb’s picture

mfb
they or he
commented
Status: Needs work » Needs review
StatusFileSize
new 2653134-d8-27.patch3.59 KB

Turns out we cannot use the file_system service when initializing a logger, because file_system depends on logger.

So, as a work-around for now I am simply calling realpath() function rather than the realpath() method.

  • mfb committed 0472672 on 8.x-2.x 
    Issue #2653134 by dakku, mfb: Add checkbox to set the verify_ssl...
mfb’s picture

mfb
they or he
commented
Status: Needs review » Fixed

Committed! Please update this issue if you find any trouble w/ the new feature.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.