Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cluster ACLs based on the cluster config's file permissions #152

Merged
merged 6 commits into from Sep 20, 2019

Conversation

MorganRodgers
Copy link
Contributor

Fixes crash when a YAML file in the clusters.d is not readable by the user

Fixes crash when a YAML file in the clusters.d is not readable by the user
@MorganRodgers MorganRodgers changed the title [WIP] Add cluster ACLs based on the cluster configs file permissions [WIP] Add cluster ACLs based on the cluster config's file permissions Sep 20, 2019
@johrstrom
Copy link
Contributor

I know Erik has thoughts on whether we should rescue errors in this library or bubble them up to higher layers. I could see it both ways, but I think if we bubble it up it's easier to show the user what happened. And I feel like that's what we need to fix, communicating the failure to the user, and the potential for failure to the developer. Same with #150 actually.

That said, if we do rescue errors here, we need to log or indicate somehow that they occurred. Otherwise we leave people scratching their heads because nothing happened and also nothing was said to have failed when clearly something, somewhere did.

@MorganRodgers
Copy link
Contributor Author

In general I take the point about libraries bubbling errors up. I this case, limiting access to clusters using file system permissions is a feature admins might reasonably expect to just work, and so supporting that behavior outside of the library means that all the apps would need to handle the error which seems unnecessary. This issue has come up for me before where I thought that we controlled access to clusters using file system permissions or ACLs. The surprising behavior was when not only did that not work, but it crashed the Dashboard before Passenger had spun up enough to give me a nice error page:

500 Internal Server Error
If you are the administrator of this website, then please read this web application's log file and/or the web server's log file to find out what went wrong.

We could log Errno::EACCES if that is desired.

@johrstrom
Copy link
Contributor

Oh i see, so it's not really failing, it's actually expected behavior. OK cool. As to logging, I think that's a precedent we're probably not able to set quite yet especially in this case.

@ericfranz
Copy link
Contributor

If a cluster config is not readable by the user it is okay to ignore the file without raising an exception or logging an error, since this is a feature - a normal case. We do not log errors to the Dashboard's log file every time a user accesses the dashboard just because an app happens to have restricted permissions on it. In other words, it wouldn't be an exceptional case - though I imagine it has happened before. There are other strategies we can take to help admins verify they intended to restrict the permissions on a cluster config file.

But using exception handling to avoid the crash is the wrong approach. Instead, check if the file is readable by the user. For example:

  if config.file? && config.readable?

# ...

  elsif config.directory?
    Pathname.glob(config.join("*.yml")).select(&:file?).select(&:readable?).each do |p|

FWIW The original method is difficult to read and should be refactored. For example, there is no reason to ensure that the block passed to any? properly returns false if the clusters array is empty because we do nothing with that return value. Instead of building an array of clusters with each, we could probably do a map. We do not need the v1 config anymore either, though removal of v1 is technically a breaking change in ood_core.

Morgan Rodgers added 2 commits September 20, 2019 12:03
Note that this removes the default handler which was throwing on existing unreadable files, but Pathname's checks (`#directory?` and `#file?`) both work on symlinks so this appears safe.
@MorganRodgers MorganRodgers changed the title [WIP] Add cluster ACLs based on the cluster config's file permissions Add cluster ACLs based on the cluster config's file permissions Sep 20, 2019
@MorganRodgers MorganRodgers merged commit d1de1a5 into master Sep 20, 2019
@MorganRodgers MorganRodgers deleted the cluster-acl-with-permissions branch September 20, 2019 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants