Caveats v2

[dstufft]

A start on implementing the ideas from #11873, the stuff that's under warehouse/macaroons/caveats/* should all be working, currently only supports a single caveat, the expiry caveat.

Includes the better error messages idea (though implemented in a different way since verification was completely rewritten with this PR) from #11885.

The most important part (IMO) of this, is that the API for writing caveats is significantly better in this PR:

@as_caveat(tag=0)
@dataclass(frozen=True, slots=True, kw_only=True)
class Expiration(Caveat):
    expires_at: int
    not_before: int

    def verify(self, request: Request, context: Any, permission: str) -> Result:
        now = int(time.time())
        if now < self.not_before or now >= self.expires_at:
            return Failure("token is expired")
        return Success()

The above is all you have to do, and it just works, and unlike the current implementation it doesn't have to run through every possible caveat, with deserialization and validation intermixed. By the time the caveat's verify method is called, the caveat system has already ensured that the data was correct (number of fields, the types of the data in those fields, etc).

The Caveat.verify method can return either a Success or a Failure(reason: str), and the failure message is what will get surfaced to the end user.

The tag value in as_caveat(tag: int) is what uniquely identifies different types of caveats, and the system will ensure that it is actually unique.

Under the covers, this serializes caveats as an array of [TAG, ... fields], where the order of fields is the order they are defined in the caveat class. This means that fields cannot be reordered, and new values have to be added to the end and they have to include a default value. Otherwise, if you want new mandatory fields or fields in a different order, you have to make a new caveat with a new tag.

The new caveat API hasn't been plumbed through the code base, so nothing works currently, and I need to reimplement the other types of caveats that we had besides expiry.

I'm not sure how I'm going to handle the old caveat format. One option is to just leave the old caveat objects stuffed in like warehouse/macaroons/caveats/legacy.py and register them in addition to our new verify function. Another option (which I think I like better), is to teach the new verify function how to adapt the old formats to the new formats before it deserializes them into a Caveat object to verify.

Fixes #11873

[woodruffw]

This new API looks really nice!

Another option (which I think I like better), is to teach the new verify function how to adapt the old formats to the new formats before it deserializes them into a Caveat object to verify.

This makes sense to me, and has (IMO) slightly more intuitive control flow -- that way there's only one top-level verify, which can take either the modern or legacy path as necessary.

[dstufft]

Yea, I personally lean towards writing adapters that can turn the old style caveats into the new style prior to actually verifying them. That way we don't have to have two implementations verifying the same thing.

[dstufft]

This should be ready to review now.

• Existing tokens are accepted by using adapter functions that can translate from the old format to the new format.
• permissions = "user" has changed semantics slightly, it now embeds a user id, and it verifies that the request.user.id is equal to that user id ¹.
• It's possible that existing tokens that were incorrectly being permissively parsed will no longer be able to be, because the new format is much stricter.
• Uses pydantic to validate that the values being given to the Caveat classes are valid and uses the Strict* variations so that pydantic doesn't coerce things like "50" to 50.
• Continues to emit the old-style format into the database for the non-authoritative fields and audit logs so that reading those doesn't have to cope with multiple forms.

I think this puts us in a place where our tokens are generally shorter, stricter, and the internal API for them is much easier to work with and reason about.

Footnotes

1. This should effectively be a no-op, since we generally look up user from the database that's associated with the macaroon. However, this is a belt and suspenders approach to make sure nothing unexpected has changed out from underneath us. ↩

[dstufft]

Oh, the API changed slightly, it's now:

from pydantic import StrictInt, StrictStr
from pydantic.dataclasses import dataclass

from warehouse.macaroons.caveats._core import Caveat, as_caveat

@as_caveat(tag=0)
@dataclass(frozen=True)
class Expiration(Caveat):
    expires_at: StrictInt
    not_before: StrictInt

    def verify(self, request: Request, context: Any, permission: str) -> Result:
        now = int(time.time())
        if now < self.not_before or now >= self.expires_at:
            return Failure("token is expired")
        return Success()

[woodruffw]

Just for visibility: #11272 will add "oidc" here, but if we merge this first (and we should!) we can avoid having to adapt it 🙂

[dstufft]

Yea, I would expect OIDC to make something like:

@as_caveat(tag=N)
@dataclass(frozen=True)
class RequestOIDC(Caveat):
    oidc_provider_id: StrictStr

    def verify(self, request: Request, context: Any, permission: str) -> Result:
        if not isinstance(request.identity, OIDCProvider):
            return Failure("token with oidc provider restriction without a OIDC Provider")

        if str(request.identity.id) != self.user_id:
            return Failure("current oidc provider does not match oidc provider restriction in token")

        return Success()

Which has the same (new) semantics, we're not pulling the oidc (or user) out of the caveats, but rather the token include an assertion that whatever request.identity is associated with the request has to be the expected OIDC Provider (or User).

The caveat itself doesn't care about how request.identity got set, it's just verifying it's the correct one. Kind of silly when the token identifier is how we're looking up that request.identity, but it adds a slight protection against confusion attacks where the request.identity comes from a different authentication source than the macaroon itself ¹.

Footnotes

1. In theory this shouldn't happen, but our code base doesn't prevent it, so it's possible? ↩

[woodruffw]

I did a quick pass over this, and it looks great! I can spend some time testing the functionality tomorrow.

[dstufft]

A nice thing that might not be obvious, is the new structure of these APIs puts the request in the verify function, so we can add caveats that make assertions about properties of the request itself (which the RequestUser already uses).

[woodruffw]

LGTM! This new structure looks really fantastic.

I'll stand up a local instance tonight and do some user testing and will re-app after that 🙂

[woodruffw]

Tested locally and confirmed that project-scoped tokens work as expected!