fix: ensure request matches offer, if sent

[dbluhm]

This change implements checking that ld proof credential requests match their corresponding offer, if an offer was sent.

If using the --auto-* flags for issuance, it was possible for the receiver of the credential offer to change values in the request and ACA-Py would accept this and issue based off of the request values. The --auto-* flags are debug flags and should not be used in production which would mean that a controller should have been able to catch this discrepancy. However, it is still expedient for ACA-Py to check that the offer and request match to avoid this slipping past the controller as well.

There is a side affect of this check. We were permitting late binding of the credential subject ID to the holder in the request. Meaning, on request, the holder will automatically (when using auto flags) insert a DID key as the credential subject ID to ensure the holder can actually present proof of possession later. These changes modify this behavior such that it only applies iff the credential subject id is not set already (e.g. in the credential offer). This enables the issuer to bind the credential to a DID other than the holder's pairwise DID if an alternate is known to the issuer. If the issuer wants to permit late binding by the holder still, the credential subject ID should be omitted in the offer.

So, to summarize, the two modifications implemented here:

• Ensure the request doesn't change the credential unless the offer explicitly omits a credential subject ID
• Only override with holder did if the credential subject ID is omitted

[dbluhm]

Relevant holder did override PR from a while back: #1235

[swcurran]

Side note on this: The —auto-* really should be for more than debug purposes and should NOT (in my opinion) be limited to debugging, so this type of checking should be enforced in ACA-Py. Not using the —auto-* flags make controllers too complex. If a controller wants to issue a credential, the details of the “happy-path” back and forth on the protocol should not matter. If a deviation from the happy-path occurs, that should be handled by the controller, possibly by just a problem-report (e.g., a simple issuer would reject a proposal response to an offer-credential message).

I’ll add that this is a change in my thinking from the early days of ACA-Py when I thought that we should write “default" controllers that implemented —auto-* flags using the controller — an approach that I know think is both slower and massively over complicates controllers.

[dbluhm]

@swcurran I'm interested in discussing that further. I don't think I disagree; this aligns with other discussions we've had about the revocation endpoints as well. But, given that it is a change from earlier days, I think it would be beneficial to tease out some of the nuances of this controller evolution.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[dbluhm]

Can we get someone to review this change? @usingtechnology @swcurran? Being perfectly frank, I'm a bit annoyed this was dropped from the 0.10.0 milestone without warning when it's just waiting on a review.

[swcurran]

Thanks @dbluhm — sorry about the lack of communication. We won’t nail down the final 0.10.0 until tomorrow at the Maintainers meeting. We needed to get an -rc0 out there with the changes to this point. We’ll see what else is to be included in the release then.

Note that we did a successful “dev0” release, which likely would have been good enough, other than putting the wrong tag on it (0.9.1-dev0). I missed the breaking change in reviewing the list of PRs to go into the release.

[usingtechnology]

@dbluhm - I will review it this morning.

[usingtechnology]

Looks good to me. Appreciate the comments in the code and the pytests.