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

A collections-like request handler to manage solr configurations on zookeeper

    Details

    • Type: New Feature
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.6, 7.0
    • Component/s: SolrCloud
    • Labels:
      None

      Description

      Managing Solr configuration files on zookeeper becomes cumbersome while using solr in cloud mode, especially while trying out changes in the configurations.

      It will be great if there is a request handler that can provide an API to manage the configurations similar to the collections handler that would allow actions like uploading new configurations, linking them to a collection, deleting configurations, etc.

      example :

      #use the following command to upload a new configset called mynewconf. This will fail if there is alredy a conf called 'mynewconf'. The file could be a jar , zip or a tar file which contains all the files for the this conf.
      curl -X POST -H 'Content-Type: application/octet-stream' --data-binary @testconf.zip http://localhost:8983/solr/admin/configs/mynewconf?sig=<the-signature>
      

      A GET to http://localhost:8983/solr/admin/configs will give a list of configs available
      A GET to http://localhost:8983/solr/admin/configs/mynewconf would give the list of files in mynewconf

      1. newzkconf.zip
        3 kB
        Ishan Chattopadhyaya
      2. newzkconf.zip
        2 kB
        Varun Rajput
      3. SOLR-6736.doc.patch
        3 kB
        Ishan Chattopadhyaya
      4. SOLR-6736.patch
        65 kB
        Ishan Chattopadhyaya
      5. SOLR-6736.patch
        54 kB
        Ishan Chattopadhyaya
      6. SOLR-6736.patch
        95 kB
        Ishan Chattopadhyaya
      7. SOLR-6736.patch
        40 kB
        Ishan Chattopadhyaya
      8. SOLR-6736.patch
        29 kB
        Varun Rajput
      9. SOLR-6736.patch
        29 kB
        Varun Rajput
      10. SOLR-6736.patch
        28 kB
        Varun Rajput
      11. SOLR-6736.patch
        15 kB
        Varun Rajput
      12. SOLR-6736.patch
        15 kB
        Varun Rajput
      13. SOLR-6736.patch
        9 kB
        Varun Rajput
      14. SOLR-6736.patch
        12 kB
        Varun Rajput
      15. SOLR-6736.patch
        8 kB
        Varun Rajput
      16. SOLR-6736-newapi.patch
        15 kB
        Varun Rajput
      17. SOLR-6736-newapi.patch
        15 kB
        Varun Rajput
      18. SOLR-6736-newapi.patch
        9 kB
        Varun Rajput
      19. test_private.pem
        0.7 kB
        Varun Rajput
      20. test_pub.der
        0.1 kB
        Varun Rajput
      21. zkconfighandler.zip
        3 kB
        Varun Rajput
      22. zkconfighandler.zip
        3 kB
        Varun Rajput

        Issue Links

          Activity

          Hide
          anshumg Anshum Gupta added a comment -

          Something like this:

          curl -X POST -H 'Content-Type: application/octet-stream' --data-binary @testconf.gz http://localhost:8983/solr/admin/configs?action=ADD

          The conf could be zip/gz/jar.

          Show
          anshumg Anshum Gupta added a comment - Something like this: curl -X POST -H 'Content-Type: application/octet-stream' --data-binary @testconf.gz http://localhost:8983/solr/admin/configs?action=ADD The conf could be zip/gz/jar.
          Hide
          varunrajput Varun Rajput added a comment -

          Yes, I have something similar in mind and will upload a patch soon.

          Show
          varunrajput Varun Rajput added a comment - Yes, I have something similar in mind and will upload a patch soon.
          Hide
          noble.paul Noble Paul added a comment -

          why action=ADD

          POST can implicitly mean add .

          Show
          noble.paul Noble Paul added a comment - why action=ADD POST can implicitly mean add .
          Hide
          varunrajput Varun Rajput added a comment -

          I have a piece of code which we use at Ask.com and have the approval to contribute. Currently it only uploads files/folder present on the box where the API call is made on, allows linking of collections and clearing them from zookeeper. I have attached the patch, it will be great to get some suggestions and contributions on this.

          Show
          varunrajput Varun Rajput added a comment - I have a piece of code which we use at Ask.com and have the approval to contribute. Currently it only uploads files/folder present on the box where the API call is made on, allows linking of collections and clearing them from zookeeper. I have attached the patch, it will be great to get some suggestions and contributions on this.
          Hide
          anshumg Anshum Gupta added a comment -

          Sure, makes sense.

          Show
          anshumg Anshum Gupta added a comment - Sure, makes sense.
          Hide
          noble.paul Noble Paul added a comment -

          Varun Rajput I have updated the syntax+semantics of this API. Please update your patch to reflect the description

          Show
          noble.paul Noble Paul added a comment - Varun Rajput I have updated the syntax+semantics of this API. Please update your patch to reflect the description
          Hide
          anshumg Anshum Gupta added a comment -

          A DELETE to remove the config too? Also, what I'm looking at here is supporting gz/zip/jar uploads.

          Show
          anshumg Anshum Gupta added a comment - A DELETE to remove the config too? Also, what I'm looking at here is supporting gz/zip/jar uploads.
          Hide
          varunrajput Varun Rajput added a comment -

          Thanks for the feedback, I will get to this later today

          Show
          varunrajput Varun Rajput added a comment - Thanks for the feedback, I will get to this later today
          Hide
          noble.paul Noble Paul added a comment -

          DELETE can wait, may be. That is something we can add later if required

          Show
          noble.paul Noble Paul added a comment - DELETE can wait, may be. That is something we can add later if required
          Hide
          varunrajput Varun Rajput added a comment -

          Hey Noble Paul and Anshum Gupta, I was able to get all the suggestions incorporated into the handler and have attached another patch. With the changes, we can do the following:

          1. POST a zipped file consisting of solr configuration files/folders
          Example:
          curl -X POST -H 'Content-Type: application/octet-stream' --data-binary @compressed.zip "http://localhost:8983/solr/admin/zkconfig?action=postconfig&postconfig.name=testconfig"

          2. POST a single file into a configuration
          Example:
          curl -X POST -H 'Content-Type: text/plain' --data-binary @testfile.txt "http://localhost:8983/solr/admin/zkconfig?action=postfile&postfile.configname=testconfig&postfile.filename=testfile.txt"

          3. Link an existing configuration in zookeeper to an existing collection
          Example:
          http://localhost:8983/solr/admin/zkconfig?action=linkconfig&linkconfig.name=testconfig&linkconfig.collection=testcollection

          4. Delete a configuration from zookeeper
          Example:
          http://localhost:8983/solr/admin/zkconfig?action=delete&delete.configname=testconfig

          5. Delete a file/folder from a configuration in zookeeper
          Example:
          http://localhost:8983/solr/admin/zkconfig?action=delete&delete.configname=testconfig&delete.path=lang/stopwords.txt

          Show
          varunrajput Varun Rajput added a comment - Hey Noble Paul and Anshum Gupta , I was able to get all the suggestions incorporated into the handler and have attached another patch. With the changes, we can do the following: 1. POST a zipped file consisting of solr configuration files/folders Example: curl -X POST -H 'Content-Type: application/octet-stream' --data-binary @compressed.zip "http://localhost:8983/solr/admin/zkconfig?action=postconfig&postconfig.name=testconfig" 2. POST a single file into a configuration Example: curl -X POST -H 'Content-Type: text/plain' --data-binary @testfile.txt "http://localhost:8983/solr/admin/zkconfig?action=postfile&postfile.configname=testconfig&postfile.filename=testfile.txt" 3. Link an existing configuration in zookeeper to an existing collection Example: http://localhost:8983/solr/admin/zkconfig?action=linkconfig&linkconfig.name=testconfig&linkconfig.collection=testcollection 4. Delete a configuration from zookeeper Example: http://localhost:8983/solr/admin/zkconfig?action=delete&delete.configname=testconfig 5. Delete a file/folder from a configuration in zookeeper Example: http://localhost:8983/solr/admin/zkconfig?action=delete&delete.configname=testconfig&delete.path=lang/stopwords.txt
          Hide
          noble.paul Noble Paul added a comment - - edited

          Varun Rajput The syntax followed by your patch is not as specified in the description. I see no reason to deviate from the plan. The syntax is as important as the functionality. BlobHandler.java implements a similar API

          Show
          noble.paul Noble Paul added a comment - - edited Varun Rajput The syntax followed by your patch is not as specified in the description. I see no reason to deviate from the plan. The syntax is as important as the functionality. BlobHandler.java implements a similar API
          Hide
          markrmiller@gmail.com Mark Miller added a comment -

          How does this address the security concerns raised in the issue Erick Erickson was working on to allow uploading config from the UI?

          Show
          markrmiller@gmail.com Mark Miller added a comment - How does this address the security concerns raised in the issue Erick Erickson was working on to allow uploading config from the UI?
          Hide
          erickerickson Erick Erickson added a comment -

          A bit of clarification, I'm not really actively working on that, assuming it'll all be superseded by the managed stuff, that's assigned to me to keep from losing track of it.

          But this is a very interesting point. The objection to being able to upload arbitrary XML from a client is a security vulnerability as per Uwe's comments here: https://issues.apache.org/jira/browse/SOLR-5287 (about half way down, dated 30-Nov-2013). It's not clear to me that this capability is similar, although I rather assume it is. Sorry for not bringing this up earlier.

          We need to be sure of this before committing.....

          Show
          erickerickson Erick Erickson added a comment - A bit of clarification, I'm not really actively working on that, assuming it'll all be superseded by the managed stuff, that's assigned to me to keep from losing track of it. But this is a very interesting point. The objection to being able to upload arbitrary XML from a client is a security vulnerability as per Uwe's comments here: https://issues.apache.org/jira/browse/SOLR-5287 (about half way down, dated 30-Nov-2013). It's not clear to me that this capability is similar, although I rather assume it is. Sorry for not bringing this up earlier. We need to be sure of this before committing.....
          Hide
          anshumg Anshum Gupta added a comment -

          Thanks for brining it up Mark and Erick. Here are a few things:

          1. This would not allow linking of configs to collections and only upload/replacing/deleting (may be) of configsets.
          2. Uploading a configset shouldn't be an issue unless a configset is actually used.
          3. The configs API allows, or at least is moving on the lines of being able to update the config via API.
          4. This issue doesn't involve exposing anything via the Admin UI.

          I may be missing out on something but so far, I think this is on similar lines as the config/blob storage API.

          Show
          anshumg Anshum Gupta added a comment - Thanks for brining it up Mark and Erick. Here are a few things: This would not allow linking of configs to collections and only upload/replacing/deleting (may be) of configsets. Uploading a configset shouldn't be an issue unless a configset is actually used. The configs API allows, or at least is moving on the lines of being able to update the config via API. This issue doesn't involve exposing anything via the Admin UI. I may be missing out on something but so far, I think this is on similar lines as the config/blob storage API.
          Hide
          markrmiller@gmail.com Mark Miller added a comment -

          I think this is on similar lines as the config/blob storage API.

          Maybe that's a security issue too

          Certainly this issue appears to be.

          Show
          markrmiller@gmail.com Mark Miller added a comment - I think this is on similar lines as the config/blob storage API. Maybe that's a security issue too Certainly this issue appears to be.
          Hide
          markrmiller@gmail.com Mark Miller added a comment -

          Hey Uwe Schindler - could we get your expert advice on this patch?

          Show
          markrmiller@gmail.com Mark Miller added a comment - Hey Uwe Schindler - could we get your expert advice on this patch?
          Hide
          anshumg Anshum Gupta added a comment -

          Sure, it's be good to know if this is actually a potential security issue. Also, I'm not really talking about this patch in particular but the issue and what it's trying to solve.

          Show
          anshumg Anshum Gupta added a comment - Sure, it's be good to know if this is actually a potential security issue. Also, I'm not really talking about this patch in particular but the issue and what it's trying to solve.
          Hide
          noble.paul Noble Paul added a comment -

          I think this is on similar lines as the config/blob storage API.

          That comment was about the syntax , not the functionality

          Maybe that's a security issue too

          I'm not sure why this is any more insecure than uploading stuff from ZkCli?

          I would say , we should just limit the scope of this to adding a whole config and not

          • individual files
          • or linking a collection to another config set.

          If you want those let us open another ticket . Dealing with too many moving parts is hard to track

          Show
          noble.paul Noble Paul added a comment - I think this is on similar lines as the config/blob storage API. That comment was about the syntax , not the functionality Maybe that's a security issue too I'm not sure why this is any more insecure than uploading stuff from ZkCli? I would say , we should just limit the scope of this to adding a whole config and not individual files or linking a collection to another config set. If you want those let us open another ticket . Dealing with too many moving parts is hard to track
          Hide
          varunrajput Varun Rajput added a comment -

          Thanks for clarifying that Noble Paul. This handler mimicks the functionality provided by ZkCli with the advantage of it being packaged as a handler.

          I can go ahead and modify the syntax/semantics described by Noble and remove the ability to upload individual files and linking collections once everyone is in agreement.

          Show
          varunrajput Varun Rajput added a comment - Thanks for clarifying that Noble Paul . This handler mimicks the functionality provided by ZkCli with the advantage of it being packaged as a handler. I can go ahead and modify the syntax/semantics described by Noble and remove the ability to upload individual files and linking collections once everyone is in agreement.
          Hide
          noble.paul Noble Paul added a comment -

          Varun Rajput
          We DON'T want to mimic ZkCli. We want to do better. There are various security implications we need to consider before we provide our API.

          I wish to keep this ticket simple.

          -1 for expanding the scope.
          I'm all for separate tickets if that is the case

          Show
          noble.paul Noble Paul added a comment - Varun Rajput We DON'T want to mimic ZkCli. We want to do better. There are various security implications we need to consider before we provide our API. I wish to keep this ticket simple. -1 for expanding the scope. I'm all for separate tickets if that is the case
          Hide
          varunrajput Varun Rajput added a comment -

          Hey Noble Paul, I meant to say the ability to upload configurations and not the implementation of ZkCli specifically. Doing better IS the aim in this ticket and I will try to address the security concerns if possible with your suggestions and input.

          I am not asking for expanding the scope and am in agreement with you on keeping this ticket simple.

          Show
          varunrajput Varun Rajput added a comment - Hey Noble Paul , I meant to say the ability to upload configurations and not the implementation of ZkCli specifically. Doing better IS the aim in this ticket and I will try to address the security concerns if possible with your suggestions and input. I am not asking for expanding the scope and am in agreement with you on keeping this ticket simple.
          Hide
          markrmiller@gmail.com Mark Miller added a comment -

          ZkCli is a command line tool. This is over http. A client has to be able to access Solr - it generally does this over http.

          If I sent an xslt file to the config directory without command line access and trigger code, then perhaps I can get command line access. This is a problem.

          Show
          markrmiller@gmail.com Mark Miller added a comment - ZkCli is a command line tool. This is over http. A client has to be able to access Solr - it generally does this over http. If I sent an xslt file to the config directory without command line access and trigger code, then perhaps I can get command line access. This is a problem.
          Hide
          noble.paul Noble Paul added a comment -

          f I sent an xslt file to the config directory without command line access and trigger code

          We are not planning to edit existing config set. This command will create new configsets . So if you create a new collection , you can use that.

          If I sent an xslt file to the config directory without command line access and trigger code, then perhaps I can get command line access. This is a problem.

          I didn't quite understand this .How does an xslt file get command line access?

          Show
          noble.paul Noble Paul added a comment - f I sent an xslt file to the config directory without command line access and trigger code We are not planning to edit existing config set. This command will create new configsets . So if you create a new collection , you can use that. If I sent an xslt file to the config directory without command line access and trigger code, then perhaps I can get command line access. This is a problem. I didn't quite understand this .How does an xslt file get command line access?
          Hide
          shalinmangar Shalin Shekhar Mangar added a comment -

          How does an xslt file get command line access?

          You should read through the comments on SOLR-5287 where the ability to edit configuration files was reverted because of security concerns.

          Show
          shalinmangar Shalin Shekhar Mangar added a comment - How does an xslt file get command line access? You should read through the comments on SOLR-5287 where the ability to edit configuration files was reverted because of security concerns.
          Hide
          noble.paul Noble Paul added a comment -

          We probably should use SOLR-7126 mechanism to sign the content before uploading it to ZK.

          Show
          noble.paul Noble Paul added a comment - We probably should use SOLR-7126 mechanism to sign the content before uploading it to ZK.
          Hide
          varunrajput Varun Rajput added a comment -

          That's a great suggestion, I will work on updating the patch to use this mechanism later today.

          Show
          varunrajput Varun Rajput added a comment - That's a great suggestion, I will work on updating the patch to use this mechanism later today.
          Hide
          noble.paul Noble Paul added a comment -

          Hold your horses.Wait for SOLR-7126 to be committed

          Show
          noble.paul Noble Paul added a comment - Hold your horses.Wait for SOLR-7126 to be committed
          Hide
          noble.paul Noble Paul added a comment -

          SOLR-7126 is committed

          Show
          noble.paul Noble Paul added a comment - SOLR-7126 is committed
          Hide
          varunrajput Varun Rajput added a comment -

          Great! I will work on making use of it, thanks!

          Show
          varunrajput Varun Rajput added a comment - Great! I will work on making use of it, thanks!
          Hide
          noble.paul Noble Paul added a comment - - edited

          Please keep in mind that you MUST have atleast public key under /keys/conf for the call to succeed. There should not be any option to do it without authentication

          Show
          noble.paul Noble Paul added a comment - - edited Please keep in mind that you MUST have atleast public key under /keys/conf for the call to succeed. There should not be any option to do it without authentication
          Hide
          shaie Shai Erera added a comment -

          On the REST API side, can we make it more REST-like? For example, instead of carrying an action parameter, we could embed that in the URL, e.g. /admin/zkconfig/config or /admin/zkconfig/file. Then the requests would look like:

          1. POST a zipped file consisting of solr configuration files/folders
          Example:
          curl -X POST -H 'Content-Type: application/octet-stream' --data-binary @compressed.zip "http://localhost:8983/solr/admin/zkconfig/config?config.name=testconfig"

          2. POST a single file into a configuration
          Example:
          curl -X POST -H 'Content-Type: text/plain' --data-binary @testfile.txt "http://localhost:8983/solr/admin/zkconfig/file/testconfig?filename=testfile.txt"

          3. Link an existing configuration in zookeeper to an existing collection
          Example:
          curl -X PUT http://localhost:8983/solr/admin/zkconfig/link/testcollection/testconfig
          Here we could have few variants, e.g. ending w/ /zkconfig/link and taking the collection and configuration name as parameters, or only one of them.

          4. Delete a configuration from zookeeper
          Example:
          curl -X DELETE http://localhost:8983/solr/admin/zkconfig/config/testconfig

          5. Delete a file/folder from a configuration in zookeeper
          Example:
          curl -X DELETE http://localhost:8983/solr/admin/zkconfig/file/testconfig?filename=lang/stopwords.txt

          What I am after is for consistency between PUT/POST/GET/DELETE commands such that if you handle a file configuration, the request looks the same (or as much as possible) between the different commands, and we let REST determine the action we should carry. For instance, deleting a file or adding a file should take, IMO, the same parameters (and not 'path' when deleting and 'filename' when posting), and we let the PUT/POST/DELETE/GET determine what should we do with the request.

          My 2 cents.

          Show
          shaie Shai Erera added a comment - On the REST API side, can we make it more REST-like? For example, instead of carrying an action parameter, we could embed that in the URL, e.g. /admin/zkconfig/config or /admin/zkconfig/file . Then the requests would look like: 1. POST a zipped file consisting of solr configuration files/folders Example: curl -X POST -H 'Content-Type: application/octet-stream' --data-binary @compressed.zip "http://localhost:8983/solr/admin/zkconfig/config?config.name=testconfig" 2. POST a single file into a configuration Example: curl -X POST -H 'Content-Type: text/plain' --data-binary @testfile.txt "http://localhost:8983/solr/admin/zkconfig/file/testconfig?filename=testfile.txt" 3. Link an existing configuration in zookeeper to an existing collection Example: curl -X PUT http://localhost:8983/solr/admin/zkconfig/link/testcollection/testconfig Here we could have few variants, e.g. ending w/ /zkconfig/link and taking the collection and configuration name as parameters, or only one of them. 4. Delete a configuration from zookeeper Example: curl -X DELETE http://localhost:8983/solr/admin/zkconfig/config/testconfig 5. Delete a file/folder from a configuration in zookeeper Example: curl -X DELETE http://localhost:8983/solr/admin/zkconfig/file/testconfig?filename=lang/stopwords.txt What I am after is for consistency between PUT/POST/GET/DELETE commands such that if you handle a file configuration, the request looks the same (or as much as possible) between the different commands, and we let REST determine the action we should carry. For instance, deleting a file or adding a file should take, IMO, the same parameters (and not 'path' when deleting and 'filename' when posting), and we let the PUT/POST/DELETE/GET determine what should we do with the request. My 2 cents.
          Hide
          noble.paul Noble Paul added a comment -

          Shai Erera it is already made rest like if you look at the description. I'm tempted to just restrict this to support just upload of a configset because we have not yet assessed the security implications of these and the implications of changing config of a running collection.

          Show
          noble.paul Noble Paul added a comment - Shai Erera it is already made rest like if you look at the description. I'm tempted to just restrict this to support just upload of a configset because we have not yet assessed the security implications of these and the implications of changing config of a running collection.
          Hide
          shaie Shai Erera added a comment -

          Oh OK, I was referring to the comment with all those 5 examples, didn't notice the description includes different examples. If that's the case, that's much better!

          Show
          shaie Shai Erera added a comment - Oh OK, I was referring to the comment with all those 5 examples, didn't notice the description includes different examples. If that's the case, that's much better!
          Hide
          varunrajput Varun Rajput added a comment - - edited

          Hey guys,

          I have an initial patch with the changes to make use of SOLR-7126 as suggested by Noble Paul for signing the config set to be uploaded to zookeeper, without which the upload will be rejected. The API is also updated to reflect the syntax in the description and the scope narrowed to just push a config set as discussed.

          Please review this and give me your feedback. I am yet to add test cases in this patch, so pointers to existing ones for similar scenarios will be great.

          Thanks,
          Varun

          Show
          varunrajput Varun Rajput added a comment - - edited Hey guys, I have an initial patch with the changes to make use of SOLR-7126 as suggested by Noble Paul for signing the config set to be uploaded to zookeeper, without which the upload will be rejected. The API is also updated to reflect the syntax in the description and the scope narrowed to just push a config set as discussed. Please review this and give me your feedback. I am yet to add test cases in this patch, so pointers to existing ones for similar scenarios will be great. Thanks, Varun
          Hide
          noble.paul Noble Paul added a comment -

          do you really need to get direct access to httprequest. It is totally avoidable

           HttpServletRequest httpServletRequest = (HttpServletRequest) req
                      .getContext().get("httpRequest");
                  if (httpServletRequest == null) {
                    rsp.add("error", "No HttpServletRequest found. Set the \"addHttpRequestToContext\" attribute"
                            + "in the \"requestParsers\" to true in the solrconfig.xml");
                    return;
          

          why does a CoreContainer level handler need a core? It is asking for a NullPointerException

          CoreContainer coreContainer = req.getCore().getCoreDescriptor()
                  .getCoreContainer();
          
          Show
          noble.paul Noble Paul added a comment - do you really need to get direct access to httprequest. It is totally avoidable HttpServletRequest httpServletRequest = (HttpServletRequest) req .getContext().get("httpRequest"); if (httpServletRequest == null) { rsp.add("error", "No HttpServletRequest found. Set the \"addHttpRequestToContext\" attribute" + "in the \"requestParsers\" to true in the solrconfig.xml"); return; why does a CoreContainer level handler need a core? It is asking for a NullPointerException CoreContainer coreContainer = req.getCore().getCoreDescriptor() .getCoreContainer();
          Hide
          varunrajput Varun Rajput added a comment -

          Thanks for the feedback Noble Paul. I have updated the patch to use the content streams provided by the request as used in the BlobHandler. Also, I noticed that the AdminHandler is deprecated, so, while addressing the NullPointerException in getting the core, I have also included this handler in the CoreContainer and made changes to NodeConfig to similarly reflect the usage of the CollectionsHandler.

          Show
          varunrajput Varun Rajput added a comment - Thanks for the feedback Noble Paul . I have updated the patch to use the content streams provided by the request as used in the BlobHandler. Also, I noticed that the AdminHandler is deprecated, so, while addressing the NullPointerException in getting the core, I have also included this handler in the CoreContainer and made changes to NodeConfig to similarly reflect the usage of the CollectionsHandler.
          Hide
          noble.paul Noble Paul added a comment -

          There is no testcase yet

          Show
          noble.paul Noble Paul added a comment - There is no testcase yet
          Hide
          varunrajput Varun Rajput added a comment -

          Hi Noble Paul, I've written up tests for most part but I am facing an issue. Attached is the patch and a zip whose contents should be in the "zkconfighandler" folder under test-files. Maybe you can help me out in this:

          In the last part of the test, I am able to test the authenticity of the signed zip but cannot verify the upload of files within the zip. With some debug, I noticed I couldnt reach within the zipentry loop while reading the content stream.

          Also, I had to use "name" param for the config because when I try using it in the path, the solr dispatch filter throws an exception saying not found.

          Any help will be great, thanks!

          Show
          varunrajput Varun Rajput added a comment - Hi Noble Paul , I've written up tests for most part but I am facing an issue. Attached is the patch and a zip whose contents should be in the "zkconfighandler" folder under test-files. Maybe you can help me out in this: In the last part of the test, I am able to test the authenticity of the signed zip but cannot verify the upload of files within the zip. With some debug, I noticed I couldnt reach within the zipentry loop while reading the content stream. Also, I had to use "name" param for the config because when I try using it in the path, the solr dispatch filter throws an exception saying not found. Any help will be great, thanks!
          Hide
          shaie Shai Erera added a comment -

          I'm tempted to just restrict this to support just upload of a configset because we have not yet assessed the security implications of these and the implications of changing config of a running collection

          Noble Paul, I've read the discussion on SOLR-5287 and was wondering if you can explain why limiting this API to only uploading a configset addresses any of the security vulnerabilities? The configset may include anything that I want, including XSLT files which may be able to hamper the system, correct?

          Is it only because we cannot associate a configset with a collection when we issue a /collections?action=CREATE command that you consider it safe? I.e. the configset will exist in ZK, but not really used? If so, why enabling this at all?

          Show
          shaie Shai Erera added a comment - I'm tempted to just restrict this to support just upload of a configset because we have not yet assessed the security implications of these and the implications of changing config of a running collection Noble Paul , I've read the discussion on SOLR-5287 and was wondering if you can explain why limiting this API to only uploading a configset addresses any of the security vulnerabilities? The configset may include anything that I want, including XSLT files which may be able to hamper the system, correct? Is it only because we cannot associate a configset with a collection when we issue a /collections?action=CREATE command that you consider it safe? I.e. the configset will exist in ZK, but not really used? If so, why enabling this at all?
          Hide
          noble.paul Noble Paul added a comment -

          This does more. It is done securely.

          Show
          noble.paul Noble Paul added a comment - This does more. It is done securely.
          Hide
          anshumg Anshum Gupta added a comment -

          I think it would make sense to hold this until SOLR-7274 and SOLR-7275 (more importantly) get committed. I'm working on security in solr and I think I should be done with it in time for 5.2.

          Show
          anshumg Anshum Gupta added a comment - I think it would make sense to hold this until SOLR-7274 and SOLR-7275 (more importantly) get committed. I'm working on security in solr and I think I should be done with it in time for 5.2.
          Hide
          varunrajput Varun Rajput added a comment -

          Hey Guys,
          I've been trying to work through the issue I am facing and am kind of stuck. When I stream a text or xml file, I am able to get the file and read it's contents from the request's content stream but the same is not working out for zip files. Am I doing it wrong if I am doing it like this:

          for (ContentStream contentStream : req.getContentStreams()) {
          InputStream inputStream = contentStream.getStream();
          ZipInputStream zis = new ZipInputStream(inputStream, StandardCharsets.UTF_8);
          ...
          break;
          }
          

          Is there an issue or a different way to deal with Archived file formats?

          Show
          varunrajput Varun Rajput added a comment - Hey Guys, I've been trying to work through the issue I am facing and am kind of stuck. When I stream a text or xml file, I am able to get the file and read it's contents from the request's content stream but the same is not working out for zip files. Am I doing it wrong if I am doing it like this: for (ContentStream contentStream : req.getContentStreams()) { InputStream inputStream = contentStream.getStream(); ZipInputStream zis = new ZipInputStream(inputStream, StandardCharsets.UTF_8); ... break ; } Is there an issue or a different way to deal with Archived file formats?
          Hide
          noble.paul Noble Paul added a comment -

          The blobhandler does exactly the same. However, you just post the patch in whatever gotten you have I shall try to debug

          Show
          noble.paul Noble Paul added a comment - The blobhandler does exactly the same. However, you just post the patch in whatever gotten you have I shall try to debug
          Hide
          varunrajput Varun Rajput added a comment -

          Hey Noble Paul, I even tried doing it the way BlobHandler does it by using the SimplePostTool to get the ByteBuffer and use that to create the ZipInputStream but that doesn't work either.

          I am attaching the patch containing the test case where all of the tests are working fine including the authentication using the keys except for the last 2 assertions which check for presence of files which should have been received from the zip being streamed. Also attached is a zip file whose contents should be put in "solr/core/src/test-files/zkconfighandler" folder.

          Thanks in advance for your help!

          Show
          varunrajput Varun Rajput added a comment - Hey Noble Paul , I even tried doing it the way BlobHandler does it by using the SimplePostTool to get the ByteBuffer and use that to create the ZipInputStream but that doesn't work either. I am attaching the patch containing the test case where all of the tests are working fine including the authentication using the keys except for the last 2 assertions which check for presence of files which should have been received from the zip being streamed. Also attached is a zip file whose contents should be put in "solr/core/src/test-files/zkconfighandler" folder. Thanks in advance for your help!
          Hide
          noble.paul Noble Paul added a comment -
          java.lang.RuntimeException: Cannot find resource in classpath or in file-system (relative to CWD): zkconfighandler/test_pub.der
          	at __randomizedtesting.SeedInfo.seed([9C0000C0E86607B4:14543F1A469A6A4C]:0)
          	at org.apache.solr.SolrTestCaseJ4.getFile(SolrTestCaseJ4.java:1763)
          	at org.apache.solr.handler.admin.ZkConfigHandlerTest.readFile(ZkConfigHandlerTest.java:260)
          	at org.apache.solr.handler.admin.ZkConfigHandlerTest.setupKeysInZk(ZkConfigHandlerTest.java:221)
          	at org.apache.solr.handler.admin.ZkConfigHandlerTest.test(ZkConfigHandlerTest.java:59)
          	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
          	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
          

          where is the .DER file? .pem file does not work for java

          Show
          noble.paul Noble Paul added a comment - java.lang.RuntimeException: Cannot find resource in classpath or in file-system (relative to CWD): zkconfighandler/test_pub.der at __randomizedtesting.SeedInfo.seed([9C0000C0E86607B4:14543F1A469A6A4C]:0) at org.apache.solr.SolrTestCaseJ4.getFile(SolrTestCaseJ4.java:1763) at org.apache.solr.handler.admin.ZkConfigHandlerTest.readFile(ZkConfigHandlerTest.java:260) at org.apache.solr.handler.admin.ZkConfigHandlerTest.setupKeysInZk(ZkConfigHandlerTest.java:221) at org.apache.solr.handler.admin.ZkConfigHandlerTest.test(ZkConfigHandlerTest.java:59) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) where is the .DER file? .pem file does not work for java
          Hide
          varunrajput Varun Rajput added a comment -

          Hi Noble Paul,

          The three files, [ newzkconf.zip, test_private.pem, test_pub.der ] are zipped into the zkconfighandler.zip which I had attached in my last comment. We need to create a folder "zkconfighandler" under the test-files directory and put these files under that location (solr/core/src/test-files/zkconfighandler).

          I am attaching each of those 3 files individually here as well.

          Show
          varunrajput Varun Rajput added a comment - Hi Noble Paul , The three files, [ newzkconf.zip, test_private.pem, test_pub.der ] are zipped into the zkconfighandler.zip which I had attached in my last comment. We need to create a folder "zkconfighandler" under the test-files directory and put these files under that location (solr/core/src/test-files/zkconfighandler). I am attaching each of those 3 files individually here as well.
          Hide
          noble.paul Noble Paul added a comment -

          We need a change of plan right now.

          The problem I see is that this handler is enabled by default right now. That means anyone who starts a cluster now will have this feature enabled. I propose a combination of a system property and the new authentication system for security

          1) A node must be started with -Denable.zk.write.handler=true for it to enable this handler
          2) The default security model will kick in automatically if security is enabled . So, we may not need to sign the payload

          Show
          noble.paul Noble Paul added a comment - We need a change of plan right now. The problem I see is that this handler is enabled by default right now. That means anyone who starts a cluster now will have this feature enabled. I propose a combination of a system property and the new authentication system for security 1) A node must be started with -Denable.zk.write.handler=true for it to enable this handler 2) The default security model will kick in automatically if security is enabled . So, we may not need to sign the payload
          Hide
          anshumg Anshum Gupta added a comment -

          I propose a combination of a system property and the new authentication system for security

          +1 on System property. The choice of using the authentication mechanism should be optional though. We need to make sure of the following:

          • Disable this handler by default.
          • Enabling this should be a bootstrap/system prop instead of an API call, else it's not secure enough.
            The above approach seems in synch with both of those.

          -Denable.zk.write.handler=true

          Something like a zk.(remote)upload sounds better in my opinion.

          Show
          anshumg Anshum Gupta added a comment - I propose a combination of a system property and the new authentication system for security +1 on System property. The choice of using the authentication mechanism should be optional though. We need to make sure of the following: Disable this handler by default. Enabling this should be a bootstrap/system prop instead of an API call, else it's not secure enough. The above approach seems in synch with both of those. -Denable.zk.write.handler=true Something like a zk.(remote)upload sounds better in my opinion .
          Hide
          varunrajput Varun Rajput added a comment - - edited

          That's a good point, I agree. So, assuming I understood this correctly, I have made the changes to enable the handler only if the system property is set and removed the signature check assuming the default security model with do the job.

          After making the above changes, I verified that all the tests work fine. I was able to verify the presence of files from the zip in zookeeper as well as verify their actual content. I am attaching the patch here again with the changes.

          The "solr/core/src/test-files/zkconfighandler" folder should now contain the newzkconf.zip (attached previously) and the two xml files (present in the patch).

          Thanks for your feedback and help!

          Show
          varunrajput Varun Rajput added a comment - - edited That's a good point, I agree. So, assuming I understood this correctly, I have made the changes to enable the handler only if the system property is set and removed the signature check assuming the default security model with do the job. After making the above changes, I verified that all the tests work fine. I was able to verify the presence of files from the zip in zookeeper as well as verify their actual content. I am attaching the patch here again with the changes. The "solr/core/src/test-files/zkconfighandler" folder should now contain the newzkconf.zip (attached previously) and the two xml files (present in the patch). Thanks for your feedback and help!
          Hide
          varunrajput Varun Rajput added a comment -

          If we go with the name of the handler, I would say we use

           enable.zk.config.handler 
          Show
          varunrajput Varun Rajput added a comment - If we go with the name of the handler, I would say we use enable.zk.config.handler
          Hide
          varunrajput Varun Rajput added a comment -

          Hey Anshum Gupta and Noble Paul, are we waiting on anything more? I see the ticket was updated by Anshum, what are the next steps?

          Show
          varunrajput Varun Rajput added a comment - Hey Anshum Gupta and Noble Paul , are we waiting on anything more? I see the ticket was updated by Anshum, what are the next steps?
          Hide
          noble.paul Noble Paul added a comment -

          Do we have a working patch which with the new recommendations?

          Show
          noble.paul Noble Paul added a comment - Do we have a working patch which with the new recommendations?
          Hide
          varunrajput Varun Rajput added a comment -

          Yes, I had incorporated all the suggested changes as recommended in the last working patch that I uploaded. The handler is disabled by default and can be enabled by setting the system property as recommended by Anshum Gupta.

          As said earlier, I verified that all the tests work fine and was also able to verify the presence of files from the zip in zookeeper as well as verify their actual content.

          Also, according to Anshum's opinion, I am updating the name of the system property as well, to

          zk.upload

          and attaching a new patch. Although this can be easily changed as desired in ZkConfigParams.

          Show
          varunrajput Varun Rajput added a comment - Yes, I had incorporated all the suggested changes as recommended in the last working patch that I uploaded. The handler is disabled by default and can be enabled by setting the system property as recommended by Anshum Gupta . As said earlier, I verified that all the tests work fine and was also able to verify the presence of files from the zip in zookeeper as well as verify their actual content. Also, according to Anshum's opinion, I am updating the name of the system property as well, to zk.upload and attaching a new patch. Although this can be easily changed as desired in ZkConfigParams.
          Hide
          noble.paul Noble Paul added a comment -

          Thanks. I shall look into it

          Show
          noble.paul Noble Paul added a comment - Thanks. I shall look into it
          Hide
          erickerickson Erick Erickson added a comment -

          This keeps recurring and recurring and recurring. I wonder if a graphical UI exists that we could reference, seems like it would be isolated enough.

          Show
          erickerickson Erick Erickson added a comment - This keeps recurring and recurring and recurring. I wonder if a graphical UI exists that we could reference, seems like it would be isolated enough.
          Hide
          varunrajput Varun Rajput added a comment -

          Hi Erick, this is a handler like the Collections handler. A GUI can be built on top of this API as wells as the Collections API but I don't think it should be within the scope of this ticket because we had planned to keep this ticket simple as suggested by Noble Paul.

          Show
          varunrajput Varun Rajput added a comment - Hi Erick, this is a handler like the Collections handler. A GUI can be built on top of this API as wells as the Collections API but I don't think it should be within the scope of this ticket because we had planned to keep this ticket simple as suggested by Noble Paul .
          Hide
          erickerickson Erick Erickson added a comment -

          Varun Rajput Agreed, but we can't think about building up a GUI unless we agree that putting up arbitrary XML files is allowed. Stefan and I had the "brilliant" idea to edit solrconfig.xml files from the admin UI. Even that was unacceptable as it opened up the ability to push xml to Solr..... well, maybe not-so-brilliant.

          Show
          erickerickson Erick Erickson added a comment - Varun Rajput Agreed, but we can't think about building up a GUI unless we agree that putting up arbitrary XML files is allowed. Stefan and I had the "brilliant" idea to edit solrconfig.xml files from the admin UI. Even that was unacceptable as it opened up the ability to push xml to Solr..... well, maybe not-so-brilliant.
          Hide
          varunrajput Varun Rajput added a comment -

          I see. It will definitely be great to have a UI on top of this and in terms of the arbitrary XML files, I will let Anshum Gupta comment on that given he's worked on the authentication and authorization modules.

          Show
          varunrajput Varun Rajput added a comment - I see. It will definitely be great to have a UI on top of this and in terms of the arbitrary XML files, I will let Anshum Gupta comment on that given he's worked on the authentication and authorization modules.
          Hide
          noble.paul Noble Paul added a comment -

          Varun Rajput Agreed, but we can't think about building up a GUI unless we agree that putting up arbitrary XML files is allowed

          The fact is it is your system and you can put an arbitrary xml or even executable file if you wish to. But it should only be allowed to a person who has the permissions to do so. Building a GUI first is like putting the cart before the horse. We need to first define the workflow involved in adding a certain artifact into the system. The questions we need to ask is

          1. Is the user allowed to add it?
          2. How can we avoid/minimize the harm caused by human errors even if you are authorized to perform a certain action
          3. What are the steps involved in a person gaining those permissions. Is it possible to circumvent it.

          It may be possible to hack this and gain access. But, we do not want Solr to be weakest link in the whole ecosystem. For instance, in this ticket we say that that, the user needs to enable this handler with a system property. Which means that the hacker will have to gain access to the file system first to put in the property there.

          Show
          noble.paul Noble Paul added a comment - Varun Rajput Agreed, but we can't think about building up a GUI unless we agree that putting up arbitrary XML files is allowed The fact is it is your system and you can put an arbitrary xml or even executable file if you wish to. But it should only be allowed to a person who has the permissions to do so. Building a GUI first is like putting the cart before the horse. We need to first define the workflow involved in adding a certain artifact into the system. The questions we need to ask is Is the user allowed to add it? How can we avoid/minimize the harm caused by human errors even if you are authorized to perform a certain action What are the steps involved in a person gaining those permissions. Is it possible to circumvent it. It may be possible to hack this and gain access. But, we do not want Solr to be weakest link in the whole ecosystem. For instance, in this ticket we say that that, the user needs to enable this handler with a system property. Which means that the hacker will have to gain access to the file system first to put in the property there.
          Hide
          varunrajput Varun Rajput added a comment -

          To add to what Noble Paul said, Anshum Gupta has added security modules that can restrict access to users and what they can do with Solr. Noble Paul, did you get a chance to verify the patch?

          Show
          varunrajput Varun Rajput added a comment - To add to what Noble Paul said, Anshum Gupta has added security modules that can restrict access to users and what they can do with Solr. Noble Paul , did you get a chance to verify the patch?
          Hide
          thelabdude Timothy Potter added a comment -

          Just tuning in to this one ... what about adding individual files after a configset has been created? Is that envisioned for this API? If so, will that use the signing stuff of SOLR-7126? Personally, I think it's a pain to reload the whole config directory as a zip if I want to change one word in my protected words file for example.

          Show
          thelabdude Timothy Potter added a comment - Just tuning in to this one ... what about adding individual files after a configset has been created? Is that envisioned for this API? If so, will that use the signing stuff of SOLR-7126 ? Personally, I think it's a pain to reload the whole config directory as a zip if I want to change one word in my protected words file for example.
          Hide
          varunrajput Varun Rajput added a comment -

          Hi Timothy Potter, the vision for this API is to keep it simple to begin with, as suggested by Noble Paul. A had a version which allows a single file upload as well which can be added later.

          Show
          varunrajput Varun Rajput added a comment - Hi Timothy Potter , the vision for this API is to keep it simple to begin with, as suggested by Noble Paul . A had a version which allows a single file upload as well which can be added later.
          Hide
          thelabdude Timothy Potter added a comment -

          I think not having a way to upload a single file makes the API harder, not easier. Very often, people just want to change a single file in the config and repackaging the whole directory to re-upload is cumbersome. I'd vote for tackling both (full dir and single file)

          Show
          thelabdude Timothy Potter added a comment - I think not having a way to upload a single file makes the API harder, not easier. Very often, people just want to change a single file in the config and repackaging the whole directory to re-upload is cumbersome. I'd vote for tackling both (full dir and single file)
          Hide
          varunrajput Varun Rajput added a comment -

          I can understand the pain to reload the whole configuration as I've been through that. We can open another ticket for that as Noble Paul said in his comment:
          "I would say , we should just limit the scope of this to adding a whole config and not
          individual files
          or linking a collection to another config set.
          If you want those let us open another ticket . Dealing with too many moving parts is hard to track"

          If we have a consensus amongst all, I can easily include the functionality to upload a single file as well to this one.

          Show
          varunrajput Varun Rajput added a comment - I can understand the pain to reload the whole configuration as I've been through that. We can open another ticket for that as Noble Paul said in his comment: "I would say , we should just limit the scope of this to adding a whole config and not individual files or linking a collection to another config set. If you want those let us open another ticket . Dealing with too many moving parts is hard to track" If we have a consensus amongst all, I can easily include the functionality to upload a single file as well to this one.
          Hide
          anshumg Anshum Gupta added a comment -

          Thanks Varun. I've been wanting to get to this for a while now. Also, thanks for being patient

          A couple of things here. I agree with Tim Potter that handling both single file updates and uploading an entire config set both are required and make sense. However, we could just handle entire configsets with this issue as long as the approach allows for adding single file uploads cleanly.

          Also, let's get this patch inline with SOLR-7789 so we share the end-point that has already been introduced to handle configs. I plan to review this patch in detail later today.

          Show
          anshumg Anshum Gupta added a comment - Thanks Varun. I've been wanting to get to this for a while now. Also, thanks for being patient A couple of things here. I agree with Tim Potter that handling both single file updates and uploading an entire config set both are required and make sense. However, we could just handle entire configsets with this issue as long as the approach allows for adding single file uploads cleanly. Also, let's get this patch inline with SOLR-7789 so we share the end-point that has already been introduced to handle configs. I plan to review this patch in detail later today.
          Hide
          gchanan Gregory Chanan added a comment -

          Also, let's get this patch inline with SOLR-7789 so we share the end-point that has already been introduced to handle configs. I plan to review this patch in detail later today.

          What's the status of this?

          I'm also unclear what the exact scope of this JIRA is. It sounds like we are just limiting this jira to allowing you to upload entire config sets, is that correct? I'm adding a LIST command in SOLR-7995 and a GET command at SOLR-8054. It sounds like neither of those efforts would conflict with this, except that the GET command should use the same parameter names for e.g. individual files / zipped vs streamed. I'd rather not duplicate work, so let me know.

          Show
          gchanan Gregory Chanan added a comment - Also, let's get this patch inline with SOLR-7789 so we share the end-point that has already been introduced to handle configs. I plan to review this patch in detail later today. What's the status of this? I'm also unclear what the exact scope of this JIRA is. It sounds like we are just limiting this jira to allowing you to upload entire config sets, is that correct? I'm adding a LIST command in SOLR-7995 and a GET command at SOLR-8054 . It sounds like neither of those efforts would conflict with this, except that the GET command should use the same parameter names for e.g. individual files / zipped vs streamed. I'd rather not duplicate work, so let me know.
          Hide
          anshumg Anshum Gupta added a comment -

          I plan on resuming work on this one later today. If you have something in place already for GET, go ahead with it, else I'll take it up and keep it in sync with all of the other Config set work that has recently been done.

          Let's move all of the GET stuff from here and only use SOLR-8054 for it.

          Show
          anshumg Anshum Gupta added a comment - I plan on resuming work on this one later today. If you have something in place already for GET, go ahead with it, else I'll take it up and keep it in sync with all of the other Config set work that has recently been done. Let's move all of the GET stuff from here and only use SOLR-8054 for it.
          Hide
          varunrajput Varun Rajput added a comment -

          Hey Anshum Gupta, I was busy with stuff so I couldn't get to this before but I am uploading a new patch using the work already done in https://issues.apache.org/jira/browse/SOLR-7789 as suggested by you. Bringing this functionality along the lines of SOLR-7789 needed quite a few changes so this patch is only an initial draft. I would first like to get an opinion from you and Gregory Chanan if this looks good and I can add tests as well.

          Show
          varunrajput Varun Rajput added a comment - Hey Anshum Gupta , I was busy with stuff so I couldn't get to this before but I am uploading a new patch using the work already done in https://issues.apache.org/jira/browse/SOLR-7789 as suggested by you. Bringing this functionality along the lines of SOLR-7789 needed quite a few changes so this patch is only an initial draft. I would first like to get an opinion from you and Gregory Chanan if this looks good and I can add tests as well.
          Hide
          varunrajput Varun Rajput added a comment -

          Uploading an updated patch with the tests using the new API Anshum Gupta

          Show
          varunrajput Varun Rajput added a comment - Uploading an updated patch with the tests using the new API Anshum Gupta
          Hide
          gchanan Gregory Chanan added a comment -

          The approach of doing the upload directly in the handler seems problematic because it doesn't involve the exclusivity enforcement in the Overseer. Are you sure if you do things like concurrently UPLOAD and DELETE things will be left in a sensible state? I guess the alternative is writing the zip into the zookeeper message (could that overrun the jute.maxbuffer) or uploading it someone else in ZK and having the Overseer copy it to the correct location. The later means you have to upload twice, but I'm guessing this operation is rare anyway.

          I also don't see any signature checking for security. I don't see how we can have this feature without security.

          More minor comments:

          +      if (action == ConfigSetAction.UPLOAD) {
          +        if (isUploadEnabled) {
          +          hanldeConfigUploadRequest(req, rsp);
          +          return;
          +        } else {
          +          throw new SolrException(SolrException.ErrorCode.UNAUTHORIZED, 
          +              "Uploads are not enabled. Please set the system property \"" 
          +                  + ConfigSetParams.ENABLE_CONFIGSET_UPLOAD + "\" to true");
          +        }
          +      }
          

          I don't see why this can't follow the usual operation.call path. Also handle is not spelled correctly.

          +    String httpMethod = (String) req.getContext().get("httpMethod");
          +    if (!"POST".equals(httpMethod)) {
          +      throw new SolrException(ErrorCode.BAD_REQUEST,
          +          "The upload action supports POST requests only");
          +    }
          

          If we are going to check this stuff, I'd rather enforce it for all ConfigSet requests. This can be done by storing the allowed verbs in the ConfigSetAction enum itself or somewhere in the handler.

          +    if (zkClient.exists(configPathInZk, true)) {
          +      throw new SolrException(ErrorCode.SERVER_ERROR,
          +          "The configuration " + configSetName + " already exists in zookeeper");
          +    }
          +    
          

          This looks like it should be a BAD_REQUEST?

          +    for (ContentStream contentStream : req.getContentStreams()) {
                ..
          +      break;
          +    }
          

          This just reads the first content stream? Why have a loop then?

          How about solrj classes for the requests/responses that you can use in the test, instead of hand parsing everything?

          +    //Checking error when mo configuration name is specified in request
          

          mo -> no

          Show
          gchanan Gregory Chanan added a comment - The approach of doing the upload directly in the handler seems problematic because it doesn't involve the exclusivity enforcement in the Overseer. Are you sure if you do things like concurrently UPLOAD and DELETE things will be left in a sensible state? I guess the alternative is writing the zip into the zookeeper message (could that overrun the jute.maxbuffer) or uploading it someone else in ZK and having the Overseer copy it to the correct location. The later means you have to upload twice, but I'm guessing this operation is rare anyway. I also don't see any signature checking for security. I don't see how we can have this feature without security. More minor comments: + if (action == ConfigSetAction.UPLOAD) { + if (isUploadEnabled) { + hanldeConfigUploadRequest(req, rsp); + return ; + } else { + throw new SolrException(SolrException.ErrorCode.UNAUTHORIZED, + "Uploads are not enabled. Please set the system property \" " + + ConfigSetParams.ENABLE_CONFIGSET_UPLOAD + "\" to true "); + } + } I don't see why this can't follow the usual operation.call path. Also handle is not spelled correctly. + String httpMethod = ( String ) req.getContext().get( "httpMethod" ); + if (! "POST" .equals(httpMethod)) { + throw new SolrException(ErrorCode.BAD_REQUEST, + "The upload action supports POST requests only" ); + } If we are going to check this stuff, I'd rather enforce it for all ConfigSet requests. This can be done by storing the allowed verbs in the ConfigSetAction enum itself or somewhere in the handler. + if (zkClient.exists(configPathInZk, true )) { + throw new SolrException(ErrorCode.SERVER_ERROR, + "The configuration " + configSetName + " already exists in zookeeper" ); + } + This looks like it should be a BAD_REQUEST? + for (ContentStream contentStream : req.getContentStreams()) { .. + break ; + } This just reads the first content stream? Why have a loop then? How about solrj classes for the requests/responses that you can use in the test, instead of hand parsing everything? + //Checking error when mo configuration name is specified in request mo -> no
          Hide
          varunrajput Varun Rajput added a comment -

          Thanks Gregory Chanan for the detailed review. Please also refer to my previous patch (https://issues.apache.org/jira/secure/attachment/12768656/SOLR-6736-newapi.patch) in which I did go the route of using Overseer but passing the content stream in the zookeeper message didn't work out as it got converted into a string. Maybe there is a better way of doing that which didn't occur to me. I can take your help in going that route or an alternate one.

          As for the security, this is implemented using a system property which needs to be set before starting up solr, as suggested by a few members in this ticket. The flag "isUploadEnabled" checks if uploading configsets is enabled.

             public ConfigSetsHandler(final CoreContainer coreContainer) {
               this.coreContainer = coreContainer;
          +    isUploadEnabled = Boolean.parseBoolean(System.getProperty(
          +        ConfigSetParams.ENABLE_CONFIGSET_UPLOAD, ConfigSetParams.ENABLE_CONFIGSET_UPLOAD_DEFAULT));
             }
          

          I will take care of the minor corrections, thanks for pointing them out!

          Show
          varunrajput Varun Rajput added a comment - Thanks Gregory Chanan for the detailed review. Please also refer to my previous patch ( https://issues.apache.org/jira/secure/attachment/12768656/SOLR-6736-newapi.patch ) in which I did go the route of using Overseer but passing the content stream in the zookeeper message didn't work out as it got converted into a string. Maybe there is a better way of doing that which didn't occur to me. I can take your help in going that route or an alternate one. As for the security, this is implemented using a system property which needs to be set before starting up solr, as suggested by a few members in this ticket. The flag "isUploadEnabled" checks if uploading configsets is enabled. public ConfigSetsHandler( final CoreContainer coreContainer) { this .coreContainer = coreContainer; + isUploadEnabled = Boolean .parseBoolean( System .getProperty( + ConfigSetParams.ENABLE_CONFIGSET_UPLOAD, ConfigSetParams.ENABLE_CONFIGSET_UPLOAD_DEFAULT)); } I will take care of the minor corrections, thanks for pointing them out!
          Hide
          gchanan Gregory Chanan added a comment -

          Please also refer to my previous patch (https://issues.apache.org/jira/secure/attachment/12768656/SOLR-6736-newapi.patch) in which I did go the route of using Overseer but passing the content stream in the zookeeper message didn't work out as it got converted into a string

          Is that the correct link? I don't see any Overseer changes there.

          Show
          gchanan Gregory Chanan added a comment - Please also refer to my previous patch ( https://issues.apache.org/jira/secure/attachment/12768656/SOLR-6736-newapi.patch ) in which I did go the route of using Overseer but passing the content stream in the zookeeper message didn't work out as it got converted into a string Is that the correct link? I don't see any Overseer changes there.
          Hide
          varunrajput Varun Rajput added a comment - - edited

          Yes, the changes I made are in OverseerConfigSetMessageHandler. If the link doesn't get you to the right patch, you may open the patch named "SOLR-6736-newapi.patch" that I uploaded on October 25, 2015 from the attachments.

          Show
          varunrajput Varun Rajput added a comment - - edited Yes, the changes I made are in OverseerConfigSetMessageHandler. If the link doesn't get you to the right patch, you may open the patch named " SOLR-6736 -newapi.patch" that I uploaded on October 25, 2015 from the attachments.
          Hide
          gchanan Gregory Chanan added a comment -

          Ok, I see it now. I'll take a look when I get the chance.

          Show
          gchanan Gregory Chanan added a comment - Ok, I see it now. I'll take a look when I get the chance.
          Hide
          varunrajput Varun Rajput added a comment -

          Great, thanks in advance.

          Show
          varunrajput Varun Rajput added a comment - Great, thanks in advance.
          Hide
          anshumg Anshum Gupta added a comment -

          Thanks for the patch Varun.

          It would also be good to leave room for allowing/disallowing/checking certain properties in the config file itself, even if that requires some sort of parsing e.g. remoteStreaming etc. to make sure that the files don't contain undesirable settings. Just to be clear, I'm not saying we hard-code this in there, but leave room for either configuration or extension.

          Also, in addition to what Greg mentioned, here are a few more minor suggestions:

          • the use of final keyword isn't required here:
            final String ENABLE_CONFIGSET_UPLOAD = "configs.upload";
            final String ENABLE_CONFIGSET_UPLOAD_DEFAULT = "false";
            
          • In ConfigSetsHandler, the error code shouldn't be UNAUTHORIZED as no other user/credentials would be allowed to do this. Perhaps a FORBIDDEN or BAD_REQUEST would serve this better?
            throw new SolrException(SolrException.ErrorCode.UNAUTHORIZED, 
                          "Uploads are not enabled. Please set the system property \"" 
                              + ConfigSetParams.ENABLE_CONFIGSET_UPLOAD + "\" to true");
            
          • With 'Exception' there, you should remove everything else from ConfigSetsHandler here:
              private void hanldeConfigUploadRequest(SolrQueryRequest req, SolrQueryResponse rsp) throws IOException,
                  KeeperException, InterruptedException, Exception {
            
          • Very small and trivial but there are a few unwanted imports.
          Show
          anshumg Anshum Gupta added a comment - Thanks for the patch Varun. It would also be good to leave room for allowing/disallowing/checking certain properties in the config file itself, even if that requires some sort of parsing e.g. remoteStreaming etc. to make sure that the files don't contain undesirable settings. Just to be clear, I'm not saying we hard-code this in there, but leave room for either configuration or extension. Also, in addition to what Greg mentioned, here are a few more minor suggestions: the use of final keyword isn't required here: final String ENABLE_CONFIGSET_UPLOAD = "configs.upload" ; final String ENABLE_CONFIGSET_UPLOAD_DEFAULT = " false " ; In ConfigSetsHandler, the error code shouldn't be UNAUTHORIZED as no other user/credentials would be allowed to do this. Perhaps a FORBIDDEN or BAD_REQUEST would serve this better? throw new SolrException(SolrException.ErrorCode.UNAUTHORIZED, "Uploads are not enabled. Please set the system property \" " + ConfigSetParams.ENABLE_CONFIGSET_UPLOAD + "\" to true "); With 'Exception' there, you should remove everything else from ConfigSetsHandler here: private void hanldeConfigUploadRequest(SolrQueryRequest req, SolrQueryResponse rsp) throws IOException, KeeperException, InterruptedException, Exception { Very small and trivial but there are a few unwanted imports.
          Hide
          varunrajput Varun Rajput added a comment -

          Hi Anshum, thanks for the suggestions. I will put all these in, in the next patch. In terms of checking certain properties, we can do that but this upload is for the complete config set and not a single file. Do you have any suggestions on doing that for a complete config set upload?

          Show
          varunrajput Varun Rajput added a comment - Hi Anshum, thanks for the suggestions. I will put all these in, in the next patch. In terms of checking certain properties, we can do that but this upload is for the complete config set and not a single file. Do you have any suggestions on doing that for a complete config set upload?
          Hide
          varunrajput Varun Rajput added a comment -

          Hey Gregory Chanan, did you get a chance to look into this?

          Show
          varunrajput Varun Rajput added a comment - Hey Gregory Chanan , did you get a chance to look into this?
          Hide
          erickerickson Erick Erickson added a comment -

          SOLR-8378 avoids the problem of uploading arbitrary XML in a browser, it uses a client jar instead. Perhaps that JIRA can provide the necessary functionality?

          Show
          erickerickson Erick Erickson added a comment - SOLR-8378 avoids the problem of uploading arbitrary XML in a browser, it uses a client jar instead. Perhaps that JIRA can provide the necessary functionality?
          Hide
          varunrajput Varun Rajput added a comment -

          Anshum Gupta and Gregory Chanan, I have updated the patch with all the minor changes you both suggested. Please provide feedback on the upload as well as config property check questions I had asked so I can update the patch for those as well. Thanks!

          Show
          varunrajput Varun Rajput added a comment - Anshum Gupta and Gregory Chanan , I have updated the patch with all the minor changes you both suggested. Please provide feedback on the upload as well as config property check questions I had asked so I can update the patch for those as well. Thanks!
          Hide
          varunrajput Varun Rajput added a comment -

          Hey Erick Erickson, as discussed previously, the problem of uploading through browser is already addressed by having the user enable it. Uploading a configset or config file through the browser is much easier and convenient for the user while adding more actions to the ConfigSetsAPI.

          Show
          varunrajput Varun Rajput added a comment - Hey Erick Erickson , as discussed previously, the problem of uploading through browser is already addressed by having the user enable it. Uploading a configset or config file through the browser is much easier and convenient for the user while adding more actions to the ConfigSetsAPI.
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment - - edited

          I have created a branch jira/solr-6736 with the latest patch (after updating it for master).

          Regarding the security vulnerability that this new API exposes, I have the following thoughts to take this forward:

          1. We can allow unauthenticated/unauthorized users to upload a configset, but mark such configsets with a "trusted=false" flag while storing in ZK (metadata on the configset's znode). If this endpoint is secured using authorization and authentication, then we can store the uploaded configsets with "trusted=true".
          2. Upon creation of a collection using an untrusted configset, any attempt to register a "vulnerable" component, e.g. StatelessScriptUpdateProcessor, XsltUpdateRequestHandler, DataImportHandler etc., should fail with an error that indicates that the configset was not trusted and it can be made trusted by enabling authentication/authorization for the API endpoint and re-uploading the configset. Same error when using a config API command to register any update handler using an untrusted configset.
          3. Ensure that untrusted configsets never overwrite existing trusted configsets.

          As a separate exercise, we should audit our use of the XML parser to ensure XXE attacks are not possible on XML files, either uploaded from here/elsewhere or loaded from the disk.

          Varun Rajput, Anshum Gupta, Noble Paul, WDYT?

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - - edited I have created a branch jira/solr-6736 with the latest patch (after updating it for master). Regarding the security vulnerability that this new API exposes, I have the following thoughts to take this forward: We can allow unauthenticated/unauthorized users to upload a configset, but mark such configsets with a "trusted=false" flag while storing in ZK (metadata on the configset's znode). If this endpoint is secured using authorization and authentication, then we can store the uploaded configsets with "trusted=true". Upon creation of a collection using an untrusted configset, any attempt to register a "vulnerable" component, e.g. StatelessScriptUpdateProcessor, XsltUpdateRequestHandler, DataImportHandler etc., should fail with an error that indicates that the configset was not trusted and it can be made trusted by enabling authentication/authorization for the API endpoint and re-uploading the configset. Same error when using a config API command to register any update handler using an untrusted configset. Ensure that untrusted configsets never overwrite existing trusted configsets. As a separate exercise, we should audit our use of the XML parser to ensure XXE attacks are not possible on XML files, either uploaded from here/elsewhere or loaded from the disk. Varun Rajput , Anshum Gupta , Noble Paul , WDYT?
          Hide
          hgadre Hrishikesh Gadre added a comment - - edited

          Ishan Chattopadhyaya

          We can allow unauthenticated/unauthorized users to upload a configset,

          I am not following why this endpoint needs to be "unsecure" ? If it is to simplify the development process, then that can be mitigated by setting up a unsecure Solr cluster in the staging environment. In case of a production environment all endpoints must be authenticated using the configured mechanism if security is enabled. This request handler should also implement PermissionNameProvider interface so that only users which have "CONFIG_EDIT_PERM" can update it.

          Show
          hgadre Hrishikesh Gadre added a comment - - edited Ishan Chattopadhyaya We can allow unauthenticated/unauthorized users to upload a configset, I am not following why this endpoint needs to be "unsecure" ? If it is to simplify the development process, then that can be mitigated by setting up a unsecure Solr cluster in the staging environment. In case of a production environment all endpoints must be authenticated using the configured mechanism if security is enabled. This request handler should also implement PermissionNameProvider interface so that only users which have "CONFIG_EDIT_PERM" can update it.
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment - - edited

          If it is to simplify the development process, then that can be mitigated by setting up a unsecure Solr cluster in the staging environment. In case of a production environment all endpoints must be authenticated using the configured mechanism if security is enabled.

          This seems like a sound approach in theory, but often times users don't follow proper procedures for deployment and end up exposing their deployments without proper authentication/authorization. This extra security is to save such users from potential remote code execution based attacks. Our guidelines should, anyway, be for admins to enable security before going to production.

          Having this feature disabled out of the box was the other alternative that was explored above (to protect users who might end up exposing their cluster without securing it first), but I think it is inconvenient and can (and should) be avoided.

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - - edited If it is to simplify the development process, then that can be mitigated by setting up a unsecure Solr cluster in the staging environment. In case of a production environment all endpoints must be authenticated using the configured mechanism if security is enabled. This seems like a sound approach in theory, but often times users don't follow proper procedures for deployment and end up exposing their deployments without proper authentication/authorization. This extra security is to save such users from potential remote code execution based attacks. Our guidelines should, anyway, be for admins to enable security before going to production. Having this feature disabled out of the box was the other alternative that was explored above (to protect users who might end up exposing their cluster without securing it first), but I think it is inconvenient and can (and should) be avoided.
          Hide
          noble.paul Noble Paul added a comment -

          don't disable DataImportHandler, just disable ScriptTransformer

          Show
          noble.paul Noble Paul added a comment - don't disable DataImportHandler , just disable ScriptTransformer
          Hide
          hgadre Hrishikesh Gadre added a comment -

          Ishan Chattopadhyaya

          This seems like a sound approach in theory, but often times users don't follow proper procedures for deployment and end up exposing their deployments without proper authentication/authorization. This extra security is to save such users from potential remote code execution based attacks. Our guidelines should, anyway, be for admins to enable security before going to production.

          Oh I think I misunderstood your earlier statement. So this would not affect the setups which have security enabled.

          Show
          hgadre Hrishikesh Gadre added a comment - Ishan Chattopadhyaya This seems like a sound approach in theory, but often times users don't follow proper procedures for deployment and end up exposing their deployments without proper authentication/authorization. This extra security is to save such users from potential remote code execution based attacks. Our guidelines should, anyway, be for admins to enable security before going to production. Oh I think I misunderstood your earlier statement. So this would not affect the setups which have security enabled.
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment - - edited

          So this would not affect the setups which have security enabled.

          Right.

          If this endpoint is secured using authorization and authentication, then we can store the uploaded configsets with "trusted=true".

          Those "trusted" configsets can be used to create collections without any restrictions.

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - - edited So this would not affect the setups which have security enabled. Right. If this endpoint is secured using authorization and authentication, then we can store the uploaded configsets with "trusted=true". Those "trusted" configsets can be used to create collections without any restrictions.
          Hide
          varunrajput Varun Rajput added a comment -

          I like the approach of "trusted" configsets, how would the restrictions on vulnerable components be imposed?

          Show
          varunrajput Varun Rajput added a comment - I like the approach of "trusted" configsets, how would the restrictions on vulnerable components be imposed?
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment -

          Initial patch based on my proposal above.

          1. Adds configsets with trusted=false (TODO: this should be based on whether authz is enabled for /admin)
          2. Fail update requests using XSLT
          3. TODO: Fail to load StatelessScriptUpdateProcessor if configset is untrusted
          4. TODO: Fail to load script transformer in DIH if configset is untrusted
          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - Initial patch based on my proposal above. Adds configsets with trusted=false (TODO: this should be based on whether authz is enabled for /admin) Fail update requests using XSLT TODO: Fail to load StatelessScriptUpdateProcessor if configset is untrusted TODO: Fail to load script transformer in DIH if configset is untrusted
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment - - edited

          Added new patch with the following:

          1. AuthorizationResponse now contains the permission that permitted the request to succeed.
          2. ConfigSetsHandler sets "trusted" as true/false depending on whether the /admin/configs endpoint was protected or not.
          3. For untrusted configsets, don't allow StatelessScriptUpdateProcess and the XSLT processing of XmlLoader (formerly XsltUpdateHandler). Added tests for the same.
          4. TODO: Do the same for DIH's script transformer and RunExecutableListener.

          Noble Paul, please review. If you suggest, I can split out the authorization framework changes into a separate, linked issue.

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - - edited Added new patch with the following: AuthorizationResponse now contains the permission that permitted the request to succeed. ConfigSetsHandler sets "trusted" as true/false depending on whether the /admin/configs endpoint was protected or not. For untrusted configsets, don't allow StatelessScriptUpdateProcess and the XSLT processing of XmlLoader (formerly XsltUpdateHandler). Added tests for the same. TODO: Do the same for DIH's script transformer and RunExecutableListener. Noble Paul , please review. If you suggest, I can split out the authorization framework changes into a separate, linked issue.
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment -

          Based on an offline discussion with Noble, I've made the following changes in this patch:

          1. A configset is trusted if authentication is enabled and the request has been populated with a user principal. Not validating whether authorization is in place or not anymore.
          2. Initializing the vulnerable plugins using SolrCoreAware.inform(SolrCore) instead of NamedList.
          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - Based on an offline discussion with Noble, I've made the following changes in this patch: A configset is trusted if authentication is enabled and the request has been populated with a user principal. Not validating whether authorization is in place or not anymore. Initializing the vulnerable plugins using SolrCoreAware.inform(SolrCore) instead of NamedList.
          Hide
          noble.paul Noble Paul added a comment -

          Looks good. I guess we should throw the exception at component execution time rather than component load time. If we throw an exception at load time , the core fails to load and the user won't know why it was not loaded. OTOH if you throw an exception at component run time , the response may have an error message . At load time, just add a warning message

          Show
          noble.paul Noble Paul added a comment - Looks good. I guess we should throw the exception at component execution time rather than component load time. If we throw an exception at load time , the core fails to load and the user won't know why it was not loaded. OTOH if you throw an exception at component run time , the response may have an error message . At load time, just add a warning message
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment -

          I thought about that, and decided to fail at the loading time instead of execution time. The reason was that I think that it is better for a user to know upfront that the collection, as uploaded through the configset API, won't work as desired. Not failing at the collection creation time could mislead the user into believing everything went fine, but his queries/updates would fail much later.

          the core fails to load and the user won't know why it was not loaded

          Towards this, I've added the following exception message that would be thrown when the collection creation fails.

          The configset for this collection was uploaded without any authentication in place,  and this operation is not available for collections with untrusted configsets. To have this feature, re-upload the configset after enabling authentication and authorization.
          

          Do you think that makes sense?

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - I thought about that, and decided to fail at the loading time instead of execution time. The reason was that I think that it is better for a user to know upfront that the collection, as uploaded through the configset API, won't work as desired. Not failing at the collection creation time could mislead the user into believing everything went fine, but his queries/updates would fail much later. the core fails to load and the user won't know why it was not loaded Towards this, I've added the following exception message that would be thrown when the collection creation fails. The configset for this collection was uploaded without any authentication in place, and this operation is not available for collections with untrusted configsets. To have this feature, re-upload the configset after enabling authentication and authorization. Do you think that makes sense?
          Hide
          noble.paul Noble Paul added a comment -

          A lot of components are initialized lazily as well. So we don't know at which point it fails and whether the user gets the message at collection creation time or not.

          Show
          noble.paul Noble Paul added a comment - A lot of components are initialized lazily as well. So we don't know at which point it fails and whether the user gets the message at collection creation time or not.
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment -

          Even if a component is loaded lazily, throwing an exception at the time of initialization seems better than initializing the component without errors and throwing errors at the time of the request. That way, we throw the exception as soon as possible technically. It will just prevent surprises for the user, say during a production deployment, whereby the requests would be guaranteed to be executed as desired if the initialization has succeeded.

          One of the vulnerable plugins, RunExecutableListener, is usually executed during a commit. An exception during a commit could lead to potential data loss. At least for that plugin (and for every other vulnerable plugin as well), I'd prefer if the user got the error during the initialization.

          What do you think?

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - Even if a component is loaded lazily, throwing an exception at the time of initialization seems better than initializing the component without errors and throwing errors at the time of the request. That way, we throw the exception as soon as possible technically. It will just prevent surprises for the user, say during a production deployment, whereby the requests would be guaranteed to be executed as desired if the initialization has succeeded. One of the vulnerable plugins, RunExecutableListener, is usually executed during a commit. An exception during a commit could lead to potential data loss. At least for that plugin (and for every other vulnerable plugin as well), I'd prefer if the user got the error during the initialization. What do you think?
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment - - edited

          Updating the patch. This is mostly ready now.

          This iteration of the patch fixes the TODO items from above.

          1. RunExecutableListener doesn't initialize if configset is untrusted.
          2. DIH's ScriptTransformer doesn't initialize if configset is untrusted.

          Other changes that this patch introduces, overall:

          1. Introduces action=UPLOAD to /admin/configs endpoint, that accepts a zip file through POST.
          2. If authentication is not enabled during upload, the configset is considered "untrusted".
          3. XSLT transformer cannot be used (tr=) if configset is untrusted.
          4. StatelessScriptUpdateProcessor doesn't initialize if configset is untrusted.
          5. config-edit permission can now be used for protecting this UPLOAD action.
          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - - edited Updating the patch. This is mostly ready now. This iteration of the patch fixes the TODO items from above. RunExecutableListener doesn't initialize if configset is untrusted. DIH's ScriptTransformer doesn't initialize if configset is untrusted. Other changes that this patch introduces, overall: Introduces action=UPLOAD to /admin/configs endpoint, that accepts a zip file through POST. If authentication is not enabled during upload, the configset is considered "untrusted". XSLT transformer cannot be used (tr=) if configset is untrusted. StatelessScriptUpdateProcessor doesn't initialize if configset is untrusted. config-edit permission can now be used for protecting this UPLOAD action.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 6b0217b7cbff1216bb4ffbecdba02eb8c5dd3df6 in lucene-solr's branch refs/heads/master from Ishan Chattopadhyaya
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6b0217b ]

          SOLR-6736: Adding support for uploading zipped configsets using ConfigSets API

          Show
          jira-bot ASF subversion and git services added a comment - Commit 6b0217b7cbff1216bb4ffbecdba02eb8c5dd3df6 in lucene-solr's branch refs/heads/master from Ishan Chattopadhyaya [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6b0217b ] SOLR-6736 : Adding support for uploading zipped configsets using ConfigSets API
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 254218e80ca54203079a6591fa84edfaccaedea8 in lucene-solr's branch refs/heads/branch_6x from Ishan Chattopadhyaya
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=254218e ]

          SOLR-6736: Adding support for uploading zipped configsets using ConfigSets API

          Show
          jira-bot ASF subversion and git services added a comment - Commit 254218e80ca54203079a6591fa84edfaccaedea8 in lucene-solr's branch refs/heads/branch_6x from Ishan Chattopadhyaya [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=254218e ] SOLR-6736 : Adding support for uploading zipped configsets using ConfigSets API
          Hide
          noble.paul Noble Paul added a comment - - edited
          @Override
            public Name getPermissionName(AuthorizationContext ctx) {
              switch (ctx.getHttpMethod()) {
                case "GET":
                  return Name.CONFIG_READ_PERM;
                case "POST":
                  return Name.CONFIG_EDIT_PERM;
                default:
                  return null;
              }
            }
          

          is wrong. POST is accepted in all methods. u should check for explicit action

          Show
          noble.paul Noble Paul added a comment - - edited @Override public Name getPermissionName(AuthorizationContext ctx) { switch (ctx.getHttpMethod()) { case "GET" : return Name.CONFIG_READ_PERM; case "POST" : return Name.CONFIG_EDIT_PERM; default : return null ; } } is wrong. POST is accepted in all methods. u should check for explicit action
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 6d948debc632082f58373a3276860c347152f099 in lucene-solr's branch refs/heads/master from Ishan Chattopadhyaya
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6d948de ]

          SOLR-6736: Fix authorization permissions

          Show
          jira-bot ASF subversion and git services added a comment - Commit 6d948debc632082f58373a3276860c347152f099 in lucene-solr's branch refs/heads/master from Ishan Chattopadhyaya [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6d948de ] SOLR-6736 : Fix authorization permissions
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 23e3582c95d899aec26a9dfcb895eac0b0e1bd06 in lucene-solr's branch refs/heads/branch_6x from Ishan Chattopadhyaya
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=23e3582 ]

          SOLR-6736: Fix authorization permissions

          Show
          jira-bot ASF subversion and git services added a comment - Commit 23e3582c95d899aec26a9dfcb895eac0b0e1bd06 in lucene-solr's branch refs/heads/branch_6x from Ishan Chattopadhyaya [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=23e3582 ] SOLR-6736 : Fix authorization permissions
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment -

          Thanks Varun Rajput and everyone else who contributed.

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - Thanks Varun Rajput and everyone else who contributed.
          Hide
          varunrajput Varun Rajput added a comment -

          Thanks everyone!

          Show
          varunrajput Varun Rajput added a comment - Thanks everyone!
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment -

          Cassandra Targett, please review the documentation changes for this issue.

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - Cassandra Targett , please review the documentation changes for this issue.
          Hide
          ctargett Cassandra Targett added a comment -

          please review the documentation changes for this issue.

          Thanks Ishan Chattopadhyaya. I only noticed one thing: In AsciiDoc, you need to put a blank line between a paragraph and a bulleted list (at L#182). Otherwise it will render as one whole paragraph, which isn't what you're going for.

          Show
          ctargett Cassandra Targett added a comment - please review the documentation changes for this issue. Thanks Ishan Chattopadhyaya . I only noticed one thing: In AsciiDoc, you need to put a blank line between a paragraph and a bulleted list (at L#182). Otherwise it will render as one whole paragraph, which isn't what you're going for.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 5c4f0a27a327dba22e121680a19c192a53b8d75e in lucene-solr's branch refs/heads/branch_6_6 from Ishan Chattopadhyaya
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5c4f0a2 ]

          SOLR-10446, SOLR-6736: Ref guide documentation

          Show
          jira-bot ASF subversion and git services added a comment - Commit 5c4f0a27a327dba22e121680a19c192a53b8d75e in lucene-solr's branch refs/heads/branch_6_6 from Ishan Chattopadhyaya [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5c4f0a2 ] SOLR-10446 , SOLR-6736 : Ref guide documentation
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit f358c6834d3957b73690d73e49c021644c2f61fb in lucene-solr's branch refs/heads/branch_6x from Ishan Chattopadhyaya
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f358c68 ]

          SOLR-10446, SOLR-6736: Ref guide documentation

          Show
          jira-bot ASF subversion and git services added a comment - Commit f358c6834d3957b73690d73e49c021644c2f61fb in lucene-solr's branch refs/heads/branch_6x from Ishan Chattopadhyaya [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f358c68 ] SOLR-10446 , SOLR-6736 : Ref guide documentation
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 2eb324f9bae1553c9c68c4a740a4f865b0ec6da5 in lucene-solr's branch refs/heads/master from Ishan Chattopadhyaya
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2eb324f ]

          SOLR-10446, SOLR-6736: Ref guide documentation

          Show
          jira-bot ASF subversion and git services added a comment - Commit 2eb324f9bae1553c9c68c4a740a4f865b0ec6da5 in lucene-solr's branch refs/heads/master from Ishan Chattopadhyaya [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2eb324f ] SOLR-10446 , SOLR-6736 : Ref guide documentation
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment -
          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - Thanks Cassandra Targett .
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 069e6a78e0c2201b6b98a129144a63ecc32b4fee in lucene-solr's branch refs/heads/master from Ishan Chattopadhyaya
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=069e6a7 ]

          SOLR-6736: Improve the configset UPLOAD example in reference guide

          Show
          jira-bot ASF subversion and git services added a comment - Commit 069e6a78e0c2201b6b98a129144a63ecc32b4fee in lucene-solr's branch refs/heads/master from Ishan Chattopadhyaya [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=069e6a7 ] SOLR-6736 : Improve the configset UPLOAD example in reference guide
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 2184a43e3e10e5b7de2f29c0d4cd59df21754730 in lucene-solr's branch refs/heads/branch_7x from Ishan Chattopadhyaya
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2184a43 ]

          SOLR-6736: Improve the configset UPLOAD example in reference guide

          Show
          jira-bot ASF subversion and git services added a comment - Commit 2184a43e3e10e5b7de2f29c0d4cd59df21754730 in lucene-solr's branch refs/heads/branch_7x from Ishan Chattopadhyaya [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2184a43 ] SOLR-6736 : Improve the configset UPLOAD example in reference guide
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit e68b3cabe27c91b2c22f6dd8aac48a0b198440c7 in lucene-solr's branch refs/heads/branch_7_0 from Ishan Chattopadhyaya
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e68b3ca ]

          SOLR-6736: Improve the configset UPLOAD example in reference guide

          Show
          jira-bot ASF subversion and git services added a comment - Commit e68b3cabe27c91b2c22f6dd8aac48a0b198440c7 in lucene-solr's branch refs/heads/branch_7_0 from Ishan Chattopadhyaya [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e68b3ca ] SOLR-6736 : Improve the configset UPLOAD example in reference guide
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 05a702095dfd8c34370dcbdd25cb51ce296b0586 in lucene-solr's branch refs/heads/branch_6x from Ishan Chattopadhyaya
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=05a7020 ]

          SOLR-6736: Improve the configset UPLOAD example in reference guide

          Show
          jira-bot ASF subversion and git services added a comment - Commit 05a702095dfd8c34370dcbdd25cb51ce296b0586 in lucene-solr's branch refs/heads/branch_6x from Ishan Chattopadhyaya [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=05a7020 ] SOLR-6736 : Improve the configset UPLOAD example in reference guide
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 927d10660f875f96465417698f28aecec8a7b948 in lucene-solr's branch refs/heads/branch_6_6 from Ishan Chattopadhyaya
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=927d106 ]

          SOLR-6736: Improve the configset UPLOAD example in reference guide

          Show
          jira-bot ASF subversion and git services added a comment - Commit 927d10660f875f96465417698f28aecec8a7b948 in lucene-solr's branch refs/heads/branch_6_6 from Ishan Chattopadhyaya [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=927d106 ] SOLR-6736 : Improve the configset UPLOAD example in reference guide
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment -

          Fixed the example for this. FYI, Erik Hatcher.

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - Fixed the example for this. FYI, Erik Hatcher .

            People

            • Assignee:
              ichattopadhyaya Ishan Chattopadhyaya
              Reporter:
              varunrajput Varun Rajput
            • Votes:
              0 Vote for this issue
              Watchers:
              15 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development