Conversation
The use of less robust hashes would prevent a system deploying Accumulo from meeting FIPS requirements so by updating it we avoid forcing downstream users seeking FIPS certification from getting an exception.
ctubbsii
left a comment
ctubbsii
left a comment
There was a problem hiding this comment.
Unfortunately, this will break existing users. For us to accept something like this, we would need to account for upgrades.
See also this old JIRA issue (before we migrated to using GitHub for issue tracking): https://issues.apache.org/jira/browse/ACCUMULO-4044
|
Thanks for the contributions. Just wanted to point out that the accumulo dev mailing list or the Accumulo slack channel (https://the-asf.slack.com/messages/CERNB8NDC) can be used if you have questions on impacts / secondary considerations. |
|
@ctubbsii is your request specifically that the PR be changed to allow for upgrades that maintain user credentials? |
|
e.g. would an approach where the algorithm is configurable but defaults to the current SHA-256 be acceptable? that would allow existing users to continue to work as-is while allowing those with more stringent requirements to make a trade-off decision on starting from a new set of user credentials vs trying to get an exception in a certification process? |
I didn't necessarily have a request. Just an observation that merging this as-is would break users on upgrade, and that it overlapped with an old JIRA issue.
That approach would allow new users to use stronger hashing, while not affecting current users. However, if we make it configurable, that would provide a path for eager security-conscious users to break themselves by switching the algorithm. I think a better approach would be to implement an upgrade path to SHA-512 by detecting the old format when a user authenticates, and the system automatically replacing it with a new ZK node using the crypt(3) format with commons-codec. On startup, we can warn if there are any users in ZK that have passwords hashed with the old format, with instructions to authenticate to upgrade. This would take care of the upgrade issue, ensure stronger security by default, would not allow users to break themselves, and would use a standard serialization that is robust in the face of new algorithms being added in future. If somebody wants to do this, I know I would appreciate it, and I think it would add great value to the project. @BukrosSzabolcs is welcome to tackle it, if they are interested. |
|
Given unlimited volunteer effort I agree that such an implementation would be nice. Can you think of a technical justification for a veto for the simpler incremental change where the hash is configurable, presuming it is paired with a suitable warning on compatibility? |
|
Adding a change to the upgrade code is not that big of deal anymore, since it is now done in one spot. There is already a method for upgrading data in ZK going from 2.0 to 2.1. See |
This seems super reasonable to me. |
I was operating on the assumption that there was already an interested volunteer willing to pursue this a little bit. Is that not the case? I don't think the solution I explained above is that complex, and I'm happy to help as needed. I think this would be a good opportunity to encourage a contributor, if they are interested!
A possible technical justification for a veto would be that the Authenticator component is pluggable already, and there is no reason to implement half measures when a use case for specific Authenticator needs can already be satisfied with what's there now: copy/rename/replace hash/deploy... no change to Accumulo needed if all one wants to do is use SHA-512. My hope in discussing the design of the preferred solution was to encourage a possible contribution from an interested party. If such a party does not exist, I would prefer to do it myself rather than go with an intermediate solution. However, my highest preference was to encourage a contributor effort. Vetoing was the last thing on my mind. |
The problem with this upgrade code is that we can't upgrade our hashes without knowing the original passwords, and we don't know that until we can successfully authenticate using a provided password. We might be able to transform our existing passwords into something that is readable using commons-codec, but I doubt we use the salt exactly the same way. |
My only worry in this line of thinking is that it hinges on your seniority and can act as a form of exclusion. I would hope that we could accept the contribution that Szabolcs wants to do as long as it is not harmful. If you would want to later come along and change this, it would, of course, be in your purview to do so. When you say "I'd rather it be done this way and I'll do it that way if you don't want to", it removes all incentive for the contributor to stay engaged. My $0.02. |
|
I was researching the PR and assessing the impact that Christopher pointed out - he just types faster. I am also concerned with breaking existing installations and would need to see either an upgrade path or have it implemented as an option. I tried to point out that we have a couple of communication channels available where we could work this out while encouraging additional contributions. I did not want to see Szabolcs become frustrated if the PR was not immediately accepted - the code and the contribution are welcome, but there are additional considerations that may not be obvious at the start. If there had been an issue / question raised first, or if there was a dialog, via email or slack, then maybe those could have worked out. Taking the time and effort to create a code change is welcome, but engaging with the community first might alter the approach, make the change easier to integrate and would help ensure that everyone's interest and concerns are also addressed. The intent of the PR is valid and I'm assuming being driven by a requirement, but there also is an obligation to the existing user base. I would not expect the community to accept allowing a change that broke existing installations on an upgrade without providing some path forward. |
|
@EdColeman to clarify, the problem with making the change verbatim as it is now (s/256/512/) is understood and not being contested. The question is: if using sha512 instead of sha256 was something which was configurable (opt-in via the user), and there was a contribution for that, would it be accepted? Christopher had stated that he would prefer that not happen, which is how we got into the current discussion. My point was that there is always a "more elegant" solution which can be had. If there is a simple solution which is not harmful, I would err on accepting such a solution (configurable PW Hash algorithm) and letting those with appetite build their elegant solutions on their own time. |
I think you missed part of what @ctubbsii said, pointing out how that solution would be harmful:
Making it configurable is better but still potentially harmful and incomplete. |
I do not agree that "lack of upgrade support" is a harmful path when behind configuration. The only pain felt is when the user chooses to change this, and that would be pain they choose to self-inflict. Harm, to me, is something that a user could not avoid. Please correct me if I've missed some subtlety as to how a configurable password algorithm would be considered harmful. |
I was not saying "lack of upgrade support" is harmful. I was saying providing the user with a way to easily harm themselves would be when they believe they are improving the security of their system.
Harmful or not, this is dangerous. Best case scenario for an existing cluster would be that Accumulo just won't start. Worst case would be locking users out of a system and having to re-initialize a completely new cluster. I don't know what would happen without testing. |
That is why we have documentation. If you want to see documentation with such a configuration option, please say that. If you want something else to come along with a configuration option, please say exactly what you'd want to see. I'm trying to take what is very lofty "harmful" assertions and get these translated into specific, tangible issues that aren't just "this could be bad". |
I created an GitHub issue: #1788 |
|
@BukrosSzabolcs Since you opened this issue, I would like to ask if you are interested in contributing to the design I proposed, or if you are interested in contributing to the configurable solution proposed by others (with the understanding that it may be replaced by a different implementation in future), or if you have any suggestions for a different design that addresses some of the concerns raised here. Or, would implementing your own alternate implementation of the pluggable Authenticator interface be adequate to meet your needs until Accumulo has updated its implementation? |
|
I would like to thank everyone for the feedback, now I can see this change was a "bit" rushed. Sorry for not communicating my intentions on other channels I was under the impression the PR would suffice. I'll look up the slack channel to avoid issues like this in the future. |
Awesome! Let me know if I can be of any help. And, thank you for your interest in Accumulo, and your patience in the process. Sometimes the process is turbulent, but I'm confident we can all work together to make Accumulo even better in the end! I'll close this current PR, so we can start fresh with a new conversation when things have progressed to a new PR, but feel free to continue to comment here or on the tracking issue Mike created and linked above, if there's more to discuss (or the mailing list or Slack). |
6 participants
The use of less robust hashes would prevent a system deploying Accumulo
from meeting FIPS requirements so by updating it we avoid forcing
downstream users seeking FIPS certification from getting an exception.