Caveats should return True or False, not raise

[ewjoachim]

This is #8562 with one more commit to take care of #8591/#8565.
EDIT: Actually no, read below for detail, but this PR does 1 thing: make the code much more explicit regarding raising vs returning False in macaroon-related code to express the idea that there was a problem when verifying a macaroon.

In addition to the 100% coverage that has proven unsufficient twice in a row, here are the tests I did:

$ git rev-parse --short HEAD
dbf49b83

$ cat ~/.pypirc|grep -v password
[distutils]
index-servers =
    pypi
    testpypi
    local-warehouse

[...]

[local-warehouse]
repository = http://localhost/legacy/
username = __token__

$ cd ../umbrella
$ twine upload -r local-warehouse dist/*
Uploading distributions to http://localhost/legacy/
Uploading raincoat_umbrella-1.0.0-py2-none-any.whl
100%|████████████████████████████████████████████████████████████████████████████████████████| 7.94k/7.94k [00:02<00:00, 2.85kB/s]
Uploading raincoat_umbrella-1.0.0-py3-none-any.whl
100%|████████████████████████████████████████████████████████████████████████████████████████| 8.70k/8.70k [00:03<00:00, 2.88kB/s]
Uploading raincoat-umbrella-1.0.0.tar.gz
100%|████████████████████████████████████████████████████████████████████████████████████████| 6.78k/6.78k [00:00<00:00, 31.2kB/s]

I've tested that the same twine upload on my previous PR failed, and now it works.

That being said, I want to add integration tests too, to avoid regressions. I'll add the integration tests in a different unrelated PR that we can merge before this one to make sure.

[ewjoachim]

This approach is not going to work.

I'm getting more and more familiar with the way pymacaroons works, and there's a problem.
In order for a macaroon to be valid, it checks the signature attached to the cacaroon, comparing it with the computed signature which is comprised of:
HMAC(HMAC(HMAC(HMAC(key, identifier), caveat0), caveat1), caveat2) (...)

The only caveats that are considered are those that are verified, which means the verifier got at least 1 positive callback for this caveat.

In this PR's code we have:

    def verify(self, key: str) -> None:
        self.verifier.satisfy_general(V1Caveat(self))
        self.verify_signature(key=key)

    def verify_signature(...):
        ...
        self.verifier.verify(self.macaroon, key)

When I wrote this I thought verifier.satisfy_general was checking the caveat and verifier.verify was checking the signature. Actually, verifier.satisfy_general registers a caveat check to be run, and verifier.verify runs all checks including caveats & signature (well, caveat check is just a part of signature check in the end).

This means:

• verify_signature method name is false and misleading
• Calling verify will work as before, but calling verify_signature directly will attempt to check the signature without registering a callback for the caveat. There's a default callback that (long story short) returns False, which means the signature will never match, because it will never consider the caveat, and because the signature attached to the macaroon includes the caveat, then verify_signature will never find a working signature.

If we wanted it to work, when solely checking the signature, one would need to attach a satisfy_general callback along the lines of lambda c: True.

Back to the drawing board.

[ewjoachim]

In a comical turn of event, I think I'll split this splitted PR into

• Setting those raise/return parts straight, without adding the verify_signature
• Add the verify signature. If we still need it. I'm thinking again about @di 's comment and now that I have a better understanding of how caveats work, we can have a macaroon whose signature is only valid for a specific set of met and unmet caveats, and there's no way to tell, so we presented with a given macaroon, we can say whether its signature IS valid or not if we know what caveats are supposed to be met or not, but knowing if a signature CAN in general be valid would require 2^(number of caveats) checks. So yeah, considering that a leak of the id is equivalent to a leak of the macaroon itself become much more interesting.

That would mean I'd remove the verify_signature altogether, this would not be needed anymore for #8563 and the problem described above disappears.

Sometimes, less is more.

[ewjoachim]

I've done that.

[ewjoachim]

Closing in favor of #9264 which is (to me) a much better approach of the problem.