Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

frederikbosch
Copy link
Contributor

@frederikbosch frederikbosch commented Feb 20, 2023

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.

@frederikbosch frederikbosch changed the title move all encoding and decoding to version specific files Move all encoding and decoding of asymmetric keys to specific version files Feb 20, 2023
@paragonie-security
Copy link
Contributor

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

@paragonie-security paragonie-security merged commit 89386bf into paragonie:master Apr 1, 2023
@paragonie-security
Copy link
Contributor

2eb176f ensures code that uses PASETO doesn't have to rewrite their usage to use your refactored code.

@aidantwoods
Copy link
Contributor

aidantwoods commented Apr 1, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants