fix: add `accept: application/json` header to remote signer requests and improve invalid response handling #6587

cnupy · 2024-03-24T17:33:49Z

Motivation

Some Remote Signing Service implementations do not return by default response in application/json format and the signing fails.

Description

This pull request modifies externalSignerClient.ts to add the accept: application/json request header, instructing the remote signer to return the response as json.

Steps to test or reproduce

Reproducible using Diva DVT as remote signer.

CLAassistant · 2024-03-24T17:33:54Z

All committers have signed the CLA.

codecov · 2024-03-24T17:57:05Z

Codecov Report

Merging #6587 (facf88c) into unstable (3076b4c) will decrease coverage by 0.02%.
The diff coverage is 13.33%.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #6587      +/-   ##
============================================
- Coverage     61.70%   61.69%   -0.02%     
============================================
  Files           556      556              
  Lines         58799    58820      +21     
  Branches       1886     1887       +1     
============================================
+ Hits          36283    36287       +4     
- Misses        22475    22492      +17     
  Partials         41       41

nflaig

Thanks @cnupy for looking into this, let's try to get this in our next release

I think there needs to be a general handler for text vs json response, this logic is basically the same across all three functions.

I think it is ok right now to have a fix we can include for Diva but long term we need to use a declarative API client as we do for other APIs in @lodestar/api package and test it against the spec.

packages/cli/src/cmds/validator/signers/index.ts

packages/validator/src/util/externalSignerClient.ts

nflaig · 2024-03-27T09:56:49Z

It would also good to make sure we test against Diva in our CI, there are already generic signer tests here

lodestar/packages/validator/test/e2e/web3signer.test.ts

Line 15 in 971871e

