Possible endorsement processing bug -- RevRegEntry transaction not being signed

[swcurran]

A team has reported that there might be a problem with the RevRegEntry endorsement process -- the transaction is not being properly signed by the Endorser when requested by the Author. The problem is only apparent on ledgers where the RevRegEntry transactions must be signed by an Endorser, and not all Indy ledgers require such a signature. The Sovrin MainNet does require it. It appears when the first RevRegEntry is processed (along with the RevRegDef), the transaction is signed (or perhaps it is part of the RevRegDef transaction?), but subsequent RevRegEntry transactions are not signed by the Endorser.

The first part of this task is to review the code and see if the endorser flow for a RevRegEntry is indeed processed in the same way as all other transactions. This may be through inspection or by observing the Author-Endorser interactions for publishing the RevRegs.

If the issue is confirmed, a fix would be appreciated.

This is an urgent issue for the team facing the problem and any help from the community would be appreciated.

[WadeBarnes]

I've provided @ianco with an auth_rules batch file (attached, rename to auth_rules) that sets the rules for REVOC_REG_DEF and REVOC_REG_ENTRY to match Sovrin MainNet. So he can test things more easily locally with von-network.

auth_rules.txt

[WadeBarnes]

These are the tests that can be run after the rules are applied; https://github.com/hyperledger/aries-cloudagent-python/blob/main/demo/features/0586-sign-transaction.feature#L101

[WadeBarnes]

Related discord discussion can be found here.

[ianco]

I've run a quick test of the test indicated by @WadeBarnes above ^^^ and it passes with a "standard" von-network, but fails with the auth rules applied, error is:

aiohttp.web_exceptions.HTTPBadRequest: Ledger request error. Request failed: client request invalid: UnauthorizedClientRequest('FykwANS9NA4cumckKMQCJn', 1692817581637881892, 'Rule for this action is: 1 TRUSTEE signature is required and needs to be owner OR 1 STEWARD signature is required and needs to be owner OR 1 ENDORSER signature is required and needs to be owner\nFailed checks:\nConstraint: 1 TRUSTEE signature is required and needs to be owner, Error: Not enough TRUSTEE signatures\nConstraint: 1 STEWARD signature is required and needs to be owner, Error: Not enough STEWARD signatures\nConstraint: 1 ENDORSER signature is required and needs to be owner, Error: Not enough ENDORSER signatures').

Question - is "indy" wallet supported any more?

[swcurran]

Re: that last question — Question - is "indy" wallet supported any more?. It is, but deprecated, so we don’t care about that.

Questions:

1. Does the test work if Askar is used but nothing else is changed?
2. Can you see if the Endorser has signed the transaction? The report on Discord suggested that they traced the code enough to see that the request to invoke the Endorser might be wrong, such that the Endorser is not actually signing the transaction. This would be a bug in ACA-Py.
3. I read the error message as saying the rule is defined such that the “Endorser must be the owner” (same for Trustee, Steward), as opposed to “Must be signed by an Endorser and the owner”. If I’m reading the error correctly, then the bug is in the config rules on Sovrin MainNet — and it has never been detected because everyone using the network has made their Issuer DID an Endorser.

@WadeBarnes — is the LSBC DID on Sovrin MainNet still an Endorser?

[swcurran]

Reading the rules, I think there is a bug in the rules. Note the rules for Add RevRegDef and Add RevRegEntry, below.

For Add RevRegDef, the Endorser/Trustee/Steward does not have to be the owner, for Add RevRegEntry, the Endorser/Trustee/Steward does have to be owner.

@ianco — could you please do a change to the rules to change the highlighted “true” to “false” and see if that fixes the error.

I still would like to have confirmation that the Endorser signature is on the transaction — so we can confirm it is not a bug in ACA-Py.

Add Revocation Registry Definition

- Require 1 Endorser signature

ledger auth-rule txn_type=REVOC_REG_DEF action=ADD field=* old_value=* new_value=* constraint="{"constraint_id":"OR","auth_constraints":[{"sig_count":1,"role":"101","constraint_id":"ROLE","need_to_be_owner":false}]}"

Add Revocation Registry Entry

- Require 1 Owner signature

ledger auth-rule txn_type=REVOC_REG_ENTRY action=ADD field=* old_value=* new_value=* constraint="{"constraint_id":"OR","auth_constraints":[{"sig_count":1,"role":"0","constraint_id":"ROLE","need_to_be_owner":true},{"sig_count":1,"role":"2","constraint_id":"ROLE","need_to_be_owner":true},{"sig_count":1,"role":"101","constraint_id":"ROLE","need_to_be_owner":true}]}"

[ianco]

Re: that last question — Question - is "indy" wallet supported any more?. It is, but deprecated, so we don’t care about that.

Of course not, but it matters if it works with an indy wallet but not with an askar wallet. (Helps to isolate where the problem is.) FYI I checked with a couple of older aca-py versions and neither indy nor askar works.

Questions:

1. Does the test work if Askar is used but nothing else is changed?

Yes as I mentioned - it passes with a "standard" von-network (no updates to the auth rules) but fails with the new rules applied (matching sovrin mainnet)

2. Can you see if the Endorser has signed the transaction? The report on Discord suggested that they traced the code enough to see that the request to invoke the Endorser might be wrong, such that the Endorser is not actually signing the transaction. This would be a bug in ACA-Py.

I've attached the log of the integration test - it looks to me like the transaction isn't getting endorsed.

T002.1-RFC0586.log

3. I read the error message as saying the rule is defined such that the “Endorser must be the owner” (same for Trustee, Steward), as opposed to “Must be signed by an Endorser and the owner”. If I’m reading the error correctly, then the bug is in the config rules on Sovrin MainNet — and it has never been detected because everyone using the network has made their Issuer DID an Endorser.

I agree it looks like the rule is incorrect:

UnauthorizedClientRequest('PuMupSAdss6JnZYZZkKYNF', 1692819379803914562, 'Rule for this action is: 
1 TRUSTEE signature is required and needs to be owner 
OR 
1 STEWARD signature is required and needs to be owner 
OR 
1 ENDORSER signature is required and needs to be owner

[ianco]

@ianco — could you please do a change to the rules to change the highlighted “true” to “false” and see if that fixes the error.

I'll give this a try

[swcurran]

Sorry — I’m not being clear. Does the test fail with Indy, pass with Askar with the same Auth Rules? From what I think you are saying, the tests are failing when the auth_rules change, regardless of whether Indy or Askar are used.

If the transaction is not being endorsed, that sounds like a bug in ACA-Py, doesn’t it? Assuming the endorser requirement to ACA-Py is that every transaction to be written to a ledger must be endorsed. AFAIK — that was the intention, to assume that the ledgers all require an Endorser signature.

[ianco]

@ianco — could you please do a change to the rules to change the highlighted “true” to “false” and see if that fixes the error.

I'll give this a try

I made the change to the auth rules and the test still fails. (I'm not sure about the syntax of the rules, if the endorser is required then the transaction still must be signed by the "owner" of the revocation registry, I'm not sure how that is reflected in the rule.)

It looks to me like aca-py isn't endorsing this transaction, so any ledger constraints on this type of transaction will make the test fail.

FYI to run the test ./run_bdd -t @T002.1-RFC0586 in the demo directory. If you're running a local von-network (the default) you can update the ledger auth constraints per @WadeBarnes 's documentation above ^^^

[swcurran]

OK thanks. Confirms this is a bug in ACA-Py. Now to get it fixed…

@ianco, would you be able to do a scan of the code and see if you see anything obvious? Sorry to ask, but you know this code the best...

[ianco]

I suspect endorsement is just not implemented for this type of transaction, but I'll take a look and provide some pointers one way or the other (how to fix or implement).

As an aside I don't think this transaction should require endorsing. It introduces a potential impediment to the issuer, if they have a need to quickly revoke a credential, but they're dependant on an endorser who may or may not have specific service levels that they are supporting.

[ianco]

The fix for this is (potentially) fairly straightforward. The same ledger entry (txn type 114) is used for both the initial revocation registry accumulator txn (published when the revocation registry is created) as well as any updates (published as credentials are revoked). The first transaction is sent to the endorser, the second isn't.

In the revocation routes.py code (https://github.com/hyperledger/aries-cloudagent-python/blob/main/aries_cloudagent/revocation/routes.py#L1278) the code to publish the initial entry is called as:

        rev_entry_resp = await rev_reg.send_entry(
            profile,
            write_ledger=write_ledger,
            endorser_did=endorser_did,
        )

... however when a credential is revoked, the code in the revocation manager.py (https://github.com/hyperledger/aries-cloudagent-python/blob/main/aries_cloudagent/revocation/manager.py#L153) is:

              await issuer_rr_upd.send_entry(self._profile)

... which defaults to writing the txn to the ledger directly, rather than using an endorser (if configured).

To have the subsequent revocation entries use the endorser, the two parameters (write_ledger and endorser_did) need to be added to the calls to the send_entry() function (based on whatever is configured).

Additionally the unit and integration tests need to be updated.

[ianco]

One other thing to consider - since most ledgers don't require this transaction to be endorsed, I suggest either aca-py check the ledger config to check if these transactions need to be endorsed or not, or an additional aca-py parameter added to specify to endorse these transactions (so they do not get endorsed if not required by the ledger).

[swcurran]

Thanks, @ianco — much appreciated!! We could add (yet another!!) configuration parameter. But I think it be best that we just use the same pattern for these transactions. We assumed this was already there, and is a topic we have been talking about from the Endorser side — e.g. the Endorser wanting to control over whether it will still endorse the transactions.

[hiteshgh]

assigned to @shaangill025 and ready to go

[WadeBarnes]

The fixes in #2558 do not seem to be working.

I built ghcr.io/hyperledger/aries-cloudagent-python:py3.9-pr2558-pre0 off commit cfd9aed and deployed into our Shared Services dev environment for testing.

Testing was done with the BC VC Pilot agent that is connected to an endorser service.

I revoked this credential before deploying the pre-release image; https://candyscan.idlab.org/tx/CANDY_DEV/domain/33483

• As expected there is no endorser signature on the transaction.

I then deployed the pre-release image and revoked this credential; https://candyscan.idlab.org/tx/CANDY_DEV/domain/33484

• I was expecting the transaction to go through the endorser workflow and be endorsed before it was written, but there is no endorser signature on the transaction.

Logs from an additional revocation attempt:

• https://candyscan.idlab.org/tx/CANDY_DEV/domain/33485
  • No endorser signature.
• RevokingAgentDebugLogs.txt
• EndorserDebugLogs.txt

[swcurran]

Another one for you @amanji . Not sure what is happening after the fix was completed, but seems not to be working. Will be a new PR (if needed) to fix something in PR #2588. Perhaps how the feature is being used in the deployment is the issue (although it should be automatic…).

[amanji]

The fixes in #2558 do not seem to be working.

I built ghcr.io/hyperledger/aries-cloudagent-python:py3.9-pr2558-pre0 off commit cfd9aed and deployed into our Shared Services dev environment for testing.

Testing was done with the BC VC Pilot agent that is connected to an endorser service.

I revoked this credential before deploying the pre-release image; https://candyscan.idlab.org/tx/CANDY_DEV/domain/33483

• As expected there is no endorser signature on the transaction.

I then deployed the pre-release image and revoked this credential; https://candyscan.idlab.org/tx/CANDY_DEV/domain/33484

• I was expecting the transaction to go through the endorser workflow and be endorsed before it was written, but there is no endorser signature on the transaction.

Logs from an additional revocation attempt:

• https://candyscan.idlab.org/tx/CANDY_DEV/domain/33485

  • No endorser signature.

• RevokingAgentDebugLogs.txt

• EndorserDebugLogs.txt

Mind posting what the startup arguments or ENV settings are for the BC VC Pilot agent?

[amanji]

I've run through this a number of times and these are my current observations:

• Endorsement will be only be automatically executed when the agent is specifically run with the author endorser protocol role, (This is done either with the --endorser-protocol-role startup argument or by setting the ACAPY_ENDORSER_ROLE environment variable).
• Endorsement can be explicitly requested via the API by passing the create_transaction_for_endorser param in as part of a request.
• Even if the --auto-request-endorsement startup argument (or the ACAPY_AUTO_REQUEST_ENDORSEMENT environment variable is set), endorsement requests will only be sent by the agent if the endorser protocol role is also set to author (as described above).

I'm not entirely sure what could be failing without understanding which settings were used for the scenario described above for the BC VC Pilot agent.

In terms of running the BDD test @T002.1-RFC0586:

• Each API call in the flow explicitly passes the create_transaction_for_endorser and the endorser's conn_id request params (which will request endorsement of transactions). The only exception to this is the final step of publishing revocation entries (after an issued credential is revoked). This is the only step that doesn't explicitly pass those parameters, and without knowing more about how the agent is set to run, it is unlikely configured to automatically request endorser signing (at least this is what I think).
• In fact, when setting up a local endorser and author agent specifically, and running through every single step of this BDD test without explicitly passing request parameters for endorsement, the transactions are still sent for signing before publishing to the ledger.
• If the endorser protocol role is not set, requests that require ledger writes will attempt to publish automatically without signing by the endorser.

I may be overlooking something obvious here.

[WadeBarnes]

Mind posting what the startup arguments or ENV settings are for the BC VC Pilot agent?

[WadeBarnes]

I'm going to test this again with the 0.12.0rc0 release, in case there was something wrong with the prerelease image I created.

[WadeBarnes]

Testing with 0.12.0rc0 was successful.

• BC VC Pilot dev agent was upgraded to 0.12.0rc0.
  • This agent is connected to our CANdy Dev endorser.
• An existing credential was revoked, and the corresponding REVOC_REG_ENTRY was routed through the endorser for signing.
  • https://candyscan.idlab.org/tx/CANDY_DEV/domain/33592

[WadeBarnes]

Closing this as 0.12.0rc0 resolves the issues.

[swcurran]

YAY!!!