Problem/Motivation

From the OpenID 2.0 specification:

URL Identifiers MUST then be further normalized by both following redirects when retrieving their content and finally applying the rules in Section 6 of RFC3986 to the final destination URL. This final URL MUST be noted by the Relying Party as the Claimed Identifier and be used when requesting authentication.

Redirects are not currently followed.

Proposed resolution

Alter the return value of hook_openid_discovery_method_info() to include the new claimed identifier after following redirects, and use this value when available. See #12 for more information. Patch in #30 implements this change.

Remaining tasks

The current patch includes updated tests and hook documentation.

Evaluate the possibility of backporting this fix to D7 and D6. There is an API change in the expected return structure of a hook, so to make it backwards-compatible, D7 and D6 would need to add code checking for this structure and assign the list of services based on whether the new structure is used. If the return value from the hook has the keys 'services', it uses the new format, but if it returns a flat array of services, it uses the old format.

User interface changes

None.

API changes

The expected return value of hook_openid_discovery_method_info() changes from a flat array of services to an associative array with the following structure.:

  • 'services' (required) an array of discovered services (including OpenID version, endpoint URI, etc).
  • 'claimed_id' (optional) new claimed identifer, found by following HTTP redirects during the services discovery.

Original report by Heine

From the OpenID 2.0 specification:

URL Identifiers MUST then be further normalized by both following redirects when retrieving their content and finally applying the rules in Section 6 of RFC3986 to the final destination URL. This final URL MUST be noted by the Relying Party as the Claimed Identifier and be used when requesting authentication.

Emphasis added.

I see no evidence for following redirects.

Files: 
CommentFileSizeAuthor
#40 575810-40_openid_redirect_minimal_fix.patch1.07 KBwojtha
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#36 575810-36_openid_redirect_dr7.patch16.42 KBwojtha
PASSED: [[SimpleTest]]: [MySQL] 35,967 pass(es).
[ View ]
#32 575810-32-openid_redirect_better_api.patch14.27 KBwojtha
PASSED: [[SimpleTest]]: [MySQL] 33,677 pass(es).
[ View ]
#30 575810-30-openid_redirect_better_api.patch14.27 KBwojtha
PASSED: [[SimpleTest]]: [MySQL] 33,677 pass(es).
[ View ]
#28 575810-28-openid_redirect_better_api.patch29.22 KBwojtha
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in 575810-28-openid_redirect_better_api.patch.
[ View ]
#21 575810-21-openid_redirect_better_api.patch16.29 KBwojtha
PASSED: [[SimpleTest]]: [MySQL] 29,936 pass(es).
[ View ]
#20 575810-20-openid_redirect_better_api.patch11.91 KBwojtha
PASSED: [[SimpleTest]]: [MySQL] 29,835 pass(es).
[ View ]
#19 575810-19-openid_redirect_better_api.patch6.8 KBwojtha
PASSED: [[SimpleTest]]: [MySQL] 29,407 pass(es).
[ View ]
#17 575810-17-openid_redirect_better_api.patch6.64 KBwojtha
PASSED: [[SimpleTest]]: [MySQL] 29,391 pass(es).
[ View ]
#14 575810-15-openid_redirect_better_api.patch6.61 KBwojtha
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/openid/openid.module.
[ View ]
#12 openid_follow_redirects_575810-4.patch3.23 KBwojtha
PASSED: [[SimpleTest]]: [MySQL] 31,560 pass(es).
[ View ]
#10 openid_follow_redirects_575810-3.patch3.3 KBwojtha
PASSED: [[SimpleTest]]: [MySQL] 31,579 pass(es).
[ View ]
#7 openid_follow_redirects_575810-2.patch3.16 KBwojtha
PASSED: [[SimpleTest]]: [MySQL] 31,535 pass(es).
[ View ]
#4 openid_follow_redirects_575810.patch3.02 KBwojtha
PASSED: [[SimpleTest]]: [MySQL] 31,575 pass(es).
[ View ]

Comments

c960657’s picture

