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

Hiding keystore and truststore passwords from /admin/info/* outputs

    Details

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

      Description

      Passing keystore and truststore password is done by system properties, via cmd line parameter.
      As result, /admin/info/properties and /admin/info/system will print out the received password.

      Proposing solution to automatically redact value of any system property before output, containing the word password, and replacing its value with ******.

      1. SOLR-10076.6x.patch
        11 kB
        Mano Kovacs
      2. SOLR-10076.patch
        10 kB
        Mano Kovacs
      3. SOLR-10076.patch
        8 kB
        Mano Kovacs

        Activity

        Hide
        manokovacs Mano Kovacs added a comment -

        Is there any objection about the approach? Does this count as API change (assuming that somebody depends on the exposed passwords)?

        If there is no objection, I will start working on the patch.

        Show
        manokovacs Mano Kovacs added a comment - Is there any objection about the approach? Does this count as API change (assuming that somebody depends on the exposed passwords)? If there is no objection, I will start working on the patch.
        Hide
        janhoy Jan Høydahl added a comment -

        Been thinking about the same, but perhaps instead of a generic rule about containing password, we could have a property somewhere for what paths to hide. I would also like to hide the content of some ZK nodes such as security.json, and there may also be other places where passwords are exposed through props or APIs...

        Ideal would be if this could be coupled with Authorization, so that certain info could be controlled through group membership in AuthorizationPlugin?

        Show
        janhoy Jan Høydahl added a comment - Been thinking about the same, but perhaps instead of a generic rule about containing password, we could have a property somewhere for what paths to hide. I would also like to hide the content of some ZK nodes such as security.json, and there may also be other places where passwords are exposed through props or APIs... Ideal would be if this could be coupled with Authorization, so that certain info could be controlled through group membership in AuthorizationPlugin?
        Hide
        manokovacs Mano Kovacs added a comment -

        Been thinking about the same, but perhaps instead of a generic rule about containing password, we could have a property somewhere for what paths to hide.

        Thanks Jan Høydahl for the feedback! I was also thinking of a pattern-based parameter masking for input password. I prepared a patch with a RedactionUtils that I will extend with external parameters and upload it shortly.

        I would also like to hide the content of some ZK nodes such as security.json, and there may also be other places where passwords are exposed through props or APIs...

        I was not aware of the security.json exposing password, I created a separate jira for that as well (SOLR-10100).

        Ideal would be if this could be coupled with Authorization, so that certain info could be controlled through group membership in AuthorizationPlugin?

        In general, I would not add password visibility based on privileges. I think passwords should not be revertible, as that would expose them to the reliability of the authorization plugin and the admin users' cautiousness. For me it would somewhat beat the purpose of this jira: reducing the exposure of the security credentials. Do you see any business-case when you would grant certain roles to view these passwords?

        Show
        manokovacs Mano Kovacs added a comment - Been thinking about the same, but perhaps instead of a generic rule about containing password, we could have a property somewhere for what paths to hide. Thanks Jan Høydahl for the feedback! I was also thinking of a pattern-based parameter masking for input password. I prepared a patch with a RedactionUtils that I will extend with external parameters and upload it shortly. I would also like to hide the content of some ZK nodes such as security.json, and there may also be other places where passwords are exposed through props or APIs... I was not aware of the security.json exposing password, I created a separate jira for that as well ( SOLR-10100 ). Ideal would be if this could be coupled with Authorization, so that certain info could be controlled through group membership in AuthorizationPlugin? In general, I would not add password visibility based on privileges. I think passwords should not be revertible, as that would expose them to the reliability of the authorization plugin and the admin users' cautiousness. For me it would somewhat beat the purpose of this jira: reducing the exposure of the security credentials. Do you see any business-case when you would grant certain roles to view these passwords?
        Hide
        manokovacs Mano Kovacs added a comment -

        Attaching patch.

        Show
        manokovacs Mano Kovacs added a comment - Attaching patch.
        Hide
        ichattopadhyaya Ishan Chattopadhyaya added a comment -

        I was not aware of the security.json exposing password

        Passwords are not exposed. Salted hashes of the passwords are, though.

        In general, I would not add password visibility based on privileges. I think passwords should not be revertible, as that would expose them to the reliability of the authorization plugin and the admin users' cautiousness.

        In the case of security.json, we should encourage and try to ensure that proper authorization is in place while starting a Solr cluster. To an authorized admin user, I don't see why we shouldn't show salted hashes of passwords. Anyway, we can deal with that issue on SOLR-7890/SOLR-10100.

        Show
        ichattopadhyaya Ishan Chattopadhyaya added a comment - I was not aware of the security.json exposing password Passwords are not exposed. Salted hashes of the passwords are, though. In general, I would not add password visibility based on privileges. I think passwords should not be revertible, as that would expose them to the reliability of the authorization plugin and the admin users' cautiousness. In the case of security.json, we should encourage and try to ensure that proper authorization is in place while starting a Solr cluster. To an authorized admin user, I don't see why we shouldn't show salted hashes of passwords. Anyway, we can deal with that issue on SOLR-7890 / SOLR-10100 .
        Hide
        manokovacs Mano Kovacs added a comment -

        Thank you for the feedback, Ishan Chattopadhyaya.

        Do you think the redaction of command line password could be handled as the first patch contains?

        Show
        manokovacs Mano Kovacs added a comment - Thank you for the feedback, Ishan Chattopadhyaya . Do you think the redaction of command line password could be handled as the first patch contains?
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        This looks okay to me. We probably want to push users towards configuring this in a way it's not on the command line though, right? It's nice not to expose it via the web UI when we see it, but you also don't really want it on the command line as that stuff is pretty easy to introspect via people that should not.

        Our doc should probably encourage people to use system property on the command line alternatives or we should look at disabling / warning when it's done. I know our start scripts recently still set some of this ssl stuff via the command line, but if that is still the case, we should fix that too.

        Show
        markrmiller@gmail.com Mark Miller added a comment - This looks okay to me. We probably want to push users towards configuring this in a way it's not on the command line though, right? It's nice not to expose it via the web UI when we see it, but you also don't really want it on the command line as that stuff is pretty easy to introspect via people that should not. Our doc should probably encourage people to use system property on the command line alternatives or we should look at disabling / warning when it's done. I know our start scripts recently still set some of this ssl stuff via the command line, but if that is still the case, we should fix that too.
        Hide
        manokovacs Mano Kovacs added a comment - - edited

        Thank you Mark Miller for your comment.

        We probably want to push users towards configuring this in a way it's not on the command line though, right?

        I agree that this is more like a workaround in the current state. It could also work as a second layer of protection if passwords being passed in command line. I would assume that getting the list of running processes on a server would require higher privileges than accessing the admin-ui, which suggests that the passwords should not be exposed there.
        Also, the /admin/info/properties API would expose password were set differently.

        I know our start scripts recently still set some of this ssl stuff via the command line, but if that is still the case, we should fix that too.

        Is there a jira for that? I would be happy looking into it.

        Show
        manokovacs Mano Kovacs added a comment - - edited Thank you Mark Miller for your comment. We probably want to push users towards configuring this in a way it's not on the command line though, right? I agree that this is more like a workaround in the current state. It could also work as a second layer of protection if passwords being passed in command line. I would assume that getting the list of running processes on a server would require higher privileges than accessing the admin-ui, which suggests that the passwords should not be exposed there. Also, the /admin/info/properties API would expose password were set differently. I know our start scripts recently still set some of this ssl stuff via the command line, but if that is still the case, we should fix that too. Is there a jira for that? I would be happy looking into it.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        So I think we want to make sure the search for 'password' is case insensitive due to things like javax.net.ssl.trustStorePassword. Could use a test for that too.

        We should move RedactionUtils.java to org.apache.solr.util probably.

        Greg did something similar in Cloudera Search lucene-solr repo as a temporary hack, but used '-REDACTED-' I think that is more clear than the ****** redaction string.

        Given the affect this could have on tools/scripts that read output, I think it's not a huge deal if we changed it, but I don't see a strong reason to do it and that should usually favour back compat, even if we would guess those affected might be very few. We can do it by default in 7 and anyone looking for this in 6.5 and beyond will know they need it and it didn't exist in 6.4 and < and can turn it on. Seems like the least friction.

        Show
        markrmiller@gmail.com Mark Miller added a comment - So I think we want to make sure the search for 'password' is case insensitive due to things like javax.net.ssl.trustStorePassword. Could use a test for that too. We should move RedactionUtils.java to org.apache.solr.util probably. Greg did something similar in Cloudera Search lucene-solr repo as a temporary hack, but used '- REDACTED -' I think that is more clear than the ****** redaction string. Given the affect this could have on tools/scripts that read output, I think it's not a huge deal if we changed it, but I don't see a strong reason to do it and that should usually favour back compat, even if we would guess those affected might be very few. We can do it by default in 7 and anyone looking for this in 6.5 and beyond will know they need it and it didn't exist in 6.4 and < and can turn it on. Seems like the least friction.
        Hide
        manokovacs Mano Kovacs added a comment -

        Mark Miller, thank you for the review and comments!

        • I added test for case-sensitive property name, it in fact was not properly working.
        • I changed the redaction text to the one that Greg added. Actually this patch is the generalization of his original intent.
        • I made the system property redaction configurable with default true. 6.x backport only need to vary by the default value of that configuration to have it turned off.
        Show
        manokovacs Mano Kovacs added a comment - Mark Miller , thank you for the review and comments! I added test for case-sensitive property name, it in fact was not properly working. I changed the redaction text to the one that Greg added. Actually this patch is the generalization of his original intent. I made the system property redaction configurable with default true. 6.x backport only need to vary by the default value of that configuration to have it turned off.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 91c3f78f8fafbd95cd375bb114e80831ba50d525 in lucene-solr's branch refs/heads/master from markrmiller
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=91c3f78 ]

        SOLR-10076: Hide keystore and truststore passwords from /admin/info/* outputs.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 91c3f78f8fafbd95cd375bb114e80831ba50d525 in lucene-solr's branch refs/heads/master from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=91c3f78 ] SOLR-10076 : Hide keystore and truststore passwords from /admin/info/* outputs.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        Thanks Mano Kovacs! I fixed the testDisabledRedaction tests (was still the same as when enabled), everything else looks good. Can you put up a backport with it defaulting to off?

        Show
        markrmiller@gmail.com Mark Miller added a comment - Thanks Mano Kovacs ! I fixed the testDisabledRedaction tests (was still the same as when enabled), everything else looks good. Can you put up a backport with it defaulting to off?
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit ddda27e4deab45b9a6bfec8d61319b00f88e27f6 in lucene-solr's branch refs/heads/master from Christine Poerschke
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=ddda27e ]

        SOLR-10076: 'String.format(Locale.ROOT,...' instead of (forbidden API) 'String.format(...)'

        Show
        jira-bot ASF subversion and git services added a comment - Commit ddda27e4deab45b9a6bfec8d61319b00f88e27f6 in lucene-solr's branch refs/heads/master from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=ddda27e ] SOLR-10076 : 'String.format(Locale.ROOT,...' instead of (forbidden API) 'String.format(...)'
        Hide
        manokovacs Mano Kovacs added a comment -

        Thanks Mark Miller! Sorry for the non-finished test, I will be more careful next time.

        I attached the 6x backport with default false configuration. It includes Christine Poerschke's patch about forbidden API. (Did not know about that, thank you!)

        Show
        manokovacs Mano Kovacs added a comment - Thanks Mark Miller ! Sorry for the non-finished test, I will be more careful next time. I attached the 6x backport with default false configuration. It includes Christine Poerschke 's patch about forbidden API. (Did not know about that, thank you!)
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 81e85993bd32f6475e762086bbd4f32dec76ca53 in lucene-solr's branch refs/heads/branch_6x from markrmiller
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=81e8599 ]

        SOLR-10076: Hide keystore and truststore passwords from /admin/info/* outputs.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 81e85993bd32f6475e762086bbd4f32dec76ca53 in lucene-solr's branch refs/heads/branch_6x from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=81e8599 ] SOLR-10076 : Hide keystore and truststore passwords from /admin/info/* outputs.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        Thanks Mano Kovacs! We missed 6.5, so I have to move the changes entry on master and then I'll close.

        Show
        markrmiller@gmail.com Mark Miller added a comment - Thanks Mano Kovacs ! We missed 6.5, so I have to move the changes entry on master and then I'll close.
        Hide
        jira-bot ASF subversion and git services added a comment -

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

        SOLR-10076: Move changes entry to 6.6 release.

        Show
        jira-bot ASF subversion and git services added a comment - Commit e11c86f6e4f85fc4ea561283cf6d2fa8c8df2208 in lucene-solr's branch refs/heads/master from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e11c86f ] SOLR-10076 : Move changes entry to 6.6 release.

          People

          • Assignee:
            markrmiller@gmail.com Mark Miller
            Reporter:
            manokovacs Mano Kovacs
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development