Opened 12 years ago

Closed 8 years ago

#1608 enhancement closed duplicate (duplicate)

Add low-level support for WebDAV access controls

Reported by: Wilfredo Sánchez Vega Owned by:
Priority: normal Milestone: Web2-Gold-Master
Component: web2.dav Keywords:
Cc: Wilfredo Sánchez Vega, Thijs Triemstra Branch:
Author: dreid

Description

To support WebDAV access controls, we need some methods on DAV resources for getting and setting ACLs, for checking them, and for obtaining information about DAV principals.

Change History (36)

comment:1 Changed 12 years ago by Wilfredo Sánchez Vega

Owner: changed from jknight to Wilfredo Sánchez Vega

comment:2 Changed 12 years ago by Wilfredo Sánchez Vega

Status: newassigned

Done on branches/wsanchez/acl from trunk r16337.

comment:3 Changed 12 years ago by Wilfredo Sánchez Vega

Work includes checks for permissions in DAV methods. Includes a bunch of tests.

The default ACL for resources allows all access to all principals. (This is the level of permissions we have before.)

This implementation always returns the "unauthenticated" principal as the current principal for all requests. We need to tie it into the cred system in order to make further use of it.

comment:4 Changed 12 years ago by Wilfredo Sánchez Vega

Keywords: review added

comment:5 Changed 12 years ago by Wilfredo Sánchez Vega

Owner: changed from Wilfredo Sánchez Vega to Jean-Paul Calderone
Status: assignednew

comment:6 Changed 12 years ago by Wilfredo Sánchez Vega

Owner: changed from Jean-Paul Calderone to oubiwann

comment:7 Changed 12 years ago by Wilfredo Sánchez Vega

Owner: changed from oubiwann to David Reid

comment:8 Changed 12 years ago by David Reid

Keywords: review removed
Owner: changed from David Reid to Wilfredo Sánchez Vega

merge forward to wsanchez/acl-1608-2

comment:9 Changed 12 years ago by David Reid

Why is ACL support on the IDAVResource interface, and not on a seperate interface, and implemented as a subclass of DAVResource instead of directly on it? This division of labor seems like it would be beneficial?

comment:10 Changed 12 years ago by David Reid

Cc: David Reid added

comment:11 Changed 12 years ago by David Reid

twisted.web2.dav.static.DAVFile.getURI makes a silly assumption about the root being a static resource.

comment:12 Changed 11 years ago by Glyph

I just had a peek at the code. I know it's not ready for review yet (dreid repeated this several times ;-)) but I thought that there might be some issues with the design so I figured I'd pop in now rather than complain after somebody else reviews it and it gets merged...

Twisted generally tries to adhere to a particular school of security design. I'd call this "capability-based" but really we are doing a subset of capability-based things due to limitations in Python, UNIX, etc etc.

I filed ticket #2042 for one instance of this problem, which is why I was looking at the DAV ACL branch in the first place.

The rule of thumb that I use is that you should avoid conditionals to secure operations, and prefer method-dispatch as the control flow mechanism instead. e.g. avoid code like this:

class XFactory:
 def getX(self, user):
  return X()

class X:
 def y(self, z):
  if self.checkPermission('y'):
   doOperationY(self, z)
  else:
   raise AccessViolation('y not allowed')

And prefer, instead, code like this:

class XFactory:
 def getX(self, user):
  if user.authorizedFor('y'):
   return SecureX()
  else:
   return NotSecureX()

class SecureX:
 def y(self, z):
  doOperationY(self, z)

class NotSecureX:
 def y(self, z):
  raise AccessViolation('y not allowed')

The DAV branch seems to be written in the former style.

At first this appears to be a tedious academic distinction, because the two styles are identical for this case, and in the latter you have to write more code.

However, for more sophisticated use-cases it rapidly becomes useful. First of all, if you are going to be writing your own implementations of the implied interface here, let's call it IY, in the first style the easy implementation is to forget the required security check (because it's your method's responsibility) but in the second style the responsibility lies with the invoking code, so you won't be called unless the calling code has already been authorized for your operation.

For even more sophisticated cases, it is necessary. What if the security of the operation is dependent upon properties of both self and z? The checkPermission method isn't adequate any more, it doesn't have enough information.

The reason that this is particularly important in Twisted, rather than just being generally better design, is that Cred is designed to work with this security model. The role of XFactory is filled by the Realm, which should hopefully set up your security rules in advance.

