DDS-2738, add https pinning, cover with tests #18
Conversation
andrevka
force-pushed
the
DDS-2738
branch
3 times, most recently
from
c1186a9
to
b518f21
Compare
src/Sk/SmartId/Util/Curl.php
Outdated
| . self::RP_API_PUBLIC_KEY_VALID_FROM_2019_11_01_TO_2021_11_05; | ||
| } | ||
|
|
||
| public static function setPublicKeysFromArray(array $public_keys) |
There was a problem hiding this comment.
Good to see someone is solving SSL pinning issue. By the way, doesn't this approach violate encapsulation of this library by exposing internal utility logic? Wouldn't it be more better to add extra layer so the pinning can be configured through AuthenticationRequestBuilder?
There was a problem hiding this comment.
Hmm, shouldn't the pinning be responsibility of AuthenticationResponseValidator?
The downside is that in this way it is not possible to use readily available pinning functionality provided by curl. It requires more complex solution.
On the other hand, other solution maybe does not force to raise minimum PHP requirement. Pinning of curl (CURLOPT_PINNEDPUBLICKEY) is available starting PHP 7.0.7.
There was a problem hiding this comment.
Hmm, shouldn't the pinning be responsibility of AuthenticationResponseValidator?
Probably not. Digging in code reveals that this is meant to validate data about authenticated user. After the successful authentication server returns user data which is the input of \Sk\SmartId\Api\AuthenticationResponseValidator::validate. The certificate in this input parameter is actually user certificate. Moreover, AuthenticationResponseValidator does not have information about server certificate/public key therefore can not carry out this responsibility anyway.
There was a problem hiding this comment.
@andrevka could you change Curl::useOnlyDemoPublicKey() and others to be non-static?
Did it
There was a problem hiding this comment.
Hmm, shouldn't the pinning be responsibility of AuthenticationResponseValidator?
Probably not. Digging in code reveals that this is meant to validate data about authenticated user. After the successful authentication server returns user data which is the input of \Sk\SmartId\Api\AuthenticationResponseValidator::validate. The certificate in this input parameter is actually user certificate. Moreover, AuthenticationResponseValidator does not have information about server certificate/public key therefore can not carry out this responsibility anyway.
I think i have resolved the issue, you can check it out.
|
@andrevka could you change Curl::useOnlyDemoPublicKey() and others to be non-static? |
|
Is it possible to merge methods The motivation is to keep test and production code as identical as possible in end application. Then you have the ability to ship a code that is actually tested 100%. I would propose to separate public keys into separate repository (or pinset in current context) and have the end developer have the possibility to choose how he or she wants to manage pinning. Example: # Package will detect suitable keys
$client->usePublicKeyPinning(PublicKeyPinset::detectByUrl($ur));
# Developer can force to use only demo site keys
$client->usePublicKeyPinning(PublicKeyPinset::demoSite());
# Developer can force to use only live site keys
$client->usePublicKeyPinning(PublicKeyPinset::liveSite());
# Developer is free to choose whatever key he or she wants to use.
$keys = array(
'sha256//QLZIaH7Qx9Rjq3gyznQuNsvwMQb7maC5L4SLu/z5qNU=',
'whatever format curl\'s CURLOPT_PINNEDPUBLICKEY option accepts'
);
$client->usePublicKeyPinning($keys);In my opinion this makes pinning configuration more flexible and also makes Client interface more readable and lean. |
|
Merged to version 1.5 |
No description provided.