search

paragonie / paseto

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

`Parser` shouldn't produce signable `Builder`s? #36

Closed aidantwoods closed 6 years ago

aidantwoods commented 6 years ago

In the 'public' case of: https://github.com/paragonie/paseto/blob/c44d33467662f23d3d6ef7ea22f8926333a8e763/src/Parser.php#L190-L220

$this->key is checked to be an instance of AsymmetricPublicKey, which is later passed to JsonToken::setKey. However, this requires the given key to be an instance of AsymmetricSecretKey in the 'public' case. https://github.com/paragonie/paseto/blob/c44d33467662f23d3d6ef7ea22f8926333a8e763/src/JsonToken.php#L374-L382

Obviously there's going to be a type mis-match here, but on a more general note:

Given that the JsonToken is constructed with a key for signing, I don't think it would ever make sense to have the parser produce a JsonToken in the public case, because it would require knowledge of the secret key to sign, but the parser is only really concerned with verification which requires only the public key (which is likely all the receiver of a signed object has?).

I think perhaps the key/signing should be decoupled from the JsonToken so that the parser can produce these objects without requiring a key, and the key should be provided when signing takes place (perhaps in a new SignableJsonToken class for example that is constructed using a JsonToken and a key).

paragonie-scott commented 6 years ago

Yes, I think that would be a better design. I was trying to oversimplify before.

paragonie-scott commented 6 years ago

I believe this is closable now?

aidantwoods commented 6 years ago

Will this not still cause a type mismatch? https://github.com/paragonie/paseto/blob/851a5f519deb67af94beafe72cd577d3804c4873/src/Parser.php#L207

As ->setKey still requires a secret key in the asymmetric crypto case, but the parser has a public one.

i.e. shouldn't the parser produce a JsonToken (since they now do not require a key)

aidantwoods commented 6 years ago

I think if you add a test case where the parser uses a 'public' purpose you'll get an error when trying to construct the builder (at present the tests only use 'local' in the parser and then convert the resulting object to the asymmetric case).

paragonie-scott commented 6 years ago

Shit, you're right.

paragonie-scott commented 6 years ago

https://github.com/paragonie/paseto/commit/63c2af7e6f54433e58e7deb615b02c70895e8e7d side-steps the anti-patterns with the parser. I'll make sure the builder doesn't do anything weird either.

aidantwoods commented 6 years ago

An idea I had was to make a couple of empty interfaces extending KeyInterface named SendingKey and ReceivingKey (would match language in https://github.com/paragonie/paseto/pull/37/commits/cbd01fe9da2cd638c15036ebafec1414ad24f46f#diff-fa390ca15b142c05e438dcbc19a23aa5R65) and make AsymetricPublicKey implement ReceivingKey, AsymetricSecretKey implement SendingKey and SymmetricKey implement both. This might help declare the key intent upfront to catch this earlier in future ;-)

aidantwoods commented 6 years ago

I'll update #37 for the current master, since it defers the key intent just configuring a couple of array consts in Purpose, and then allows you to defer key and purpose validity checks there depending on intent.

paragonie-scott commented 6 years ago

Okay, I'll stop making changes to dev-master for the evening then. :)

aidantwoods commented 6 years ago

Closing due to https://github.com/paragonie/paseto/commit/63c2af7e6f54433e58e7deb615b02c70895e8e7d :)