I don't know enough about DAV ACLs to be sure that this isn't required by the protocol somehow, but the DAVPrincipalResource.checkCredentials method - which doesn't seem to be called anywhere yet - seems to be a warning sign of a pattern of potential security holes. The password-check should be done once, inside a cred Portal.login call. Security-critical code like that is very important to avoid duplicating.

wsanchez, I'll try to be around when you're back from Burning Man to chat about potential design ideas, since I may not know enough about DAV to appreciate all the issues here.

comment:13 in reply to:  12 Changed 11 years ago by David Reid

Replying to glyph:

I don't know enough about DAV ACLs to be sure that this isn't required by the protocol somehow, but the DAVPrincipalResource.checkCredentials method - which doesn't seem to be called anywhere yet - seems to be a warning sign of a pattern of potential security holes. The password-check should be done once, inside a cred Portal.login call. Security-critical code like that is very important to avoid duplicating.

Getting rid of the usage of that was the first thing I did when I started working on this branch, that code should have been purged long ago, now it has. So that's one less negative thing we have to worry about. :)

comment:14 Changed 11 years ago by Wilfredo Sánchez Vega

Status: newassigned

comment:15 Changed 11 years ago by Wilfredo Sánchez Vega

Component: web2web2.dav

comment:16 Changed 11 years ago by Wilfredo Sánchez Vega

Priority: normalhigh

comment:17 Changed 11 years ago by Wilfredo Sánchez Vega

glyph: To your point, I kinda get where you are going, but I think that's difficult to do for us. I think that trying to shoehorn the DAV logic into your design pattern will result in hard to follow code. In particular, it would be hard to read alongside the spec and understand whether you are following the spec or not.

It's not uncommon to have some access violations happen during request processing, not just before you start processing. As a contrived example:

class XFactory:
  def getX(self, user):
    return X()

class X:
  def y(self, request):
    if not self.checkPermission("Y", request):
      raise AccessViolation("Y not allowed")

    ok = []
    errors = []
    count = 0

    for item in list of items():
      if count >= 10:
        raise TooManyViolation("You're not allowed to do > 10 Y's")

      if self.checkPermission("A", request):
        doA(request)
        ok.append("A")
      else:
        errors.append("A")
      count += 1

    return MultiStatusReponse(ok, errors)

That is, some access exceptions will be raised before we really do anything and will turn into an appropriate response code. Others will happen after we've started processing. In some cases, we simply store the errors as we go and provide a summary or what worked and what didn't at the end.

One example of this is a recursive (depth=infinity) DELETE operation. We don't know we're going to raise until we get to a resource that we're not allowed to DELETE. Trying to determine this up-front is relatively expensive (have to walk the tree twice), plus DAV is wierd and wants you to continue past the errors anyway.

That is, the factory class has no idea whether the authorization is allowed until the operation has been tried, and in some cases, until well into performing the operation.

comment:18 Changed 11 years ago by Wilfredo Sánchez Vega

Cc: Glyph added

comment:19 Changed 11 years ago by Wilfredo Sánchez Vega

OK, we're thinking we can try this refactoring on a different branch. Some thoughts from glyph, for future reference:

[4:50pm] glyph: wsanchez: I do believe that the capabilities pattern that I'm talking about can be applied to the cases you're talking about in your comment though [4:50pm] glyph: wsanchez: for example, instead of doing doA in a loop in 'def y', call item.y(request) recursively [4:51pm] glyph: You need to implement the permission check in the symbolic equivalent of "getChild" anyway, and that would let you re-use it [4:51pm] glyph: you're going to have to catch exceptions coming up from there in the loop, sure, but I think that's still pretty easy to follow

comment:20 Changed 11 years ago by Wilfredo Sánchez Vega

Oops. A little better, perhaps:

[4:50pm] glyph: wsanchez: I do believe that the capabilities pattern that I'm talking about can be applied to the cases you're talking about in your comment though
[4:50pm] glyph: wsanchez: for example, instead of doing doA in a loop in 'def y', call item.y(request) recursively
[4:51pm] glyph: You need to implement the permission check in the symbolic equivalent of "getChild" anyway, and that would let you re-use it
[4:51pm] glyph: you're going to have to catch exceptions coming up from there in the loop, sure, but I think that's still pretty easy to follow

comment:21 Changed 11 years ago by Wilfredo Sánchez Vega

Milestone: Web2-Gold-Master

I think we need this before calling Web2 stable, because it effects existing ACL API in the trunk; attaching to the GM milestone.

comment:22 Changed 11 years ago by Wilfredo Sánchez Vega

Merged forward to dav-acl-1608

comment:23 Changed 11 years ago by Wilfredo Sánchez Vega

Merged forward to branches/dav-acl-1608-2

comment:24 Changed 11 years ago by Wilfredo Sánchez Vega

Cc: David Reid Glyph removed
Keywords: review added; dav removed

Tests pass on: debian-py2.3-select

debian-py2.4-select suse-py2.5-select debian-py2.4-reactors osx-py2.4-select osx-py2.4-select-gc win32-py2.4-er

I was unable to get the other builders to attempt tests.

comment:25 Changed 11 years ago by Wilfredo Sánchez Vega

Owner: changed from Wilfredo Sánchez Vega to Glyph
Status: assignednew

glyph, any chance you can review this branch? I tried to pull out as much as I could into separate tickets, but this last batch is fairly inter-related.

Most of the code here is by Cyrus Daboo, with some cleanup and smaller additions by me and dreid.

comment:26 Changed 11 years ago by Glyph

Priority: highhighest
Status: newassigned

Reviews should be "highest". Reviewin'.

comment:27 Changed 11 years ago by Glyph

Keywords: review removed
Owner: changed from Glyph to Wilfredo Sánchez Vega
Status: assignednew

