hanicz commented on code in PR #1353:
URL: https://github.com/apache/knox/pull/1353#discussion_r3829065617
##########
gateway-server/src/main/java/org/apache/knox/gateway/util/KnoxCLI.java:
##########
@@ -1866,9 +1865,7 @@ private char[] getSystemPassword(Topology t) throws
NoSuchProviderException, Mis
protected Subject getSubject(Ini config) throws BadSubjectException {
try {
ThreadContext.unbindSubject();
- @SuppressWarnings("deprecation")
- Factory factory = new IniSecurityManagerFactory(config);
- org.apache.shiro.mgt.SecurityManager securityManager =
(org.apache.shiro.mgt.SecurityManager) factory.getInstance();
+ org.apache.shiro.mgt.SecurityManager securityManager = new
BasicIniEnvironment(config).getSecurityManager();
Review Comment:
BasicIniEnvironment should be destroyed after use, this might be a leak.
##########
gateway-provider-security-shiro/src/main/java/org/apache/knox/gateway/shirorealm/KnoxLdapRealm.java:
##########
@@ -744,16 +752,15 @@ protected String getUserDn( final String principal )
throws IllegalArgumentExcep
@Override
protected AuthenticationInfo createAuthenticationInfo(AuthenticationToken
token, Object ldapPrincipal, Object ldapCredentials, LdapContext ldapContext)
throws NamingException {
- HashRequest.Builder builder = new HashRequest.Builder();
- Hash credentialsHash =
hashService.computeHash(builder.setSource(token.getCredentials()).setAlgorithmName(HASHING_ALGORITHM).build());
- return new SimpleAuthenticationInfo(token.getPrincipal(),
credentialsHash.toHex(), credentialsHash.getSalt(), getName());
+ final ByteSource credentialsSalt = new
SecureRandomNumberGenerator().nextBytes();
+ final SimpleHash credentialsHash = new SimpleHash(HASHING_ALGORITHM,
token.getCredentials(), credentialsSalt, HASHING_ITERATIONS);
+ return new SimpleAuthenticationInfo(token.getPrincipal(),
credentialsHash.toHex(), credentialsSalt, getName());
}
- private static String expandTemplate(final String template, final Matcher
input) {
- return expandTemplate(template, input, false);
- }
+ /** How a substituted template value must be escaped for its target context.
*/
+ private enum EscapeMode { NONE, FILTER, DN }
Review Comment:
nit: I think this should be moved to the beginning of the class. Its more
readable since we use it before its introduction. Also NONE is never used.
##########
gateway-provider-security-shiro/src/main/java/org/apache/knox/gateway/shirorealm/KnoxLdapRealm.java:
##########
@@ -762,8 +769,10 @@ private static String expandTemplate( final String
template, final Matcher input
String lookupValue = input.group( lookupIndex );
if (lookupValue == null) {
lookupValue = "";
- } else if (escapeForLdapFilter) {
+ } else if (escapeMode == EscapeMode.FILTER) {
lookupValue = escapeLdapSearchFilterValue(lookupValue);
+ } else if (escapeMode == EscapeMode.DN) {
+ lookupValue = escapeDnValue(lookupValue);
Review Comment:
If someone provides {0} as username it results in an infinite loop which
could lead to denial of service attacks.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
