You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2014/08/08 19:38:49 UTC
[GitHub] incubator-brooklyn pull request: brooklyn.properties permissions c...
GitHub user aledsage opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/115
brooklyn.properties permissions check
This change will cause brooklyn to fail to start if brooklyn.properties do not have permission x00 (e.g. 400, 600 or 700).
e.g.:
brooklyn launch --localBrooklynProperties localBrooklyn.properties
_ _ _
| |__ _ __ ___ ___ | | _| |_ _ _ __ (R)
| '_ \| '__/ _ \ / _ \| |/ / | | | | '_ \
| |_) | | | (_) | (_) | <| | |_| | | | |
|_.__/|_| \___/ \___/|_|\_\_|\__, |_| |_|
|___/ 0.7.0-SNAPSHOT
2014-08-08 18:31:48,829 ERROR Startup error: Invalid permissions for file localBrooklyn.properties; expected 600 but was -rw-r--r--
Startup error: Invalid permissions for file localBrooklyn.properties; expected 600 but was -rw-r--r--
Note the "Adds FileUtil" commit is a duplicate of code in PR #114. Suggest we merge that one first (depending which one folk get around to reviewing first).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/incubator-brooklyn fix/brooklynProperties-permissions-check
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/115.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #115
----
commit e04d5d019f8675c83c7cbe7f4c04d6daf5de57f8
Author: Aled Sage <al...@gmail.com>
Date: 2014-08-08T16:59:47Z
Adds FileUtil (see PR #114)
commit 0d235c050ab45b5bdef153f9fec1bec60fde7ae9
Author: Aled Sage <al...@gmail.com>
Date: 2014-08-08T17:25:46Z
BROOKLYN-15: enforce brooklyn.properties permissions
- brooklyn.properties must be something like 600; otherwise
brooklyn will fail to start
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: brooklyn.properties permissions c...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/115#issuecomment-52169399
Agree it's not worth addressing minor niggles that will become moot when we switch to Java 7! Merge away :o)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: brooklyn.properties permissions c...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/115#issuecomment-52060049
@aledsage The changes work as expected on Windows.
To actually check the ACLs on Windows we could use command line tools, analogous to the current approach for Linux (cacls.exe). Once we transition to Java 7 we could use the built-in ACL functionality.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: brooklyn.properties permissions c...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/115#discussion_r16181078
--- Diff: usage/launcher/src/main/java/brooklyn/launcher/BrooklynLauncher.java ---
@@ -485,6 +498,37 @@ public BrooklynLauncher start() {
return this;
}
+ private void checkFileReadable(String file) {
+ File f = new File(Os.tidyPath(file));
+ if (!f.exists()) {
+ throw new FatalRuntimeException("File "+file+" does not exist");
+ }
+ if (!f.isFile()) {
+ throw new FatalRuntimeException(file+" is not a file");
+ }
+ if (!f.canRead()) {
+ throw new FatalRuntimeException(file+" is not readable");
+ }
+ }
+
+ private void checkFilePermissionsX00(String file) {
+ File f = new File(Os.tidyPath(file));
+ if (f.exists() && f.isFile() && f.canRead()) {
+ try {
+ Maybe<String> permission = FileUtil.getFilePermissions(f);
+ if (permission.isAbsent()) {
+ LOG.debug("Could not determine permissions of global brooklyn properties file; assuming ok: "+f);
--- End diff --
Not necessarily the global properties file. Strictly speaking, not necessarily a properties file at all (based on the name of the method) but that's being picky.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: brooklyn.properties permissions c...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/115#discussion_r16181473
--- Diff: usage/launcher/src/main/java/brooklyn/launcher/BrooklynLauncher.java ---
@@ -485,6 +498,37 @@ public BrooklynLauncher start() {
return this;
}
+ private void checkFileReadable(String file) {
+ File f = new File(Os.tidyPath(file));
+ if (!f.exists()) {
+ throw new FatalRuntimeException("File "+file+" does not exist");
+ }
+ if (!f.isFile()) {
+ throw new FatalRuntimeException(file+" is not a file");
+ }
+ if (!f.canRead()) {
+ throw new FatalRuntimeException(file+" is not readable");
+ }
+ }
+
+ private void checkFilePermissionsX00(String file) {
+ File f = new File(Os.tidyPath(file));
+ if (f.exists() && f.isFile() && f.canRead()) {
+ try {
+ Maybe<String> permission = FileUtil.getFilePermissions(f);
+ if (permission.isAbsent()) {
+ LOG.debug("Could not determine permissions of global brooklyn properties file; assuming ok: "+f);
+ } else {
+ if (!permission.get().substring(4).equals("------")) {
+ throw new FatalRuntimeException("Invalid permissions for file "+file+"; expected 600 but was "+permission.get());
+ }
+ }
+ } catch (FileNotFoundException e) {
+ throw Exceptions.propagate(e); // should not happen; did f.exists() above
+ }
+ }
+ }
+
--- End diff --
`checkFileReadable()` test also performed by calling method, so arguable not required (although I think this should be a generic utility method).
"Check-then-act" is a race condition; suggest handling both cases identically (ideally without duplicating code).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: brooklyn.properties permissions c...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/115#discussion_r16231122
--- Diff: usage/launcher/src/main/java/brooklyn/launcher/BrooklynLauncher.java ---
@@ -485,6 +498,37 @@ public BrooklynLauncher start() {
return this;
}
+ private void checkFileReadable(String file) {
+ File f = new File(Os.tidyPath(file));
+ if (!f.exists()) {
+ throw new FatalRuntimeException("File "+file+" does not exist");
+ }
+ if (!f.isFile()) {
+ throw new FatalRuntimeException(file+" is not a file");
+ }
+ if (!f.canRead()) {
+ throw new FatalRuntimeException(file+" is not readable");
+ }
+ }
+
+ private void checkFilePermissionsX00(String file) {
+ File f = new File(Os.tidyPath(file));
+ if (f.exists() && f.isFile() && f.canRead()) {
+ try {
+ Maybe<String> permission = FileUtil.getFilePermissions(f);
+ if (permission.isAbsent()) {
+ LOG.debug("Could not determine permissions of global brooklyn properties file; assuming ok: "+f);
+ } else {
+ if (!permission.get().substring(4).equals("------")) {
+ throw new FatalRuntimeException("Invalid permissions for file "+file+"; expected 600 but was "+permission.get());
+ }
+ }
+ } catch (FileNotFoundException e) {
+ throw Exceptions.propagate(e); // should not happen; did f.exists() above
+ }
+ }
+ }
+
--- End diff --
Tricky to cleanly always do the check before acting, without duplicating the checks (even if the code is reused by extracting to a shared method). I'm inclined to rely on `builder.globalPropertiesFile` not breaking if you give it a file that does not exist. It won't give a particularly nice warning (it might even ignore silently!). If the file is deleted between our first check and the builder doing its stuff then so be it.
Removing duplicate checks from `checkFilePermissionsX00`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: brooklyn.properties permissions c...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/115#discussion_r16230862
--- Diff: usage/launcher/src/main/java/brooklyn/launcher/BrooklynLauncher.java ---
@@ -485,6 +498,37 @@ public BrooklynLauncher start() {
return this;
}
+ private void checkFileReadable(String file) {
+ File f = new File(Os.tidyPath(file));
+ if (!f.exists()) {
+ throw new FatalRuntimeException("File "+file+" does not exist");
+ }
+ if (!f.isFile()) {
+ throw new FatalRuntimeException(file+" is not a file");
+ }
+ if (!f.canRead()) {
+ throw new FatalRuntimeException(file+" is not readable");
+ }
+ }
+
+ private void checkFilePermissionsX00(String file) {
+ File f = new File(Os.tidyPath(file));
+ if (f.exists() && f.isFile() && f.canRead()) {
+ try {
+ Maybe<String> permission = FileUtil.getFilePermissions(f);
+ if (permission.isAbsent()) {
+ LOG.debug("Could not determine permissions of global brooklyn properties file; assuming ok: "+f);
+ } else {
+ if (!permission.get().substring(4).equals("------")) {
+ throw new FatalRuntimeException("Invalid permissions for file "+file+"; expected 600 but was "+permission.get());
--- End diff --
Yeah, at one point I started writing a converter from `rwxr--r--` format to numeric, and then abandoned it as didn't seem worth it. Not ideal having mix but i think we can live with it for now? When we switch to Java 7, we'll completely change the `getFilePermissions` stuff.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: brooklyn.properties permissions c...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/115#discussion_r16231372
--- Diff: usage/launcher/src/test/java/brooklyn/launcher/BrooklynLauncherTest.java ---
@@ -219,6 +221,7 @@ public void testReloadBrooklynPropertiesRestoresProgrammaticProperties() throws
@Test
public void testReloadBrooklynPropertiesFromFile() throws Exception {
File globalPropertiesFile = File.createTempFile("local-brooklyn-properties-test", ".properties");
+ FileUtil.setFilePermissionsTo600(globalPropertiesFile);
--- End diff --
`FileUtil` (and thus these methods) was added in PR #114
Problem is its implementation is based on `File.setReadable(boolean readable, boolean ownerOnly)`, so it would need to interpret the hex and make appropriate set of calls. And then we'd throw away the code when we can use Java 7. Let's refactor when we have more than a requirement for `setFilePermissionsTo600` and `setFilePermissionsTo700` - they are both marked `@Beta`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: brooklyn.properties permissions c...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/115#issuecomment-52060275
Now that PR #114 is merged, I've rebased this to remove the commit from here about FileUtil.
@alasdairhodge could you review please?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: brooklyn.properties permissions c...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/115#discussion_r16180904
--- Diff: usage/launcher/src/main/java/brooklyn/launcher/BrooklynLauncher.java ---
@@ -440,8 +444,17 @@ public BrooklynLauncher start() {
if (managementContext == null) {
if (brooklynProperties == null) {
Builder builder = new BrooklynProperties.Factory.Builder();
- if (globalBrooklynPropertiesFile != null) builder.globalPropertiesFile(globalBrooklynPropertiesFile);
- if (localBrooklynPropertiesFile != null) builder.localPropertiesFile(localBrooklynPropertiesFile);
+ if (globalBrooklynPropertiesFile != null) {
+ // brooklyn.properties stores passwords (web-console and cloud credentials),
+ // so ensure it has sensible permissions
+ checkFilePermissionsX00(globalBrooklynPropertiesFile);
+ builder.globalPropertiesFile(globalBrooklynPropertiesFile);
+ }
+ if (localBrooklynPropertiesFile != null) {
+ checkFileReadable(localBrooklynPropertiesFile);
+ checkFilePermissionsX00(localBrooklynPropertiesFile);
+ builder.localPropertiesFile(localBrooklynPropertiesFile);
+ }
--- End diff --
No `checkFileReadable(globalBrooklynProperties)`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: brooklyn.properties permissions c...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/115#issuecomment-52063508
Minor niggles, but good enhancement to have.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: brooklyn.properties permissions c...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/115#issuecomment-52169366
Received message from @alasdairhodge saying he's happy for this to be merged; so merging now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: brooklyn.properties permissions c...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/115#discussion_r16181802
--- Diff: usage/launcher/src/test/java/brooklyn/launcher/BrooklynLauncherTest.java ---
@@ -219,6 +221,7 @@ public void testReloadBrooklynPropertiesRestoresProgrammaticProperties() throws
@Test
public void testReloadBrooklynPropertiesFromFile() throws Exception {
File globalPropertiesFile = File.createTempFile("local-brooklyn-properties-test", ".properties");
+ FileUtil.setFilePermissionsTo600(globalPropertiesFile);
--- End diff --
No `FileUtil` diffs in this commit? Surely that method didn't already exist?!
Suggest it's cleaner to have `setFilePermissions(File, int)`, and to use hex instead of decimal when invoking.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: brooklyn.properties permissions c...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/115#discussion_r16181267
--- Diff: usage/launcher/src/main/java/brooklyn/launcher/BrooklynLauncher.java ---
@@ -485,6 +498,37 @@ public BrooklynLauncher start() {
return this;
}
+ private void checkFileReadable(String file) {
+ File f = new File(Os.tidyPath(file));
+ if (!f.exists()) {
+ throw new FatalRuntimeException("File "+file+" does not exist");
+ }
+ if (!f.isFile()) {
+ throw new FatalRuntimeException(file+" is not a file");
+ }
+ if (!f.canRead()) {
+ throw new FatalRuntimeException(file+" is not readable");
+ }
+ }
+
+ private void checkFilePermissionsX00(String file) {
+ File f = new File(Os.tidyPath(file));
+ if (f.exists() && f.isFile() && f.canRead()) {
+ try {
+ Maybe<String> permission = FileUtil.getFilePermissions(f);
+ if (permission.isAbsent()) {
+ LOG.debug("Could not determine permissions of global brooklyn properties file; assuming ok: "+f);
+ } else {
+ if (!permission.get().substring(4).equals("------")) {
+ throw new FatalRuntimeException("Invalid permissions for file "+file+"; expected 600 but was "+permission.get());
--- End diff --
Odd mix of numeric and flag formats; maybe combine both: `rwxr--r-- (744)`
Also, not strictly correct to say "expected 600".
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: brooklyn.properties permissions c...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/115
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: brooklyn.properties permissions c...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/115#issuecomment-52169253
Incorporated review comments (except those I responded to explicitly), and also incorporated @neykov 's comment in PR #114.
@alasdairhodge can you take another look please and see if you're happy for this to be merged?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: brooklyn.properties permissions c...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/115#issuecomment-51634058
@neykov the check will currently be skipped on windows. Could you please review, and confirm that I haven't broken it for Windows?! Also, if there is a good way to check file permissions on windows (see `brooklyn.util.io.FileUtil.getFilePermissions(File)`) then can you let me know.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---