paragonie / paseto

Platform-Agnostic Security Tokens
https://paseto.io
Other
3.23k stars 108 forks source link

Keys and protocols #46

Closed aidantwoods closed 6 years ago

aidantwoods commented 6 years ago

For example in https://github.com/paragonie/paseto/blob/1a40019a61fb28d77e7005e4badcbc273eb04b18/src/Keys/AsymmetricSecretKey.php#L36-L44

The given protocol version is used to determine conditions for key construction. However the protocol version of a key is not checked when using the key directly with a protocol, e.g. in https://github.com/paragonie/paseto/blob/1a40019a61fb28d77e7005e4badcbc273eb04b18/src/Protocol/Version2.php#L105-L121

Which means that the conditions checked in the key's constructor cannot be guaranteed (since version 1 AsymmetricSecretKey key may be constructed and passed to version 2 as a key for example).

This I suppose leads to the more general question: if keys are dependent on a version, should there be distinct types for these different versions?

At the very least I think that the protocols should be checking the version associated with any keys they receive?

paragonie-scott commented 6 years ago

if keys are dependent on a version, should there be distinct types for these different versions?

I don't know if we should require distinct types for the different versions. I think checking that the key was intended for the same version is sufficient, but if it helps people to have Version2AsymmetricSecretKey extends AsymmetricSecretKey that simply hard-codes the version parameter, then I'm not against that either.

aidantwoods commented 6 years ago

Yes this seems like a reasonable middle ground. Checking the version is the main thing anyway, whether it be via types or otherwise.

If the type restrictions aren't going to be narrowed at point of use, then even instead of additional types like Version2AsymmetricSecretKey extends AsymmetricSecretKey where the second constructor parameter is ignored, it might be simpler to just add a named version constructor to the keys? e.g.

public static function version2(string $keyData): self {
    return new self($keyData, new Version2);
}
paragonie-scott commented 6 years ago

Yes, that could work as well.

aidantwoods commented 6 years ago

Although if doing the above, it might be best to adopt a convention on whether to use v2 or version2 to avoid tripping people up (since I think v2 is used in protocol collection).

paragonie-scott commented 6 years ago

https://github.com/paragonie/paseto/commit/a29c2c7581b58dd2c19421c4f3df0c7a04304979

aidantwoods commented 6 years ago

I ended up tying the keys explicitly to the version implementation they'll be used in, and made this interlocking part of the protocol interface (diff in https://github.com/aidantwoods/swift-paseto/commit/ccfeb28c9257e3001909366295f0362203e3f54d incase its of any interest). i.e. a version can only use keys which are specialised over the version itself.

(In the case of the Swift implementation, exceptions form part of the language type information, and it's a compile error not to deal with potential exceptions if you use a throwing function – so preventing the wrong version being used altogether means not having to introduce exceptions in some places :) ).