Hi,
with the patch the config file is only read if it is - a regular file - owner and group are 0 (root) - file permissions are 600
This patch depends on the config_from_fd patch currently under review.
bye, Sumit
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/28/2009 11:49 AM, Sumit Bose wrote:
Hi,
with the patch the config file is only read if it is
- a regular file
- owner and group are 0 (root)
- file permissions are 600
This patch depends on the config_from_fd patch currently under review.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Nack.
As discussed on IRC, the lstat is redundant. All of the necessary file-type checks can be performed with the fstat, with no risk of race condition.
- -- Stephen Gallagher RHCE 804006346421761
Looking to carve out IT costs? www.redhat.com/carveoutcosts/
Stephen Gallagher wrote:
On 09/28/2009 11:49 AM, Sumit Bose wrote:
Hi,
with the patch the config file is only read if it is
- a regular file
- owner and group are 0 (root)
- file permissions are 600
This patch depends on the config_from_fd patch currently under review.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Nack.
As discussed on IRC, the lstat is redundant. All of the necessary file-type checks can be performed with the fstat, with no risk of race condition.
I thought we had some plans to move part of this functionality into ini interface. Should we? I think there was a ticket about this to make ini config interface deal with special files and permissions.
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/28/2009 12:24 PM, Stephen Gallagher wrote:
On 09/28/2009 11:49 AM, Sumit Bose wrote:
Hi,
with the patch the config file is only read if it is
- a regular file
- owner and group are 0 (root)
- file permissions are 600
This patch depends on the config_from_fd patch currently under review.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Nack.
As discussed on IRC, the lstat is redundant. All of the necessary file-type checks can be performed with the fstat, with no risk of race condition.
Per conversation on IRC, this patch is approved. I didn't realize at first that we want to exclude symlinks as well from the SSSD. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
- -- Stephen Gallagher RHCE 804006346421761
Looking to carve out IT costs? www.redhat.com/carveoutcosts/
On 09/28/2009 01:52 PM, Stephen Gallagher wrote:
On 09/28/2009 12:24 PM, Stephen Gallagher wrote:
On 09/28/2009 11:49 AM, Sumit Bose wrote:
Hi,
with the patch the config file is only read if it is
- a regular file
- owner and group are 0 (root)
- file permissions are 600
This patch depends on the config_from_fd patch currently under review.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Nack.
As discussed on IRC, the lstat is redundant. All of the necessary file-type checks can be performed with the fstat, with no risk of race condition.
Per conversation on IRC, this patch is approved. I didn't realize at first that we want to exclude symlinks as well from the SSSD. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Updated Sumit's patch to use the new interface. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
On Mon, Sep 28, 2009 at 02:51:11PM -0400, Stephen Gallagher wrote:
On 09/28/2009 01:52 PM, Stephen Gallagher wrote:
On 09/28/2009 12:24 PM, Stephen Gallagher wrote:
On 09/28/2009 11:49 AM, Sumit Bose wrote:
Hi,
with the patch the config file is only read if it is
- a regular file
- owner and group are 0 (root)
- file permissions are 600
This patch depends on the config_from_fd patch currently under review.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Nack.
As discussed on IRC, the lstat is redundant. All of the necessary file-type checks can be performed with the fstat, with no risk of race condition.
Per conversation on IRC, this patch is approved. I didn't realize at first that we want to exclude symlinks as well from the SSSD. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Updated Sumit's patch to use the new interface.
ACK (is this a self-ACK?)
bye, Sumit
On 09/28/2009 03:05 PM, Sumit Bose wrote:
On Mon, Sep 28, 2009 at 02:51:11PM -0400, Stephen Gallagher wrote:
On 09/28/2009 01:52 PM, Stephen Gallagher wrote:
On 09/28/2009 12:24 PM, Stephen Gallagher wrote:
On 09/28/2009 11:49 AM, Sumit Bose wrote:
Hi,
with the patch the config file is only read if it is
- a regular file
- owner and group are 0 (root)
- file permissions are 600
This patch depends on the config_from_fd patch currently under review.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Nack.
As discussed on IRC, the lstat is redundant. All of the necessary file-type checks can be performed with the fstat, with no risk of race condition.
Per conversation on IRC, this patch is approved. I didn't realize at first that we want to exclude symlinks as well from the SSSD. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Updated Sumit's patch to use the new interface.
ACK (is this a self-ACK?)
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
After a bit of thought, I realized that the code in confdb_init_db needed to be reordered a bit. If the permissions on the file changed, but not its contents, we would never detect it, because we were checking whether the modification time has changed first. Changing file permissions only does not update the modification time, so until the file actually had new data written into it, it would have happily kept loading a potentially world-readable config file.
I've now moved the check_and_open_readonly() call to the beginning of the confdb_init_db routine and converted the modification time stat() to an fstat() on the returned file descriptor.
Please re-review.
On Fri, Oct 02, 2009 at 03:20:33PM -0400, Stephen Gallagher wrote:
On 09/28/2009 03:05 PM, Sumit Bose wrote:
On Mon, Sep 28, 2009 at 02:51:11PM -0400, Stephen Gallagher wrote:
On 09/28/2009 01:52 PM, Stephen Gallagher wrote:
On 09/28/2009 12:24 PM, Stephen Gallagher wrote:
On 09/28/2009 11:49 AM, Sumit Bose wrote:
Hi,
with the patch the config file is only read if it is
- a regular file
- owner and group are 0 (root)
- file permissions are 600
This patch depends on the config_from_fd patch currently under review.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Nack.
As discussed on IRC, the lstat is redundant. All of the necessary file-type checks can be performed with the fstat, with no risk of race condition.
Per conversation on IRC, this patch is approved. I didn't realize at first that we want to exclude symlinks as well from the SSSD. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Updated Sumit's patch to use the new interface.
ACK (is this a self-ACK?)
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
After a bit of thought, I realized that the code in confdb_init_db needed to be reordered a bit. If the permissions on the file changed, but not its contents, we would never detect it, because we were checking whether the modification time has changed first. Changing file permissions only does not update the modification time, so until the file actually had new data written into it, it would have happily kept loading a potentially world-readable config file.
I've now moved the check_and_open_readonly() call to the beginning of the confdb_init_db routine and converted the modification time stat() to an fstat() on the returned file descriptor.
Please re-review.
Although I cannot follow the argument I think it is a good idea to move check_and_open_readonly() to the top,
ACK.
I have updated the sssd.conf man page a the check_and_open_readonly() tests in "[PATCH] more documentation and test for sssd.conf", please review.
bye, Sumit
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/05/2009 03:15 AM, Sumit Bose wrote:
On Fri, Oct 02, 2009 at 03:20:33PM -0400, Stephen Gallagher wrote:
On 09/28/2009 03:05 PM, Sumit Bose wrote:
On Mon, Sep 28, 2009 at 02:51:11PM -0400, Stephen Gallagher wrote:
On 09/28/2009 01:52 PM, Stephen Gallagher wrote:
On 09/28/2009 12:24 PM, Stephen Gallagher wrote:
On 09/28/2009 11:49 AM, Sumit Bose wrote: > Hi,
> with the patch the config file is only read if it is > - a regular file > - owner and group are 0 (root) > - file permissions are 600
> This patch depends on the config_from_fd patch currently under review.
> bye, > Sumit
> ------------------------------------------------------------------------
> _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://fedorahosted.org/mailman/listinfo/sssd-devel
Nack.
As discussed on IRC, the lstat is redundant. All of the necessary file-type checks can be performed with the fstat, with no risk of race condition.
Per conversation on IRC, this patch is approved. I didn't realize at first that we want to exclude symlinks as well from the SSSD. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Updated Sumit's patch to use the new interface.
ACK (is this a self-ACK?)
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
After a bit of thought, I realized that the code in confdb_init_db needed to be reordered a bit. If the permissions on the file changed, but not its contents, we would never detect it, because we were checking whether the modification time has changed first. Changing file permissions only does not update the modification time, so until the file actually had new data written into it, it would have happily kept loading a potentially world-readable config file.
I've now moved the check_and_open_readonly() call to the beginning of the confdb_init_db routine and converted the modification time stat() to an fstat() on the returned file descriptor.
Please re-review.
Although I cannot follow the argument I think it is a good idea to move check_and_open_readonly() to the top,
ACK.
I have updated the sssd.conf man page a the check_and_open_readonly() tests in "[PATCH] more documentation and test for sssd.conf", please review.
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Even despite the ack, I think maybe this needs more clarification.
The way the confdb_init_db() routine used to work: 1) stat the config file. If the mtime > confdb.ldb's lastUpdated time, reread the config. 2) Run check_and_open_readonly()
The problem with this is that chmod doesn't update mtime on a file. This means that if someone created a valid config, then started the sssd, then chmod-ed the config file, it would not be noticed by check_and_open_readonly().
The approach in this patch makes it so that check_and_open_readonly() is ALWAYS run, but we only do the actual config file processing if the file itself has changed.
I noticed when I was testing your patch that just running chmod didn't trigger the failure to load, so I investigated why and fixed it.
- -- Stephen Gallagher RHCE 804006346421761
Looking to carve out IT costs? www.redhat.com/carveoutcosts/
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/05/2009 03:15 AM, Sumit Bose wrote:
On Fri, Oct 02, 2009 at 03:20:33PM -0400, Stephen Gallagher wrote:
On 09/28/2009 03:05 PM, Sumit Bose wrote:
On Mon, Sep 28, 2009 at 02:51:11PM -0400, Stephen Gallagher wrote:
On 09/28/2009 01:52 PM, Stephen Gallagher wrote:
On 09/28/2009 12:24 PM, Stephen Gallagher wrote:
On 09/28/2009 11:49 AM, Sumit Bose wrote: > Hi,
> with the patch the config file is only read if it is > - a regular file > - owner and group are 0 (root) > - file permissions are 600
> This patch depends on the config_from_fd patch currently under review.
> bye, > Sumit
> ------------------------------------------------------------------------
> _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://fedorahosted.org/mailman/listinfo/sssd-devel
Nack.
As discussed on IRC, the lstat is redundant. All of the necessary file-type checks can be performed with the fstat, with no risk of race condition.
Per conversation on IRC, this patch is approved. I didn't realize at first that we want to exclude symlinks as well from the SSSD. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Updated Sumit's patch to use the new interface.
ACK (is this a self-ACK?)
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
After a bit of thought, I realized that the code in confdb_init_db needed to be reordered a bit. If the permissions on the file changed, but not its contents, we would never detect it, because we were checking whether the modification time has changed first. Changing file permissions only does not update the modification time, so until the file actually had new data written into it, it would have happily kept loading a potentially world-readable config file.
I've now moved the check_and_open_readonly() call to the beginning of the confdb_init_db routine and converted the modification time stat() to an fstat() on the returned file descriptor.
Please re-review.
Although I cannot follow the argument I think it is a good idea to move check_and_open_readonly() to the top,
ACK.
I have updated the sssd.conf man page a the check_and_open_readonly() tests in "[PATCH] more documentation and test for sssd.conf", please review.
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Pushed to master.
- -- Stephen Gallagher RHCE 804006346421761
Looking to carve out IT costs? www.redhat.com/carveoutcosts/
sssd-devel@lists.fedorahosted.org