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

Extension Signing request endpoint #982

Merged
merged 34 commits into from
May 9, 2024

Conversation

usmansaleem
Copy link
Contributor

@usmansaleem usmansaleem commented Mar 20, 2024

PR Description

An extension sign endpoint /api/v1/eth2/ext/sign/:identifier which can be enabled with cli option --Xsigning-ext-enabled=true The cli option is associated with eth2 subcommand. For example ./web3signer eth2 --Xsigning-ext-enabled=true .... This extension covers some additional signing types and use-cases which are not covered by the remoting API specs and hence should be used at your own risk.

The format of request payload is:

{
  "type" : "PROOF_OF_VALIDATION",
  "platform" : "dappnode",
  "timestamp" : "1711338489397"
}

Only PROOF_OF_VALIDATION extension type is supported at the moment. Any other type would result in an error.

The signature is hex encoded string calculated as:

signature = BLS.sign(payload.getBytes()).asHexString()

For default accept type (*/*) and JSON accept type headers, the response is in following format:
Note:
Payload is returned in the response as Base64 encoded String.
Signature is returned as Hex encoded String

{
    "payload": "eyJ0eXBlIjoiUFJPT0ZfT0ZfVkFMSURBVElPTiIsInBsYXRmb3JtIjoiQVQiLCJ0aW1lc3RhbXAiOiIxNzE0NTI0NTQ0NDA3In0=",
    "signature": "0xb792ace1bae87b02457957bfa753065094e153855049074c880b943e7239d4277f4b2384d6aa19a3c03043b92bc8e90302ae48b9ac6b0a5e50796fc5e1d3993e9df6621ad6906e7a754147a9ccacdc9e7c18c1be355a647db19591904575bd30"
}

This would allow the users of this endpoint to verify the payload is signed by appropriate private key.

Fixed Issue(s)

Fixes #976

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

Testing

  • I thought about testing these changes in a realistic/non-local environment.

@usmansaleem usmansaleem marked this pull request as ready for review March 21, 2024 02:13
@usmansaleem usmansaleem changed the title Generic Sign endpoint Extension Signing request endpoint Mar 25, 2024
@usmansaleem usmansaleem marked this pull request as draft March 26, 2024 21:10
@usmansaleem usmansaleem marked this pull request as ready for review March 27, 2024 02:28
@usmansaleem usmansaleem requested a review from jframe March 27, 2024 02:28
Comment on lines 24 to 26
@JsonProperty(value = "platform", required = true) String platform,
@JsonProperty(value = "timestamp", required = true) String timestamp,
@JsonProperty(value = "pubkey", required = true) String pubkey) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These look like specific properties for a PROOF_OF_VALIDATION type rather than a generic signing type. Perhaps just rename SigningExtensionBody to ProofOfValidationBody?

}

private static String headerBase64() {
return BaseEncoding.base64().encode("{\"alg\": \"BLS\", \"typ\": \"BLS_SIG\"}".getBytes(UTF_8));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need a header? This is done differently from how we sign everything else.

routingContext.response().putHeader("Content-Type", JSON_UTF_8);
routingContext
.response()
.end(new JsonObject().put("data", dataToSign).put("signature", blsSigBase64).encode());
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to include the original data, the sender already hash that. And this change response format to being specific for extension signing

@usmansaleem usmansaleem requested a review from jframe April 15, 2024 05:53
Copy link
Contributor

@siladu siladu left a comment

Choose a reason for hiding this comment

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

What do we now in terms of API docs for this?

I'm thinking it will be hard for a future user to understand how to use this API, e.g. what should they put for "platform"? Also, wondering if platform is strictly necessary in the request?

@@ -2,6 +2,9 @@

## Next Version

### Features Added
- Added endpoint `/api/v1/eth2/ext/sign/:identifier` which is enabled using cli option `--Xsigning-ext-enabled=true`. This endpoint allows signing of additional data not covered by the remoting API specs. [#982](https://github.com/Consensys/web3signer/pull/982)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder if we should put this under a web3signer-specific namespace? e.g. /w3s/v1/eth2/sign, /w3s/v1/sign or/v1/eth2/w3s/sign.

There's precedent in teku for spec vs teku-specific https://consensys.github.io/teku/#tag/Teku and I think there's some merit for web3signer to follow that pattern.
Other CLs too: https://lighthouse-book.sigmaprime.io/api-lighthouse.html

@@ -13,6 +13,7 @@
package tech.pegasys.web3signer.core.service.http;

/** Operation IDs as defined in web3signer.yaml */
@Deprecated(forRemoval = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this deprecated?

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 class was used when we had vertx "openapi router". We have since removed that router and this interface is unused.

public record ProofOfValidationBody(
@JsonProperty(value = "type", required = true) SigningExtensionType type,
@JsonProperty(value = "platform", required = true) String platform,
@JsonProperty(value = "timestamp", required = true) String timestamp) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be timestamp typed? Makes sense to me to be the same as other timestamps, i.e. UInt64

Not sure platform should be types...what is platform actually used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am thinking to use uint64 as well because that would allow numerical literals both quoted and unquoted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@usmansaleem usmansaleem requested a review from jframe May 8, 2024 09:23
@usmansaleem usmansaleem merged commit 1c039cb into Consensys:master May 9, 2024
7 checks passed
@usmansaleem usmansaleem deleted the sign_extension branch May 9, 2024 23:45
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.

Let the user sign an arbitrary message with its keystore
3 participants