You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Isabel Jimenez <co...@isabeljimenez.com> on 2014/03/12 12:03:28 UTC
Review Request 19093: Display warning for credentials file permissions
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19093/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Vinod Kone.
Bugs: MESOS-1087
https://issues.apache.org/jira/browse/MESOS-1087
Repository: mesos-git
Description
-------
Adding permissions check on credentials file and deleting TODO comment
Diffs
-----
3rdparty/libprocess/3rdparty/stout/Makefile.am 1075b46
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 3f475a4
3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp PRE-CREATION
src/master/master.cpp 2a40333
Diff: https://reviews.apache.org/r/19093/diff/
Testing
-------
Thanks,
Isabel Jimenez
Re: Review Request 19093: Display warning for credentials file permissions
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19093/#review36911
-----------------------------------------------------------
Patch looks great!
Reviews applied: [19093]
All tests passed.
- Mesos ReviewBot
On March 12, 2014, 11:07 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19093/
> -----------------------------------------------------------
>
> (Updated March 12, 2014, 11:07 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Bugs: MESOS-1087
> https://issues.apache.org/jira/browse/MESOS-1087
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adding permissions check on credentials file and deleting TODO comment
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 1075b46
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 3f475a4
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp PRE-CREATION
> src/master/master.cpp 2a40333
>
> Diff: https://reviews.apache.org/r/19093/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 19093: Display warning for credentials file permissions
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On March 14, 2014, 5:46 a.m., Adam B wrote:
> > src/master/master.cpp, lines 299-300
> > <https://reviews.apache.org/r/19093/diff/3/?file=518207#file518207line299>
> >
> > - Please remove the '!' at the end of this warning message. Exclamations points are rarely needed in log messages, especially when the severity is already expressed by the log level. We usually don't even put periods at the end of log messages.
> > - The word 'insufficient' insufficiently explains what's wrong with the permissions on the file. Explain that the credentials file is globally readable/writable, and recommend a new setting (e.g. 'chmod o-rwx' or 'rw-r-----' or '640').
Isabel was just copying my suggestion for a log message (which included the exclamation point) and she had already pointed out that 'insufficient' was insufficient. ;) I think explaining the problem is great but I don't think it's necessary to tell people how to fix it.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19093/#review37158
-----------------------------------------------------------
On March 13, 2014, 8:47 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19093/
> -----------------------------------------------------------
>
> (Updated March 13, 2014, 8:47 a.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone.
>
>
> Bugs: MESOS-1087
> https://issues.apache.org/jira/browse/MESOS-1087
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adding permissions check on credentials file and deleting TODO comment
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 1075b46
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 3f475a4
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp PRE-CREATION
> src/master/master.cpp 2a40333
>
> Diff: https://reviews.apache.org/r/19093/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 19093: Display warning for credentials file permissions
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On March 14, 2014, 5:46 a.m., Adam B wrote:
> > src/master/master.cpp, lines 299-300
> > <https://reviews.apache.org/r/19093/diff/3/?file=518207#file518207line299>
> >
> > - Please remove the '!' at the end of this warning message. Exclamations points are rarely needed in log messages, especially when the severity is already expressed by the log level. We usually don't even put periods at the end of log messages.
> > - The word 'insufficient' insufficiently explains what's wrong with the permissions on the file. Explain that the credentials file is globally readable/writable, and recommend a new setting (e.g. 'chmod o-rwx' or 'rw-r-----' or '640').
>
> Benjamin Hindman wrote:
> Isabel was just copying my suggestion for a log message (which included the exclamation point) and she had already pointed out that 'insufficient' was insufficient. ;) I think explaining the problem is great but I don't think it's necessary to tell people how to fix it.
I changed the log message to a modified copy of the same case 'ssh' log message.
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19093/#review37158
-----------------------------------------------------------
On March 14, 2014, 8:08 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19093/
> -----------------------------------------------------------
>
> (Updated March 14, 2014, 8:08 a.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone.
>
>
> Bugs: MESOS-1087
> https://issues.apache.org/jira/browse/MESOS-1087
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adding permissions check on credentials file and deleting TODO comment
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 1075b46
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 3f475a4
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp PRE-CREATION
> src/master/master.cpp 2a40333
>
> Diff: https://reviews.apache.org/r/19093/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 19093: Display warning for credentials file permissions
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19093/#review37158
-----------------------------------------------------------
Looks good. Just a few style nits, a rename, and a suggested log message rewording.
3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp
<https://reviews.apache.org/r/19093/#comment68561>
s/getuid/setgid/ for "set-group-ID on execution"
3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp
<https://reviews.apache.org/r/19093/#comment68560>
indentation
3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp
<https://reviews.apache.org/r/19093/#comment68562>
setgid
3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp
<https://reviews.apache.org/r/19093/#comment68564>
Double-space please
src/master/master.cpp
<https://reviews.apache.org/r/19093/#comment68565>
Line up with the '<<' on the line above
src/master/master.cpp
<https://reviews.apache.org/r/19093/#comment68566>
- Please remove the '!' at the end of this warning message. Exclamations points are rarely needed in log messages, especially when the severity is already expressed by the log level. We usually don't even put periods at the end of log messages.
- The word 'insufficient' insufficiently explains what's wrong with the permissions on the file. Explain that the credentials file is globally readable/writable, and recommend a new setting (e.g. 'chmod o-rwx' or 'rw-r-----' or '640').
- Adam B
On March 13, 2014, 1:47 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19093/
> -----------------------------------------------------------
>
> (Updated March 13, 2014, 1:47 a.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone.
>
>
> Bugs: MESOS-1087
> https://issues.apache.org/jira/browse/MESOS-1087
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adding permissions check on credentials file and deleting TODO comment
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 1075b46
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 3f475a4
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp PRE-CREATION
> src/master/master.cpp 2a40333
>
> Diff: https://reviews.apache.org/r/19093/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 19093: Display warning for credentials file permissions
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On March 14, 2014, 6:13 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp, line 47
> > <https://reviews.apache.org/r/19093/diff/3/?file=518206#file518206line47>
> >
> > One of these things is not like the other. ;) I'd like to make two suggestions:
> >
> > (1) Replace 'readable', 'writeable', and 'executable' with 'r', 'w', and 'x' respectively and keep 'rwx'.
> >
> > (2) Replace 'rwx' with 'all' (or another name you find more natural).
> >
> > Even though (1) is a bit more esoteric I still think it's easier to grok than S_IRUSR, S_IWUSR, etc. and it's fairly reminiscent of using 'chmod' on the command line.
> >
> > What do you think?
I made (1), I agree on the consistency with 'chmod' usage.
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19093/#review37164
-----------------------------------------------------------
On March 14, 2014, 8:08 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19093/
> -----------------------------------------------------------
>
> (Updated March 14, 2014, 8:08 a.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone.
>
>
> Bugs: MESOS-1087
> https://issues.apache.org/jira/browse/MESOS-1087
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adding permissions check on credentials file and deleting TODO comment
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 1075b46
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 3f475a4
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp PRE-CREATION
> src/master/master.cpp 2a40333
>
> Diff: https://reviews.apache.org/r/19093/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 19093: Display warning for credentials file permissions
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19093/#review37164
-----------------------------------------------------------
Looks great! A few more cleanups and then we'll get this committed.
3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp
<https://reviews.apache.org/r/19093/#comment68572>
Please add a newline between these two (that's the style in the code base).
3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp
<https://reviews.apache.org/r/19093/#comment68573>
We don't indent anything within namespaces in our codebase, please pull this back -2.
3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp
<https://reviews.apache.org/r/19093/#comment68569>
You've got an extra space on this line too.
3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp
<https://reviews.apache.org/r/19093/#comment68570>
One of these things is not like the other. ;) I'd like to make two suggestions:
(1) Replace 'readable', 'writeable', and 'executable' with 'r', 'w', and 'x' respectively and keep 'rwx'.
(2) Replace 'rwx' with 'all' (or another name you find more natural).
Even though (1) is a bit more esoteric I still think it's easier to grok than S_IRUSR, S_IWUSR, etc. and it's fairly reminiscent of using 'chmod' on the command line.
What do you think?
src/master/master.cpp
<https://reviews.apache.org/r/19093/#comment68571>
Please add a newline between these two: there's a lot of text here and it's easier to read when they're separated.
- Benjamin Hindman
On March 13, 2014, 8:47 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19093/
> -----------------------------------------------------------
>
> (Updated March 13, 2014, 8:47 a.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone.
>
>
> Bugs: MESOS-1087
> https://issues.apache.org/jira/browse/MESOS-1087
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adding permissions check on credentials file and deleting TODO comment
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 1075b46
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 3f475a4
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp PRE-CREATION
> src/master/master.cpp 2a40333
>
> Diff: https://reviews.apache.org/r/19093/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 19093: Display warning for credentials file permissions
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On March 21, 2014, 11:59 p.m., Ben Mahler wrote:
> > Looks like we forgot to split apart this change between stout and Mesos. We typically break apart commits to libprocess, stout as they are libraries that are also maintained on github.
> >
> > There were also some hard tabs here, where we normally use spaces, but I fixed these for you so don't worry about cleaning them up!
> >
Thank you, I commented in JIRA about seeing that the librarie was on GitHub but it seemed inactive on its repository, and still active directly on Mesos repository. Sorry about that. Also didn't see hard tabs on my IDE config I will update it for next time. Thank you again for pointing it out.
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19093/#review38236
-----------------------------------------------------------
On March 14, 2014, 8:08 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19093/
> -----------------------------------------------------------
>
> (Updated March 14, 2014, 8:08 a.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone.
>
>
> Bugs: MESOS-1087
> https://issues.apache.org/jira/browse/MESOS-1087
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adding permissions check on credentials file and deleting TODO comment
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 1075b46
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 3f475a4
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp PRE-CREATION
> src/master/master.cpp 2a40333
>
> Diff: https://reviews.apache.org/r/19093/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 19093: Display warning for credentials file permissions
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19093/#review38236
-----------------------------------------------------------
Looks like we forgot to split apart this change between stout and Mesos. We typically break apart commits to libprocess, stout as they are libraries that are also maintained on github.
There were also some hard tabs here, where we normally use spaces, but I fixed these for you so don't worry about cleaning them up!
- Ben Mahler
On March 14, 2014, 8:08 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19093/
> -----------------------------------------------------------
>
> (Updated March 14, 2014, 8:08 a.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone.
>
>
> Bugs: MESOS-1087
> https://issues.apache.org/jira/browse/MESOS-1087
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adding permissions check on credentials file and deleting TODO comment
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 1075b46
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 3f475a4
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp PRE-CREATION
> src/master/master.cpp 2a40333
>
> Diff: https://reviews.apache.org/r/19093/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 19093: Display warning for credentials file permissions
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19093/#review37258
-----------------------------------------------------------
Ship it!
Awesome! I'll get this submitted now.
- Benjamin Hindman
On March 14, 2014, 8:08 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19093/
> -----------------------------------------------------------
>
> (Updated March 14, 2014, 8:08 a.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone.
>
>
> Bugs: MESOS-1087
> https://issues.apache.org/jira/browse/MESOS-1087
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adding permissions check on credentials file and deleting TODO comment
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 1075b46
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 3f475a4
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp PRE-CREATION
> src/master/master.cpp 2a40333
>
> Diff: https://reviews.apache.org/r/19093/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 19093: Display warning for credentials file permissions
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19093/
-----------------------------------------------------------
(Updated March 14, 2014, 8:08 a.m.)
Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone.
Bugs: MESOS-1087
https://issues.apache.org/jira/browse/MESOS-1087
Repository: mesos-git
Description
-------
Adding permissions check on credentials file and deleting TODO comment
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/Makefile.am 1075b46
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 3f475a4
3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp PRE-CREATION
src/master/master.cpp 2a40333
Diff: https://reviews.apache.org/r/19093/diff/
Testing
-------
Thanks,
Isabel Jimenez
Re: Review Request 19093: Display warning for credentials file permissions
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19093/
-----------------------------------------------------------
(Updated March 13, 2014, 8:47 a.m.)
Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone.
Bugs: MESOS-1087
https://issues.apache.org/jira/browse/MESOS-1087
Repository: mesos-git
Description
-------
Adding permissions check on credentials file and deleting TODO comment
Diffs
-----
3rdparty/libprocess/3rdparty/stout/Makefile.am 1075b46
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 3f475a4
3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp PRE-CREATION
src/master/master.cpp 2a40333
Diff: https://reviews.apache.org/r/19093/diff/
Testing
-------
Thanks,
Isabel Jimenez
Re: Review Request 19093: Display warning for credentials file permissions
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19093/
-----------------------------------------------------------
(Updated March 13, 2014, 7:01 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Hi thank you for your comments, I hope it's better now and more generic. I added rwx and sticky bit in the struct. I find the message: "Permissions on credentials file are insufficient!" a little ambiguous.
Bugs: MESOS-1087
https://issues.apache.org/jira/browse/MESOS-1087
Repository: mesos-git
Description
-------
Adding permissions check on credentials file and deleting TODO comment
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/Makefile.am 1075b46
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 3f475a4
3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp PRE-CREATION
src/master/master.cpp 2a40333
Diff: https://reviews.apache.org/r/19093/diff/
Testing
-------
Thanks,
Isabel Jimenez
Re: Review Request 19093: Display warning for credentials file permissions
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On March 12, 2014, 11:36 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp, line 23
> > <https://reviews.apache.org/r/19093/diff/2/?file=517092#file517092line23>
> >
> > How about introducing a new Permissions struct here?
> >
> > struct Permissions
> > {
> > explicit Permissions(mode_t mode)
> > {
> > owner.readable = mode & magic_to_determine_if_mode_is_readable;
> > owner.writable = mode & magic_to_determine_if_mode_is_writable;
> > owner.executable = mode & magic_to_determine_if_mode_is_executable;
> > ...
> > }
> >
> > struct {
> > bool readable;
> > bool writable;
> > bool executable;
> > } owner;
> >
> > struct {
> > bool readable;
> > bool writable;
> > bool executable;
> > } group;
> >
> > struct {
> > bool readable;
> > bool writable;
> > bool executable;
> > } others;
> >
> > bool setuid;
> > bool setgid;
> > };
> >
> > Then in code you could do:
> >
> > Try<os::Permissions> permissions = os::permissions(path);
> >
> > if (!permissions.isError()) {
> > if (permissions.get().others.readable ||
> > permissions.get().others.writable ||
> > permissions.get().others.executable) {
> > LOG(WARNING) << "Permissions on credentials file are insufficient!";
> > }
> > }
> >
> > The slight downside here is that we'll be spending some CPU cycles constructing a Permissions object, but that's highly unlikely to be a performance issue. The upside here is the code becomes very explicit and therefore maintainable!
>
> Ben Mahler wrote:
> What about sharing the struct definition across 'owner' 'group' and 'others'?
>
> struct {
> bool readable;
> bool writable;
> bool executable;
> } owner, group, others;
Definitely!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19093/#review37012
-----------------------------------------------------------
On March 12, 2014, 11:07 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19093/
> -----------------------------------------------------------
>
> (Updated March 12, 2014, 11:07 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Bugs: MESOS-1087
> https://issues.apache.org/jira/browse/MESOS-1087
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adding permissions check on credentials file and deleting TODO comment
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 1075b46
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 3f475a4
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp PRE-CREATION
> src/master/master.cpp 2a40333
>
> Diff: https://reviews.apache.org/r/19093/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 19093: Display warning for credentials file permissions
Posted by Ben Mahler <be...@gmail.com>.
> On March 12, 2014, 11:36 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp, line 23
> > <https://reviews.apache.org/r/19093/diff/2/?file=517092#file517092line23>
> >
> > How about introducing a new Permissions struct here?
> >
> > struct Permissions
> > {
> > explicit Permissions(mode_t mode)
> > {
> > owner.readable = mode & magic_to_determine_if_mode_is_readable;
> > owner.writable = mode & magic_to_determine_if_mode_is_writable;
> > owner.executable = mode & magic_to_determine_if_mode_is_executable;
> > ...
> > }
> >
> > struct {
> > bool readable;
> > bool writable;
> > bool executable;
> > } owner;
> >
> > struct {
> > bool readable;
> > bool writable;
> > bool executable;
> > } group;
> >
> > struct {
> > bool readable;
> > bool writable;
> > bool executable;
> > } others;
> >
> > bool setuid;
> > bool setgid;
> > };
> >
> > Then in code you could do:
> >
> > Try<os::Permissions> permissions = os::permissions(path);
> >
> > if (!permissions.isError()) {
> > if (permissions.get().others.readable ||
> > permissions.get().others.writable ||
> > permissions.get().others.executable) {
> > LOG(WARNING) << "Permissions on credentials file are insufficient!";
> > }
> > }
> >
> > The slight downside here is that we'll be spending some CPU cycles constructing a Permissions object, but that's highly unlikely to be a performance issue. The upside here is the code becomes very explicit and therefore maintainable!
What about sharing the struct definition across 'owner' 'group' and 'others'?
struct {
bool readable;
bool writable;
bool executable;
} owner, group, others;
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19093/#review37012
-----------------------------------------------------------
On March 12, 2014, 11:07 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19093/
> -----------------------------------------------------------
>
> (Updated March 12, 2014, 11:07 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Bugs: MESOS-1087
> https://issues.apache.org/jira/browse/MESOS-1087
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adding permissions check on credentials file and deleting TODO comment
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 1075b46
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 3f475a4
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp PRE-CREATION
> src/master/master.cpp 2a40333
>
> Diff: https://reviews.apache.org/r/19093/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 19093: Display warning for credentials file permissions
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19093/#review37012
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp
<https://reviews.apache.org/r/19093/#comment68265>
How about introducing a new Permissions struct here?
struct Permissions
{
explicit Permissions(mode_t mode)
{
owner.readable = mode & magic_to_determine_if_mode_is_readable;
owner.writable = mode & magic_to_determine_if_mode_is_writable;
owner.executable = mode & magic_to_determine_if_mode_is_executable;
...
}
struct {
bool readable;
bool writable;
bool executable;
} owner;
struct {
bool readable;
bool writable;
bool executable;
} group;
struct {
bool readable;
bool writable;
bool executable;
} others;
bool setuid;
bool setgid;
};
Then in code you could do:
Try<os::Permissions> permissions = os::permissions(path);
if (!permissions.isError()) {
if (permissions.get().others.readable ||
permissions.get().others.writable ||
permissions.get().others.executable) {
LOG(WARNING) << "Permissions on credentials file are insufficient!";
}
}
The slight downside here is that we'll be spending some CPU cycles constructing a Permissions object, but that's highly unlikely to be a performance issue. The upside here is the code becomes very explicit and therefore maintainable!
src/master/master.cpp
<https://reviews.apache.org/r/19093/#comment68258>
How about:
const string& path =
strings::remove(flags.credentials.get(), "file://", strings::PREFIX);
All uses of 'path' are fine without the "file://" prefix.
src/master/master.cpp
<https://reviews.apache.org/r/19093/#comment68257>
Let's use LOG(WARNING) here since this is just advisory.
- Benjamin Hindman
On March 12, 2014, 11:07 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19093/
> -----------------------------------------------------------
>
> (Updated March 12, 2014, 11:07 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Bugs: MESOS-1087
> https://issues.apache.org/jira/browse/MESOS-1087
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adding permissions check on credentials file and deleting TODO comment
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 1075b46
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 3f475a4
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp PRE-CREATION
> src/master/master.cpp 2a40333
>
> Diff: https://reviews.apache.org/r/19093/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 19093: Display warning for credentials file permissions
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19093/
-----------------------------------------------------------
(Updated March 12, 2014, 11:07 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
small typo in warning message.
Bugs: MESOS-1087
https://issues.apache.org/jira/browse/MESOS-1087
Repository: mesos-git
Description
-------
Adding permissions check on credentials file and deleting TODO comment
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/Makefile.am 1075b46
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 3f475a4
3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp PRE-CREATION
src/master/master.cpp 2a40333
Diff: https://reviews.apache.org/r/19093/diff/
Testing
-------
Thanks,
Isabel Jimenez