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

Add from_pem_private_key/from_pem_public_key methods #109

Merged

Conversation

letmaik
Copy link
Collaborator

@letmaik letmaik commented Feb 24, 2023

Addresses one half of #44.

Note on tests: The existing tests are a bit inconsistent between key types and I didn't want to change that in this PR which is why the new tests keep the existing style within each test file but may look a bit odd as a set.

@letmaik letmaik requested a review from plietar February 24, 2023 17:14
@letmaik letmaik marked this pull request as ready for review March 2, 2023 16:46
pycose/keys/cosekey.py Outdated Show resolved Hide resolved
pycose/keys/okp.py Outdated Show resolved Hide resolved
pycose/keys/okp.py Outdated Show resolved Hide resolved
pycose/keys/okp.py Outdated Show resolved Hide resolved
pycose/keys/okp.py Show resolved Hide resolved
pycose/keys/rsa.py Outdated Show resolved Hide resolved
pycose/keys/rsa.py Outdated Show resolved Hide resolved
:return: An initialized COSE Key object.
"""

for key_type in cls._key_types.values():
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iterating over _key_types is not wrong, but I find it a bit weird, especially considering that _key_types actually have three entries per key-type. On the other hand I'm not sure I can think of a much cleaner solution other than having just a list of key types? Or a separate map from cryptography types to pycose types. None of these are particularly elegant either, so maybe it's fine to keep this as is.

@letmaik
Copy link
Collaborator Author

letmaik commented Mar 8, 2023

We also discussed offline whether accepting objects from the cryptography library is the right choice, compared to a serialized form like PEM. Accepting PEM would keep cryptography as an implementation detail and not leak into the public API. On the flip-side, most users interested in this functionality will likely already use cryptography to parse their DER/PEM keys, and accepting them in their parsed form would be more convenient in that case. More opinions on this are welcome.

@letmaik letmaik changed the title Add from_cryptography_key method Add from_pem_private_key/from_pem_public_key methods Mar 8, 2023
@letmaik
Copy link
Collaborator Author

letmaik commented Mar 8, 2023

I've switched over to accepting PEM as key format as that is more universal and avoids forcing a specific crypto library.

@sonarcloud
Copy link

sonarcloud bot commented Mar 20, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

1 similar comment
@sonarcloud
Copy link

sonarcloud bot commented May 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link

sonarcloud bot commented Dec 15, 2023

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@TimothyClaeys TimothyClaeys merged commit b4f5f96 into TimothyClaeys:master Dec 15, 2023
12 checks passed
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.

None yet

3 participants