aptos-labs / aptos-core

Aptos is a layer 1 blockchain built to support the widespread use of blockchain through better technology and user experience.
https://aptosfoundation.org
Other
6.17k stars 3.64k forks source link

[Feature Request] Simplify account key rotation #4066

Closed JackyWYX closed 1 year ago

JackyWYX commented 2 years ago

[Feature Request] Simplify account key rotation

1. Proposal

The interface of rotate_authentication_key can be simplified as:

public entry fun rotate_authentication_key_simplified(
    address: address,
    from_scheme: u8,
    from_public_key_bytes: vector<u8>,
    to_scheme: u8,
    to_public_key_bytes: vector<u8>,
    cap_rotate_key: vector<u8>,
) { ... }

The simplified interface shall keep the same security level as the current implementation while help improves user experience in multi-sig key rotation greatly.

2. Motivation

Momentum Safe is building the secure and user-friendly multi-signature wallet solutions on Move ecosystem.

The system is built on top of the Aptos native Multi-Ed25519 implementation, and we are using the authentication key rotation as our solution to update the owners and threshold of a multi-sig wallet. The current interface for rotating the authentication key is

// aptos-core/aptos-move/framework/aptos-framework/account.move: 228
public entry fun rotate_authentication_key(
    account: &signer,
    from_scheme: u8,
    from_public_key_bytes: vector<u8>,
    to_scheme: u8,
    to_public_key_bytes: vector<u8>,
    cap_rotate_key: vector<u8>,
    cap_update_table: vector<u8>,
) acquires Account, OriginatingAddress { ... }

The interface is safe and strict. However, it is also quite complicated - it requires three signatures to construct a valid auth-key rotation transaction, which is especially complicated for multi-sig where the signatures are submitted and assembled asynchronizely:

  1. The owners before key rotataion need to sign on RotationProofChallenge, and collect enough signatures to form cap_rotate_key.
  2. The owners after key rotation need to sign on RotationProofChallenge, and collect enough signatures to form cap_update_table.
  3. The owners need to wait until the previous 2 signatures are collected, and then sign and collect enough signature of the key rotation payload.

Example

Let's take an example, for auth key rotation:

The process for collecting signatures is as follows:

  1. 2 of 3 from A, B, C sign on RotationProofChallenge.
  2. 3 of 4 from C, D, E, F sign on RotationProofChallenge.
  3. 2 of 3 from A, B, C wait for signatures from previous two steps are collected, and sign on transaction payload.
  4. The key rotation tranaction payload and signatures are constructed and submitted on-chain.

Thus the current implementation of key rotation requires at least two round to collect signatures async, meanwhile users will need to wait in each round, which is not a user-friendly process to change multi-sig owners.

Thus we propose the simplified version of rotate_authentication_key to reduce the overhead for key rotation, while keep the same security checks in protocol level.

3. Pitch

For the three signatures used in rotate_authentication_key, two of them can be optional:

3.1. Signature for s: &signer

The signer object in move requires a signature from the sender wallet that sign on the transaction payload to prove that the transaction is sent by a specific user.

However, in this specific use case, the signature to prove the authentication key rotation has already been verified for RotationProofChallenge, which is a combination of sequence number, originator (address), current auth key, and the new public key, which has already contains all essential arguments for key rotation change.

So the signer object here is possible to be simplified to the address object which won't introduce any security concerns, but will help remove one round of signature aggregation in multi-sig key rotation.

3.2. Signature for cap_update_table

The verification of cap_update_table was initially designed to ensure that the user have all the information to access the new wallet after key rotation.

But this signature is not an essential for key rotation. The new key check would be better to be ensured by the upper application layer. As the framework level, it is recommended to provide as minimum dependancy as possible, as long as the protocol security premises are not compromised.

4. Solution and implementation

Here is the solution that we propose:

4.1. Replace signer object with address

As discussed in 3.1, the verification of auth key is already done by verifying cap_rotate_key. So it is possible to change the first argument

account: &signer

to

addr: address

Since it's the address of the account that is only used in the sub function calls (the signer object is not used in the function), it is safe to directly change the account to address.

4.2. Set cap_update_table as optional

As discussed in 3.2, cap_update_table is recommended to set as an option to check, with some function warnings to alert the developer of the possibility that user may not have access after the key rotation. So it is recommended to add another key rotation function as:

public entry fun rotate_authentication_key_unverified_new(
    addr: address,
    from_scheme: u8,
    from_public_key_bytes: vector<u8>,
    to_scheme: u8,
    to_public_key_bytes: vector<u8>,
    cap_rotate_key: vector<u8>,
)

5. Alternatives

5.1. Add a new entry function

Instead of introducing a breaking API change to modify the existing signature rotate_authentication_key, it is recommended to add another interface for the simplified API, E.g.

public entry fun rotate_authentication_key_simplified(
    addr: address,
    from_scheme: u8,
    from_public_key_bytes: vector<u8>,
    to_scheme: u8,
    to_public_key_bytes: vector<u8>,
    cap_rotate_key: vector<u8>,
)

6. Conclusion

