On Thu, Aug 29, 2013 at 9:12 PM, Stephen Frost <sfr...@snowman.net> wrote: > You will not find me argueing to allow that in normal operation, or most > direct-catalog hacking. I'd be much happier if we had all the ALTER, > etc, options necessary to prevent any need to ever touch the catalogs. > Similairly, it'd be nice to have more permission options which reduce > the need for anyone to be superuser.
Sure, I agree. But you can't burden this patch with the job of conforming to what you and I think the project policy ought to be, but isn't. Right now, as things stand today, the superuser is one step short of God Almighty. The only way in which we even attempt to restrict the superuser is to try to keep her from hijacking the postgres login account. But we don't even try to do that very thoroughly, as has recently been pointed out on other threads; there are several plausible attack vectors for her to do just that. This patch fits right into that existing philosophy. If we take this patch, and I think we should, and later change our policy, then the permissions around this may require adjustment to fit the new policy. Fine! But that policy change is properly a separate discussion. >> Now, you can argue that people are more likely to render the database >> nonfunctional by changing GUC settings than they are to do it by >> corrupting the system catalogs, but I'm not sure I believe it. We >> can, after all, validate that any setting a user supplies is a valid >> value for that setting before writing it out to the configuration >> file. It might still make the system fail to start if - for example - >> it causes too many semaphores to be reserved, or something like that. >> But why should we think that such mistakes will be common? If >> anything, it sounds less error-prone to me than hand-editing the >> configuration file, where typing something like on_exit_error=maybe >> will make the server fail to start. We can eliminate that whole >> category of error, at least for people who choose to use the new >> tools. > > That category of error is much more likely to get caught by the sysadmin > doing the restart after the update.. If you edit postgresql.conf manually today, you will have no warning of ANY sort of error you might make. With a feature like this, we can catch a large subset of things you might do wrong *before we even modify the file*. Yes, there will be some that slip through, but an imperfect mechanism for catching mistakes is better than no mechanism at all. > ALTER SYSTEM doesn't provide any > way to restart the DB to make sure it worked. > That, in turn, encourages > modification of the config parameters w/o a restart, which is a pretty > bad idea, imv. "vi" doesn't provide any way to restart the DB to make sure it worked, either, and never will. However, ALTER SYSTEM could be extended in exactly that way: we could have ALTER SYSTEM RESTART. Of course some people are likely to argue that's a bad idea, so we'd have to have that discussion and think carefully about the issues, but there's nothing intrinsic that restricts us from giving ALTER SYSTEM any arbitrary set of capabilities we might want it to have. Even if we don't ever do that, we're no worse off than today. Well, with one exception. If we make it easier to modify the configuration file, then people are more likely to do it, and thus more likely to do it wrong. But you could use that argument against any change that improves ease-of-use. And ease-of-use is a feature, not a bug. >> If you're using the term "dangerous" to refer to a security exposure, >> I concede there is some incremental exposure from allowing this by >> default. But it's not a terribly large additional exposure. It's >> already the case that if adminpack is available the super-user can do >> whatever he or she wants, because she or he can write to arbitrary >> files inside the data directory. > > This is only true if -contrib is installed, which a *lot* of admins > refuse to do, specifically because of such insane modules as this. > Therefore, I would argue that it's not nearly as small a change wrt > exposure as you believe. Strictly speaking, I don't believe it's a huge > security hole or something, but I do think it's going to surprise admins > who aren't following the lists. If we take the patch, it will be in the release notes, just like any other feature. I suspect there will be more people *pleasantly* surprised than anything else. If there's anything I've learned about ease-of-use as a 10+-year veteran of PostgreSQL, it's that being able to do everything via a database connection is really, really, really convenient. That's why foreign data wrappers are awesome. Instead of installing a foreign data wrapper for Oracle and pointing it at an Oracle server and then sucking data down over the link to store or process or whatever, you put all that logic in a client which knows how to talk to both PostgreSQL and Oracle. It turns out, though, that having that capability *in the server* is really valuable to people. It doesn't let you do anything you couldn't do otherwise, but it's extremely convenient. It's also why we keep hearing requests to put a scheduler in core. It's not that you *can't* schedule tasks using cron or Scheduled Tasks on Windows; it's that it's much more convenient to be able to do it all in one place, namely PostgreSQL. It is 100% possible for us to tell all of our users, sorry, you've got to keep using vi to change configuration settings, because we don't trust you to do that via SQL. But I think it's the WRONG decision. Users like being trusted. And it's not that hard for the DBA to disable this or any other feature if he or she feels a real need to do so. > This wouldn't be the first time we've said "do it externally and when > we figure out a better answer then we'll put it into core." No, but I don't think there's anything terribly wrong with *this* answer. It's the product of a lot of people hashing out a lot of issues, and I think it's pretty good. Pushing things out externally is appropriate when the best design is unclear and especially when little or no core support is required, but I think we have a pretty broad consensus on the general contours of how this should work, and some core support is needed. Someone could certainly do something better than adminpack without core integration, but what's being proposed here is better still. >> 1. disabling the feature by default, and providing no way for it to be >> turned on remotely, and > > Yes, that's quite intentional because it would make someone knowledgable > about the *overall system* be aware of this rather than having it sprung > on them when they discover some change has made the database unable to > start. We add new features in every release. I don't see that this one is dangerous enough that it needs to be hedged about with disclaimers we don't attach to any of our other features. My favorite example is the release where we changed the default behavior of TRUNCATE so that it cascades to child tables instead of affecting only the parent. Talk about springing changes on people that could have catastrophic consequences. People have to read the release notes before upgrading to new major versions, or they will get surprised. That's why we have release notes, and that's why we have stable minor versions. I would also just like to note in passing that I believe the shared memory changes in 9.3 eliminate the #1 reason for "my database won't start after a config change", which IME has always been that the change pushed up the shared memory allocation just beyond what the OS would allow. That hazard affected a large number of different configuration parameters, and it doesn't exist any more. You can still bump max_connections or max_autovacuum_workers or the new max_worker_processes high enough to eat up all the system's semaphores, and I'm sure someone has killed their system that way before, but I've personally not run into it. You might also increase the size of shared memory so much that you actually overrun what's available on the box, rather than just overrunning the limit on System V shared memory, but that requires a much larger adjustment to the parameter values and is therefore likely to be the result of abject stupidity rather than simple bad luck. > While I appreciate that the original thrust behind this may be trying to > replace adminpack, I find that module to be utterly horrendous in what > it does and if we don't replicate all that it can do, so much the > better. Consider a similar proposal as ALTER SYSTEM which simply > rewrites pg_hba.conf, ala how adminpack could. Would we punt on the > hard questions around things like how to specify the order in which > entries in pg_hba.conf are maintained? We could have an ALTER HBA today > which takes a line number to insert a string of text into, but is it a > good idea or design? Certainly not. My point here is simply that > focusing on replacing adminpack is not the basis for a feature or > design. I partially agree with that, but the point of mentioning adminpack is that the horse is already halfway out of the barn. I think this feature is pretty good as designed and not half-baked at all, but even if you feel otherwise it's surely a heck of a lot better than what exists today. >> #3 is a bad idea in my >> book - it would break some of my existing benchmarking scripts, which >> do initdb; cat >>$PGDATA/postgresql.conf <<EOM. But even if it were a >> good idea, it isn't a necessary prerequisite for this patch. > > Too often do we consider sloppy handling like this, which is great for a > dev or testing environment, to be appropriate for production systems. :( Well, IMHO, the solution to that is to stop relying on hand-edited files. I've never really liked the fact that postgresql.conf is a text file rather than somehow part of the database guts, requiring a special tool (that can do the appropriate validation) to make changes. But that ship has already sailed and doesn't seem likely to reverse course any time soon. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
