Bind cache entries also contain the app marker now #24
Conversation
This makes it impossible to abuse the bind cache to authenticate to another app using the same credentials. Closes #20
|
This also fixes another potential issue: |
| if self.factory.bind_service_account: | ||
| yield self.bind_service_account() | ||
| self.bound = True | ||
| if self.factory.is_bind_cached(request.dn, app_marker, request.auth): |
There was a problem hiding this comment.
This is potentially inefficient because it resolves the username in any case before checking if the (DN, app marker, password) tuple is contained in the bind cache.
But this is also a security feature if the user was removed in the meantime :)
There was a problem hiding this comment.
I think some people, who optimze away all ldap requests will not like this.
May be we can make this configurable - one day (undocumented) ;-)
There was a problem hiding this comment.
I guess so! :) I opened #25 to keep track of that.
| self._cache = {} | ||
|
|
||
| def add_to_cache(self, dn, password): | ||
| def add_to_cache(self, dn, app_marker, password): |
There was a problem hiding this comment.
What is good pythonian?
Would we one day define a cacheObject instead of a tuple, which has the attributes
- dn
- app_markes
- password
and then define comparison methods?
There was a problem hiding this comment.
Hmm, good question! I'd probably go for something like def add_to_cache(self, cache_object, password), where cache_object=(dn, app_marker). Simply because it is no problem to print cache_object to the log, whereas we should never print password to the log :)
| if self.factory.bind_service_account: | ||
| yield self.bind_service_account() | ||
| self.bound = True | ||
| if self.factory.is_bind_cached(request.dn, app_marker, request.auth): |
There was a problem hiding this comment.
I think some people, who optimze away all ldap requests will not like this.
May be we can make this configurable - one day (undocumented) ;-)
This makes it impossible to abuse the bind cache
to authenticate to another app using the same credentials.
Closes #20