Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-7826

Permission issues when creating cores with bin/solr as root user

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.3, master (7.0)
    • Component/s: None
    • Labels:

      Description

      Ran into an interesting situation on IRC today.

      Solr has been installed as a service using the shell script install_solr_service.sh ... so it is running as an unprivileged user.

      User is running "bin/solr create" as root. This causes permission problems, because the script creates the core's instanceDir with root ownership, then when Solr is instructed to actually create the core, it cannot create the dataDir.

      Enhancement idea: When the install script is used, leave breadcrumbs somewhere so that the "create core" section of the main script can find it and su to the user specified during install.

      1. SOLR-7826_sameuser.patch
        8 kB
        Jan Høydahl
      2. SOLR-7826.patch
        2 kB
        Jan Høydahl
      3. SOLR-7826.patch
        7 kB
        Binoy Dalal

        Issue Links

          Activity

          Hide
          elyograg Shawn Heisey added a comment -

          I know, the user should not be logged in as root. Let's just not have that flamewar, OK?

          Show
          elyograg Shawn Heisey added a comment - I know, the user should not be logged in as root. Let's just not have that flamewar, OK?
          Hide
          elyograg Shawn Heisey added a comment -

          At the very least, the script should probably detect uid 0 and display a warning saying that creating cores as root might cause permission issues.

          Show
          elyograg Shawn Heisey added a comment - At the very least, the script should probably detect uid 0 and display a warning saying that creating cores as root might cause permission issues.
          Hide
          janhoy Jan Høydahl added a comment -

          I believe creating cores as root will cause problems every single time, so why allow it at all? Perhaps bin/solr always should bail out early if executed as root, perhaps with an --runasrootonyourownrisk param to override?

          Show
          janhoy Jan Høydahl added a comment - I believe creating cores as root will cause problems every single time, so why allow it at all? Perhaps bin/solr always should bail out early if executed as root, perhaps with an --runasrootonyourownrisk param to override?
          Hide
          elyograg Shawn Heisey added a comment -

          Perhaps bin/solr always should bail out early if executed as root, perhaps with an --runasrootonyourownrisk param to override?

          Sounds awesome to me. There's another project that does something similar to protect the user from themselves, and the option to explicitly force the action is not documented anywhere except in the program output, which I think is a reasonable thing to do here.

          I want to say that it's the linux raid tools (mdadm) that has the undocumented "I really know what I'm doing, please proceed" option, but I can no longer remember ... and google isn't helpful since it's not documented.

          Show
          elyograg Shawn Heisey added a comment - Perhaps bin/solr always should bail out early if executed as root, perhaps with an --runasrootonyourownrisk param to override? Sounds awesome to me. There's another project that does something similar to protect the user from themselves, and the option to explicitly force the action is not documented anywhere except in the program output, which I think is a reasonable thing to do here. I want to say that it's the linux raid tools (mdadm) that has the undocumented "I really know what I'm doing, please proceed" option, but I can no longer remember ... and google isn't helpful since it's not documented.
          Hide
          elyograg Shawn Heisey added a comment -

          Found this info related to the "mkraid" command. I can't seem to find that command on any of my Linux installs right now, so it appears this command is not currently part of the mdadm package, and may be an old utility that has not survived to the present. I still like the idea of requiring an option that can only be discovered by actually trying to perform the action that's considered unsafe.

          -f, --force, --really-force, -R
          
              Forces the initialization, even if data or filesystems are detected on any of the block devices to be included in the array. This is a fail-safe to prevent uninitiated users from accidentally destroying their data. The --really-force or -R flag is undocumented in the command help and manual pages. When the -f or --force flags are used, mkraid will display an additional warning and request that the command be retyped with the --really-force flag.
          

          I have seen another program with a "force" option that's quite long and essentially makes the user type out something like "I acknowledge that this is a really bad idea" to run the command. I don't think we need to go that far, but I was quite amused by it.

          Show
          elyograg Shawn Heisey added a comment - Found this info related to the "mkraid" command. I can't seem to find that command on any of my Linux installs right now, so it appears this command is not currently part of the mdadm package, and may be an old utility that has not survived to the present. I still like the idea of requiring an option that can only be discovered by actually trying to perform the action that's considered unsafe. -f, --force, --really-force, -R Forces the initialization, even if data or filesystems are detected on any of the block devices to be included in the array. This is a fail-safe to prevent uninitiated users from accidentally destroying their data. The --really-force or -R flag is undocumented in the command help and manual pages. When the -f or --force flags are used, mkraid will display an additional warning and request that the command be retyped with the --really-force flag. I have seen another program with a "force" option that's quite long and essentially makes the user type out something like "I acknowledge that this is a really bad idea" to run the command. I don't think we need to go that far, but I was quite amused by it.
          Hide
          elyograg Shawn Heisey added a comment -

          I'm going to assume that the "id" command (/usr/bin/id on Ubuntu and redhat-based systems) is present in the system and that the short options on a commercial Unix behave like the gnu version. On Linux, the "id" command is in the same package (coreutils) as "ls" so I think this is a safe assumption.

          Show
          elyograg Shawn Heisey added a comment - I'm going to assume that the "id" command (/usr/bin/id on Ubuntu and redhat-based systems) is present in the system and that the short options on a commercial Unix behave like the gnu version. On Linux, the "id" command is in the same package (coreutils) as "ls" so I think this is a safe assumption.
          Hide
          elyograg Shawn Heisey added a comment -

          Initial attempts are not working completely, and I'm fighting with a flaky Internet connection at the location where I'm doing the work. If I manage to get something that works right, I'll upload a patch.

          Show
          elyograg Shawn Heisey added a comment - Initial attempts are not working completely, and I'm fighting with a flaky Internet connection at the location where I'm doing the work. If I manage to get something that works right, I'll upload a patch.
          Hide
          janhoy Jan Høydahl added a comment -

          Tagging this as newdev, as it should be 3 lines in bin/solr

          Show
          janhoy Jan Høydahl added a comment - Tagging this as newdev , as it should be 3 lines in bin/solr
          Hide
          binoydalal93@gmail.com Binoy Dalal added a comment -

          I would like to take a shot at this.
          So the idea is to put in an extra option and a warning to users right?

          Show
          binoydalal93@gmail.com Binoy Dalal added a comment - I would like to take a shot at this. So the idea is to put in an extra option and a warning to users right?
          Hide
          elyograg Shawn Heisey added a comment -

          Binoy Dalal: Exactly. If the script detects uid 0 when creating cores (probably more accurate than checking the username), abort, unless the special option is provided.

          This check is not required when creating collections in cloud mode, because the script doesn't touch the filesystem. Solr itself handles the filesystem work.

          Show
          elyograg Shawn Heisey added a comment - Binoy Dalal : Exactly. If the script detects uid 0 when creating cores (probably more accurate than checking the username), abort, unless the special option is provided. This check is not required when creating collections in cloud mode, because the script doesn't touch the filesystem. Solr itself handles the filesystem work.
          Hide
          binoydalal93@gmail.com Binoy Dalal added a comment -

          I've made modifications to the script and SolrCLI.java to support this but I don't think this approach really solves anything.
          Considering that solr was run as an unprivileged user, when creating the cores as root without the option, the script throws an error and bails.
          But when the option is used, the user will still be unable to create a core since the code will throw an AcessDeniedException, so usage of the option makes no difference whatsoever.
          I think it would make more sense if the user weren't allowed to create cores at all using root, or if the AccessDeniedException was caught and a suitable warning was provided to the user.

          I would like to know your thoughts on this.

          Show
          binoydalal93@gmail.com Binoy Dalal added a comment - I've made modifications to the script and SolrCLI.java to support this but I don't think this approach really solves anything. Considering that solr was run as an unprivileged user, when creating the cores as root without the option, the script throws an error and bails. But when the option is used, the user will still be unable to create a core since the code will throw an AcessDeniedException, so usage of the option makes no difference whatsoever. I think it would make more sense if the user weren't allowed to create cores at all using root, or if the AccessDeniedException was caught and a suitable warning was provided to the user. I would like to know your thoughts on this.
          Hide
          elyograg Shawn Heisey added a comment -

          If the user has started Solr as root (rather than running the installer script and starting the service), then running "bin/solr create" as root is not a problem, and this is where the option comes in.

          Show
          elyograg Shawn Heisey added a comment - If the user has started Solr as root (rather than running the installer script and starting the service), then running "bin/solr create" as root is not a problem, and this is where the option comes in.
          Hide
          binoydalal93@gmail.com Binoy Dalal added a comment -

          Ok. That makes sense. I'll finish off the patch and put it up asap.
          Thanks.

          Show
          binoydalal93@gmail.com Binoy Dalal added a comment - Ok. That makes sense. I'll finish off the patch and put it up asap. Thanks.
          Hide
          binoydalal93@gmail.com Binoy Dalal added a comment - - edited

          I'm having a bit of an issue with the current implementation so I'll just outline my approach, and state the problem:
          1) Detect the user while solr is first started.
          2) Pass the user detected in (1) to SolrCLI and the option if any.
          3) Perform the user and option check if user=root.
          4) Return appropriate message.

          I'm facing a problem in step 1 and 2. I am not sure of how to store the detected user so that the next time the script is run, it can find that user. I've tried exporting the variable but that doesn't seem to work without adding the variable to permanent list of environment variables, which I don't think is something that should be done.

          Another option that I've thought of is to write the user id to a file like the solr port is written and use that in subsequent script calls for user checking, and delete it once solr is stopped. I think this would be a good approach.

          Please advise.

          – EDIT –
          I've tested out the file creation approach and it works fine.

          To recap the requirements, to see if I missed something:
          1) If solr is started as root, and user tries to create a core as root then display warning with the option to user.
          2) If solr is started as root, and user tries to create a core as root with option, allow creation of core.
          3) If solr is started as an unprivileged user and creation of a core is attempted with root, an exception is thrown with the default behaviour (no changes to be made here).

          Let me know if I've missed something.

          Thanks.

          Show
          binoydalal93@gmail.com Binoy Dalal added a comment - - edited I'm having a bit of an issue with the current implementation so I'll just outline my approach, and state the problem: 1) Detect the user while solr is first started. 2) Pass the user detected in (1) to SolrCLI and the option if any. 3) Perform the user and option check if user=root. 4) Return appropriate message. I'm facing a problem in step 1 and 2. I am not sure of how to store the detected user so that the next time the script is run, it can find that user. I've tried exporting the variable but that doesn't seem to work without adding the variable to permanent list of environment variables, which I don't think is something that should be done. Another option that I've thought of is to write the user id to a file like the solr port is written and use that in subsequent script calls for user checking, and delete it once solr is stopped. I think this would be a good approach. Please advise. – EDIT – I've tested out the file creation approach and it works fine. To recap the requirements, to see if I missed something: 1) If solr is started as root, and user tries to create a core as root then display warning with the option to user. 2) If solr is started as root, and user tries to create a core as root with option, allow creation of core. 3) If solr is started as an unprivileged user and creation of a core is attempted with root, an exception is thrown with the default behaviour (no changes to be made here). Let me know if I've missed something. Thanks.
          Hide
          binoydalal93@gmail.com Binoy Dalal added a comment -

          Shawn/Jan,
          I've tested out this patch with the lucene-solr trunk.
          It works as per the requirements I've stated in my previous comment.

          Please review and let me know if the approach I've taken is fine or if I've missed something.

          If all's good then this patch is ready to be committed I guess. So one of the committers can take it up.

          Show
          binoydalal93@gmail.com Binoy Dalal added a comment - Shawn/Jan, I've tested out this patch with the lucene-solr trunk. It works as per the requirements I've stated in my previous comment. Please review and let me know if the approach I've taken is fine or if I've missed something. If all's good then this patch is ready to be committed I guess. So one of the committers can take it up.
          Hide
          nicerobot nicerobot added a comment - - edited

          Using 6.2. I haven't tested the patch. This is a usable workaround. e.g.

          sudo -u solr bin/solr create -c demo
          
          Show
          nicerobot nicerobot added a comment - - edited Using 6.2. I haven't tested the patch. This is a usable workaround. e.g. sudo -u solr bin/solr create -c demo
          Hide
          janhoy Jan Høydahl added a comment -

          I took a fresh look at this, and came up with a much simpler patch with 10 new lines in bin/solr only. I think this is sufficient for now, no need to remember which user started Solr, we simply always warn if run by root. I have no idea of whether the same problem occurs as Administrator user on Windows, so not trying to fix that.

          Show
          janhoy Jan Høydahl added a comment - I took a fresh look at this, and came up with a much simpler patch with 10 new lines in bin/solr only. I think this is sufficient for now, no need to remember which user started Solr, we simply always warn if run by root. I have no idea of whether the same problem occurs as Administrator user on Windows, so not trying to fix that.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 7561461f738a447856bb93b0a847b0200fff4c9c in lucene-solr's branch refs/heads/master from Jan Høydahl
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7561461 ]

          SOLR-7826: Refuse "bin/solr create" if run as root, unless -force is specified

          Show
          jira-bot ASF subversion and git services added a comment - Commit 7561461f738a447856bb93b0a847b0200fff4c9c in lucene-solr's branch refs/heads/master from Jan Høydahl [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7561461 ] SOLR-7826 : Refuse "bin/solr create" if run as root, unless -force is specified
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 6b10283765cf015aad59f054919134662d060c3b in lucene-solr's branch refs/heads/branch_6x from Jan Høydahl
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6b10283 ]

          SOLR-7826: Refuse "bin/solr create" if run as root, unless -force is specified

          (cherry picked from commit 7561461)

          Show
          jira-bot ASF subversion and git services added a comment - Commit 6b10283765cf015aad59f054919134662d060c3b in lucene-solr's branch refs/heads/branch_6x from Jan Høydahl [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6b10283 ] SOLR-7826 : Refuse "bin/solr create" if run as root, unless -force is specified (cherry picked from commit 7561461)
          Hide
          janhoy Jan Høydahl added a comment -

          Thanks for the discussion and the contributions. Binoy/Shawn, I hope you don't feel bad that I did not use your approach/patch. I thought it was a bit overkill. Since the installer script by default creates a "solr" user for starting solr, it would be very uncommon for Solr to be run as root, and absolutely not something we would recommend. So now we always warn.

          The question is if we should discourage and warn about starting solr as root as well, since this is not recommended?

          Show
          janhoy Jan Høydahl added a comment - Thanks for the discussion and the contributions. Binoy/Shawn, I hope you don't feel bad that I did not use your approach/patch. I thought it was a bit overkill. Since the installer script by default creates a "solr" user for starting solr, it would be very uncommon for Solr to be run as root, and absolutely not something we would recommend. So now we always warn. The question is if we should discourage and warn about starting solr as root as well, since this is not recommended?
          Hide
          elyograg Shawn Heisey added a comment -

          The question is if we should discourage and warn about starting solr as root as well, since this is not recommended?

          I would say yes to this. I'd even go a little bit further. The script should refuse to run (without the force option) any command that creates or modifies filesystem data – but only if Solr also writes to the same filesystem location. This would mean that creating collections in cloud mode and options like upconfig and downconfig would be perfectly acceptable to run as root.

          Show
          elyograg Shawn Heisey added a comment - The question is if we should discourage and warn about starting solr as root as well, since this is not recommended? I would say yes to this. I'd even go a little bit further. The script should refuse to run (without the force option) any command that creates or modifies filesystem data – but only if Solr also writes to the same filesystem location. This would mean that creating collections in cloud mode and options like upconfig and downconfig would be perfectly acceptable to run as root.
          Hide
          janhoy Jan Høydahl added a comment -
          Show
          janhoy Jan Høydahl added a comment - SOLR-9547
          Hide
          janhoy Jan Høydahl added a comment -

          Documented the -force flag and removed warning box in ref-guide https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=50234737&selectedPageVersions=50&selectedPageVersions=49, as the script will now warn the user itself

          Show
          janhoy Jan Høydahl added a comment - Documented the -force flag and removed warning box in ref-guide https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=50234737&selectedPageVersions=50&selectedPageVersions=49 , as the script will now warn the user itself
          Hide
          hossman Hoss Man added a comment -

          I know i'm late to the party, but FWIW: I think adding a -force option and treating "root" as special still leaves a lot of room for the underlying problem to occur (and in general i think adding a -force option that's only supported by one (sub-)command is a bad idea – more on that below) ...

          Just rejecting root won't help if solr is the effective UID of the process, but user bob runs bin/solr create and the new core directories wind up owned by bob but not readable by solr. Likewise, running as root may be perfectly fine, if the original install (foolishly) installed as root

          What really matters is that if bin/solr create is used to try and create new core directories, those new core directories should really be owned by whatever use owns the cores parent directory, and have the same user:group permissions – because that way, regardless of what effective UI the solr process is running under, there's no risk that Solr will be able to find the new core dir, but not read the new core dir.

          ie:

          • we don't have to do anything special to keep track of what user installed solr, or treat root special
          • all we have to do is compare whoami to stat -c '%U' on the cores directory, and complain if they don't match

          My general thoughts on -force:

          even if we switch to comparing the current user to the directory owner instead of treating "root" as special, a -force option could still be supported i guess, but doesn't really seem necessary and in general i would say we should avoid it unless/until we really think through all of the possible commands where we might want to enforce some restrictions unless -force is specified. because a user who sees that there is a -force option for some bin/solr commands would have a reasonable expectation that they will be "protected" unless they specify -force on other risky solr commands as well (ie: deleting a core that's currently LOADed?, delete ZK nodes currently used by a collection? downloading files from ZK and overwriting existing files on disk? uploading a config set and overwritting an existing config set with the same name? etc...)

          In general, i'm -0 to the changes made by this issue - i don't think Solr, on the whole, is better off with these changes, and I'd encourage the folks who worked on this jira to consider rolling them back and replacing them with a `whoami` == `stat -c '%U' .../cores` type comparison instead.

          Show
          hossman Hoss Man added a comment - I know i'm late to the party, but FWIW: I think adding a -force option and treating "root" as special still leaves a lot of room for the underlying problem to occur (and in general i think adding a -force option that's only supported by one (sub-)command is a bad idea – more on that below) ... Just rejecting root won't help if solr is the effective UID of the process, but user bob runs bin/solr create and the new core directories wind up owned by bob but not readable by solr . Likewise, running as root may be perfectly fine, if the original install (foolishly) installed as root What really matters is that if bin/solr create is used to try and create new core directories, those new core directories should really be owned by whatever use owns the cores parent directory, and have the same user:group permissions – because that way, regardless of what effective UI the solr process is running under, there's no risk that Solr will be able to find the new core dir, but not read the new core dir. ie: we don't have to do anything special to keep track of what user installed solr, or treat root special all we have to do is compare whoami to stat -c '%U' on the cores directory, and complain if they don't match My general thoughts on -force : even if we switch to comparing the current user to the directory owner instead of treating "root" as special, a -force option could still be supported i guess, but doesn't really seem necessary and in general i would say we should avoid it unless/until we really think through all of the possible commands where we might want to enforce some restrictions unless -force is specified. because a user who sees that there is a -force option for some bin/solr commands would have a reasonable expectation that they will be "protected" unless they specify -force on other risky solr commands as well (ie: deleting a core that's currently LOADed?, delete ZK nodes currently used by a collection? downloading files from ZK and overwriting existing files on disk? uploading a config set and overwritting an existing config set with the same name? etc...) In general, i'm -0 to the changes made by this issue - i don't think Solr, on the whole, is better off with these changes, and I'd encourage the folks who worked on this jira to consider rolling them back and replacing them with a `whoami` == `stat -c '%U' .../cores` type comparison instead.
          Hide
          elyograg Shawn Heisey added a comment -

          +1 to comments by Hoss Man. I just opened SOLR-9590 for exploration.

          If "bin/solr create" IS run as root, the idea of paying attention to the owner of the parent directory and matching it seems like a good idea too.

          Show
          elyograg Shawn Heisey added a comment - +1 to comments by Hoss Man . I just opened SOLR-9590 for exploration. If "bin/solr create" IS run as root, the idea of paying attention to the owner of the parent directory and matching it seems like a good idea too.
          Hide
          janhoy Jan Høydahl added a comment -

          Just rejecting root won't help if solr is the effective UID of the process, but user bob runs bin/solr create and the new core directories wind up owned by bob but not readable by solr.

          Is this a real or theoretical problem? Testing on Ubuntu shows that the /var/solr folder is not writable by other than the solr user, and new folders created by a user has group "solr". I tested running bin/solr create -c foo with another user, and got

          solr2@acc999d2179f:/opt/solr$ bin/solr create -c newcore
          
          ERROR: Failed to create new core instance directory: /var/solr/data/newcore
          

          On most other systems where some "staff" group may be used, folder permission is "rwxr-xr-x" as far as I know, so a random other user cannot create files in another users area.

          So I think the current fix solves the problem at hand. But I agree it could be solved more generically using stat. I'll leave that for future improvements. Patches welcome.

          Likewise, running as root may be perfectly fine, if the original install (foolishly) installed as root

          Well, since SOLR-9547 we warn against running solr as root, so fewer users will make that mistake, and if they do, they need to -force both start and create commands.

          because a user who sees that there is a -force option for some bin/solr commands would have a reasonable expectation that they will be "protected" unless they specify -force on other risky solr commands as well

          Currently, the -force option is added for the create and start commands, but it is not advertised in -h printout, so users would only know about it if trying to start solr or create cores as root. The documentation in RefGuide clearly tells what the command is for.

          You may be right that we could add even more protection for users by adding -force flags for other situations as well, please open new JIRAs for those.

          Show
          janhoy Jan Høydahl added a comment - Just rejecting root won't help if solr is the effective UID of the process, but user bob runs bin/solr create and the new core directories wind up owned by bob but not readable by solr. Is this a real or theoretical problem? Testing on Ubuntu shows that the /var/solr folder is not writable by other than the solr user, and new folders created by a user has group "solr". I tested running bin/solr create -c foo with another user, and got solr2@acc999d2179f:/opt/solr$ bin/solr create -c newcore ERROR: Failed to create new core instance directory: /var/solr/data/newcore On most other systems where some "staff" group may be used, folder permission is "rwxr-xr-x" as far as I know, so a random other user cannot create files in another users area. So I think the current fix solves the problem at hand. But I agree it could be solved more generically using stat . I'll leave that for future improvements. Patches welcome. Likewise, running as root may be perfectly fine, if the original install (foolishly) installed as root Well, since SOLR-9547 we warn against running solr as root, so fewer users will make that mistake, and if they do, they need to -force both start and create commands. because a user who sees that there is a -force option for some bin/solr commands would have a reasonable expectation that they will be "protected" unless they specify -force on other risky solr commands as well Currently, the -force option is added for the create and start commands, but it is not advertised in -h printout, so users would only know about it if trying to start solr or create cores as root. The documentation in RefGuide clearly tells what the command is for. You may be right that we could add even more protection for users by adding -force flags for other situations as well, please open new JIRAs for those.
          Hide
          hossman Hoss Man added a comment -

          Is this a real or theoretical problem? ...

          By definition it's a theoretical problem because this code added here hasn't been released yet – that doesn't mean we shouldn't give serious consideration to it ... we shouldn't have to wait for users to get screwed by bugs before we discuss if there is a better solution.

          Testing on Ubuntu shows that the /var/solr folder is not writable by other than the solr user, and new folders created by a user has group "solr"....

          you seem to be assuming that people only install using the installation script, and that no one might ever changes the default groups/perms of the solr user. On platforms where people install solr manually (either because the install script doesn't support their os, or because they choose to) the default group/perms of those directories could be anything.

          We shouldn't make bin/solr only work well – or fail cleanly – if you install exactly as we expect you to (and never change any file system perms, or group masks) when it's just as easy to make bin/solr work well and fail cleanly anytime by testing the current directory stats

          Show
          hossman Hoss Man added a comment - Is this a real or theoretical problem? ... By definition it's a theoretical problem because this code added here hasn't been released yet – that doesn't mean we shouldn't give serious consideration to it ... we shouldn't have to wait for users to get screwed by bugs before we discuss if there is a better solution. Testing on Ubuntu shows that the /var/solr folder is not writable by other than the solr user, and new folders created by a user has group "solr".... you seem to be assuming that people only install using the installation script, and that no one might ever changes the default groups/perms of the solr user. On platforms where people install solr manually (either because the install script doesn't support their os, or because they choose to) the default group/perms of those directories could be anything. We shouldn't make bin/solr only work well – or fail cleanly – if you install exactly as we expect you to (and never change any file system perms, or group masks) when it's just as easy to make bin/solr work well and fail cleanly anytime by testing the current directory stats
          Hide
          janhoy Jan Høydahl added a comment -

          we shouldn't have to wait for users to get screwed by bugs before we discuss if there is a better solution.

          Before this patch we had no safeguards whatsoever, but now we protect against inadvertently messing up by running as root. In my eyes that is improvement over perfection. It will always be possible for you or others to open new issues and work on improving usability even more.

          Show
          janhoy Jan Høydahl added a comment - we shouldn't have to wait for users to get screwed by bugs before we discuss if there is a better solution. Before this patch we had no safeguards whatsoever, but now we protect against inadvertently messing up by running as root. In my eyes that is improvement over perfection. It will always be possible for you or others to open new issues and work on improving usability even more.
          Hide
          hossman Hoss Man added a comment -

          Forgive me, I thought i mentioned this before but...

          Well, since SOLR-9547 we warn against running solr as root, so fewer users will make that mistake, and if they do, they need to -force both start and create commands.

          Except that SOLR-9547 is really just a spin off of this issue, with the same broader problem of UID mismatches between the user running the command and the owner of the files on disk – It's not really an independent reason to add -force. The same "fix" I'm suggesting here is also applicable to bin/solr start (ie: don't allow solr to start unless `whoami` matches the owner of the cores directory on the filesystem)


          ... In my eyes that is improvement over perfection. ...

          And we are both entitled to our opinions – In my eyes:

          • even if the initial bug report was specific to running as root, that is a single example of an underlying problem that causes strange behavior/errors anytime the user running the command isn't the same as the user owning the files on disk.
          • the solution(s) you committed (both here and in SOLR-9547) only address in the special case of running as root – which I view as a bandaid over the underlying problem
          • the solution you committed introduces a new "api" / feature (-force) which we are now in a position of needing to support/consider moving forward.

          ...hence my concern that on the whole, this isn't a "net" overall improvement – we've "fixed" the initial bug as reported, but not the underlying problem; and the way we've fixed it has increased the surface area of the "bin/solr command line api" in a way that I find confusing and will find hard to explain/justify to users moving forward.

          It will always be possible for you or others to open new issues and work on improving usability even more.

          Agreed, but now any such improvements in the future will be hamstrung in terms of supporting the -force option added here.

          If the only change made in this jira (and SOLR-9547) was the bandaid to fail fast when run as root – then i would 100% agree with your view that those changes are an improvement to the current situation, even if not a perfect solution to the underlying problem. But I don't personally think adding a " -force feature" like this (as a side effect of a bug fix) is a good idea until/unless it is more carefully and consistently thought out for all commands.

          Please don't think I'm trying to brow beat you into reverting this change – You stepped up to provide a fix when I and many others didn't, so I'm in no position to argue with you about it.

          If I find the time/inclination to put in the work needed to implement & test a more complete solution to the underlying problem before we release a version of Solr with -force in it, then I'll re-raise the question of whether -force is actually a good idea. Until then, i was just hoping to persuade you to voluntarily revert it – If I haven't convinced you it's a bad idea, then i haven't convinced you – and i'm ok with that.

          Show
          hossman Hoss Man added a comment - Forgive me, I thought i mentioned this before but... Well, since SOLR-9547 we warn against running solr as root, so fewer users will make that mistake, and if they do, they need to -force both start and create commands. Except that SOLR-9547 is really just a spin off of this issue, with the same broader problem of UID mismatches between the user running the command and the owner of the files on disk – It's not really an independent reason to add -force . The same "fix" I'm suggesting here is also applicable to bin/solr start (ie: don't allow solr to start unless `whoami` matches the owner of the cores directory on the filesystem) ... In my eyes that is improvement over perfection. ... And we are both entitled to our opinions – In my eyes: even if the initial bug report was specific to running as root, that is a single example of an underlying problem that causes strange behavior/errors anytime the user running the command isn't the same as the user owning the files on disk. the solution(s) you committed (both here and in SOLR-9547 ) only address in the special case of running as root – which I view as a bandaid over the underlying problem the solution you committed introduces a new "api" / feature ( -force ) which we are now in a position of needing to support/consider moving forward. ...hence my concern that on the whole, this isn't a "net" overall improvement – we've "fixed" the initial bug as reported, but not the underlying problem; and the way we've fixed it has increased the surface area of the " bin/solr command line api" in a way that I find confusing and will find hard to explain/justify to users moving forward. It will always be possible for you or others to open new issues and work on improving usability even more. Agreed, but now any such improvements in the future will be hamstrung in terms of supporting the -force option added here. If the only change made in this jira (and SOLR-9547 ) was the bandaid to fail fast when run as root – then i would 100% agree with your view that those changes are an improvement to the current situation, even if not a perfect solution to the underlying problem. But I don't personally think adding a " -force feature" like this (as a side effect of a bug fix) is a good idea until/unless it is more carefully and consistently thought out for all commands. Please don't think I'm trying to brow beat you into reverting this change – You stepped up to provide a fix when I and many others didn't, so I'm in no position to argue with you about it. If I find the time/inclination to put in the work needed to implement & test a more complete solution to the underlying problem before we release a version of Solr with -force in it, then I'll re-raise the question of whether -force is actually a good idea. Until then, i was just hoping to persuade you to voluntarily revert it – If I haven't convinced you it's a bad idea, then i haven't convinced you – and i'm ok with that.
          Hide
          janhoy Jan Høydahl added a comment -

          Experimented with Java 8's file attribute API, which works well and avoids using stat in bash. See attached SOLR-7826_sameuser.patch for a solution where we don't look for root or -force, but bail out if not same user as SOLR_HOME.

          I created a new AssertTool for this, which can also be used for other scripting, such as to assert that Solr is runing:

          usage: bin/solr assert  [-r] [-s <url>] [-u <dir>] [-x <dir>]
           -help                        Print this message
           -r,--not-root                Makes sure we are not the root user
           -s,--started <url>           Makes sure Solr is started on a certain URL
           -u,--same-user <directory>   Makes sure we run as same user that owns
                                        <directory>
           -verbose                     Generate verbose log messages
           -x,--exists <directory>      Requires directory <directory> to exist
          
          Show
          janhoy Jan Høydahl added a comment - Experimented with Java 8's file attribute API, which works well and avoids using stat in bash. See attached SOLR-7826 _sameuser.patch for a solution where we don't look for root or -force, but bail out if not same user as SOLR_HOME. I created a new AssertTool for this, which can also be used for other scripting, such as to assert that Solr is runing: usage: bin/solr assert [-r] [-s <url>] [-u <dir>] [-x <dir>] -help Print this message -r,--not-root Makes sure we are not the root user -s,--started <url> Makes sure Solr is started on a certain URL -u,--same-user <directory> Makes sure we run as same user that owns <directory> -verbose Generate verbose log messages -x,--exists <directory> Requires directory <directory> to exist
          Hide
          janhoy Jan Høydahl added a comment -

          But then should it not be allowed to create SOLR_HOME by hand as another user, and then make sure that the solr user has full access through its group memberships? Or equivalent ACL rights for Windows? Seems potentially more trappy than the root check...

          Show
          janhoy Jan Høydahl added a comment - But then should it not be allowed to create SOLR_HOME by hand as another user, and then make sure that the solr user has full access through its group memberships? Or equivalent ACL rights for Windows? Seems potentially more trappy than the root check...
          Hide
          hossman Hoss Man added a comment -

          1. I love your new AssertTool code
          2. ...

          But then should it not be allowed to create SOLR_HOME by hand as another user, and then make sure that the solr user has full access through its group memberships? Or equivalent ACL rights for Windows? Seems potentially more trappy than the root check...

          That's a good point ... I feel like enforcing that the same user be used every where is the lesser of the evils – but only if we had been doing that since day #1 in bin/solr. If we start enforcing that now that might screw people with existing installs like you describe.

          I honestly don't know how i feel about this issue anymore.

          Maybe we should just stick with "only root is special / prohibited" behavior for now (either using the code you already committed, or your new AssertTool code) and consider more restrictive "use the same user everywhere, but -force will " let you use any user" type logic in 7.0?

          Show
          hossman Hoss Man added a comment - 1. I love your new AssertTool code 2. ... But then should it not be allowed to create SOLR_HOME by hand as another user, and then make sure that the solr user has full access through its group memberships? Or equivalent ACL rights for Windows? Seems potentially more trappy than the root check... That's a good point ... I feel like enforcing that the same user be used every where is the lesser of the evils – but only if we had been doing that since day #1 in bin/solr . If we start enforcing that now that might screw people with existing installs like you describe. I honestly don't know how i feel about this issue anymore. Maybe we should just stick with "only root is special / prohibited" behavior for now (either using the code you already committed, or your new AssertTool code) and consider more restrictive "use the same user everywhere, but -force will " let you use any user" type logic in 7.0?
          Hide
          janhoy Jan Høydahl added a comment -

          I think I'll leave things as they are for now. Same-user policy for 7.0 sounds ok. Perhaps see where the breadcrumbs effort in SOLR-9590 leads us, but that will anyway not help for manual installs.

          One idea could be to determine whether Solr has been started before or not, i.e. by looking for a file that is always created by the Solr process, such as $SOLR_LOGS_DIR/solr.log or $SOLR_HOME/<directory>/data/index, and require that user. If Solr has not been started before, let the start command succeed as any user, but test first that the user has write access to both SOLR_HOME and SOLR_LOGS_DIR?

          Show
          janhoy Jan Høydahl added a comment - I think I'll leave things as they are for now. Same-user policy for 7.0 sounds ok. Perhaps see where the breadcrumbs effort in SOLR-9590 leads us, but that will anyway not help for manual installs. One idea could be to determine whether Solr has been started before or not, i.e. by looking for a file that is always created by the Solr process, such as $SOLR_LOGS_DIR/solr.log or $SOLR_HOME/<directory>/data/index, and require that user. If Solr has not been started before, let the start command succeed as any user, but test first that the user has write access to both SOLR_HOME and SOLR_LOGS_DIR?
          Hide
          janhoy Jan Høydahl added a comment -

          Moved the AssertTool code to new issue SOLR-9610

          Show
          janhoy Jan Høydahl added a comment - Moved the AssertTool code to new issue SOLR-9610
          Hide
          shalinmangar Shalin Shekhar Mangar added a comment -

          Closing after 6.3.0 release.

          Show
          shalinmangar Shalin Shekhar Mangar added a comment - Closing after 6.3.0 release.

            People

            • Assignee:
              janhoy Jan Høydahl
              Reporter:
              elyograg Shawn Heisey
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development