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

fix(grpc): sign and verify message by ed25519 #1667

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

BuidlWithShivam
Copy link

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

Attention: Patch coverage is 81.39535% with 8 lines in your changes missing coverage. Please review.

Project coverage is 76.41%. Comparing base (86a8ba9) to head (ad5f73b).
Report is 40 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1667      +/-   ##
==========================================
+ Coverage   75.07%   76.41%   +1.34%     
==========================================
  Files         234      241       +7     
  Lines       12156    18479    +6323     
==========================================
+ Hits         9126    14121    +4995     
- Misses       2582     3898    +1316     
- Partials      448      460      +12     

@Ja7ad Ja7ad requested a review from b00f January 14, 2025 13:42
@b00f
Copy link
Collaborator

b00f commented Jan 14, 2025

@BuidlWithShivam,

Thank you so much for this amazing PR. It’s great to see how smart you are in understanding the logic and fixing this issue. Good job!
By thee way, I feel we can improve it in some ways.

First of all, it’s great to see that you can detect the type of PrivateKey by their prefixes. This approach is better than checking for errors in the public key. We can do the same for the public key as well, like this:

	maybeBLSPublicKey := func(str string) bool {
		return strings.Contains(strings.ToLower(str), "public1p")
	}

	maybeEd25519PublicKey := func(str string) bool {
		return strings.Contains(strings.ToLower(str), "public1r")
	}
	switch {
	case maybeBLSPublicKey(req.PublicKey):
		/// ...

	case maybeEd25519PublicKey(req.PublicKey):
		/// ...

	default:
		return nil, status.Error(codes.InvalidArgument, "invalid public key")
	}

Now, we can make another improvement here. PublicKeys are interfaces. We can simplify the above code by creating a function that takes a string and returns a PublicKey as an interface.

We can define a function like this:

func (*utilServer) publicKeyFromString(pubStr string) (crypto.PublicKey, error) {
	maybeBLSPublicKey := func(str string) bool {
		return strings.Contains(strings.ToLower(str), "secret1p")
	}

	maybeEd25519PublicKey := func(str string) bool {
		return strings.Contains(strings.ToLower(str), "secret1e")
	}

	switch {
	case maybeBLSPublicKey(pubStr):
		blsPub, err := bls.PublicKeyFromString(pubStr)
		if err != nil {
			return nil, status.Error(codes.InvalidArgument, "invalid private key")
		}
		return blsPub, nil

	case maybeEd25519PublicKey(pubStr):
		ed25519Pub, err := ed25519.PublicKeyFromString(pubStr)
		if err != nil {
			return nil, status.Error(codes.InvalidArgument, "invalid private key")
		}
		return ed25519Pub, nil

	default:
		return nil, status.Error(codes.InvalidArgument, "invalid private key")
	}
}

Finally we can make the same for PrivateKey

@b00f b00f modified the milestones: v1.7.0, v1.8.0 Jan 14, 2025
@BuidlWithShivam
Copy link
Author

@b00f Sure. Thanks for the review. I was not aware that the public key also has the same structure. Tried to find across the code but couldn't. Thanks for the suggestion on the interface as well. I just saw two different structs and missed the interface. I will implement those.

@BuidlWithShivam
Copy link
Author

@b00f Made the changes. Can you please check.

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