Version:6.x-dev» 7.x-dev
wojtha’s picture

Assigned:wojtha» Unassigned
Priority:Major» Normal
Status:Needs work» Needs review
StatusFileSize
new3.02 KB
PASSED: [[SimpleTest]]: [MySQL] 31,575 pass(es).
[ View ]

After this patch openid implementation will follow redirects and change of claimed id will be reflected in the scope of openid_discovery() and openid_begin().

Unfortonutelly little API change is needed: hook_openid_discovery_method_info() callbacks should pass $claimed_id by reference and not by value. Alternative approaches which could be considered: changing claimed_id in $_SESSION or in $GLOBAL context.

wojtha’s picture

Redirects and Claimed id schema change is also mentioned in 11.5.2. HTTP and HTTPS URL Identifiers:

... If the same end user controls the same URL, differing only by scheme, and it is desired that the Identifier be the HTTPS URL, it is RECOMMENDED that a redirect be issued from the HTTP URL to the HTTPS URL. ... If an attacker could gain control of the HTTP URL, it would have no effect on the HTTPS URL, since the HTTP URL is not ever used as an Identifier except to initiate the discovery process.

wojtha’s picture

Test passed, but drupal.org doesn't respond this time because of network trouble, so the test status wasn't updated.

wojtha’s picture

StatusFileSize
new3.16 KB
PASSED: [[SimpleTest]]: [MySQL] 31,535 pass(es).
[ View ]

Patch cleanup + couple of fixes:

- Removed $result->redirect_code code checking, because when it comes from drupal_http_redirect it could be only one of the (301, 302 and 307).
- Added second URL normalization as spec requires.

<?php
if (!empty($result->redirect_code)) {
 
$claimed_id = openid_normalize($result->redirect_url);
}
?>

There could be 2 redirects, but no other check is needed, because drupal_http_request() is super powerful and it will do it for us (check option 'max_redirects' at drupal_http_request()).

Heine’s picture

drupal_http_request is also broken :(

#1081068: drupal_http_request inconsistent redirect_url

Heine’s picture

I don't see that in 7.2.4, but I can see why you think so:

URL Identifiers MUST then be further normalized by both following redirects when retrieving their content and finally applying the rules in Section 6 of [RFC3986] to the final destination URL

The both indicates that you need to do 1) follow redirects and 2) applying the rules in Section 6 of RFC3986.

Not both redirects :)

That also means we still need to check the response code of drupal_http_request as it simply ends following redirects when max_redirect is reached instead of throwing an error. If this happens, the Identifier cannot be used.

wojtha’s picture

StatusFileSize
new3.3 KB
PASSED: [[SimpleTest]]: [MySQL] 31,579 pass(es).
[ View ]

Yeah true, I understood the "both" in a bad way. So here is new patch...

But checking if $result->code contains redirect code seems to me like a workaround, what about returning error from drupal_http_request when max redirects was reached?

c960657’s picture

+    // TODO: drupal_http_request should return error if reaches max allowed
+    // redirects

Please create a ticket rather than filing bugs/feature requests as comments.

