feature(security): Adds functions to create and validate HMAC tokens #8057
Conversation
|
Does not really fix #7824 as site secret is still not publicly through API, but this PR will reduce the need to have it available a bit. Don't know if it will cover all usecases. |
| $this->assertTrue((bool)preg_match('~^[a-zA-Z0-9\-_]{30,}$~', $mac)); | ||
|
|
||
| $mac = $crypto->getHmac([1234, 'foo'], $key); | ||
| $this->assertTrue((bool)preg_match('~^[a-zA-Z0-9\-_]{30,}$~', $mac)); |
There was a problem hiding this comment.
What is this magic regex?
|
Looks cool. Bummer about the new global method but whatevs I guess. |
| @@ -264,10 +264,10 @@ public function generateActionToken($timestamp) { | |||
| $site_secret = _elgg_services()->siteSecret->get(); | |||
There was a problem hiding this comment.
i believe you can drop this, as you are now defaulting
i check with all our own usecases and it covers them all. So really happy to see this in core |
| @@ -425,3 +425,30 @@ You can also access the tokens from javascript: | |||
| These are refreshed periodically so should always be up-to-date. | |||
|
|
|||
|
|
|||
| Security Tokens | |||
| =============== | |||
| If you need to pass data to a user and trust that the user has returned it to you unaltered, you should use | |||
There was a problem hiding this comment.
I had to read this twice to understand it. Could this be explained better?
|
I've tried to address all these. Note that this will invalidate any unused tokens such as email validations/invitations. It uses a binary form of the site key instead of the hex/b64 encodings and also includes separators in the data. I walked back the idea of auto-serializing data passed in because I was uncomfortable with having HMAC not match other implementations, but I'm still a bit torn on this. If someone fails to use a good separator, an attacker can move characters between variables: E.g. using @ewinslow Re global functions: https://community.elgg.org/discussion/view/2062019/ideas-for-exposing-elgg-oo-services |
mrclay
force-pushed
the
token_svc_7824
branch
2 times, most recently
from
f189623
to
6f70e77
Compare
|
LGTM |
| if ($raw) { | ||
| // try to return binary key | ||
| if ($secret[0] === 'z') { | ||
| // new keys are "z" + base64URL |
There was a problem hiding this comment.
Why z? Might be best to document.
mrclay
force-pushed
the
token_svc_7824
branch
2 times, most recently
from
a0282de
to
8e8a7bf
Compare
|
I'm thinking of an API like: // HMAC with direct set of data
$token = elgg_build_hmac()->setData("$foo|$bar")->getToken();
$is_valid = elgg_build_hmac()->setData("$foo|$bar")->matchesToken($token);
// preferred usage: handles separation for you
$token = elgg_build_hmac()
->addData($foo)
->addData($bar)
->getToken(); |
HMAC uses a binary encoding of the site key instead of the Base64 or hex encodings, and improves the docs of the site secret component. Fixes Elgg#7824
|
I reworked this so that it can accept an array of strings as HMAC data, and spits out an object instead of breaking generation/validation into two functions. |
|
LGTM |
feature(security): Adds functions to create and validate HMAC tokens
|
I am unable to get this to work. For whatever reasons tokens do not match. Using hash_hmac() for now. |
|
@hypeJunction did you make sure the types match? http://learn.elgg.org/en/1.11/guides/actions.html#security-tokens |
|
I did.
|
|
I am not sure if this affects the matching alrgorithm, but link was generated with elgg_build_hmac() and validation was being done without engine boot using Hmac component. |
|
Ah, few things: server key is decoded from b64/hex to binary before use, and the hash created is b64URL encoded. I should've made all those steps easily accessible without boot. |
|
At the very least I'll add that to docs for this function |
This also has getHmac() use a binary encoding of the site key instead of the Base64 or hex encodings.
Fixes #7824