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

Remove gojwk package #41

Merged
merged 1 commit into from
Mar 6, 2025
Merged

Remove gojwk package #41

merged 1 commit into from
Mar 6, 2025

Conversation

akhromium
Copy link
Contributor

As discussed in PM. Part of the dependencies update activity.


// DecodePublicKey decodes Key to public key.
func (j *Key) DecodePublicKey() (crypto.PublicKey, error) {
if _, ok := supportedKeyTypes[j.Kty]; !ok {
Copy link
Member

Choose a reason for hiding this comment

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

Just a minor suggestion: I’m not sure if we need a separate supportedKeyTypes map. Returning an error in the end of the func should be sufficient. Otherwise, we’d need to constantly remember to keep the map updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was done for the future support of EC or other Key types.

Copy link
Member

Choose a reason for hiding this comment

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

My point is we can simplify this and have code something like

switch j.Kty {
case "RSA":
  // handle RSA key
case "EC":
  // handle ecliptic curve key
default:
  return nil, fmt.Errorf("unsupported key type %s", j.Kty)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed to improve with the next iteration

@akhromium akhromium force-pushed the remove_gojwk branch 2 times, most recently from 16d5dab to 55a1fba Compare March 5, 2025 16:57
@vasayxtx vasayxtx merged commit 86e30e4 into acronis:main Mar 6, 2025
5 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.

2 participants