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

Inmemory cryptokey #1196

Closed

Conversation

eugene-borovov
Copy link
Contributor

Added the ability to use cryptographic keys stored in memory.

Signed-off-by: Eugene Borovov <[email protected]>
Signed-off-by: Eugene Borovov <[email protected]>
Signed-off-by: Eugene Borovov <[email protected]>
Signed-off-by: Eugene Borovov <[email protected]>
Signed-off-by: Eugene Borovov <[email protected]>
@eugene-borovov
Copy link
Contributor Author

This is basically a duplicate of #1180 except that key creation is deferred until used. This is done so that when the JWT configuration is centralized, it would be easier to take the key creation to the factory and leave the CryptoKey as a DTO. Plus, the tests are updated to use a key from a file or from memory.

@Sephster
Copy link
Member

Thanks for the PR @eugene-borovov - There is a lot that has changed in this PR such as the file permissions etc. I'm going to try and push through #1180 instead as the PR is a bit more focused. If you believe the changes here warrant inclusion in the repo, I'd welcome a PR after #1180 has been merged in. Thanks for your contributions here and sorry I'm not progressing this at the moment.

@Sephster Sephster closed this Mar 14, 2021
@eugene-borovov
Copy link
Contributor Author

The bulk of the changes are in-memory key usage tests. I can remove these tests if they are redundant or confusing. In any case, this is a temporary solution until the JWT configuration is moved to the dependency container #1154.

@Sephster
Copy link
Member

Thank you for the offer but we will try and get #1180 merged in and then add any additions from that point. Thanks for the offer of additional work here though, it is appreciated.

@eugene-borovov
Copy link
Contributor Author

eugene-borovov commented Mar 14, 2021

I am a Windows user, so the Linux rights check does not work in development environment and does not allow me to run tests. I also have to configure enabling rights verification in production environment separately. So I added a Windows check.

Please consider including Windows verification in the file permissions check code.

@Sephster
Copy link
Member

The windows code is something we will likely remove in future because it is problematic and I'm not convinced this library should be checking file permissions.

Happy to review such a change but this PR was supposed to be for adding memory crypto keys. I think the inclusion of the Windows permissions change means this PR is doing too many things and makes it harder to review.

It will be more likely PRs are accepted if they are focussed on succinct small changes because it takes me a long time to review code, to ensure it complies to all the various RFCs this library has to conform to.

Thanks for your understanding in this matter.

@eugene-borovov
Copy link
Contributor Author

Thank you for advice. I will try to make future PRS small and focused on one goal.

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.

2 participants