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

Change hard-coded keysize from 512 to 1024

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.4, 7.0
    • Component/s: None
    • Security Level: Public (Default Security Level. Issues are Public)
    • Labels:
      None

      Description

      In order to configure our dataSource without requiring a plaintext password in the configuration file, we extended JdbcDataSource to create our own custom implementation. Our dataSource config now looks something like this:

      <dataSource type="com.foo.FooDataSource" driver="oracle.jdbc.OracleDriver" url="jdbc:oracle:thin:@db-host-machine:1521:tst1" user="testuser" password="{ENC}{1.1}1ePOfWcbOIU056gKiLTrLw=="/>
      

      We are using the RSA JSAFE Crypto-J libraries for encrypting/decrypting the password. However, this seems to cause an issue when we try use Solr in a Cloud Configuration (using Zookeeper). The error is "Strong key gen and multiprime gen require at least 1024-bit keysize." Full log attached.

      This seems to be due to the hard-coded value of 512 in the org.apache.solr.util.CryptoKeys$RSAKeyPair class:

      public RSAKeyPair() {
        KeyPairGenerator keyGen = null;
        try {
          keyGen = KeyPairGenerator.getInstance("RSA");
        } catch (NoSuchAlgorithmException e) {
          throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
        }
        keyGen.initialize(512);
      

      I pulled down the Solr code, changed the hard-coded value to 1024, rebuilt it, and now everything seems to work great.

      1. solr.log
        10 kB
        Jeremy Martini
      2. SOLR-9609.patch
        2 kB
        Erick Erickson
      3. SOLR-9609.patch
        6 kB
        Erick Erickson
      4. SOLR-9609.patch
        0.7 kB
        Erick Erickson
      5. SOLR-9609.patch
        0.7 kB
        Jeremy Martini

        Activity

        Hide
        erickerickson Erick Erickson added a comment - - edited

        Slight modification to use a system variable that defaults to 1024.

        Running full test suite now, if all goes well I intend to check this in sometime in the next couple of days.

        Thanks Jeremy!

        Everyone:
        Any good section of the ref guide that documenting this parameter should go? On a quick scan I didn't see an obvious spot.

        Show
        erickerickson Erick Erickson added a comment - - edited Slight modification to use a system variable that defaults to 1024. Running full test suite now, if all goes well I intend to check this in sometime in the next couple of days. Thanks Jeremy! Everyone: Any good section of the ref guide that documenting this parameter should go? On a quick scan I didn't see an obvious spot.
        Hide
        hgadre Hrishikesh Gadre added a comment -

        Erick Erickson Since this a cluster wide (rather than a host or server specific) configuration, I think it should come from security.json rather than a system property. This will also allow us to make other parameters (e.g. algorithm name etc.) configurable. What do you think?

        Show
        hgadre Hrishikesh Gadre added a comment - Erick Erickson Since this a cluster wide (rather than a host or server specific) configuration, I think it should come from security.json rather than a system property. This will also allow us to make other parameters (e.g. algorithm name etc.) configurable. What do you think?
        Hide
        erickerickson Erick Erickson added a comment -

        Not quite sure. Is the security.json file necessary for anything else in this scenario? A system var is at least simple, but you're certainly right, this is a system-wide property. This is certainly security-related so that this seems like a good idea...

        Anyone else have an opinion here?

        BTW, I'm at Lucene/Solr Revolution, so may not get back to this for a bit...

        Show
        erickerickson Erick Erickson added a comment - Not quite sure. Is the security.json file necessary for anything else in this scenario? A system var is at least simple, but you're certainly right, this is a system-wide property. This is certainly security-related so that this seems like a good idea... Anyone else have an opinion here? BTW, I'm at Lucene/Solr Revolution, so may not get back to this for a bit...
        Hide
        erickerickson Erick Erickson added a comment -

        Hrishikesh: Thinking about this a little more, and revealing my total ignorance of these environments....

        Does this make sense even in non-cloud modes? On a very quick look at the code, security.json is only relevant in cloud mode, so this wouldn't be an option for non-cloud...

        Or is is sufficient to just deal with cloud mode for the nonce and deal with any request for non-cloud support if/when it comes up? And if it doesn't come up before ZK becomes "the one source of truth" we won't have to deal with it.

        WDYT?

        Show
        erickerickson Erick Erickson added a comment - Hrishikesh : Thinking about this a little more, and revealing my total ignorance of these environments.... Does this make sense even in non-cloud modes? On a very quick look at the code, security.json is only relevant in cloud mode, so this wouldn't be an option for non-cloud... Or is is sufficient to just deal with cloud mode for the nonce and deal with any request for non-cloud support if/when it comes up? And if it doesn't come up before ZK becomes "the one source of truth" we won't have to deal with it. WDYT?
        Hide
        hgadre Hrishikesh Gadre added a comment -

        Erick Erickson As per SOLR-9481 - the idea seems to be to use security.json even for standalone mode. The only difference is that it will be stored on local file-system instead of Zookeeper.

        BTW I am not opposed to using a system property for this configuration. But as per my understanding we are storing security.json (as well as other configs such as solr.xml etc.) in Zookeeper to avoid having to specify such configurations on each host separately. Hence I think we should be consistent with this design goal by always having security.json to manage such service-level configs and not requiring users to specify java system properties.

        Jan Høydahl let us know if you have any thoughts...

        Show
        hgadre Hrishikesh Gadre added a comment - Erick Erickson As per SOLR-9481 - the idea seems to be to use security.json even for standalone mode. The only difference is that it will be stored on local file-system instead of Zookeeper. BTW I am not opposed to using a system property for this configuration. But as per my understanding we are storing security.json (as well as other configs such as solr.xml etc.) in Zookeeper to avoid having to specify such configurations on each host separately. Hence I think we should be consistent with this design goal by always having security.json to manage such service-level configs and not requiring users to specify java system properties. Jan Høydahl let us know if you have any thoughts...
        Hide
        erickerickson Erick Erickson added a comment -

        Ah, that explains it. I was looking at 6x code when I claimed security.json wasn't being used in stand-alone. 9481 hasn't been back-ported to 6x yet.

        bq: But as per my understanding we are storing security.json (as well as other configs such as solr.xml etc.) in Zookeeper to avoid having to specify such configurations on each host separately

        This makes total sense to me. Plus I don't want to ever have to track down that the system property was set differently on one of 250 nodes because somehow the startup script wasn't propagated, having it on ZK makes much more sense, thanks for suggesting it.

        As far as going forward: Once using security.json in stand-alone is committed to 6x, we can make this one happen. It's a pretty trivial change all told.

        Show
        erickerickson Erick Erickson added a comment - Ah, that explains it. I was looking at 6x code when I claimed security.json wasn't being used in stand-alone. 9481 hasn't been back-ported to 6x yet. bq: But as per my understanding we are storing security.json (as well as other configs such as solr.xml etc.) in Zookeeper to avoid having to specify such configurations on each host separately This makes total sense to me. Plus I don't want to ever have to track down that the system property was set differently on one of 250 nodes because somehow the startup script wasn't propagated, having it on ZK makes much more sense, thanks for suggesting it. As far as going forward: Once using security.json in stand-alone is committed to 6x, we can make this one happen. It's a pretty trivial change all told.
        Hide
        janhoy Jan Høydahl added a comment -

        This sounds to me as a type of configuration that most people would leave to default (we change it to 1024 in code), and those that need to change it will do it once, no need for the flexibility of security.json with edit API etc? If so, I'd just create a sysProp for it and perhaps wire it to an SOLR_CRYPTO_X env-var which could be placed in solr.in.sh.

        Show
        janhoy Jan Høydahl added a comment - This sounds to me as a type of configuration that most people would leave to default (we change it to 1024 in code), and those that need to change it will do it once, no need for the flexibility of security.json with edit API etc? If so, I'd just create a sysProp for it and perhaps wire it to an SOLR_CRYPTO_X env-var which could be placed in solr.in.sh.
        Hide
        erickerickson Erick Erickson added a comment -

        Jan Høydahl As a sysprop every solr.in.sh file (or whatever) would have to be modified, leaving the chance of one of your N nodes not getting the update. Putting it up on Zookeeper in security.json makes that much less likely.

        Hmmm, but what about sequencing here? In order to pull it from security.json, we need to be able to connect to Zookeeper. I'm assuming that this is irrelevant for fetching the security.json file from Zookeeper? You see where this is going, if we have to have this value correctly set in order to get data from Zookeeper, then it must go in solr.in.sh......

        That said, I don't have a strong opinion here although I slightly lean towards putting this in the security.json file unless that'd be a problem.

        NOTE: SOLR-9481 appears to have been committed to 6x, so if we choose to put this in security.json we can go forward with this ticket.

        I've assigned it to myself to not lose track of it, but anyone else who wants to pick it up please feel free.

        Erick

        Show
        erickerickson Erick Erickson added a comment - Jan Høydahl As a sysprop every solr.in.sh file (or whatever) would have to be modified, leaving the chance of one of your N nodes not getting the update. Putting it up on Zookeeper in security.json makes that much less likely. Hmmm, but what about sequencing here? In order to pull it from security.json, we need to be able to connect to Zookeeper. I'm assuming that this is irrelevant for fetching the security.json file from Zookeeper? You see where this is going, if we have to have this value correctly set in order to get data from Zookeeper, then it must go in solr.in.sh...... That said, I don't have a strong opinion here although I slightly lean towards putting this in the security.json file unless that'd be a problem. NOTE: SOLR-9481 appears to have been committed to 6x, so if we choose to put this in security.json we can go forward with this ticket. I've assigned it to myself to not lose track of it, but anyone else who wants to pick it up please feel free. Erick
        Hide
        janhoy Jan Høydahl added a comment - - edited

        My reasoning is that it is not likely something you ever need to change, so using solr.in for overriding looks better than needing to upload stuff to ZK config. And the risk of having different solr.in files is not unique to this setting. We already need SOLR_ZK_HOST to be synchronized as well as probably SSL settings. I think this is in the same neighborhood of deciding the basic wiring of how hosts will talk to eachother. That's why I also think urlScheme could move from being a clusterProp to becoming a sysProp in solr.in, or being auto-resolved from SSL props.

        And SOLR-9481 is only in master so far btw...

        Show
        janhoy Jan Høydahl added a comment - - edited My reasoning is that it is not likely something you ever need to change, so using solr.in for overriding looks better than needing to upload stuff to ZK config. And the risk of having different solr.in files is not unique to this setting. We already need SOLR_ZK_HOST to be synchronized as well as probably SSL settings. I think this is in the same neighborhood of deciding the basic wiring of how hosts will talk to eachother. That's why I also think urlScheme could move from being a clusterProp to becoming a sysProp in solr.in, or being auto-resolved from SSL props. And SOLR-9481 is only in master so far btw...
        Hide
        shalinmangar Shalin Shekhar Mangar added a comment -

        This should go into security.json. It is a system-wide setting. Please don't force people to go edit solr.in.sh files to set this property across their clusters.

        Show
        shalinmangar Shalin Shekhar Mangar added a comment - This should go into security.json. It is a system-wide setting. Please don't force people to go edit solr.in.sh files to set this property across their clusters.
        Hide
        erickerickson Erick Erickson added a comment -

        So here's a first cut at what it would take to move this to security.json (hey, it compiles, but it's completely untested).

        The reason I'm putting this up is that putting this value in security.json starts to get...messy and I'm wondering if the elegance of putting this in security.json outweighs the messiness.

        The basic problem is that CryptoKeys doesn't have access to SecurityConfig. So we can either pass CoreContainer down to CryptoKeys (yuuuuck, talk about architectural badness) or have CryptoKeys take a length for this buffer.

        The second option requires that the caller get the new value from the SecurityConfig object if it's different than the default and pass that in to the c'tor. Which I made the PKIAuthenticationPlugin do.

        Unless we make the default CryptoKeys c'tor private, any new classes using CryptoKeys has to remember to get the value from security.json which could be forgotten. I suppose we can just deprecate the default c'tor and then caveat emptor. My guess is that just making the default c'tor private is unacceptable as it breaks back-compat.

        All this to allow different buffer lengths to be chosen. So is it really worth it? It seems we have several options:
        1> go ahead and put it in security.json because it's the right thing to do.
        2> allow it to be configurable with a sysvar, which is much easier.
        3> hard code it to some future-friendly value like 4096. This assumes that having a value larger than necessary is OK.

        Or am I missing some way to get around this mess?

        P.S. there's a nocommit in there (should we really have a main method in CryptoKeys?) and I'll take out a change in the PKI test unless we deprecate the default c'tor in CryptoKeys if we decide to put this in security.json after all.

        Show
        erickerickson Erick Erickson added a comment - So here's a first cut at what it would take to move this to security.json (hey, it compiles, but it's completely untested). The reason I'm putting this up is that putting this value in security.json starts to get...messy and I'm wondering if the elegance of putting this in security.json outweighs the messiness. The basic problem is that CryptoKeys doesn't have access to SecurityConfig. So we can either pass CoreContainer down to CryptoKeys (yuuuuck, talk about architectural badness) or have CryptoKeys take a length for this buffer. The second option requires that the caller get the new value from the SecurityConfig object if it's different than the default and pass that in to the c'tor. Which I made the PKIAuthenticationPlugin do. Unless we make the default CryptoKeys c'tor private, any new classes using CryptoKeys has to remember to get the value from security.json which could be forgotten. I suppose we can just deprecate the default c'tor and then caveat emptor. My guess is that just making the default c'tor private is unacceptable as it breaks back-compat. All this to allow different buffer lengths to be chosen. So is it really worth it? It seems we have several options: 1> go ahead and put it in security.json because it's the right thing to do. 2> allow it to be configurable with a sysvar, which is much easier. 3> hard code it to some future-friendly value like 4096. This assumes that having a value larger than necessary is OK. Or am I missing some way to get around this mess? P.S. there's a nocommit in there (should we really have a main method in CryptoKeys?) and I'll take out a change in the PKI test unless we deprecate the default c'tor in CryptoKeys if we decide to put this in security.json after all.
        Hide
        erickerickson Erick Erickson added a comment -
        Show
        erickerickson Erick Erickson added a comment - Jan Høydahl Shalin Shekhar Mangar Hrishikesh Gadre any comments on this?
        Hide
        hgadre Hrishikesh Gadre added a comment -

        Erick Erickson Here is my initial analysis,

        If we can fix this problem, then rest of the implementation is trivial. If you look at the relevant code in CoreContainer.java - you can see that this is something easy to fix.

        https://github.com/apache/lucene-solr/blob/f22b1da261b93f60687431b161828e2fb27fdc8f/solr/core/src/java/org/apache/solr/core/CoreContainer.java#L462

        If we can read security.json at this point, we can easily invoke PKIAuthenticationPlugin#init(...) method and pass appropriate configuration parameters to it. Now the question how to separate the config parameters for client facing auth plugin (e.g. Basic auth) from solr <->solr communication plugin (i.e. PKIAuthenticationPlugin) ? I can think of couple of options

        • Have a predefined prefix for the config params related to PKIAuthenticationPlugin. This way we can specify all the params in the "authentication" section of security.json
        • Define a new section for PKIAuthenticationPlugin (or specifically authentication plugin for solr <->solr communication)

        Let me know if it looks ok to you. I can try this myself.

        Show
        hgadre Hrishikesh Gadre added a comment - Erick Erickson Here is my initial analysis, Currently PKIAuthenticationPlugIn has given a special status in the sense that its initialization doesn't follow the pattern used by rest of the authentication plugins. Specifically the configuration parameters of this plugin are not initialized via init(...) method. https://github.com/apache/lucene-solr/blob/358bdd490b1b15f3af6a355f93a98caf83594b18/solr/core/src/java/org/apache/solr/security/PKIAuthenticationPlugin.java#L85 If we can fix this problem, then rest of the implementation is trivial. If you look at the relevant code in CoreContainer.java - you can see that this is something easy to fix. https://github.com/apache/lucene-solr/blob/f22b1da261b93f60687431b161828e2fb27fdc8f/solr/core/src/java/org/apache/solr/core/CoreContainer.java#L462 If we can read security.json at this point, we can easily invoke PKIAuthenticationPlugin#init(...) method and pass appropriate configuration parameters to it. Now the question how to separate the config parameters for client facing auth plugin (e.g. Basic auth) from solr <->solr communication plugin (i.e. PKIAuthenticationPlugin) ? I can think of couple of options Have a predefined prefix for the config params related to PKIAuthenticationPlugin. This way we can specify all the params in the "authentication" section of security.json Define a new section for PKIAuthenticationPlugin (or specifically authentication plugin for solr <->solr communication) Let me know if it looks ok to you. I can try this myself.
        Hide
        hgadre Hrishikesh Gadre added a comment -

        Oh BTW I didn't describe how the keysize will be propagated in this case.

        Once we read all the config params via init() method, we can create the CryptoKeys.RSAKeyPair object appropriately.

        https://github.com/apache/lucene-solr/blob/f22b1da261b93f60687431b161828e2fb27fdc8f/solr/core/src/java/org/apache/solr/core/CoreContainer.java#L462

        Show
        hgadre Hrishikesh Gadre added a comment - Oh BTW I didn't describe how the keysize will be propagated in this case. Once we read all the config params via init() method, we can create the CryptoKeys.RSAKeyPair object appropriately. https://github.com/apache/lucene-solr/blob/f22b1da261b93f60687431b161828e2fb27fdc8f/solr/core/src/java/org/apache/solr/core/CoreContainer.java#L462
        Hide
        hgadre Hrishikesh Gadre added a comment -

        Erick Erickson Also I don't think we need to worry about the default constructor problem for CryptoKeys class. Currently only PKIAuthenticationHandler is using it. Even if someone used the default constructor in future, we can always have them use the other constructor. I feel it is more important to provide a consistent configuration mechanism across various authentication plugins.

        Show
        hgadre Hrishikesh Gadre added a comment - Erick Erickson Also I don't think we need to worry about the default constructor problem for CryptoKeys class. Currently only PKIAuthenticationHandler is using it. Even if someone used the default constructor in future, we can always have them use the other constructor. I feel it is more important to provide a consistent configuration mechanism across various authentication plugins.
        Hide
        erickerickson Erick Erickson added a comment -

        Hrishikesh Gadre The current patch already reads a variable from the security.json file (well, at least the code's in place to do that) and initializes the CryptoKey based on that parameter, or the default of 1024 if it's not present. Not tested, but looks fairly straightforward assuming (which I haven't verified yet) that security.json is already available.

        My question is more along the lines of whether all this is worth it to have the flexibility of changing the length in CyptoKey via configuration. This is starting to seem like the tail wagging the dog. I mean all this cruft to change one integer? Is there a measurable downside to just allocating 4096 on the theory that that'll be good enough for the foreseeable future?

        'cause then there's documentation. Then there's supporting it in future. And what about some kind of API support. And what about......

        Seems like a lot of cycles to implement something of dubious utility.

        Show
        erickerickson Erick Erickson added a comment - Hrishikesh Gadre The current patch already reads a variable from the security.json file (well, at least the code's in place to do that) and initializes the CryptoKey based on that parameter, or the default of 1024 if it's not present. Not tested, but looks fairly straightforward assuming (which I haven't verified yet) that security.json is already available. My question is more along the lines of whether all this is worth it to have the flexibility of changing the length in CyptoKey via configuration. This is starting to seem like the tail wagging the dog. I mean all this cruft to change one integer? Is there a measurable downside to just allocating 4096 on the theory that that'll be good enough for the foreseeable future? 'cause then there's documentation. Then there's supporting it in future. And what about some kind of API support. And what about...... Seems like a lot of cycles to implement something of dubious utility.
        Hide
        erickerickson Erick Erickson added a comment -

        OK, I'm going to just hard-code it to 1024 and be done with it. This is taking far too long for something that only one person has found so far.

        I'll leave a comment in the code that if we have to revisit it we should see the discussion here.

        Show
        erickerickson Erick Erickson added a comment - OK, I'm going to just hard-code it to 1024 and be done with it. This is taking far too long for something that only one person has found so far. I'll leave a comment in the code that if we have to revisit it we should see the discussion here.
        Hide
        erickerickson Erick Erickson added a comment -

        Patch I'm committing. NOTE: the larger patch from a couple of days ago has extracting this from security.json should we want to pursue that at some point.

        Show
        erickerickson Erick Erickson added a comment - Patch I'm committing. NOTE: the larger patch from a couple of days ago has extracting this from security.json should we want to pursue that at some point.
        Hide
        erickerickson Erick Erickson added a comment -

        Messed up JIRA number, here are the commits, typed 9606 instead of 9609:

        Commit e402a304bf97ead8c2a7f00a745e837fe0c6d449 in lucene-solr's branch refs/heads/master from Erick Erickson
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e402a30 ]

        SOLR-9606: Change hard-coded keysize from 512 to 1024

        Commit 8bd4ad36c5297cfd2c39be807a7f099cda4ec13e in lucene-solr's branch refs/heads/branch_6x from Erick Erickson
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8bd4ad3 ]

        SOLR-9606: Change hard-coded keysize from 512 to 1024
        (cherry picked from commit e402a30)

        Show
        erickerickson Erick Erickson added a comment - Messed up JIRA number, here are the commits, typed 9606 instead of 9609: Commit e402a304bf97ead8c2a7f00a745e837fe0c6d449 in lucene-solr's branch refs/heads/master from Erick Erickson [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e402a30 ] SOLR-9606 : Change hard-coded keysize from 512 to 1024 Commit 8bd4ad36c5297cfd2c39be807a7f099cda4ec13e in lucene-solr's branch refs/heads/branch_6x from Erick Erickson [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8bd4ad3 ] SOLR-9606 : Change hard-coded keysize from 512 to 1024 (cherry picked from commit e402a30)

          People

          • Assignee:
            erickerickson Erick Erickson
            Reporter:
            jpm290 Jeremy Martini
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development