Auth post-mortem (Pyramid 2.0) #3422
Comments
|
We'd be more than happy to see someone take it on and try it. The only concerns that we have from core is that we want to make sure that it is backwards compatible and that may be an issue since the API's don't map well to each other. It's something that has been discussed quite a bit on IRC, and in private, but there are some other open issues around authn/authz, mostly related to being able to access the request object. |
|
Hmm... Backwards compatibility will be a bit awkward, but I think doable. I'll see if I can sketch something up. |
|
Okay, here's a quick sketch: master...luhn:auth-sketch
The new interfaces deviate from the post-mortem, since I figured this sketch was a chance to exercise some creativity/opinions. The sketch could be adjusted to better match the post-mortem if that's what the core wants.
Apologies if I'm overstepping my bounds. |
|
Some comments:
|
|
Thank you for your comments!
After reading your feedback, I do think I got ahead of myself with that sketch. My apologies. I should make sure I understand the post-mortem a bit better. So, looking at the sample application in the post-mortem, it shows |
|
Edit: You can probably skip this, I don't think I agree with it anymore. Okay, I’ve spent quite a bit of time revisiting your feedback, the post-mortem, and my own opinions. I think the post-mortem is on track with the Identity interface, the framework should be able to provide built-ins that cover the majority of use cases ( The idea of Authorization being driven by a The thing I do take issue with is Having
|
|
Reviewing the implementation of the builtin authentication policies, it looks like |
|
One thing that I find hard to square is Pyramid's idea that I guess that's what the post-mortem is getting at with Look at me, saying "minimal time invest from you" and here I am writing walls of text only to reach where you guys have been all along. Sigh. |
|
Just in case I haven't annoyed everybody yet, I'm going to try again. I propose we remove
Whether or not it's included has negligible effect on the implementation, so I'll get started now regardless. |
|
@mmerickel may have some ideas and thoughts too. I'd also like to ping @dstufft if he's willing to give his feedback on how the current authn/authz is fairing in warehouse. |
|
Well there's a lot to talk about with the security apis. I think one of the major questions we need to ask is whether we want to redefine the protocol between the authn and authz policies. Right now they communicate via prinicipals (a list of strings). This is restrictive, things like macaroons can not be distilled down into a declarative list. A more flexible system could support a richer protocol here. I think a common request would be for some form of opaque object specific to the identity policy. For example, there are things right now that are lost when using repoze.who or authtkt, etc because they are capable of encoding extra data alongside the userid but when we use The system we have right now: A possible improvement: This is one of the major recognitions of the post-mortem. That the identity policies should be highly reusable, more opaque and that the authorization policy is the extension point where users should write their own code to handle the identities and translate them into something meaningful. The second major goal I have is that remember / forget should be more useful for non-cookie-based authentication. I'd like to come up with some way to define them so they can return the token or body instead of only headers. For example a dict with Basically all of this is in line with the post mortem, and the issue has always been how to implement this in a bw-compatible-ish way. It does not need to be backward compatible, but there must be a clear migration path for people to use their old setup with pyramid without rewriting the actual policies. Anyway this is just me getting some thoughts down to get involved. I hope it is somewhat helpful. |
|
I would think
|
|
I'm happy to help make something happen with I glad to hear strict bw compat isn't a requirement, I'd like to break it a little if only so we don't have to append "new_" to everything we do. I agree on the identity being an opaque object. One thing I really like about the post-mortem API is |
|
Or maybe instead of an opaque object, there could be a |
|
Okay, I took some ideas swimming around my head (splitting auth into three parts; user identity object) and vomited them into Python: https://gist.github.com/luhn/4cba3eeb26c295ce70e2feb4da04e757 I'm not sure how much I like it myself. Are there too many knobs? Is IUserIdentity really the way to go? Does map nicely to the old auth API. |
|
Forgive me for the organization of this, it's very brain dumpy. Personally, I've never quite understood the use case of In Warehouse, we've made an effort to remove any security footguns like that, and for Basic Auth One thing we've added to all of the In Warehouse we're using pyramid_multiauth to handle the fact that we have multiple ways to authenticate (session, basic, macaroon in the near future). I'm not sure if keeping this implemented as a meta One major concern I have with splitting up of I think I feel like overall, the current class IUserIdentity:
id: Any
class IAuthenticationPolicy:
# It's possible we could just say that this returns ``Any``, and treat the object
# it returns as completely opaque. The only reason not to do that is if any part
# of Pyramid needs to *actually* know the structure of this object.
def __call__(self, request: Request) -> Optional[IUserIdentity]:
...This simplifies the authentication policy, making it dead easy to write your own. Of course Pyramid can provide some stand in ones (like it does currently), but those policies wouldn't need to be extensible like they currently are, because they're simple if someone needs something special from them, they can just write their own. Now this does get rid of the distinction between A quick look at how we might implement these things as they stand. @attr.s(auto_attribs=True, frozen=True, slots=True)
class UserIdentity:
id: Any
class SessionAuthenticationPolicy:
def __init__(self, prefix='auth.'):
self._prefix = prefix
self._userid_key = prefix + 'userid'
def __call__(self, request) -> UserIdentity:
if self._userid_key in request.session:
return UserIdentity(id=request.session.get(self._userid_key))
class BasicAuthenticationPolicy:
def __init__(self, check: Callable[[str, str, Request], UserIdentity], realm='Realm'):
self._check = check
self._realm = realm
def __call__(self, request) -> UserIdentity:
credentials = extract_http_basic_credentials(request)
if (
credentials
and self._check(credentials.username, credentials.password, request)
):
return self._check(UserIdentity)
@attr.s(auto_attribs=True, frozen=True, slots=True)
class MacaroonUserIdentity:
id: Any
macaroon: Macaroon
class MacaroonAuthenticationPolicy:
def __init__(self, get_key: Callable[[Macaroon, request], bytes]):
self._get_key = get_key
def __call__(self, request) -> UserIdentity:
# extract_http_macaroon is a fake method that returns a high
# level macaroon object that has a number of supported functionality
# that none of the current Macaroon libraries currently implement.
macaroon = extract_http_macaroon(request)
# For @mmerickel, verifies the data serialized in the identifier, but not
# the entire Macaroon or the caveat language.
macaroon.verify_data(key=self._get_key(macaroon))
return MacaroonUserIdentity(
id=macaroon.data["user.id"],
# Macaroon's user identity object also includes the macaroon that
# we authenticated the user from, this is because attempting to
# authorize the user has to be further attenuated by the Macaroon
# itself, so simply using the user's ID is not enough information.
macaroon=macaroon,
)As I write this, I realize that this most closely resembles the Now, there is the fact we no longer have a way for user's to specify they want to specifically access the user, but only if it still exists in their persistent data store. Personally I feel like this probably can roughly be equated into two categories:
For the first case, a very trivial wrapper policy can implement that easily, for instance in Warehouse we could implement it like: class DatabaseUserAuthenticationPolicy:
def __init__(self, policy):
self._policy = policy
def __call__(self, request):
identity = self._policy(request)
if identity is not None:
if request.db.query(User).filter_by(id=identity.id).first():
return identityFor the second case, It seems like that distinction could just as easily be determined by calling a function instead of a property on the request. So something like: def identity_in_db(request, identity):
return bool(request.db.query(User).filter_by(id=identity.id).first())
def my_view(request):
# Some code where I don't care if the user exists in the database anymore.
print(f"The user {request.identity.id!r} attempted to do something")
# Some code where I *do* care if the user exists in the database:
if identity_in_db(request, request.identity):
return {"user.id": request.identity.id}
else:
return {}I feel like this is a lot clearer and will involve a lot less custom code having to be dealt with inside of the This is somewhat similar to how the above proposal works, except it wants to standardize some sort of API (and thus an interface) that will provide a standard way to access the request identity, but after first verifying it still exists in the database. I can understand that desire, if there is an API that we need to provide in Pyramid that needs to check that. Otherwise I feel like the above is sufficient, and we don't need to provide an interface to let people avoid writing a 2 line request property if they really want it. Moving on to Authorization. The biggest deficiency in Warehouse's view in the current API is that it more or less assumes that given a set of principles, the permissions granted to those principles are more or less static, or at least are properties only of the context the permission is being asked of, and that the request itself cannot further attenuate the permissions. Smaller complaints about the AuthZ framework is that much like the AuthN policy, the use of principles feels particularly constraining here, and I feel like it would be much nicer to simply operate on an identity, and let AuthZ policies implement principles if they make sense. The fact that principles are generally strings, means that even if yo work around the request problem above with a thread local, you're likely going to be duplicating work (and possibly getting things wrong if if you're combining something like The API in the proposal above for AuthZ looks reasonable to me honestly, something like: class IAuthorizationPolicy:
def __call__(context, identity, permission, request):
...I would probably argue over the ordering of the arguments ;) but otherwise it looks sane to me. You can easily implement a current gen Pyramid style ACL policy on top of this by moving the class MacaroonAuthorizationPolicy:
def __init__(self, policy):
self._policy = policy
def __call__(context, identity, permission, request):
if hasattr(identity, "macaroon"):
macaroon = identity.macaroon
# Actually go through and provide all of the information
# needed to verify our caveats, this could be something
# like checking that the given permission is in the list of
# permissions in a permissions caveat, checking the request
# IP address to ensure it came from the correct time, checking
# some property of the object, or whatever.
if not macaroon.verify():
return Denied
# Dispatch to the underlying AuthZ policy, that implements AuthZ in
# whatever way makes sense.
return self._policy(context, identity, permission, request)Hopefully this sprawling brain dump has been generally useful! |
|
@dstufft thanks, this is great. I think everyone pretty much agrees we should stop requiring the policy to return a list of principals for flexibility. A user identity object seems like a good way to go here so long as the framework still has some way to get a string representing the user which it can use for logging/debugging purposes.
The entire point of unauthenticated_userid is that it is not like this at all. Unauthenticated just means it hasn't been verified by the application, but it should be trusted by the policy. The distinction is important. The goal is to make an authentication policy reusable. There are two steps 1) pull the identity from the request (standard, reusable logic) 2) decide if you actually want to trust it (extensible per-application logic). As such there is the unvalidated identity pulled from the request which is exactly unauthenticated. You can then extend the policy and define what authenticated means to your application, while at the same time knowing the policy that's giving you the identity to know what unauthenticated really means. Your example where you only check the I think the goal of the post mortem rings true here, and would help all of us get over this naming issue just by tweaking the location of responsibilities. The goal in my mind is that the Another thing to consider is some form of multi-policy support integrated into Pyramid such that if we do keep an identity-vs-authorization split there is a frameworky way for the authorizer to know which policy the identity is from without relying on some attribute in the identity. It's just a thought and is probably too much for Pyramid to require right now. |
|
Thanks @dstufft! I pretty much agree with you entirely. Your criticisms of my gist are valid and I think the proposal you lay out is quite elegant. |
|
@mmerickel How do you feel about the IUserIdentity interface? Edit: Nevermind, skipped over your bit "A user identity object seems like a good way to go here..." |
Yea, I would feel a lot less confused if
I would then only expose off of the request, the validated identity object and not the unvalidated object. If someone really needs to get at the unvalidated object, they can call the authentication policy APIs themselves, but that should be a special case I think, not a use case to design the primary API around. The distinction here between Basically the idea here is that the output should be more than just "claimed to be so and so" (as the current docs for |
|
If you look through all the existing policies (basic, repoze.who, authtkt) they all have some larger object they could return instead of the lowly userid. I think that's the first step is just allowing them to return that object in a semi-standard format. In basic's case it'd be the (username, password) instead of just the username. The validator object is what we currently call the groupfinder and it has definitely been a source of frustration to document. In the past few years I've modified the documentation to demonstrate subclassing instead. In this way you just define class MyAuthenticationPolicy(AuthTktAuthenticationPolicy):
def authenticated_userid(self, request):
unauthenticated_userid = self.unauthenticated_userid(request)
if unauthenticated_userid is not None:
# ... do stuff to authenticate and return a userid (it doesn't need to match the unauthenticated_userid)
def effective_principals(self, request):
principals = [Everyone]
authenticated_userid = self.authenticated_userid(request)
if authenticated_userid is not None:
# grab some extra principals here
principals.append(Authenticated)
principals.append(f'user:{authenticated_userid}')
return principalsThe api surface that Pyramid really cares about to do its job is this class ISecurityPolicy:
def identify(self, request):
""" Return a trusted and verified identity object."""
def remember(self, request, identity):
""" Return a dict containing headers, body, token that can be used in a
response to inform the client how to keep a user logged in."""
def forget(self, request, identity):
""" Return a dict containing headers, body, token that can be used in a
response to inform the client that they are logged out."""
def permits(self, identity, context, permission):
""" Return ``Allowed`` or ``Denied`` for the ``permission`` acting on ``context``."""For example the |
|
If the identity is a fully-fleged user object does anyone have compelling arguments why |
|
You might want to constraint the permissions based upon certain request properties. Like only allow accessing modifying certain objects from within a certain IP range.
Of course if you have an arbitrary object you can just smuggle the request through that arbitrary object, but that feels like working around how the API is supposed to work (since the request isn’t really part of the user identity).
…Sent from my iPhone
On Nov 20, 2018, at 6:17 PM, Michael Merickel ***@***.***> wrote:
If the identity is a fully-fleged user object does anyone have compelling arguments why permits should accept the request object? I'd need to go through old issues and see, because certainly people have asked for it but nothing has stuck in my brain as being a compelling reason.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
I'm interested if anyone has any opinions how a system would work that is more stateful. Some policy in charge of privilege escalation around login/logout, invalidating request.session, setting vary headers. I always handle these directly in my login/logout views and if |
|
I think Hard to imagine a stateful general-purpose security API, as authentication can be handled in such a myriad of ways. imo, even You make a good point about |
|
I take back everything I said about Should something like |
|
The argument against it, is that you technically only need to Vary the response if the output of the response itself is altered by accessing the user id. If you're accessing the user id to do something like send an email, log the current user or something, then you're needlessly adding a Vary. The argument for it, is that if you forget to |
|
For warehouse's specific case, You can't access the current user without decorating the view to state you're doing so, so it was easy for us to take the safe path and always add the FWIW, I think Django adds the Vary anytime you access |
|
@luhn I tend to agree about the remember / forget APIs, it's a hard problem. @mcdonc spent some time back around 1.5 trying to talk himself into |
|
I think an interesting approach would be to define some api around privilege escalation that gets invoked. There could even be an event emitted. The event/api would require the response object that is triggering the change to be passed to it. Systems could listen for this event and do things like invalidate the current session. |
|
Does anyone think it's a bad idea to merge the two policies into one? I think it's the only sane thing to do when relaxing the contract to something more general than a list of strings. The advantage before was that they could be configured separately but that only works well if there is a standard protocol for them to communicate. |
|
Can you be clear which two policies you're talking about merging together? There are a number of proposals in this thread, with varying sets of policies, so I just want to be clear :) |
|
Sorry, my focus was on the |
|
My only concern with joining them is it would make multi policy harder I think? IOW in Warehouse the user id can come from multiple sources, but we really only want to have one set of authorization for the user (although that's not entirely true with Macaroons, but it is for session vs basic auth). |
|
In a system like that I'd expect the multiauth policy to wrap the identity in something and then communicate back to the appropriate policy to invoke permits. It depends on what contract the multiauth policy imposes on the identities. It's really up to the multiauth policy but in this system where we no longer have a list of strings, the format of the identity object is opaque and thus there is not a standard permits that can handle any identity. The multiauth policy could define its own permits assuming all identities conform to some contract but that's up to the multiauth policy. |
|
While we are here... one of the things that regularly throws me off is that the auth policy is created once, and is not a factory. If the auth policy was a per request factory, then we could store values on This would also allow you to do It would also allow multi-auth more easily, since now your factory can determine what policy to use, and that can then be used to influence the authz policy more easily. |
|
I can see what you mean. Due to the lack of |
|
Kinda spitballing here, if there was a security policy factory, we could have class ISecurityPolicy:
id = Attribute('The ID of the user authenticated in the current request')
def permits(context, permission):
"Return `Allow` if this user is allowed this permission"
class MySecurityPolicy:
def __init__(self, request):
self.request = request
@property
def id(self):
return request.session['id']
@reify
def _user_obj(self):
return db.query(User).filter(User.id == self.id).one()
@property
def email(self):
return._user_obj.email
def permits(self, context, permission):
principles = self._user_obj.principles
return acl_helper(context, principles, permission)
Configurator(security_policy_factory=MySecurityPolicy)
def view(context, request):
return {'email': request.identity.email} |
|
FWIW, even as a "Pyramid 2.0" sort of thing, I don't think the bw compat problems implied by changing to a single policy is worth it. |
I think it's too early to decide until we have a concrete proposal and it's still early days for that. I agree it's not something to be done lightly. The current system does have some limitations that have been identified and there are currently some definitively wonky patterns recommended to get around them. Even the current pattern used in the tutorials to achieve a |
|
The conversation has dried up, any further issues that need to be discussed? I personally think it's worth discussing @bertjwregeer's suggestion of the security factory. If everybody's had their say, it seems @mmerickel proposal of |
The issue with caching anything is what to do at the privilege boundaries and it is why, at least in part I suspect, that Pyramid takes the approach right now of not caching anything in the auth system. However, I do agree that it's worth exploring how this might work but it is tricky and ties directly into remember/forget.
I'm not really clear at all what the |
Yeah, I figured that was why
That would be the object an auth policy returns, instead of the ID returned now. class IUserIdentity(Interface):
id = Attribute("""ID of the user.""")Good for policies that include extra metadata, like macaroons or (apparently) authticket. Also allows users to have the auth policy return a full user object if they so desire. |
|
If the only purpose of the identity is to standardize a single attribute then I'd probably push for a method on the policy like |
|
btw, to remind that |
|
@mmerickel, I feel like that method is going to cause confusion, since it's not obvious what it's being used for. Now that I think about it, what's the reason we need a user ID at all? The framework itself doesn't care (afaict) if the identity object is fully opaque, especially now that there's no boundary between authn and authz. |
|
@luhn I think it's highly useful for debugging but I don't have a problem with starting without it and seeing how things go? |
|
Hey guys, I haven't forgotten about this, sorry it's been so long. Between holidays and work I've had my plate full, hoping to pick this up again soon. |
|
I've reviewed the conversation and sketched this: master...luhn:auth-sketch-3 Authn+authz have been merged into
|
|
Awesome. I'd really like to hear from anyone who thinks this is worse than the current design which separates the two policies and enforces a transfer of principals between them. |
|
Probably not worse than the current, but I expect to see frameworks have their own pluggable thingies to these... my biggest problem has always been that the |
|
But... at least with |
Hi guys! I'm interested in taking a crack at reworking the auth system for Pyramid 2.0, based on the Auth API Post-Mortem. Would you be open to starting a discussion?
I like Pyramid a lot and would love to see a new auth API in 2.0, as mentioned in #2362. Hopefully I'm not being too brash, barging through the front door as a first-time contributor and asking to remodel kitchen. I want to be respectful of your time; once we hash out the details I'm confident I can make it happen independently with minimal time investment from you.
The text was updated successfully, but these errors were encountered: