As a follow up of #302632: Use PHP5 functionality for _openid_sha1(). Here is a patch that tests if hash extension (Default in PHP5) is available and use hash_hmac() function in _openid_hmac().

See chx's comment for details on why we can't just rely on this extension being available:

#302632-8: Use PHP5 functionality for _openid_sha1()

CommentFileSizeAuthor
#16 openid_hmac-D7-3.patch768 bytesc960657
#13 drupal8.openid-hmac.13.patch1.39 KBsun
#10 openid_hmac-A-3.patch1.4 KBc960657
#7 openid_hmac-A-2.patch2.14 KBc960657
#7 openid_hmac-B-2.patch1.2 KBc960657
#3 openid_hmac.patch1.28 KBrecidive
openid_hmac.patch1.28 KBrecidive
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

recidive’s picture

recidive CreditAttribution: recidive commented
Title: Use hash extension in _openid_hmac() is available » Use hash extension in _openid_hmac() if available

Typo in title.

cburschka’s picture

cburschka
they
CreditAttribution: cburschka commented
Status: Needs review » Needs work

This patch looks ready to go, except for one blank line containing two spaces.

recidive’s picture

recidive CreditAttribution: recidive commented
Status: Needs work » Needs review
FileSize
openid_hmac.patch1.28 KB

Ok, this time without the spaces.

Dries’s picture

Dries CreditAttribution: Dries commented

Personally, I'd not bother with having two code paths. This code shouldn't be in the critical performance path, so the existing code is probably fine. I'm tempted to mark this "won't fix".

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Closed (won't fix)

Marking this as "won't fix".

c960657’s picture

c960657 CreditAttribution: c960657 commented

We now require the hash extension (see system_requirements()), so we can now switch completely to hash_hmac().

openid_hmac-A-2.patch removes _openid_hmac() and OPENID_SHA1_BLOCKSIZE completely, openid_hmac-B-2.patch preserves them for backwards-compatibility. It think we should use the former—_openid_hmac() is a private function, and I doubt anybody outside of the OpenID module would use the OPENID_SHA1_BLOCKSIZE constant.

Edit: The patches are attached to the next comment.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Title: Use hash extension in _openid_hmac() if available » Use hash extension in _openid_hmac()
Status: Closed (won't fix) » Needs review
FileSize
openid_hmac-B-2.patch1.2 KB
openid_hmac-A-2.patch2.14 KB
recidive’s picture

recidive CreditAttribution: recidive commented

I also think the "A" patch is better.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Version: 7.x-dev » 8.x-dev

For D8, openid_hmac-A-2.patch is definitely the right choice.

c960657’s picture

c960657 CreditAttribution: c960657 commented
FileSize
openid_hmac-A-3.patch1.4 KB
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Awesome.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

#10: openid_hmac-A-3.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
drupal8.openid-hmac.13.patch1.39 KB

Re-rolled against HEAD.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Looks great. Committed/pushed to 8.x.

Status: Fixed » Closed (fixed)

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

c960657’s picture

c960657 CreditAttribution: c960657 commented
Version: 8.x-dev » 7.x-dev
Status: Closed (fixed) » Needs review
FileSize
openid_hmac-D7-3.patch768 bytes

Reroll of openid_hmac-A-2.patch for D7.

  • catch committed 06373cf on 8.3.x 
    Issue #448162 by c960657, recidive, sun: Use hash extension in...

  • catch committed 06373cf on 8.3.x 
    Issue #448162 by c960657, recidive, sun: Use hash extension in...

  • catch committed 06373cf on 8.4.x 
    Issue #448162 by c960657, recidive, sun: Use hash extension in...

  • catch committed 06373cf on 8.4.x 
    Issue #448162 by c960657, recidive, sun: Use hash extension in...

  • catch committed 06373cf on 9.1.x 
    Issue #448162 by c960657, recidive, sun: Use hash extension in...