First, the nitty gritty:

  • twisted.web2.dav.auth
    • Please pardon me if I'm referring to code that wasn't from this branch. SVN wasn't giving me a diff from 'acl.py', if that's where it originated.
    • Needs a docstring. Should explain the overall purpose of the auth system, and hopefully provide a brief example.
    • twisted.web2.dav.auth.AuthenticationWrapper
      • Miscapitalization in docstring: DAVREsource.
      • "more cred stuff"? Not very descriptive.
      • hook needs a docstring.
    • twisted.web2.dav.auth.IPrincipal
      • needs a docstring. Looks like this is actually a pretty important interface, since it's the cred interface.
      • needs some methods. I discourage "marker" interfaces because they typically actually do have some required methods or properties, and calling them "markers" just removes the documentation for same. It's better to have incomplete / incorrect documentation than to punt on it entirely.
      • should probably go in twisted.web2.dav.idav, given that there's already an interfaces module.
    • twisted.web2.dav.auth.DavRealm
      • This really ought to be in an example module, not the main implementation. In the cred model of the world, protocols really shouldn't provide their own Realm; Realm are for applications and integrated systems. If a protocol provides a Realm, it might not be compatible with an application Realm. For example, Divmod provides a database-backed realm for arbitrary protocols, in Axiom. Also, if your realm is going to hook up to other checkers, you can't depend upon the interaction of the avatar ID and the URI: avatar IDs are opaque and have to do with the implementation of the back-end: Active Directory is not necessarily going to be able to return something with the appropriate properties at that stage of the game. (Granted, cred's design could be better in this area, too, but at least the current scheme is flexible enough to let people with interest in a particular deployment fix integration issues in their realm object. That's one of the reasons it's so important to leave the implementation of that up to them.) If you are going to get rid of it in response to this, then obviously the rest of the comments here don't apply :).
      • needs a docstring.
      • coding standard: this should be DAVRealm according to the coding standard. "Acronyms should be capitalized in their entirety.". It's a bit vague and should include an example, but what that means is "HTTPClient", not "HttpClient". There are definitely a few violations in Twisted already, sorry if you caught this from a bad example. requestAvatarId is particularly galling, because I think that one was my fault :-|. Maybe we can pretend you're actually requesting the "id" from the avatar, and the fully-formed object is their "superego". Yeah.
    • twisted.web2.dav.auth.IPrincipalCredentials
      • basically the same feedback as IPrincipal
    • twisted.web2.dav.auth.PrincipalCredentials
      • needs a docstring, especially, what are the arguments to __init__?
    • twisted.web2.dav.auth.TwistedPropertyChecker
      • needs a docstring
      • You shouldn't need to adapt to IPrincipalCredentials in your requestAvatarId method: credentials is going to be a provider of your credential interface, (misdocumented as an "implementor" before we got more specific about the terminology), not something adaptable to your credential interface.
    • twisted.web2.dav.auth.TwistedPasswordProperty
      • needs a docstring
      • why isn't this registered using the magic in twisted.web2.dav.davxml? I hate the magic there, but at least it's consistent and could be transformed into something else. this ad-hoc thing seems unnecessary, but then I don't really understand what's going on.
  • twisted.web2.dav.davxml
    • the changes in this branch look fine.
    • did I mention I hate the magic here, though? I'm just re-stating my objection to import * and magical code-globbing stuff for the record. I've already registered a similar objection in #2407. I'll reserve filing an extra ticket until I have a solution to suggest, though.
  • twisted.web2.dav.element.base
    • Maybe we should actually serve something at the URL referred to by the new twisted_dav_namespace attribute. Separate issue though.
  • twisted.web2.dav.element.parser
    • twisted.web2.dav.element.parser.registerElement
      • parameters need documentation
      • parameters violate naming convention: use camelCase for parameters, underscores are for dynamic dispatch.
  • twisted.web2.dav.element.rfc2518
    • Should the protected flag be mentioned in the docstring? Just askin'.
  • twisted.web2.dav.element.rfc3744
    • General observation, not really related to this branch: ACE looks pretty complicated, but the test case only covers isAggregateOf. Just getting __init__ right should take something like 20 tests, counting the 'if's here! Absence of a test-case name is a clue that it was not developed test first :).
    • More specific observation, about this branch: previously this was an assert, but there's no test that I can see for constructing an ACE with a non-dav_namespace namespace in test_xml_rfc3744. If it's allowed it should be tested.
  • twisted.web2.dav.util
    • is de-publicing PrintXML really relevant to this feature? The branch is already huge, and it seems sort of spurious (and that does look like a useful utility...)
    • although there's a FIXME it is not deleted. One or the other should go.
  • twisted.web2.dav.http
    • twisted.web2.dav.http.NeedPrivilegesResponse
      • needs docstring. (the __init__ should probably be documented as "create a XXX" with the arguments, and the description of the class should go in the calss docstring).
      • Given the presence of the __init__ docstring this might seem like nitpicking, but remember that pydoctor will spit out "Undocumented" in the index for this class when generating the documentation.
      • This class is never mentioned in the entire test suite. Probably should be, for finer-grained testing.
    • twisted.web2.dav.http.statusForFailure
      • If an insecure path is attempted, I think it might be helpful to know, in the log, what the path was, for defense against attacks. Maybe the other logging information is good enough though.
  • twisted.web2.dav.idav
    • twisted.web2.dav.IDAVResource.findChildren
      • What does it mean that privileges should default to None? Wouldn't defaulting to an empty sequence be better? Or rather, being optional, and explicitly passing an empty sequence to mean 'no privileges'?
  • twisted.web2.dav.method.acl.http_ACL
    • Maybe I really just don't know enough about DAV but the return value of this function is not entirely clear, especially given that it uses deferredGenerator. It should document its return value in Python terms (referring to the RFC as appropriate, of course)
      • maybe it shouldn't. I think deferredGenerator is pretty error-prone. For example, are exceptions from mergeAccessControlList being handled properly? (I'm not sure they're not, it's just the sort of error I see a lot when using defgen.) Don't feel compelled to remove it if it's working out for you, this is just based on my experience with it.
    • There are several conditionals here and this function is never called from the unit tests. It really should be tested independently.
  • twisted.web2.dav.method.copymove.http_MOVE
    • no direct tests
  • twisted.web2.dav.method.delete.http_DELETE
    • no direct tests
  • twisted.web2.dav.method.get
    • all methods, and module, need docstrings
    • http_ methods need direct tests
  • twisted.web2.dav.method.prop_common
    • trailing whitespace
    • needs docstring
    • if this is apple code as well, does it require the license stuff you put in your files?
    • twisted.web2.dav.method.prop_common.responseForHref
      • needs docstring
  • twisted.web2.dav.method.report
    • twisted.web2.dav.method.report.NumberOfMatchesWithinLimits
      • needs docstring
      • should this be subclassing some more general web2 exception? is anyone going to want to catch it?
    • twisted.web2.dav.method.report.max_number_of_matches
      • coding standard violation. should be MAX_NUMBER_OF_MATCHES or maxNumberOfMatches, depending on its purpose. Is it configurable, RFC-specified, or the largest sane maximum?
    • twisted.web2.dav.method.report_acl_principal_prop_set.report_DAV__acl_principal_prop_set
  • twisted.web2.dav.method.report_expand
    • why is this log message being removed? did it serve a purpose in the first place?
  • twisted.web2.dav.method.report_principal_property_search.report_DAV__principal_property_search
    • This method is just way too long. If anything ever breaks it, NOBODY is going to be able to figure out what is going on. Again, testing smaller units would be helpful here. For example, those inner functions aren't closing over anything, either; no reason they should be inner.
    • coding standard violation on the principal_property_search parameter, should be principalPropertySearch
  • twisted.web2.dav.noneprops
    • the comment should probably be a docstring, like, "Do nothing, because..."
  • twisted.web2.dav.resource.DAVPropertyMixIn
    • the work done in this branch seems fine, but it seems like a good place for some judicious application of dynamic dispatch. Even if these aren't really overridable methods, it would read easier to have the various if name == "...": blocks broken up.
  • twisted.web2.dav.static.DAVFile.render
    • Refactor static.File to be pluggable in the appropriate place and override just the part you need. Copying this much security-critical code would be really, really bad. I think the copy isn't even correct; trunk seems to have drifted since this branch was made.
  • TODO.txt - HOORAY, this sort of thing should have been in trac in the first place...