describe("web3signer signature test", function () {

We would just have to extend this function

lodestar/packages/test-utils/src/externalSigner.ts

Line 18 in 971871e

export async function startExternalSigner({

I can't tell how complex it is to run Diva like this, but would be awesome to test against

cnupy · 2024-03-27T19:41:29Z

Thanks @cnupy for looking into this, let's try to get this in our next release

I think there needs to be a general handler for text vs json response, this logic is basically the same across all three functions.

I think it is ok right now to have a fix we can include for Diva but long term we need to use a declarative API client as we do for other APIs in @lodestar/api package and test it against the spec.

Thanks for the feedback. This is actually my very first time writing anything in TypeScript, so please excuse me if I've messed up a few things. I will do my best to address all comments for now, and will be more than happy to help with the declarative API client in the near future (once I get more familiar with the codebase).

nflaig · 2024-03-27T19:53:45Z

This is actually my very first time writing anything in TypeScript, so please excuse me if I've messed up a few things

Don't worry about that, this is a great PR to improve compatibility with Diva and also follow the spec, as we should have been supporting text responses already. Happy to assist on the PR to get it merged, and feel free to join our discord and ask any questions there as well

will be more than happy to help with the declarative API client in the near future (once I get more familiar with the codebase).

I am currently working on a major rework of the API client, it doesn't consider text responses right now as a wire format but could extend in the future. Happy to collaborate on that as well once it is ready

…move error logging and improve exception messaging for res != OK; fix content type parsing and handling of invalid content types;

cnupy · 2024-03-28T06:39:07Z

It would also good to make sure we test against Diva in our CI, there are already generic signer tests here

lodestar/packages/validator/test/e2e/web3signer.test.ts

Line 15 in 971871e

describe("web3signer signature test", function () {

We would just have to extend this function

lodestar/packages/test-utils/src/externalSigner.ts

Line 18 in 971871e

export async function startExternalSigner({

I can't tell how complex it is to run Diva like this, but would be awesome to test against

It shouldn't be hard to instantiate a Diva node container, but the actual testing might be harder than consensys/web3signer, as Diva is a distributed P2P remote signer and I'm not sure how to get it to sign a test message or return a test key store. Perhaps someone from Diva could help here. Generic tests against a pre-prepared response payloads might be easier to get the job done for now.

nflaig · 2024-03-28T10:48:48Z

but the actual testing might be harder than consensys/web3signer, as Diva is a distributed P2P remote signer and I'm not sure how to get it to sign a test message or return a test key store

Unless they have some sort of standalone or dev mode it will probably be hard based on how it is configured here. I have asked in their Discord if there is a simpler setup.

packages/validator/src/util/externalSignerClient.ts

… media types;

…ty (response error)

packages/validator/src/util/externalSignerClient.ts

cnupy · 2024-03-29T07:28:32Z

but the actual testing might be harder than consensys/web3signer, as Diva is a distributed P2P remote signer and I'm not sure how to get it to sign a test message or return a test key store

Unless they have some sort of standalone or dev mode it will probably be hard based on how it is configured here. I have asked in their Discord if there is a simpler setup.

Not much cooperation in Diva DVT discord, and their node/remotesigner is closed source for now. I guess not much can be done about that for now.

nflaig · 2024-03-29T11:09:22Z

Not much cooperation in Diva DVT discord, and their node/remotesigner is closed source for now.

I got a response but this is not possible in current release, they noted something about next release they might include it.

Based on just this PR it feels like there is definitley some work that Diva has to do, e.g. return errors if something fails. It doesn't seem like it would be stable right now to include in our CI.

I am ok with including a fix as long as we make sure web3signer is not impacted which we actively test against so should be fine there.

nflaig · 2024-03-29T13:00:56Z

OK, is there anything else I need to do?

Existing tests are passing so I think we are good on that front, if you have confirmed that Diva compatibility is fixed as well I think we can go ahead and merge this.

Will do another review round locally, but this PR should be good to include in our next release.

Thanks @cnupy for fast updates and helping us to be compatible with Diva :)

cnupy · 2024-03-29T13:23:59Z

OK, is there anything else I need to do?

Existing tests are passing so I think we are good on that front, if you have confirmed that Diva compatibility is fixed as well I think we can go ahead and merge this.

Will do another review round locally, but this PR should be good to include in our next release.

Thanks @cnupy for fast updates and helping us to be compatible with Diva :)

I can confirm that I can successfully sign messages and submit attestations using Diva as a remote signer on Holesky. I'm still getting Precondition Failed (412) errors on some requests, but this is probably related to the DVT cluster consensus. I also get Invalid aggregate and proof errors from the beacon node from time to time, but I don't think this is related to this PR.

Thanks @nflaig for your support :)

nflaig

I was talking to the Diva team about the response format, and they already support json responses for all APIs as per spec but currently default to text/plain if no Accept header is set. But mentioned this default behavior will be changed in the next release.

The main concern I have with the changes in this PR is that adding text/plain support will make it harder to reuse the generic api client for the remote signer in the future. And to be spec compliant we also don't really need to implement handling text responses as we can just state in the Accept header that we only accept json. It is only required by the server to support both content type responses.

I would suggest the following

make sure we set accept: application/json in all requests
explicitly validate content-type header in response and throw good error message if header is missing or format is unsupported (instead of some weird parsing errors)
include the error improvements to fall back to res.statusText if res.text() is empty

… error handling; add accept header to all requests;

cnupy · 2024-04-04T07:32:10Z

make sure we set accept: application/json in all requests

explicitly validate content-type header in response and throw good error message if header is missing or format is unsupported (instead of some weird parsing errors)

include the error improvements to fall back to res.statusText if res.text() is empty

All done and works as expected.

nflaig · 2024-04-04T12:11:51Z

All done and works as expected.

Awesome, thanks for the fast update. I pushed some minor updates but those shouldn't change anything. You can review and approve then we should be good to merge this

cnupy

Great. Thanks for correcting that.

nflaig · 2024-04-04T12:32:33Z

Great. Thanks for correcting that.

woops, I broke json parsing but fixed now. Want to do some more debugging locally against web3signer to see how we handle those errors etc.

If you make sure we work with Diva we are good to go 🚀

nflaig · 2024-04-04T12:56:26Z

packages/validator/src/util/externalSignerClient.ts

-  return JSON.parse(await res.text()) as T;
+function getErrorMessage(errBody: string): string {
+  try {
+    const errJson = JSON.parse(errBody) as {message: string};


No idea why neither Web3Signer nor Diva return proper errors as json but I made this a bit more future proof to handle potential json errors.

The spec does not define an error schema so we can't be sure how remote signers implement this.

I don't think it's a good idea to treat error response as json if it's not in the spec. Even if the error response is actually json, you are not guaranteed that there is a field named message (it could be msg, error or even errorMsg ...) and the result will be worse than if you had included the response content as plain text. You may also need to check the content-type of the error response before treating it as json. I know there is a try/catch block, but why do this if it's not in the spec?

Ok fair, we would have to check content type and response format is unclear. Just assumed sane format similar to beacon api. Although for errors is not as critical to check proper format and have fallback logic via try/catch.

Let's keep error handling as is, the Diva team is working on the spec will ask them about it and see if we can standardize this a bit better.

nflaig

LGTM, thanks again @cnupy for the fast response cycles

packages/validator/src/util/externalSignerClient.ts

wemeetagain · 2024-04-23T19:03:59Z

🎉 This PR is included in v1.18.0 🎉

cnupy requested a review from a team as a code owner March 24, 2024 17:33

cnupy changed the title ~~Handle remote signer non application/json response content type correctly~~ Handle remote signer non application/json response content type Mar 24, 2024

cnupy changed the title ~~Handle remote signer non application/json response content type~~ fix: handle remote signer non application/json response content type Mar 24, 2024

cnupy changed the title ~~fix: handle remote signer non application/json response content type~~ fix: handle remote signer non "application/json" response content type Mar 24, 2024

fix: handle remote signer non "application/json" response content type

e062c15

cnupy force-pushed the unstable branch from f1651c5 to e062c15 Compare March 24, 2024 17:52

cnupy added 2 commits March 25, 2024 13:07

Merge branch 'ChainSafe:unstable' into unstable

5f54826

Merge branch 'ChainSafe:unstable' into unstable

b02f666

nflaig requested changes Mar 27, 2024

View reviewed changes

nflaig added this to the v1.18.0 milestone Mar 27, 2024

Merge branch 'ChainSafe:unstable' into unstable

657caab

restrict logger to minimal set and remove cast

ecdcc6a

cnupy added 2 commits March 27, 2024 22:44

Fix typo - minamal; refactor/inline response handling functions; re…

383b07b

…move error logging and improve exception messaging for res != OK; fix content type parsing and handling of invalid content types;

fix: exception message text

23ca7ba

nflaig reviewed Mar 28, 2024

View reviewed changes

cnupy added 2 commits March 28, 2024 17:34

Removed logger in externalSignerClient; Improved handling of response…

8ce75ab

… media types;

fix: use response statusText as error message if response body is emp…

22f7131

…ty (response error)

nflaig reviewed Mar 28, 2024

View reviewed changes

packages/validator/src/util/externalSignerClient.ts Outdated Show resolved Hide resolved

cnupy added 2 commits March 28, 2024 22:48

fix: exception message text proper epmpty string check

6791ae3

fix: exception message text proper epmpty string check

f36052c

fix: lint errors

6742089

nflaig requested changes Apr 3, 2024

View reviewed changes

reintroduce handleExternalSignerResponse generic function with better…

282cc86

… error handling; add accept header to all requests;

cnupy changed the title ~~fix: handle remote signer non "application/json" response content type~~ fix: add accept: application/json header to remote signer requests and improve invalid response handling Apr 4, 2024

cnupy and others added 2 commits April 4, 2024 08:34

Merge branch 'ChainSafe:unstable' into unstable

94580d5

fix: remove forgotten check in externalSignerUpCheck function

2b8e722

cnupy requested a review from nflaig April 4, 2024 08:04

nflaig added 2 commits April 4, 2024 13:05

Simplify checking response media type

2696ea8

Include json parse error message in thrown Error

12b7bd9

Remove unnecessary string cast of err body

4364c8b

cnupy commented Apr 4, 2024

View reviewed changes

Fix json parsing

bec5c5b

More robust error handling

c3f1aa3

nflaig reviewed Apr 4, 2024

View reviewed changes

nflaig added 3 commits April 4, 2024 14:14

Formatting

3100d07

Simplify err body message parser

127909d

Revert error handling changes

facf88c

nflaig approved these changes Apr 4, 2024

View reviewed changes

nflaig changed the title ~~fix: add accept: application/json header to remote signer requests and improve invalid response handling~~ fix: set accept header in remote signer requests and improve invalid response handling Apr 4, 2024

nflaig changed the title ~~fix: set accept header in remote signer requests and improve invalid response handling~~ fix: add accept: application/json header to remote signer requests and improve invalid response handling Apr 4, 2024

nflaig changed the title ~~fix: add accept: application/json header to remote signer requests and improve invalid response handling~~ fix: add accept: application/json header to remote signer requests and improve invalid response handling Apr 4, 2024

jeluard reviewed Apr 4, 2024

View reviewed changes

packages/validator/src/util/externalSignerClient.ts Show resolved Hide resolved

packages/validator/src/util/externalSignerClient.ts Show resolved Hide resolved

jeluard self-requested a review April 4, 2024 14:42

jeluard approved these changes Apr 4, 2024

View reviewed changes

nflaig merged commit fd6691d into ChainSafe:unstable Apr 4, 2024
22 of 23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add `accept: application/json` header to remote signer requests and improve invalid response handling #6587

fix: add `accept: application/json` header to remote signer requests and improve invalid response handling #6587

cnupy commented Mar 24, 2024 •

edited

CLAassistant commented Mar 24, 2024 •

edited

codecov bot commented Mar 24, 2024 •

edited

nflaig left a comment

nflaig commented Mar 27, 2024

cnupy commented Mar 27, 2024

nflaig commented Mar 27, 2024

cnupy commented Mar 28, 2024 •

edited

nflaig commented Mar 28, 2024

cnupy commented Mar 29, 2024

nflaig commented Mar 29, 2024

nflaig commented Mar 29, 2024

cnupy commented Mar 29, 2024

nflaig left a comment

cnupy commented Apr 4, 2024

nflaig commented Apr 4, 2024

cnupy left a comment

nflaig commented Apr 4, 2024

nflaig Apr 4, 2024

cnupy Apr 4, 2024 •

edited

nflaig Apr 4, 2024

nflaig left a comment

wemeetagain commented Apr 23, 2024

fix: add accept: application/json header to remote signer requests and improve invalid response handling #6587

fix: add accept: application/json header to remote signer requests and improve invalid response handling #6587

Conversation

cnupy commented Mar 24, 2024 • edited

CLAassistant commented Mar 24, 2024 • edited

codecov bot commented Mar 24, 2024 • edited

Codecov Report

nflaig left a comment

Choose a reason for hiding this comment

nflaig commented Mar 27, 2024

cnupy commented Mar 27, 2024

nflaig commented Mar 27, 2024

cnupy commented Mar 28, 2024 • edited

nflaig commented Mar 28, 2024

cnupy commented Mar 29, 2024

nflaig commented Mar 29, 2024

nflaig commented Mar 29, 2024

cnupy commented Mar 29, 2024

nflaig left a comment

Choose a reason for hiding this comment

cnupy commented Apr 4, 2024

nflaig commented Apr 4, 2024

cnupy left a comment

Choose a reason for hiding this comment

nflaig commented Apr 4, 2024

nflaig Apr 4, 2024

Choose a reason for hiding this comment

cnupy Apr 4, 2024 • edited

Choose a reason for hiding this comment

nflaig Apr 4, 2024

Choose a reason for hiding this comment

nflaig left a comment

Choose a reason for hiding this comment

wemeetagain commented Apr 23, 2024

fix: add `accept: application/json` header to remote signer requests and improve invalid response handling #6587

fix: add `accept: application/json` header to remote signer requests and improve invalid response handling #6587

cnupy commented Mar 24, 2024 •

edited

CLAassistant commented Mar 24, 2024 •

edited

codecov bot commented Mar 24, 2024 •

edited

cnupy commented Mar 28, 2024 •

edited

cnupy Apr 4, 2024 •

edited