pluggable token auth driver #48
Conversation
implements a means to plug-in authentication token drivers by means of providing a token driver interface with a default (base) implementation for SLAPI. this change remains backwards compatible, but also permits an easy way to plug-in a different driver. token auth drivers are responsible for authenticating with an identity provider and building/returning a well formed jumpgate auth token. this is part of the keystone identity/auth workflow. implements: #47
|
@beittenc + @sudorandom -- I have a pending PR which implements this functionality. I suspect unit tests may be appropriate, but I wanted to get a 1st impression from you guys on this approach/code. Please have a look and advise. |
|
@sudorandom and I debated this one for a while this morning, but I think we've come to a consensus. I understand the need and how you've implemented it, but I think I'd prefer a different method. I'll do my best to communicate what he and I discussed, but let me know if something doesn't make sense. The need to have direct access to the authentication event within Jumpgate is going to be important for any number of providers, so exposing it within Jumpgate itself (as opposed to the SoftLayer drivers) makes sense. However, I think this is a good place to break with OpenStack's service design slightly to provide a better user experience within Jumpgate. You've understandably implemented the token auth driver within Identity, but I think it makes sense to promote it to its own top level driver that's peer to identity, compute, and the rest. The problem we were specifically trying to solve was two fold. First, we need a to allow providers to validate auth tokens that are passed in with each request in a consistent, global fashion. Second, we need to have a mechanism for them to inject global changes into the environment as a result of said authentication. We've been doing this so far with the add_hooks() method in each SoftLayer driver, which is a bit messy. If we had a full auth driver, it could flow something like this. Excuse the poor quality of the drawing. For most requests, the call would come into Jumpgate. If an auth driver was present, it would pass the request context onto the driver, which would authenticate the user and perform any other necessary tasks; for SoftLayer, this would involve creating the client object and putting it in the global environment. Then Jumpgate would pass the full request onto the actual request driver as it does now. In terms of implementation, this optional driver would be a peer to the other drivers, meaning it could use our existing dynamic loading system and not need the custom import method you created. The driver would not have any routes, but instead would inject a hook into the before hooks list so that each request would be processed accordingly. The SoftLayer implementation of the auth driver would be similar to what you have now, just residing in a new location. The only other change that would need to happen is that the initial authentication currently handled by the identity driver would need to leverage the auth driver if it exists. Hopefully all of that makes sense. If it doesn't, let me know and I'll see what I can do. Or if you'd prefer I implement it, just let me know and I'll knock it out as soon as possible. |
|
@beittenc + @sudorandom -- thanks for the response and details... I agree with most of your points above, but I would like to present a slightly different approach for your consideration.. In my mind the following aspects should be pluggable:
At 1st glance it would seem that authN of per request tokens and the actual authN done on the Request Token Authentication [DEFAULT]
...
request_authentication_driver = jumpgate.sl.boden.authn.MyRequestDriverI would see driver having a single promised method in its interface: def authenticate_request(req, resp, kwargs):Someone can implement this interface and plug in their concrete impl which is loaded and run to handle per request token authN/validation. They would obviously be able to inject request env context as needed. Of course the default would be similar to what Identity Driver Authentication Authentication Token Creation Final Notes
Thoughts? |
|
@beittenc + @sudorandom -- did you guys get a chance to consider my previous comment? Would like to get going with an impl if we can converge on an approach. Also - although I like the use of falcon, the one thing I miss from paste is the ability to configure middleware on the pipeline via ini file. For example w/r/t to this issue it seems to me the per request auth should be middleware similar to how openstack services use the keystone auth_token middleware to validate per request. To that end I mocked up a very simple means to do pluggable middleware here: https://github.com/bodenr/jumpgate/compare/bodenr;middleware-proto?expand=1 Not expecting this to be consider as a real PR, but wanted to get your thoughts on something like this. It seems to me pluggable middleware will be useful for jumpgate for consumers to plug in their own middleware. For example plug in rate limiting middleware or auditing middleware... Thoughts? |
|
In general, we agree that increasing the amount of pluggable interfaces is a good idea. Personally, I'm not sold on using "middleware" as the term, but that's something that can be changed without much trouble if needed. Having the distinction between a driver (something with routes) and a plugin (optional code used to assist drivers and general functionality) should increase the flexibility of Jumpgate significantly. We have full agreement on the request token authentication plugin. Between the discussions on here as well as the ones @sudorandom and I have had offline, there's no questions there. We are both hesitant on the identity driver authentication. In my mind, this is the responsibility of the identity driver. If the functionality is fully encapsulated within that driver, it can implement it however it pleases. I'm not able to come up with scenarios where having a plugin for this would benefit someone other than SoftLayer. Do you have some we could consider? The same goes for token generation. Why would this need to be pluggable outside the driver itself? There's no single defining method for generating a token; each implementation can do so however the developers see fit. Making it pluggable might save a small amount of code, but I'm not sure that's going to be the case in practice. I've reviewed your other pull request as part of this. Your implementation differs from what originally came to my mind, but I can see how it works and I'm onboard with it. I assume that the request_hooks and response_hooks arguments can actually be lists rather than single items. Otherwise, we'd be limiting our options in the future. As part of your other changes, I don't want to put SoftLayer-specific code into the stock config we ship with since Jumpgate is not a SoftLayer-specific tool. I'm guessing that won't happen in the full release since you just sent this to illustrate your ideas, but I figured I might as well note just so we're all on the same page. Moving the logging into a middleware plugin is awesome and I really like that change. One note on that, though, that may affect your implementation. I'm a huge fan of making the plugins be completely optional. That would necessitate changing your imports in the nyi module, however, since the logging has been changed to a plugin. At a glance, I'm not certain how that would work in practice, so I might be out of luck on this desire. This would probably be a lot easier if Python's object support was stronger. So, in short, let's go forward with the middleware plugin design, pretty much as you have it in that pull request. If possible, let's see if we can make them truly optional, but not if it makes coding too complicated or becomes non-Pythonic. And if you have example use cases or user stories for the last two plugins you suggested, please send them over so we can re-evaluate them. If anything doesn't make sense, let me know. Thanks! |
|
@beittenc thanks for the response... a few quick replies below. Request Token Plugin Identity driver authentication & token generation My example use case for making token authentication + token generation pluggable would be -- someone has a single master IMS account in SL and is providing logically mutli-tenancy under that account where they want to authenticate their logical uses say in LDAP and then always "run-as" their master IMS user for SLAPI operations... However even in this case the class refactoring approach I gave above would work with minimal effort. Pluggable request/response hooks I'll work on this approach with your suggestions in mind and do a PR when I have something ready. It's always easier to take nitty-gritty details when you can comment on actual diffs :) Thanks again guys |
|
One note about middleware... It might still be possible to use paste along with falcon. Falcon is a web framework that exposes a WSGI middleware. If we did use paste, we could use middleware instead of relying on the hooks built into falcon. This would allow us to reuse middleware from other OpenStack projects. For example, we could potentially have support for XML support from keystone itself: https://github.com/openstack/keystone/blob/master/etc/keystone-paste.ini#L70 |
implements pluggable authentication by means of introducing "middleware" hooks which can be added to the request/response pipeline. various portions of the existing pipeline are refactored into their own hooks including; validation of auth token (X-AUTH-TOKEN header), SL client binding and logging. additional refactoring is also done to make the identity token driver more extensible specifically allowing consumers to extend the token driver and plug in their own authentication and jumpgate token creation methods. finally the existing jumpgate auth token has been unified as a single json/dict. implements #47
|
@beittenc & @sudorandom -- I just added a new commit which implements the items we discussed above... Can you please have a look and provide feedback? |
|
@bodenr - I was actually talking to @sudorandom when your request came in. Between your examples and that discussion, I believe I've been convinced to make all of your examples pluggable, so you're welcome to proceed down that path rather than extending driver classes. Rather than the "middleware" name, however, we're more fans of "plugin" or something along those lines. I think this better indicates the optional nature of these objects and also allows us to inject more substantial code as middleware in the future should the need arise. As for the optional concept, you're correct about the UUID and format hooks. They should be mandatory and simply included as you've done. I meant that any additional plugins shouldn't be necessary for the core of Jumpgate to function. If a specific driver needs a plugin to operate, then that should be documented and included in the conf file. Does all of that make sense? Let me know what else you need. I should be able to keep a close eye on the repo for the next few days so we can get your stuff merged quickly. |
|
@beittenc thanks for the response... Are you saying you would prefer to have the identity token creation + authn (externally) pluggable vs extending the identity driver? To be honest its just as easy to extend the refactored driver and implement either/both of the methods you wish to override... I'm trying to understand if you now prefer to plug-in route? Thanks |
|
@sudorandom pointed out that the class extension method isn't as Pythonic as making the system pluggable. The work for both should be roughly the same, but making it pluggable allows it to be re-used by other implementations, while only increasing the Jumpgate testing footprint a small amount. So yes, I'd say go ahead and make it pluggable if you're still interested in doing so. |
this change implements the notion of pluggable API hooks whereupon providers and latch-in before/after hooks for the request/response flow. it also refactors the existing per request authN, logging, etc. into hooks. also included in this PR is the notion of pluggable identity token, token-id and authentication drivers which permits consumers to easily plug into the existing identity workflow without changing base jumpgate code. implements: #47
|
@beittenc + @sudorandom -- committed latest impl here and as you can see the ci build is clean. I would consider the latest commit fairly polished from my POV and thus I plan to start on some unit tests + pydoc, etc.. I suspect the unit tests will take a good day or 2 and therefore I would like to ask you for a quick (pre) visual code inspection just to make sure we are on the same page WRT the impl -- I'd hate to spend 2 days coding unit tests to find you guys totally disagree w/the impl and have to throw away all that work. Please advise on latest code here and I'll begin on some additional unit tests. Thanks |
|
|
||
| LOG = logging.getLogger(__name__) | ||
|
|
||
| _req_hooks = {'optional': [], 'required': []} |
There was a problem hiding this comment.
Just a style issue, globals should be all capitalized. I would prefer that we avoid globals, especially the kind we have to mutate, if at all possible.
There was a problem hiding this comment.
@sudorandom I can fix that thx...
Trying to get an overall opinion from you guys on this impl so I can move forward with the unit tests... i.e. "yes this is looking like the right direction, or no this is way off and we want you to re-impl like this"
There was a problem hiding this comment.
fyi -- fixed this using a singleton class to cache hooks
|
Yeah, I like the direction this is headed. All my comments are fairly specific to the implementation and not the design. The design looks good to me. +1 |
updates to the pluggable token, auth, hooks implementation including: pydoc for the extendible driver interfaces, unit tests, minor bug fixes, replace of global var usage in hooks with singleton pattern. implements #47
|
@beittenc & @sudorandom -- latest changes committed here (includes some unit tests, py docs, etc..) and clean CI build. From my perspective this PR is ready to merge. Therefore I would ask you guys to please review and provide specific comments for any items which would prevent a merge. Thank you |
|
Looks good to me |
|
@sudorandom -- any comments, or can you guys move forward with a merge? |
implements a means to plug-in authentication token drivers by means of
providing a token driver interface with a default (base) implementation
for SLAPI. this change remains backwards compatible, but also permits an
easy way to plug-in a different driver. token auth drivers are
responsible for authenticating with an identity provider and
building/returning a well formed jumpgate auth token. this is part of
the keystone identity/auth workflow.
implements: #47