Bugzilla – Full Text Bug Listing
| Summary: | [Security] Changes to the security realm should be verified before being applied | ||
|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Pierre Gaufillet <pierre.gaufillet> |
| Component: | cdo.core | Assignee: | Christian Damus <give.a.damus> |
| Status: | CLOSED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P2 | CC: | stepper |
| Version: | 4.3 | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Mac OS X | ||
| Whiteboard: | |||
| Bug Depends on: | |||
| Bug Blocks: | 415375 | ||
|
Description
Pierre Gaufillet
Good idea! OCL is likely overkill here because it would open a separate ServerCDOView ;-) Moving all outstanding enhancements to 4.3 I can take a look at implementing some of the more obvious consistency checks. I shall start by proposing some security realm validation rules in this bugzilla for general review. As promised, here is a proposed list of security-realm constraints that I think provide minimal viability. Note that OCL is used to specify these constraints for clarity; this is not to suggest that they be implemented as OCL.
context SecurityElement
-- an additional operation that obtains the containing CDOResource
def: cdoResource() : CDOResource =
self.eResource().oclAsType(CDOResource)
context Permission
-- an additional operation that abstracts the isApplicable(CDORevision, CDORevisionProvider, CDOBranchPoint) Java API
def: isApplicableTo(object : CDOObject) : Boolean =
...
context Assignee
-- an additional operation that queries whether an assignee has access to some object
def: hasPermission(access : Access, object : CDOObject) : Boolean =
allPermissions->exists(p|p.access >= access and p.isApplicableTo(object))
context Realm
-- ensure that *somebody* has administrative access to the security model,
-- that it is not possible to freeze out the administrator
inv 'has_administrator': allUsers->exists(hasPermission(Access::WRITE, self.cdoResource()))
-- all security items should be organized into directories
inv 'directory_organization': self.items->forAll(i|i.oclIsKindOf(Directory))
-- there can be at most one security realm in the repository
inv 'singleton': Realm.allInstances() = Set{self}
context Group
-- avoid group inheritance cycles
inv 'acyclic_inheritance': not inheritedGroups->closure(inheritedGroups)->includes(self)
context Directory
-- an additional property to access sub-directories
def: subdirectories : Set(Directory) = items->select(oclIsKindOf(Directory)).oclAsType(Directory)->asSet()
-- directories may not contain themselves
inv 'containment': subdirectories->closure(subdirectories)->excludes(self)
(In reply to Christian W. Damus from comment #4) > As promised, here is a proposed list of security-realm constraints that I > think provide minimal viability. Good work! I think it's a complete list. > Note that OCL is used to specify these > constraints for clarity; this is not to suggest that they be implemented as > OCL. The invariants below are probably rather hard to implement efficiently. Should we start to add possible trigger conditions, assuming that the starting point is SecurityManager.WriteAccessHandler.handleTransactionBeforeCommitting()? > context SecurityElement > -- an additional operation that obtains the containing CDOResource > def: cdoResource() : CDOResource = > self.eResource().oclAsType(CDOResource) > > context Permission > -- an additional operation that abstracts the isApplicable(CDORevision, > CDORevisionProvider, CDOBranchPoint) Java API > def: isApplicableTo(object : CDOObject) : Boolean = > ... > > context Assignee > -- an additional operation that queries whether an assignee has access to > some object > def: hasPermission(access : Access, object : CDOObject) : Boolean = > allPermissions->exists(p|p.access >= access and p.isApplicableTo(object)) > > context Realm > -- ensure that *somebody* has administrative access to the security model, > -- that it is not possible to freeze out the administrator > inv 'has_administrator': allUsers->exists(hasPermission(Access::WRITE, > self.cdoResource())) Probably the single most important constraint! > -- all security items should be organized into directories > inv 'directory_organization': self.items->forAll(i|i.oclIsKindOf(Directory)) This is against the structure of the model which allows for more flexibility. What is the reationale behind this constraint? > -- there can be at most one security realm in the repository > inv 'singleton': Realm.allInstances() = Set{self} I see no real value in this constraint. I could imagine that a hypothetical admin wants to switch between realms at different occasions. > context Group > -- avoid group inheritance cycles > inv 'acyclic_inheritance': not > inheritedGroups->closure(inheritedGroups)->includes(self) Good point! > context Directory > -- an additional property to access sub-directories > def: subdirectories : Set(Directory) = > items->select(oclIsKindOf(Directory)).oclAsType(Directory)->asSet() > -- directories may not contain themselves > inv 'containment': subdirectories->closure(subdirectories)->excludes(self) Ecore does already prevent containment cycles by definition and CDO does extra locking to prevent containment cycles that could otherwise be introduced through certain concurrent commit scenarios. (In reply to Eike Stepper from comment #5) > > Note that OCL is used to specify these > > constraints for clarity; this is not to suggest that they be implemented as > > OCL. > > The invariants below are probably rather hard to implement efficiently. > Should we start to add possible trigger conditions, assuming that the > starting point is > SecurityManager.WriteAccessHandler.handleTransactionBeforeCommitting()? I currently have two of the constraints prototyped, which I think are probably the only two really worth implementing in the back-end, exactly in the write-access handler. The validation is only performed on commits that have IMPACT_REALM security impact, so that, at least, short circuits all but administrative activity from incurring any additional cost. As for the cost of validating the security realm, I think the safety is worth the cost and it only actually has any impact on the administrator's transactions editing the security realm. So, I wouldn't worry about it. > > > > context Realm > > -- ensure that *somebody* has administrative access to the security model, > > -- that it is not possible to freeze out the administrator > > inv 'has_administrator': allUsers->exists(hasPermission(Access::WRITE, > > self.cdoResource())) > > Probably the single most important constraint! Yep. Prototype is in the works. > > -- all security items should be organized into directories > > inv 'directory_organization': self.items->forAll(i|i.oclIsKindOf(Directory)) > > This is against the structure of the model which allows for more > flexibility. What is the reationale behind this constraint? Just that the realm is deliberately structured from the outset with Groups, Users, and Roles directories, and these are even distinguished in the realm by dedicated references (which in a UML model would be subsets of the items reference). This suggests that the meaning of putting any model elements directly in the realm is obscure, and it feels like putting files in the root ("/") folder of a UNIX filesystem. It might be a warning, or we could just skip it because, indeed, it contradicts the model. I'm happy with that. > > -- there can be at most one security realm in the repository > > inv 'singleton': Realm.allInstances() = Set{self} > > I see no real value in this constraint. I could imagine that a hypothetical > admin wants to switch between realms at different occasions. Hmm, yes, I suppose so. My thinking was to avoid ambiguity, but the repository XML configuration definitely makes it explicit which realm is in effect and that there is only one. We can drop this one. BTW, currently, switching the realm requires taking the repository down. Switching on the fly could be an interesting enhancement. > > context Group > > -- avoid group inheritance cycles > > inv 'acyclic_inheritance': not > > inheritedGroups->closure(inheritedGroups)->includes(self) > > Good point! This is the other constraint that I have currently prototyped. > > context Directory > > -- an additional property to access sub-directories > > def: subdirectories : Set(Directory) = > > items->select(oclIsKindOf(Directory)).oclAsType(Directory)->asSet() > > -- directories may not contain themselves > > inv 'containment': subdirectories->closure(subdirectories)->excludes(self) > > Ecore does already prevent containment cycles by definition and CDO does > extra locking to prevent containment cycles that could otherwise be > introduced through certain concurrent commit scenarios. Yeah, that occurred to me some time after I wrote this, too, as soon as I looked in the model diagram and saw the black diamond. ;-) > The validation is only performed on commits that have IMPACT_REALM security impact Oh, I forgot about that nifty flag. Probably because it was kind of an octopus when I implemented it all over the stack :P > As for the cost of validating the security realm, I think the safety is > worth the cost Absolutely. > > > > > > > context Realm > > > -- ensure that *somebody* has administrative access to the security model, > > > -- that it is not possible to freeze out the administrator > > > inv 'has_administrator': allUsers->exists(hasPermission(Access::WRITE, > > > self.cdoResource())) > > > > Probably the single most important constraint! > > Yep. Prototype is in the works. > > > > -- all security items should be organized into directories > > > inv 'directory_organization': self.items->forAll(i|i.oclIsKindOf(Directory)) I'm okay with skipping it for now. > BTW, currently, switching the realm requires taking the repository down. > Switching on the fly could be an interesting enhancement. I think that's acceptable for now. I'd rather invest into a nice maintenance mode, which could be used for a number of higher-level mechasnims such as model evolution induced data migration, etc... I have pushed a new change to Gerrit for review: https://git.eclipse.org/r/18106 This implements the two critical security-realm integrity constraints identified in the preceding discussion (some user can administer the realm and group inheritance is acyclic) as EMF Validation constraints in a new SecurityValidator (in the usual EMF way). The SecurityManager's repository write-handler is updated to validate changed and new security model elements (and always the realm, itself) when detecting that a transaction is committing that has IMPACT_REALM security impact. On any error from validation, the transaction is rolled back and the client gets an exception. The communication of this rollback uses a new rollback reason code in the CDO protocol, which is converted to a new kind of CommitException that indicates server-side semantic validation failure. This rollback condition is detected on the server side by a new kind of run-time exception that a repository write-handler can throw. On the UI side of things, the simplified security form editor already handles CommitExceptions by showing the reason in a nice error dialog. The CDOEditor is updated to handle the new ValidationException kind of CommitException by showing the error message to the user (on the assumption that it would have a user-friendly, helpful message). Because the security model now has an EValidator, it needs to produce localized messages in Diagnostics. To that end, it needs a ResourceLocator. I have implemented the ResourceLocator interface in the OM.Activator. I would have like to be able to do this in some reusable class such as the OSGiActivator from the Net4j utilities, but Net4j is independent of EMF, so that wouldn't work. I don't see a better place to add a new kind of OM activator implementation that is an EMF ResourceLocator ... The changes (with some revision; thanks, Eike!) described in comment #8 are merged to git master. commit 344d002745d0f9e24c4bae1ab1b57a3a90d7aa3d The constraints implemented here are those that are critical for structural integrity of the security model (and which are not already implemented by EMF) and for maintainability of the repository. More complex checks such as contradictory access rules in a role may be considered for implementation in the client side but probably not the server side. In any case, these would be a new enhancement. Closing. Closing. |