Adding support for SE-Linux security
in [PgSql]
|
From: Stephen Frost on 11 Dec 2009 17:18 * Robert Haas (robertmhaas(a)gmail.com) wrote: > If I don't tell > you how to write the patch, you can't accuse me of moving the > goalposts (of course I've now discovered the pitfalls of that approach > as well...). Indeed, we also yell and scream when we don't know which direction the goalposts are in. ;) > > That thought had crossed my mind as well, but I wasn't convinced that > > would actually be seen as a signifigantly different API to just having > > the arguments passed inline... > > If you'll forgive me for saying so, this is exactly the sort of > thinking that I think is killing us. Who cares how it will be seen? erp. mea culpa. I meant that I didn't think of it as being enough of a novelty for you to be suggesting-it-but-not-telling-us.. It just kind of came off as too-obvious, however, the reality is that I didn't understand your suggestion, see below. > > Then again, using structures does > > allow you to add to them without having to modify the function > > definitions, > > Personal opinion time, but I think that "without having to modify the > function definitions" is a CRITICAL design component. Furthermore, > "adding to them [the structures]" shouldn't be something that only > happens when we decide to support a new security model, or we're > little better off than if we just passed a kajillion arguments. > Again, I don't want to overstate my confidence in this point, but it > feels to me like we need to pass PRE-EXISTING data structures that are > already being used for other things and happen to be the right stuff > to enable access control decisions, and to which fields that are > likely to be needed are likely to get added anyway. Ah, now that makes alot of sense.. Unfortunately, having been involved in at least some of this code in the past, sadly I don't think we have such pre-existing data structures for the information that's primairly at issue here. Specifically, the data structures we tend to have pre-existing are the ones that come from the catalog and would fall out from an OID+SubOID based API. Sure, we could pass those structures in instead of using SysCache to pull the data out, but that's not really how we 'typically' do things in the code base from my experience, and I don't really see it having alot of advantage. Using OID+SubOID and SysCache *will* pick up new things as they're added to the catalogs- for the information *in* the catalogs. There are very few data structures outside of the parse tree which include the information from the parse tree.. And to be honest, the ones that *are* out there don't typically have the world's best structure for use by this kind of a framework (thinking back to specifically what's passed around for column-level privs..). > But it's > difficult to know whether this approach can be made to work without > trying it, and there are bound to be problem cases that need to be > thought about, and that thinking will be more likely to lead to a good > result if it happens in the community, rather than by KaiGai or any > other single person in isolation. I agree with this- one issue is, unfortunately, an overabundance from KaiGai of "code-writing man-power". This is an odd situation for this community, in general, so we're having a hard time coming to grasp with it. KaiGai, can you hold off on implementing any of these approaches until we can hammer down something a bit better for you to work from as a baseline? I'll start with Robert's suggestion of a single-object example case and throw out some example code for people to shoot down of different approachs. After a few iterations of that, with comments from all (KaiGai, you're welcome to comment on it too, of course), we'll turn you loose on it again to implement fully (if you're still willing to). Following along that though, as we'd like this to be the design forum, when you come across problems or issues implementing it, could you come back to this forum and explain the issue and why it doesn't fit, as soon as you hit it? What you tend to do is disappear for a while, then come back, instead, with a full patch which works, but has places where you've gone down an unexpected path because you found a case which wasn't covered, designed a solution for it which "kinda" fits and then implemented it immediately. I feel that's something I've encouraged you to do, unfortunately, and my apologies for that. I'm trying to get time from my employer (and my wife) to dedicate to this, to hopefully avoid that happening in the future. > I'm going to vote fairly strongly against inserting function pointers > at the outset. I think that we should look at the first phase of this > project as an attempt to restructure the code so that the access > control decisions are isolated from the rest of the code. *If* we can > do that successfully, I think it will represent good progress all on > its own. Inserting function pointers is something that can be done > later if it turns out to be useful with a very small, self-contained > patch. Sure, works for me. > Part of my frustration here is that I don't see a lot of evidence that > anyone is willing to put really tangible resources into this other > than KaiGai and his employer. I feel like there is momentum here to resolve this. I'm certainly trying to. We're getting more responsive input from the security community too. As we work through and point out the commercial issues this could help solve, I'm hopeful we'll be able to improve the situation further. Josh's discussion about why this isn't some oft-requested feature is a good one, in my view. > Having it stick around as a zombie is the worst possible outcome. I agree, though I have to admit that I'm torn because I really feel this would be a great additional capability for PG. Thanks, Stephen
From: Stephen Frost on 11 Dec 2009 17:36 * Robert Haas (robertmhaas(a)gmail.com) wrote: > On Fri, Dec 11, 2009 at 4:26 PM, Stephen Frost <sfrost(a)snowman.net> wrote: > > Does that help clarify my example case? > > That case doesn't seem terribly problematic to me. It seems clear > that we'll want to pass some information about both x and y. What is > less clear is exactly what the argument types will be, and the right > answer probably depends somewhat on the structure of the existing > code, which I have not looked at. Allow me to assist- y is never in a structure once you're out of the parser: ATExecChangeOwner(Oid relationOid, Oid newOwnerId, bool recursing) I expect you'll find this is more the rule than the exception to alot of the existing PG security model, because much of it's responsibility is in what I'll call the DDL (under commands/) aspects. The DML pieces (under the executor) are a bit better about this, specifically you could pass in a RangeTblEntry, exactly how ExecCheckRTEPerms handles it. Actually, in a fit of barely-contained mirth, it strikes me that PG really has already done what you're suggesting for the 'hard' part- and the RangeTblEntry plus ExecCheckRTEPerms is exactly it. It's all the *other* checks we do for the PG security model, under commands/, that are the problem here. The parts of the code that, to be honest, I think all us database geeks have historically cared alot less about. > What I'm more concerned about is if > the access control decision in this case were based on u for PG DAC, v > for SE-PostgreSQL, and w for Robert Haas's Personal Defensive System. > If that's the case, and our function signature looks like (x,y,u,v,w), > the we should worry. Right, I understand that. What I offer in reply is that we focus our attention on using the OID+SubOID approach, as I'm suggesting, to the fullest extent possible through that mechanism, and appreciate that the other arguments passed to the function are derived specifically from the parser and therefore unlikely to be changed until and unless we change the base syntax of the command and calling function, at which time we may have to add arguments to the function signature. This would continue at least until we get to the point where we decide that the caller needs to be changed because it's got a huge function sig, and move it to something like the structure of the executor and the equivilant of ExecCheckRTEPerms() would get updated along with it, at that time. Thanks, Stephen
From: Greg Smith on 11 Dec 2009 18:20 Stephen Frost wrote: > I agree with this- one issue is, unfortunately, an overabundance from > KaiGai of "code-writing man-power". This is an odd situation for this > community, in general, so we're having a hard time coming to grasp with > it. There are plenty of parallels to when Zdenek was writing a ton of in-place upgrade code faster than anyone else was fully consuming it. The "do it right or don't do it at all" approach of the PG community seems particularly hard to reconcile with larger patches from people we don't get enough face time with. It's easy to get deadlocked and not have a good way to navigate out when faced with a set of difficult decisions and only electronic communications between participants. Shoot, you and Robert have spent time doing technical arguments in person and we still got a little rough patch on-list this week out of the debate. I hate to even use this terminology, but these big patches seem to need a project manager advocate sometimes: someone who knows everyone well enough to clear these stalled spots, smooth over any personality conflicts, and is motivated to intervene because they need the feature. I see it as a sort of scaling problem that might be a recurring one. It would be nice if we could all get better at identifying when it does happen, and perhaps find someone to help with planning before we waste so much time chasing code that isn't going to be accepted yet again. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg(a)2ndQuadrant.com www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Greg Smith on 11 Dec 2009 19:15 I just did a round of integrating some of the big-picture feedback that has shown up here since the meeting into http://wiki.postgresql.org/wiki/SEPostgreSQL_Review_at_the_BWPUG , mainly supplementing the references in the "Works outside of SELinux" section with the new suggested reading here suggested by Stephen Smalley and Joshua Brindle. I'm trying to keep that a fairly readable intro to the controversial parts rather than going deeply technical. What I'm not going to try to track is all the low-level implementation details that are bouncing around right now, my brain is too full this week to cram more about OID trivia into it right now. That would be a good idea for someone to summarize eventually and then throw that onto the wiki somewhere else, so that it's easier to remember the context of what/why decisions were made. The way Simon has been keeping an ongoing log at http://wiki.postgresql.org/wiki/Hot_Standby shows a reasonable way to organize such a thing from a similarly complicated patch. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg(a)2ndQuadrant.com www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Robert Haas on 11 Dec 2009 19:27
On Fri, Dec 11, 2009 at 5:36 PM, Stephen Frost <sfrost(a)snowman.net> wrote: > * Robert Haas (robertmhaas(a)gmail.com) wrote: >> On Fri, Dec 11, 2009 at 4:26 PM, Stephen Frost <sfrost(a)snowman.net> wrote: >> > Does that help clarify my example case? >> >> That case doesn't seem terribly problematic to me. It seems clear >> that we'll want to pass some information about both x and y. What is >> less clear is exactly what the argument types will be, and the right >> answer probably depends somewhat on the structure of the existing >> code, which I have not looked at. > > Allow me to assist- y is never in a structure once you're out of the > parser: Well this is why you're writing the patch and not me. :-) > ATExecChangeOwner(Oid relationOid, Oid newOwnerId, bool recursing) > > I expect you'll find this is more the rule than the exception to alot of > the existing PG security model, because much of it's responsibility is > in what I'll call the DDL (under commands/) aspects. The DML pieces > (under the executor) are a bit better about this, specifically you could > pass in a RangeTblEntry, exactly how ExecCheckRTEPerms handles it. > > Actually, in a fit of barely-contained mirth, it strikes me that PG > really has already done what you're suggesting for the 'hard' part- and > the RangeTblEntry plus ExecCheckRTEPerms is exactly it. It's all the > *other* checks we do for the PG security model, under commands/, that > are the problem here. The parts of the code that, to be honest, I think > all us database geeks have historically cared alot less about. Hmm, interesting. >> What I'm more concerned about is if >> the access control decision in this case were based on u for PG DAC, v >> for SE-PostgreSQL, and w for Robert Haas's Personal Defensive System. >> If that's the case, and our function signature looks like (x,y,u,v,w), >> the we should worry. > > Right, I understand that. What I offer in reply is that we focus our > attention on using the OID+SubOID approach, as I'm suggesting, to the > fullest extent possible through that mechanism, and appreciate that the > other arguments passed to the function are derived specifically from the > parser and therefore unlikely to be changed until and unless we change > the base syntax of the command and calling function, at which time we > may have to add arguments to the function signature. This would > continue at least until we get to the point where we decide that the > caller needs to be changed because it's got a huge function sig, and > move it to something like the structure of the executor and the > equivilant of ExecCheckRTEPerms() would get updated along with it, at > that time. What exactly do you mean by a SubOID? I'm not really following that part. ....Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |