CAS needs to make at least one headless HTTP request (we use cURL) to the CAS server during ticket validation. It seems that historically, this module has provided a setting for the user to provide a path to the public certificate file for the certificate authority that signed the TLS certificate used by the CAS server. This file is then used to validate the authenticity of the CAS server during any cURL requests we make. If the user doesn't specify a path, then the CAS server is not verified to be authentic.

I think we can improve this behavior a bit. The problem I see here is that our default behavior is to NOT perform validation of the CAS server. Non technical users will be more than happy to not investigate this option further and now their authentication system is flawed.

I propose we present the following radio buttons:

[ ] Verify CAS server, use default CA chain file (selected by default)
[ ] Verify CAS server, use specific CA PEM file
[ ] Do not verify CAS server (not recommended)

If the 2nd option is selected we provide a text field (like we do now) to provide the path to the PEM file. With the first option, we essentially just tell cURL that yes, we want to verify the peer, but we don't have a specific CA chain file to use. cURL should then use a default one that's located somewhere on the file system.

I think we can provide some additional explanation text as well, to help non technical users understand what these mean.

CommentFileSizeAuthor
#17 interdiff.txt3.8 KByalet
#17 2393411_17.patch10.36 KByalet
#12 2393411_12.patch7.52 KBbkosborne
#12 interdiff.txt1.92 KBbkosborne
#9 cas_2392411_9.patch7.52 KBcaseyfw
#8 cas_2392411_8.patch7.51 KBcaseyfw
#4 2392411_4.patch6.55 KByalet

Comments

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented

I'll work on a patch for this.

yalet’s picture

yalet commented

We should also consider backporting this to the 7.x version.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented

Yeah. We'd have to change the default for 7.x to be "no validation" though, since that's what it is now.

yalet’s picture

yalet commented
Assigned: bkosborne » Unassigned
Status: Active » Needs review
StatusFileSize
new 2392411_4.patch6.55 KB

Ran into this irritating behavior in the 7.x version, which motivated me to return to this issue and write it up for 8.x. Patch attached. I've fixed the existing tests, added a test for the new configuration getter, but did not yet modify the CasValidator tests to have 100% code coverage again.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented 
  1. +++ b/src/Form/CasSettings.php
@@ -103,10 +103,21 @@ class CasSettings extends ConfigFormBase {
+      '#description' => $this->t('The SSL verification behavior. It is dangerous to not verify the CA of the CAS server.'),
...
+      ),

    How about "Choose an appropriate option below for verifying the SSL certificate of your CAS server." 

  2. +++ b/src/Service/CasValidator.php
@@ -54,12 +54,29 @@ class CasValidator {
+            $options['verify'] = FALSE;

    Should we instead default to TRUE id the file doesn't exist? 

  3. +++ b/src/Service/CasValidator.php
@@ -54,12 +54,29 @@ class CasValidator {
+        case CasHelper:: CA_NONE:

    rogue space 

  4. +++ b/src/Form/CasSettings.php
@@ -103,10 +103,21 @@ class CasSettings extends ConfigFormBase {
+        CasHelper::CA_CUSTOM => $this->t('Specify a custom SSL certificate in the local filesystem. Use the field below to provide path.'),

    Yes, verify my CAS server's SSL cert. Use my own CA certificate chain provided below. 

  5. +++ b/src/Form/CasSettings.php
@@ -103,10 +103,21 @@ class CasSettings extends ConfigFormBase {
+        CasHelper::CA_NONE => $this->t('Do not verify CAS server. THIS SHOULD NOT BE USED ON PRODUCTION SITES!'),

    No, do not verify the SSL cert of my CAS server. (Note: this should NEVER be used in production). 

  6. +++ b/src/Form/CasSettings.php
@@ -103,10 +103,21 @@ class CasSettings extends ConfigFormBase {
+      '#description' => $this->t('The PEM certificate of the Certificate Authority that issued the certificate of the CAS server, used only with the custom certificate option above. If omitted, the certificate authority will not be verified.'),

    The "if omitted" wording could still be confusing to people that think they NEED to have this even though they don't. We should probably hide this field (field API lets us do that I think) unless the custom option is selected above.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Status: Needs review » Needs work
caseyfw’s picture

caseyfw commented 
+++ b/src/Form/CasSettings.php
@@ -103,10 +103,21 @@ class CasSettings extends ConfigFormBase {
+        CasHelper::CA_CUSTOM => $this->t('Specify a custom SSL certificate in the local filesystem. Use the field below to provide path.'),

This UI would greatly benefit from using the states API to rollout the cert path field when you select this option.

caseyfw’s picture

caseyfw commented
StatusFileSize
new cas_2392411_8.patch7.51 KB

Fixes @bkosborne's suggestions.

  1. Rewords settings to better reflect the verification methods used.
  2. Implements the states API to display the custom cert field only when the relevant option is selected.
  3. Adds verification that (if specified) the custom cert file actually exists.
  4. Changes the fail behaviour where custom certificate is unset to use default certs - previous verification should render this highly unlikely.

I considered implementing hook_requirements to ensure the custom cert file exists, but it would just add unnecessary overhead to each request, especially given Guzzle 6's remarkably sane failure-mode behaviour in regards to verification certs: http://guzzle.readthedocs.org/en/latest/request-options.html#verify

caseyfw’s picture

caseyfw commented
StatusFileSize
new cas_2392411_9.patch7.52 KB

Fixed verification logic for patch 8.

caseyfw’s picture

caseyfw commented
Status: Needs work » Needs review
yalet’s picture

yalet commented
Status: Needs review » Needs work

When I attempt to use the 'Verify using custom [...]' option and leave the text field blank, I get a verification error on the form: 'SSL verification method custom certificate file does not exist.' This contradicts the description of the field, which says that if the field is blank, the default web server certificates will be used.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.92 KB
new 2393411_12.patch7.52 KB

Addressed the above comment

Status: Needs review » Needs work

The last submitted patch, 12: 2393411_12.patch, failed testing.

yalet’s picture

yalet commented

This looks good, but still needs an update for the tests.

  • yalet committed 273e724 on 8.x-1.x 
    Fix regression in CasValidatorTest introduced with Guzzle update....
yalet’s picture

yalet commented

The previous commit was to fix a regression in CasValidatorTest that I noticed while starting to work on updating the tests for this issue. That commit does not include any of the work in this patch, but I referenced this issue where I found the issue.

We had a test that was a spurious pass after the Guzzle update. I updated the test to actually test what it was intended to test, and made a related change to the CasValidator to generalize the types of Guzzle exceptions we catch.

yalet’s picture

yalet commented
Status: Needs work » Needs review
StatusFileSize
new 2393411_17.patch10.36 KB
new interdiff.txt3.8 KB

Tests.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented

This isn't applying cleaning against 8.x-1.x branch:

error: patch failed: tests/src/Unit/Service/CasValidatorTest.php:17
error: tests/src/Unit/Service/CasValidatorTest.php: patch does not apply

Did you generate this against 8.x-1.x?

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented

ignore that, I'm dumb

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Status: Needs review » Fixed

Committed

  • bkosborne committed edb25c4 on 8.x-1.x authored by yalet 
    Issue #2392411 by yalet, caseyfw, bkosborne: Improve robustness of TLS...

Status: Fixed » Closed (fixed)

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