More general issues:

Thanks for working so hard to get this to integrate with cred. I'd still like to see a bit more documentation on how to plug it into an existing cred configuration, what things the application programmer should be doing, etc, but it looks like it's almost right.

The test coverage here is, as you say, weak sauce. The test docstrings in most cases refer me to an individual method, not to the behavior being verified. We're not yet insisting on 100% coverage yet, since the coverage tools are so pathetic (I can't get coverage tools working on these tests for the same weird reason I can't get them to work on some other bits of code).

However, I see that you're probably ensuring coverage via careful attention to the RFC and various external testing tools. Maybe these tests actually provide better coverage than they appear to, since some of them are quite dense, with lots of nesting. When this hits re-review (it's a LOT of code, I am totally hosed and can't promise a prompt review), it would probably be good to provide the next reviewer with some instructions to run one of the external tools you're testing with, so more people are aware of this. Reviews are a good place to provide this kind of cross-pollenation. The tests pass but I am unfortunately not really sure this actually provides a working DAV server :).

Even if the unit tests and external compliance tests do ensure really good coverage, I'd still like to see a bit more granular unit testing, for two reasons. It benefits future maintainers. For this branch in particular, it benefits them in a particularly hairy and confusing area of the code. For future branches, more granular testing is a nice way to ensure that Twisted code is developed test-first, and there's really no other way to enforce that in review.

comment:28 Changed 11 years ago by Glyph

Priority: highestnormal

This was left at "highest" priority because it had just come out of review. I'm going to lower its priority to better reflect the priorities within the "web2 gold master" milestone.

comment:29 Changed 10 years ago by David Reid

author: dreid
Branch: branches/dav-acl-1608-5

(In [22681]) Branching to 'dav-acl-1608-5'

comment:30 Changed 10 years ago by David Reid

TODO notes:

these 1 objects' docstrings are not proper epytext:

twisted.web2.dav.resource.DAVResource.mergeAccessControlList

lots of pyflakes issues lots of undocumented APIs davxml does weird things with all

all the review problems above.

comment:31 Changed 10 years ago by David Reid

Branch: branches/dav-acl-1608-5branches/dav-acl-1608-6

(In [22824]) Branching to 'dav-acl-1608-6'

comment:32 Changed 9 years ago by Jean-Paul Calderone

Branch: branches/dav-acl-1608-6

All the code from this branch moved into the #3801 branch.

comment:33 Changed 9 years ago by Jean-Paul Calderone

Oops. I meant #3081.

comment:34 in reply to:  32 Changed 8 years ago by Thijs Triemstra

Cc: Thijs Triemstra added

Replying to exarkun:

All the code from this branch moved into the #3801 branch.

So this can be closed?

comment:35 Changed 8 years ago by Wilfredo Sánchez Vega

Resolution: duplicate
Status: newclosed

Um, OK

comment:36 Changed 7 years ago by <automation>

Owner: Wilfredo Sánchez Vega deleted
Note: See TracTickets for help on using tickets.