search

paragonie / paseto

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

Move all encoding and decoding of asymmetric keys to specific version files #168

Closed frederikbosch closed 1 year ago

frederikbosch commented 1 year ago

In order to prevent version specific conditions in many locations, I choose to make Keys\AsymmetricSecretKey and Keys\AsymmetricPublicKey abstract classes. This forces all instances to be of a specific version. This is a BC-break though. With this PR new Keys\AsymmetricSecretKey() and new Keys\AsymmetricPublicKey are not possible anymore. I doubt if this BC-break makes a huge impact on the users of the library. Who would have instantiated these keys using its constructor directly? But still, it is a BC-break.

If you insist in fixing this without any BC breaks, I can make that work too, but I doubt if that makes the code more maintainable. And the goal of this PR is to make the code more maintainable. With this PR you can clearly see if there are anymore bugs between encoding and decoding of the specific versions. And there are. A question that arises is what to do with new Keys\SymmetricKey()? It would be consistent to make that abstract too. I would definitely prefer to go for that.

If this would be merged, I can make tests for all the encoding and decoding and add importPem method to both versions.

paragonie-security commented 1 year ago

I'm trying to figure out why GitHub Actions didn't kick off for this PR. It's quite puzzling.

paragonie-security commented 1 year ago

https://github.com/paragonie/paseto/commit/2eb176f5a809a57eb62f97fa5009aefcae39b948 ensures code that uses PASETO doesn't have to rewrite their usage to use your refactored code.

aidantwoods commented 1 year ago

I'm trying to figure out why GitHub Actions didn't kick off for this PR. It's quite puzzling.

The push trigger only applies to this repo, so forks won't trigger it. You probably want pull_request as the main trigger, and then restrict pushes to running on main (so you don't double up on test runs when you open PRs with branches from this repo).

Example 🙂

https://github.com/aidantwoods/go-paseto/blob/main/.github/workflows/ci.yml