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

Heartbeat data #1415

Merged
merged 8 commits into from
Sep 8, 2022
Merged

Heartbeat data #1415

merged 8 commits into from
Sep 8, 2022

Conversation

vponline
Copy link
Contributor

@vponline vponline commented Sep 2, 2022

Closes #1411

@vponline vponline requested a review from a team September 2, 2022 14:48
@vponline vponline self-assigned this Sep 2, 2022
@vponline vponline linked an issue Sep 2, 2022 that may be closed by this pull request
Copy link
Contributor

@aquarat aquarat left a comment

Choose a reason for hiding this comment

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

Looks good but two comments.

(And of course it lost my other comment):
The airnode address shouldn't be necessary because it can be extracted from the signature:

            const msg = "Hallo Welt";
            const signature = await owner.signMessage(msg);
            const signer = ethers.utils.verifyMessage(msg, signature);
            expect(signer).to.equal(owner.address);

maybe @aTeoke can comment on where the requirement came from/if there's a specific reason the airnode address is needed in the payload too? :)

@vponline
Copy link
Contributor Author

vponline commented Sep 5, 2022

Looks good but two comments.

(And of course it lost my other comment): The airnode address shouldn't be necessary because it can be extracted from the signature:

            const msg = "Hallo Welt";
            const signature = await owner.signMessage(msg);
            const signer = ethers.utils.verifyMessage(msg, signature);
            expect(signer).to.equal(owner.address);

maybe @aTeoke can comment on where the requirement came from/if there's a specific reason the airnode address is needed in the payload too? :)

I might be misunderstanding what you mean, but won't chainAPI need the airnode_address field to verify the signer? Or do you mean that their API will have access to the mnemonic to extract it directly from there?

@aquarat
Copy link
Contributor

aquarat commented Sep 6, 2022

I might be misunderstanding what you mean, but won't chainAPI need the airnode_address field to verify the signer? Or do you mean that their API will have access to the mnemonic to extract it directly from there?

Their API can extract the airnode address from the signature, so you don't need an extra field.
ChainAPI uses the Airnode mnemonic via Metamask for the initial user to log in, so from that process they'll have the airnode_address which they can compare against the recovered address when they verify the signature.

To rephrase: ChainAPI will receive a heartbeat containing a heartbeat payload and a signature. They'll start by verifying the signature against the payload and in the process they will also recover the signer. If verification succeeds they check for an integration that has that signer as it's airnode_address with a matching cloud, stage and region and assuming they find a match they update the last received heartbeat for that deployment. They can't trust the contents of the payload until they've checked the signature and if they've checked the signature they would have already recovered the airnode_address.

const ethers = require('ethers');
const msg = "Hello world";
const randomWallet = ethers.Wallet.createRandom();
const signedMessage = await randomWallet.signMessage(msg);
const recoveredKey = ethers.utils.verifyMessage(msg, signedMessage);
// recoveredKey here was '0x36e56E09214926aF279ce1a6392d58e246538afa'
if (recoveredKey === randomWallet.address) console.log('Addresses match');

@vponline
Copy link
Contributor Author

vponline commented Sep 6, 2022

ChainAPI uses the Airnode mnemonic via Metamask for the initial user to log in, so from that process they'll have the airnode_address which they can compare against the recovered address when they verify the signature.

Thanks for explaining, in this case I am also not sure why it was originally included in the payload 🤔

@amarthadan
Copy link
Contributor

Thanks for explaining, in this case I am also not sure why it was originally included in the payload thinking

I think I've created the confusion here. Sorry about that 😅

Base automatically changed from heartbeat-signature to temp-v0.10.0 September 8, 2022 14:35
@vponline vponline merged commit 24801bc into temp-v0.10.0 Sep 8, 2022
@vponline vponline deleted the heartbeat-data branch September 8, 2022 15:51
Siegrift pushed a commit that referenced this pull request Nov 3, 2022
* Add heartbeat signing

* Add changeset

* Use timestamp in heartbeat signature

* Add airnode_address, cloud_provider, stage, region to heartbeat payload

* Add changeset

* Remove airnode_address from heartbeat payload

* Fix martin github account
Siegrift added a commit that referenced this pull request Nov 8, 2022
* Avoid constants with side effects (#1394)

* Remove constants test

* Fix flaky test

One of the unsafe evaluate test is flaky because the setInterval used
internally might be potentially be called 4 times until the evaluation
is suspended by the node VM. This fix is to change the tick period to
make this even less probable.

* Use API_CALL_TIMEOUT constant in tests

* Remove API_CALL_TOTAL_TIMEOUT

* Add worker timeout test

* Refactor airnode-utilities timeouts

* Rename default timeout to default blockchain call timeout

* Add changeset

* Fix lint

* Remove unused delay configuration for deployer handlers

* Merge pull request #1396 from api3dao/remove-heartbeatId

Remove heartbeatId

* Heartbeat signature (#1398)

* Add heartbeat signing

* Add changeset

* Use timestamp in heartbeat signature

* Heartbeat data (#1415)

* Add heartbeat signing

* Add changeset

* Use timestamp in heartbeat signature

* Add airnode_address, cloud_provider, stage, region to heartbeat payload

* Add changeset

* Remove airnode_address from heartbeat payload

* Fix martin github account

* Move from API keys to route-based authentication for HTTP gateways (#1418)

* Remove gateway keys from validator and configs

* Remove gateway keys from local handlers, examples, request headers, gcp verification

* Add gateway path_key uuid to deployer, terraform and openapi templates

* Add pathKey to local gateway server

* Remove unused path_key variable from terraform, add uuid to local gateway docs, fix example secrets file, fix openApi template paths

* Increase npm registry ping delay

* Fix GCP gateway output url

* Generate gateway uuid with terraform random_uuid

* Uuse constant pathKey in local gateways

* Merge branch temp-v0.10.0 into remove-gateway-api-keys

* Update local gateway pathKey constant

* Fix lint and tests

Co-authored-by: vponline <[email protected]>
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.

Add deploy data to heartbeat payload
3 participants