ctubbsii commented on pull request #1834: URL: https://github.com/apache/accumulo/pull/1834#issuecomment-744607968
> Why not keep the cryptography constants in Constants.java as a pointer to the other parts of the code? The problem with Constants.java is that it makes it hard to modularize code, and it leaks implementation details, and can also make code harder to read, because you have to track variables through multiple layers of indirection. In general, constants should be as local as possible to the code where they get used. We have very little use of these as global constants. > Now the encryption is scattered across vastly different and obscure parts of the code (KeyExtent, VisMetricsGatherer, SystemCredentials and ZkSecurityTool). No. It's not scattered. Addressing these point-by-point: 1. KeyExtent and VisMetricsGatherer are not using this in any cryptographic way. It's just part of it's local serialization code... which can change independently of crypto-related stuff. If it changes, it'd be because made a design to change the way KeyExtent or VisMetricsGatherer serializes those objects... and not because of some system wide decision pertaining to cryptographic strength. We shouldn't use a global constant unless we're trying to enforce some system wide decision, but that's not the case for these two. 2. For ZkSecurityTool, the only thing there now pertaining to SHA-256 is the code that allows us to read legacy formatted entries... which would not change based on any global decision. 3. There are no "SHA-X" constants in SystemCredentials now. There is a hard-coded constant for the salt prefix, because of a quirk with the way the commons-codec Crypt API works. We need to keep a consistent salt so that different servers don't generate different system passwords. Ideally, we'd always use the strongest hash supported by Crypt. We could programmatically extract the prefix associated with the strongest hash. However, I opted instead to create a JUnit test case to make us aware of any changes of that nature, so we can evaluate whether we need to use a different algorithm. Otherwise, it will just use SHA-512, which is Crypt's default. So, really, there's no need for a centralized constant. The only uses for the constants we previously had are either non-crypto purpose serialization, code to interpret legacy values, or necessary to keep a consistent salt that only applies in one area of the code. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
