Skip to content
This repository was archived by the owner on Jan 28, 2026. It is now read-only.

feat: signing pkg for programmatic access#36

Merged
dtbuchholz merged 10 commits intomainfrom
dtb/signer-lib
Feb 28, 2024
Merged

feat: signing pkg for programmatic access#36
dtbuchholz merged 10 commits intomainfrom
dtb/signer-lib

Conversation

@dtbuchholz
Copy link
Contributor

@dtbuchholz dtbuchholz commented Feb 27, 2024

Summary

Adds a pkg for signing requests and a sign command, modeled off of the signer CLI and uploader.go. Closes ENG-787.

Details

It offers a few methods:

  • HexToECDSA: Loads a private key from the given string and creates an ECDSA private key.
  • FileToECDSA: Loads a private key from a file and creates an ECDSA private key.
  • NewSigner: Creates a new signer with the given private key, provided by LoadPrivateKey.
  • SignFile: Signs the given file with the signer and returns the signature as bytes.
  • SignBytes: Signs the given bytes with the signer and returns the signature as bytes.
  • SignatureBytesToHex: Converts bytes so they can be used in the URL POST request to write data to a vault.

How it was tested

You can find the tests to ensure correct usage. Namely, I tested the CLI signers signature matched with the example I set up in the tests. I also created a working demo example here: https://github.com/dtbuchholz/demo-textile-http-api

@dtbuchholz
Copy link
Contributor Author

dtbuchholz commented Feb 27, 2024

@brunocalza i'm not sure if this is something we want to merge, but it was a request from IoTeX to make this available via a pkg instead of cmd. At the very least, they can access this branch.

If it's worthwhile pursuing, i can fix the linting issues.

@brunocalza
Copy link
Contributor

brunocalza commented Feb 27, 2024

@brunocalza i'm not sure if this is something we want to merge, but it was a request from IoTeX to make this available via a pkg instead of cmd. At the very least, they can access this branch.

If it's worthwhile pursuing, i can fix the linting issues.

In some sense, the Signer is already available for use as a package, just do

import "github.com/tablelandnetwork/basin-cli/internal/app"

signer := app.NewSigner(pk)
signer.SignFile("filename")

But it's a good idea to have its own package. But it would be good to refactor the CLI logic to use that package so we don't have 2 signers

Update:
nevermind. Looks like it's an internal package, so not available. Definitely a good idea to make it public.

@dtbuchholz
Copy link
Contributor Author

@brunocalza Okay gotcha, and I totally missed that impl in internal/app!

Since this is sort of a one-off, partially for fun, I can take a stab at it. So, would it make sense for me to take your existing impl in uploader.go for NewSigner, Sign, SignFile, Sum—plus, my new LoadPrivateKey method—and make those all available in pkg/signing? (They differ slightly from what I created in this PR.)

Then, do 2 things:

  1. Import the pkg/signing lib into uploader.go and use the methods accordingly
  2. Optionally, make your impl of the signer CLI available as a new sign command in the vaults CLI? i.e., it should probably be an actual feature since it's related to the vaults functionality and useful if you're doing curl requests

@brunocalza
Copy link
Contributor

@brunocalza Okay gotcha, and I totally missed that impl in internal/app!

Since this is sort of a one-off, partially for fun, I can take a stab at it. So, would it make sense for me to take your existing impl in uploader.go for NewSigner, Sign, SignFile, Sum—plus, my new LoadPrivateKey method—and make those all available in pkg/signing? (They differ slightly from what I created in this PR.)

Then, do 2 things:

  1. Import the pkg/signing lib into uploader.go and use the methods accordingly
  2. Optionally, make your impl of the signer CLI available as a new sign command in the vaults CLI? i.e., it should probably be an actual feature since it's related to the vaults functionality and useful if you're doing curl requests

That approach makes sense to me. Let me know if you need help along the way.

}
}

func newSignCommand() *cli.Command {
Copy link
Contributor Author

@dtbuchholz dtbuchholz Feb 28, 2024

Choose a reason for hiding this comment

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

new command to sign data—i.e., vaults sign --private-key abcd /path/to/file. it just print the raw signature string

Description: "The result of the `vaults account create` command will write a private key to a file, \n" +
"and this lets you retrieve the public key value for use in other commands.\n\n" +
"EXAMPLE:\n\nvaults account address /path/to/file",
UsageText: "vaults account address <file_path|hex_string>",
Copy link
Contributor Author

@dtbuchholz dtbuchholz Feb 28, 2024

Choose a reason for hiding this comment

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

this is a backlog issue from the UX improvements project: ENG-787

i.e., you can do vaults account address /path/to/pk-file, or you can do vaults account address abcd1234. i thought about adding a boolean flag like --file false, or --type file or --type string in case that's a better UX, but it's a small feature

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, i also prefer being explicit by using flags, but looks good

Copy link
Contributor

@brunocalza brunocalza left a comment

Choose a reason for hiding this comment

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

looks pretty good 💯 left some comments but no big changes really required

Description: "The result of the `vaults account create` command will write a private key to a file, \n" +
"and this lets you retrieve the public key value for use in other commands.\n\n" +
"EXAMPLE:\n\nvaults account address /path/to/file",
UsageText: "vaults account address <file_path|hex_string>",
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, i also prefer being explicit by using flags, but looks good

signature, err := signer.SignFile(filepath)
signer := signing.NewSigner(bu.privateKey)
signatureBytes, err := signer.SignFile(filepath)
signature := signing.SignatureBytesToHex(signatureBytes)
Copy link
Contributor

Choose a reason for hiding this comment

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

the err check should be above this line

@dtbuchholz
Copy link
Contributor Author

@brunocalza i agree with all of your points! i've done the following:

  • Removed the SignatureBytesToHex method
  • Removed failure tests for HexToECDSA and FileToECDSA
  • Fixed order of err check in uploader.go
  • Slight adjustment to vaults account address where it defaults to a filepath, but you can also do vaults account address --string abcd1234 such that you can explicitly specify it's a string. Thus, backward compatible and clearer on intended behavior.

@dtbuchholz dtbuchholz merged commit abe96bd into main Feb 28, 2024
@dtbuchholz dtbuchholz deleted the dtb/signer-lib branch February 28, 2024 20:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants