Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

change behavior of recover_key to better support variable length keys #7853

Merged
merged 9 commits into from
Sep 13, 2019

Conversation

b1bart
Copy link
Contributor

@b1bart b1bart commented Sep 5, 2019

Change Description

#7421 introduced support for WebAuthN keys in the recover_keys intrinsic. As both keys and signatures for WebAuthN are not fixed length actually using this intrinsic was difficult owing to the semantics where:

  • passing too small of an output buffer would assert, failing the transaction
  • there was no input that would return the required output size without risking assertion

This change attempts to modify the semantics in a way that is most backwards compatible with the existing semantics.

For the K1 and R1 suites the semantics are the same.

For WebAuthN, the semantics are:

  • output buffer sizes from [0..33) will assert (maintaining old semantics where those values would always assert)
  • output buffer sizes from [33..+inf) will write as much of the recovered key data as will fit in the output buffer and return the total available key data size as the return value.

This allows contract developers to have a reasonable buffer size for a single-call use case and fall back to allocating a larger, correctly sized, buffer if and only if that fails.

Outstanding work

  • Unit tests for the behavior which depends on EOSIO.CDT integration of the variable length key/signature types. turns out this was implementable without requiring CDT changes.

Consensus Changes

  • Consensus Changes

API Changes

  • API Changes

Documentation Additions

  • Documentation Additions

…patible with webauthn signatures and keys. This is a WIP as we can now do some better testing for webauthn semantics but it needs cleaned up. This also fixes a compilation bug introduced by accident into the test contracts
…ry support to fix integration issues caused by wrappers
@b1bart
Copy link
Contributor Author

b1bart commented Sep 10, 2019

I ended up re-enabling and fixing the rest of the api tests for the crypto api. Mostly this reverted it back to a pre eosiolib-wrapper form which I think is more appropriate for testing the intrinsic ABI as it removes obfuscating elements.

@b1bart b1bart marked this pull request as ready for review September 10, 2019 16:38
payload_ds.write(sigs.data(), sigs.size());
payload.resize(payload_ds.tellp());

//No Error Here
Copy link
Contributor

Choose a reason for hiding this comment

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

There doesn't seem to be a need for this comment. (Same goes for the other two similar comments above. In fact, the one called "Error Here" is misleading because it is directly above a call to an action that is supposed to, and does in fact, succeed.)

@b1bart b1bart merged commit 2fb1c75 into develop Sep 13, 2019
@b1bart b1bart deleted the feature/webauthn-recover-key branch September 13, 2019 14:46
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