MilovdZee commented on a change in pull request #336: URL: https://github.com/apache/tomcat/pull/336#discussion_r467140671
########## File path: java/org/apache/catalina/realm/MessageDigestCredentialHandler.java ########## @@ -32,16 +32,13 @@ /** * This credential handler supports the following forms of stored passwords: * <ul> - * <li><b>encodedCredential</b> - a hex encoded digest of the password digested - * using the configured digest</li> - * <li><b>{MD5}encodedCredential</b> - a Base64 encoded MD5 digest of the - * password</li> - * <li><b>{SHA}encodedCredential</b> - a Base64 encoded SHA1 digest of the - * password</li> - * <li><b>{SSHA}encodedCredential</b> - 20 character salt followed by the salted - * SHA1 digest Base64 encoded</li> - * <li><b>salt$iterationCount$encodedCredential</b> - a hex encoded salt, - * iteration code and a hex encoded credential, each separated by $</li> + * <li><b>encodedCredential</b> - a hex encoded digest of the password digested using the configured digest</li> + * <li><b>{MD5}encodedCredential</b> - a Base64 encoded MD5 digest of the password</li> + * <li><b>{SHA}encodedCredential</b> - a Base64 encoded SHA1 digest of the password</li> + * <li><b>{SSHA}encodedCredential</b> - 20 character SHA1 digest Base64 encoded followed by salt</li> + * <li><b>{SSHA2}encodedCredential</b> - 20 character salt followed by the salted digest Base64 encoded</li> Review comment: > What does `{SSHAv2}` add that the existing `salt$iteration$hash` doesn't already provide? I would have preferred to do the SSHA implementation differently but that would break backward compatibility. So I choose SSHAv2... I agree that I'm now using the `salt$iteration$hash` way and indeed SSHAv2 does not add functionality. Adding SSHAv2 introduces yet another option and needs support... Maybe better to refactor the existing code to be more readable? But why allow the algorithm to be set when the SSHA only works when it is set to SHA-1? I did not know that SSHA was something of a standard and I now see that the implementation is indeed correct. Only the javadoc is incorrect. I'll just do a refactor of the implementation and fix the javadoc then. And I won't add the SSHAv2 option. ---------------------------------------------------------------- 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: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org