+    if (!isset($result->error) || !in_array($result->code, array(301, 302, 307))) {
+

Isn't it better to check whether $result->code is 200?
We usually don't start an if block with a newline.

+      // Replace user entered claimed_id if we got redirect
+      // Fixes http://drupal.org/node/575810 OpenID discovery spec violation.

You usually don't need to specify references to the original ticket in your patch. People interested in the original ticket can look it up using the Git log.

+      if (!empty($result->redirect_code)) {
+        $claimed_id = openid_normalize($result->redirect_url);
+      }

Wouldn't it be more intuitive to look at $result->redirect_url instead of $result->redirect_code?

I wonder whether it would make sense in D8 to eliminate openid_normalize() in favor of the new &$claimed_id by-reference argument? It seems that normalization and discovery are more tightly coupled than currently reflected by the code (e.g. following redirects is also part of the normalization process). We do call openid_normalize() a few places outside the discovery process, but I'm not sure that is required by the spec (I just quickly browsed it).

wojtha’s picture

StatusFileSize
new3.23 KB
PASSED: [[SimpleTest]]: [MySQL] 31,560 pass(es).
[ View ]

@c960657 Ok, I mostly agree, here is new patch.

As to &$claimed_id by-reference argument: I have discussed it with Heine couple days ago on IRC. We thinks that it might be better to redesign API little bit. So discovery will not return only array of services but array with services and discovered claimed id and might be some other parameters.

From the record:

Heine: wojtha, drupal_http_request is also a prerequisite for the other patches. Wondering if we should simply change the return structure of the discovery.
wojtha: the return structure of the discovery - it is possible, but it will have serious affect on the API
wojtha: but it will be the most clean solution IMO
Heine: true, it might not be acceptable for D7 and D6
wojtha: now the discovery returns only services, so we need to isolate them to the e.g. 'services' array key and than we could add other parameters like dicovered claimed_id and so
Heine: though passing claimed_id by ref is also a bit scary when it is later used in verification, as you need to copy the claimed_id to prevent it from changing
Heine: right and properly order and/or include the special URLs for claimed id
wojtha: yes I know, but it was smallest possible change to the API, but not very defensive
Heine: Also still need to look into certificate verification with fsockopen (D6)
wojtha: I think we should try to modify the return structure of the discovery, defensive approach and better flexibility could succeed insted of passing claimed_id by ref... even if it API change, because passing claimed_id by ref through two levels is API change too and more scary as you've mentioned :-)

As to the normalization: it should be used before and during discovery and than during final assertion verification (if claimed id doesn't match, there is one more check; but current code is wrong too in this part - see #1076366: OpenID discovery spec violation - fragments are removed from claimed id ).

I also opened new issue: #1096890: drupal_http_request should return error if reaches max allowed redirects

c960657’s picture

Version:7.x-dev» 8.x-dev
Status:Needs review» Needs work

Now that D8 is open for development I guess the proper approach is to create a patch for D8 first (including the suggested API change) and then discuss D7 and D6 afterward.

wojtha’s picture

StatusFileSize
new6.61 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/openid/openid.module.
[ View ]

Patch against D8 including the API changes suggested in #12, this patch will probably fails since I didn't change any test. So now the functions of discovery methods returned by the hook_openid_discovery_method_info() have to return following array:

<?php
 
return array(
    
'services' => $services, // (array)
    
'claimed_id' => $claimed_id, // (string)
 
);
?>
wojtha’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 575810-15-openid_redirect_better_api.patch, failed testing.

wojtha’s picture

Assigned:Unassigned» wojtha
Status:Needs work» Needs review
Issue tags:+needs backport to D6, +needs backport to D7
StatusFileSize
new6.64 KB
PASSED: [[SimpleTest]]: [MySQL] 29,391 pass(es).
[ View ]
Dries’s picture

Status:Needs review» Needs work
+++ b/modules/openid/openid.module
@@ -256,16 +256,24 @@ function openid_login_validate($form, &$form_state) {
+  // Set claimed_id from discovery

Missing dot/point at end of sentence.

+++ b/modules/openid/openid.module
@@ -423,18 +431,26 @@ function _openid_xri_discovery($claimed_id) {
+          // Ignore service if CanonicalID could not be verified.

Elsewhere we write CanonicalID as two words, I believe.

+++ b/modules/openid/openid.module
@@ -423,18 +431,26 @@ function _openid_xri_discovery($claimed_id) {
+        // TODO: maybe we can just return $discovery itself

Would be good if we could resolve that TODO.

+++ b/modules/openid/openid.module
@@ -454,7 +470,18 @@ function _openid_xrds_discovery($claimed_id) {
+    // Check for HTTP error and make sure, that we reach the target. If maximum
+    // allowed redirects are exhausted, final destination URL isn't reached, but
+    // drupal_http_request will not return error. (This check for 200 result
+    // code could be removed after http://drupal.org/node/1096890 will be
+    // fixed.)

This should be marked with a @TODO so we can 'grep' for it.

wojtha’s picture

Status:Needs work» Needs review
StatusFileSize
new6.8 KB
PASSED: [[SimpleTest]]: [MySQL] 29,407 pass(es).
[ View ]
wojtha’s picture

Priority:Normal» Critical
StatusFileSize
new11.91 KB
PASSED: [[SimpleTest]]: [MySQL] 29,835 pass(es).
[ View ]

Adding tests and bumping to critical. Since this has at least same consequences as #1076366: OpenID discovery spec violation - fragments are removed from claimed id .

Issue #1120290: Provide transition for accounts with incompletely stored OpenIDs is now solving the transition of the stored Claimed Identifiers, which are invalid or incomplete mainly because of this and "stripped fragments" issue.

wojtha’s picture

StatusFileSize
new16.29 KB
PASSED: [[SimpleTest]]: [MySQL] 29,936 pass(es).
[ View ]

Better comments and API.

catch’s picture

#1081068: drupal_http_request inconsistent redirect_url says it blocks this, but it is not marked critical, nor is it not rolled into this patch - please clarify either way.

wojtha’s picture

@catch: Heine thought that the #1081068: drupal_http_request inconsistent redirect_url issue affects the following part of code (and it should theoretically), but I didn't notice in real tests of openid authentication.

+++ b/modules/openid/openid.module
@@ -458,7 +498,18 @@ function _openid_xrds_discovery($claimed_id) {
+      // Replace user entered claimed_id if we got redirect
+      if (!empty($result->redirect_url)) {
+        $claimed_id = openid_normalize($result->redirect_url);
+      }

IMHO it affects this issue only in certain conditions - it depends if number of redirects was 1 or 2 and more, thats the inconsistency.

I will focus on that during my next testing session.

xjm’s picture

Tagging issues not yet using summary template.

xjm’s picture

The patch has some documentation cleanup mixed in, which makes it hard to review. (e.g., reformatting existing docblocks and comments).

Can we remove the documentation cleanup from the patch and open it as a separate issue?

xjm’s picture

Status:Needs review» Needs work
+++ b/modules/openid/openid.moduleundefined
@@ -250,28 +250,39 @@ function openid_login_validate($form, &$form_state) {
- * @param $claimed_id The OpenID to authenticate
- * @param $return_to The endpoint to return to from the OpenID Provider
+ * @param $claimed_id
+ *   The OpenID to authenticate
+ * @param $return_to
+ *   The endpoint to return to from the OpenID Provider

This change is just a formatting cleanup, so would be better to put it in a separate patch.

+++ b/modules/openid/openid.moduleundefined
@@ -250,28 +250,39 @@ function openid_login_validate($form, &$form_state) {
-  // user_exteral_login later.
+  // user_external_login later.

Also appears to be just a commment cleanup; also for that separate issue.

+++ b/modules/openid/openid.moduleundefined
@@ -306,10 +317,12 @@ function openid_begin($claimed_id, $return_to = '', $form_values = array()) {
- * @param $response Array of returned values from the OpenID Provider.
+ * @param $response
+ *   Array of returned values from the OpenID Provider.
  *
- * @return $response Response values for further processing with
- *   $response['status'] set to one of 'success', 'failed' or 'cancel'.
+ * @return $response
+ *   Response values for further processing with $response['status'] set to one
+ *   of 'success', 'failed' or 'cancel'.

Again, separate issue.

+++ b/modules/openid/openid.moduleundefined
@@ -406,10 +432,10 @@ function openid_discovery($claimed_id) {
function openid_openid_discovery_method_info() {
-  // The discovery process will stop as soon as one discovery method succeed.
+  // The discovery process will stop as soon as one discovery method succeeds.
   // We first attempt to discover XRI-based identifiers, then standard XRDS
-  // identifiers via Yadis and HTML-based discovery, conforming to the OpenID 2.0
-  // specification.
+  // identifiers via Yadis and HTML-based discovery, conforming to the
+  // OpenID 2.0 specification.

More pure cleanup for a separate issue.

+++ b/modules/openid/openid.moduleundefined
@@ -421,24 +447,33 @@ function openid_openid_discovery_method_info() {
+          // Ignore service if Canonical ID could not be verified

Needs a period. Also "if the Canonical ID..."

+++ b/modules/openid/openid.moduleundefined
@@ -458,7 +498,18 @@ function _openid_xrds_discovery($claimed_id) {
+      // Replace user entered claimed_id if we got redirect

Needs a period. I'd suggest: "Replace the user-entered claimed_id if we received a redirect."

+++ b/modules/openid/openid.testundefined
@@ -124,6 +124,28 @@ class OpenIDFunctionalTestCase extends OpenIDWebTestCase {
+    // Single redirect

Needs a period.

+++ b/modules/openid/openid.testundefined
@@ -280,6 +302,41 @@ class OpenIDFunctionalTestCase extends OpenIDWebTestCase {
+    // Cleanup

Needs a period and should be two words, since it is a verb: "Clean up."

Edit: Silly dreditor. :)

wojtha’s picture

Assigned:wojtha» Unassigned
Status:Needs work» Needs review
StatusFileSize
new29.22 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in 575810-28-openid_redirect_better_api.patch.
[ View ]

Stripped down comments fixes as suggested @xjm in #27.

Status:Needs review» Needs work

The last submitted patch, 575810-28-openid_redirect_better_api.patch, failed testing.

wojtha’s picture

Status:Needs work» Needs review
StatusFileSize
new14.27 KB
PASSED: [[SimpleTest]]: [MySQL] 33,677 pass(es).
[ View ]

Test failed at the patch format check... CRLF converted to LF.

xjm’s picture

Added summary.

wojtha’s picture

StatusFileSize
new14.27 KB
PASSED: [[SimpleTest]]: [MySQL] 33,677 pass(es).
[ View ]

Small fix. Might be a test is needed for this since it passed the tests here (but not passed the tests in #1120290: Provide transition for accounts with incompletely stored OpenIDs).

  // Set claimed id from discovery.
-  if (!empty($service['claimed_id'])) {
+ if (!empty($discovery['claimed_id'])) {
    $claimed_id = $discovery['claimed_id'];
  }
catch’s picture

Status:Needs review» Reviewed & tested by the community

Looks good, although I do not and have no intention of reviewing it against the OpenID spec, however I'll trust wojtha and the other contributors to this issue who have.

Since we have test coverage being added at #1120290: Provide transition for accounts with incompletely stored OpenIDs there seems no reason to add an explicit test here too.

aspilicious’s picture

I managed to log in with an openid. Sounds rtbc for me than :)

webchick’s picture

Status:Reviewed & tested by the community» Needs work

This doesn't seem to apply to 7.x. I can haz a D7 patch?

wojtha’s picture

Version:8.x-dev» 7.x-dev
Status:Needs work» Needs review
StatusFileSize
new16.42 KB
PASSED: [[SimpleTest]]: [MySQL] 35,967 pass(es).
[ View ]

Backport of the #32 patch for D7, temporarily changing the issue version to test the patch.

aspilicious’s picture

Version:7.x-dev» 8.x-dev

Back to 8.x for webchick! :D

aspilicious’s picture

Status:Needs review» Reviewed & tested by the community

R T B C

webchick’s picture

Version:8.x-dev» 6.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)
Issue tags:-needs backport to D7

Great, thanks SO much!! Committed and pushed to 8.x and 7.x.

Marking down to 6.x and "to be ported".

wojtha’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new1.07 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

Oh yeah, finally :-)

For the Drupal 6, I'm suggesting the minimal fix - just passing the claimed_id by reference instead referencing by value. I is opposite of the D7 and D8 approach but Drupal 6 OpenID API is much simpler and more straightforward, it has the only hook - hook_openid. Using this approach we will stay compatible with the contrib modules.

wojtha’s picture

wojtha’s picture

Issue summary:View changes

Updated issue summary.