In this issue, we proposed two simplification of the rotate_authentication_key in account.move. The security check of the protocol is preserved by removing two signature fields in the function interface:

  1. Replace s: &signer with addr: address;
  2. Remove new signature check logic by removing cap_update_table;

This will help reduce two rounds of signature collection and greatly improve user experience of using the multi-sig wallet where user signatures are collected and submitted asynchronizely.

We also propose an alternative. Instead of introducing a breaking API change, it is also possible to add another function with name rotate_authentication_key_simplified that is used for simplified scenario of auth key rotation.

If the proposal is approved, we would love to open a pull request to contribute to Aptos code base.

PaulFidika commented 2 years ago

I like this; how about a special key-rotation method that just sends the auth-key to ZERO_AUTH_KEY, like

const ZERO_AUTH_KEY: vector = x"0000000000000000000000000000000000000000000000000000000000000000";

public entry fun rotate_authentication_key_to_zero(account: &signer).

Obviously wallets should flag and take extra precautions when signing anything involving an auth-key rotation, as a user could be permanently and irrevocably locked out of their account by mistake.

JackyWYX commented 2 years ago

I like this; how about a special key-rotation method that just sends the auth-key to ZERO_AUTH_KEY, like

const ZERO_AUTH_KEY: vector = x"0000000000000000000000000000000000000000000000000000000000000000";

public entry fun rotate_authentication_key_to_zero(account: &signer).

Obviously wallets should flag and take extra precautions when signing anything involving an auth-key rotation, as a user could be permanently and irrevocably locked out of their account by mistake.

I like the idea. It is actually the same as FREEZE THIS ADDRESS, and can be made a function that is only open to authorized users.

And yea, I would recommend the precaution on application level.

davidiw commented 2 years ago

public entry fun rotate_authentication_key_to_zero(account: &signer)

This seems like the most dangerous function ever :O.

The multisig problem seems real -- but security has its costs.

JackyWYX commented 2 years ago

public entry fun rotate_authentication_key_to_zero(account: &signer)

LOL. Yea, could have a better solution later. The issue here is about the &signer object makes it extra dangerous. But the issue is somehow unrelated to this feature request.

I agree security should have the highest priority. So the proposal requires no compromise in security. Do you have any concerns about security that relates to this change?

JackyWYX commented 2 years ago

Hey guys @alinush @davidiw @jjleng @chloeqjz, I wonder whether there is any updates about the approval of this feature. We may start implementing once approved :)

davidiw commented 2 years ago

We haven't yet had the time to review it. The team is very busy right now, though I saw your comment in another medium and have brought it up more broadly for the team to prioritize it.

alinush commented 2 years ago

Hey folks,

Thanks for opening this issue!

Please submit a PR that introduces only the rotate_authentication_key_simplified functionality which:

  1. Replaces the signer with an address, thereby removing the need for an additional signature.
  2. Removes the additional signature in the cap_update_table, thereby only relying on the signature in cap_rotate_key to authorize the key rotation.

On our end, we will evaluate if it is wise to deploy. Currently, we have a few concerns, which maybe you can give us your thoughts on.

  1. Having rotate_authentication_key_simplified could be mis-used by non-multisig wallets when implementing key rotation. Specifically, instead of doing a proper rotate_authentication_key which updates the OriginatingAddress table, a wallet might do a rotate_authentication_key_simplified, which does not update the table. This would resurface the exact account recovery issues we were trying to solve in #566 through the OriginatingAddress table.

  2. What will be other downstream consequences of introducing this rotate_authentication_key_simplified API? Because the OriginatingAddress table would become more of an OptionalOriginatingAddress table... It's difficult for us to tell.

  3. Should the rotate_authentication_key_simplified API still remove the entry for the old authentication key from the OriginatingAddress table (i.e., this line)? @davidiw, @chloeqjz, @areshand, I think it should, no?

    • Some context here: AFAICT, the cap_rotate_key was slightly misnamed by us since this signature is also what grants the capability to remove the entry for the old authentication key in the OriginatingAddress table (should there be one)
    • In contrast, cap_update_table gives the capability to add the new authentication key in the OriginatingAddress table.
  4. The rotate_authentication_key_simplified API actually allows anyone to implement a rotate_authentication_key_to_zero API that similarly also takes an address and a signature. @davidiw, @chloeqjz, @areshand, currently one cannot do that due the cap_update_table requirement. We'd have to evaluate how worrisome this is in terms of wasting space on chain.

PS: We do not understand what the use case for a rotate_authentication_key_to_zero is. Please don't add that to your PR, but feel free to start a separate issue about that.

JackyWYX commented 2 years ago

Thank @alinush for your detailed analysis on the simplified key rotation issue. Here is the TODOs in our end:

  1. Start implementing the PR for simplified key rotation change.
  2. Analyze the security concerns you mentioned here and circle back shortly.
  3. We will definitely NOT implement rotate_authentication_key_to_zero.

Thank you again for you swift decision making and detailed security analysis. Hands on it @peekpi.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 45 days with no activity. Remove the stale label or comment - otherwise this will be closed in 15 days.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 45 days with no activity. Remove the stale label or comment - otherwise this will be closed in 15 days.