-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feature/Identity] Adds Basic Auth mechanism via Internal IdP #4798
Changes from 56 commits
eae97da
283ec5c
68fb702
fe5a468
2aaccf0
a154fdb
047bbea
306f590
4a00f72
b57d7b2
6d78226
3d315df
69d6acd
ad58ff5
b87adb7
2134f73
430cc9b
c652fa2
f29ce16
ad66c3a
77c543e
78e4ac1
bfb6e6a
abf05d3
553788d
5ae8043
bb97c92
faae7f7
a2e68bf
27422fe
d09309c
7597e5b
c529707
b988ae2
96c28ca
d87f401
37c51a9
eee71cc
a1ae88a
ac4aab4
9dc87b1
1403ad2
ba32681
ae3ee0b
d6bed20
a34fcc9
2b0b7df
554aad6
41470d9
f22287d
8079479
044317b
109ee9b
cf127a7
927be7f
eddd2a2
6b1cf9d
dbfbcb3
c14ab0c
1ab5c96
4b0aa3a
5ac1c09
5faad80
aac8bfd
4848e59
a747867
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,9 @@ | |
import com.fasterxml.jackson.core.JsonParser; | ||
import com.fasterxml.jackson.core.JsonProcessingException; | ||
import com.fasterxml.jackson.databind.InjectableValues; | ||
import com.fasterxml.jackson.databind.MapperFeature; | ||
import com.fasterxml.jackson.databind.ObjectMapper; | ||
import com.fasterxml.jackson.databind.json.JsonMapper; | ||
import com.fasterxml.jackson.dataformat.yaml.YAMLFactory; | ||
|
||
import java.io.IOException; | ||
|
@@ -18,14 +20,18 @@ | |
* @opensearch.experimental | ||
*/ | ||
public class DefaultObjectMapper { | ||
public static final ObjectMapper objectMapper = new ObjectMapper(); | ||
public static ObjectMapper objectMapper; | ||
public final static ObjectMapper YAML_MAPPER = new ObjectMapper(new YAMLFactory()); | ||
private static final ObjectMapper defaulOmittingObjectMapper = new ObjectMapper(); | ||
|
||
static { | ||
objectMapper = JsonMapper.builder() | ||
.enable(JsonParser.Feature.STRICT_DUPLICATE_DETECTION) | ||
.disable(MapperFeature.CAN_OVERRIDE_ACCESS_MODIFIERS) // to prevent access denied exception by Jackson | ||
.build(); | ||
|
||
objectMapper.setSerializationInclusion(Include.NON_NULL); | ||
// objectMapper.enable(DeserializationFeature.FAIL_ON_TRAILING_TOKENS); | ||
objectMapper.enable(JsonParser.Feature.STRICT_DUPLICATE_DETECTION); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. enable/ disable are deprecated and JsonMapper.builder() is suggested to be used for building a mapper |
||
defaulOmittingObjectMapper.setSerializationInclusion(Include.NON_DEFAULT); | ||
defaulOmittingObjectMapper.enable(JsonParser.Feature.STRICT_DUPLICATE_DETECTION); | ||
YAML_MAPPER.enable(JsonParser.Feature.STRICT_DUPLICATE_DETECTION); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
/* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* The OpenSearch Contributors require contributions made to | ||
* this file be licensed under the Apache-2.0 license or a | ||
* compatible open source license. | ||
*/ | ||
|
||
package org.opensearch.authn; | ||
|
||
public class HttpHeaderToken implements AuthenticationToken { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. HttpHeaderToken seems like a better candidate for an abstract class with BasicToken class implementing, future extensions such as BearerToken. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oo..nice idea..i'll refactor |
||
|
||
public final static String HEADER_NAME = "Authorization"; | ||
private final String headerValue; | ||
|
||
public HttpHeaderToken(final String headerValue) { | ||
this.headerValue = headerValue; | ||
} | ||
|
||
public String getHeaderValue() { | ||
return headerValue; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,13 +10,21 @@ | |
import java.util.Collections; | ||
import java.util.Map; | ||
|
||
/** | ||
* A non-volatile and immutable object in the storage. | ||
* | ||
* @opensearch.experimental | ||
*/ | ||
|
||
public class User { | ||
|
||
@JsonProperty(value = "primary_principal") | ||
private StringPrincipal primaryPrincipal; | ||
|
||
@JsonProperty(value = "hash") | ||
private String bcryptHash; | ||
|
||
@JsonProperty(value = "attributes") | ||
private Map<String, String> attributes = Collections.emptyMap(); | ||
|
||
@JsonProperty(value = "primary_principal") | ||
|
@@ -25,8 +33,8 @@ public StringPrincipal getPrimaryPrincipal() { | |
} | ||
|
||
@JsonProperty(value = "primary_principal") | ||
public void setPrimaryPrincipal(StringPrincipal primaryPrincipal) { | ||
this.primaryPrincipal = primaryPrincipal; | ||
public void setPrimaryPrincipal(StringPrincipal principal) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe swap the function to be just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll revert this change, to use |
||
this.primaryPrincipal = principal; | ||
} | ||
|
||
@JsonProperty(value = "hash") | ||
|
@@ -39,10 +47,12 @@ public void setBcryptHash(String bcryptHash) { | |
this.bcryptHash = bcryptHash; | ||
} | ||
|
||
@JsonProperty(value = "attributes") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really a review comment just curious though, what does this syntax denote? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When serializing/deserializing a JSON object from/to a class, the mapper will know which field to map against when it see There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh gotcha. Thank you for clearing that up. Overall this looks great! |
||
public Map<String, String> getAttributes() { | ||
return attributes; | ||
} | ||
|
||
@JsonProperty(value = "attributes") | ||
public void setAttributes(Map<String, String> attributes) { | ||
this.attributes = attributes; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -109,7 +109,7 @@ protected AuthenticationInfo doGetAuthenticationInfo(AuthenticationToken token) | |
return sai; | ||
} else { | ||
// Bad password | ||
throw new IncorrectCredentialsException(); | ||
throw new IncorrectCredentialsException("Incorrect credentials"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May be worthwhile to make a standardized error message at the top i.e. |
||
} | ||
} | ||
// Don't know what to do with this token | ||
|
@@ -240,7 +240,7 @@ public User removeUser(String primaryPrincipal) { | |
} | ||
|
||
/** | ||
* Generates an Exception message String | ||
* Generates an Exception message | ||
* @param primaryPrincipal to be added to this message | ||
* @return the exception message string | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,8 @@ | |
|
||
import java.io.IOException; | ||
import java.net.URL; | ||
import java.security.AccessController; | ||
import java.security.PrivilegedAction; | ||
import java.util.Iterator; | ||
import java.util.concurrent.ConcurrentHashMap; | ||
import java.util.concurrent.ConcurrentMap; | ||
|
@@ -37,7 +39,27 @@ public static ConcurrentMap<String, User> readUsersAsMap(String pathToInternalUs | |
ObjectNode o = (ObjectNode) subjectNode; | ||
o.put("primary_principal", primaryPrincipal); | ||
String subjectNodeString = DefaultObjectMapper.writeValueAsString((JsonNode) o, false); | ||
User user = DefaultObjectMapper.readValue(subjectNodeString, User.class); | ||
|
||
/** | ||
* Reflects access permissions to prevent jackson databind from throwing InvalidDefinitionException | ||
* Counter-part is added in security.policy to grant jackson-databind ReflectPermission | ||
* | ||
* {@code | ||
* com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Cannot access public org.opensearch.authn.User() | ||
* (from class org.opensearch.authn.User; failed to set access: access denied | ||
* ("java.lang.reflect.ReflectPermission" "suppressAccessChecks") | ||
* } | ||
* | ||
* TODO: Check if there is a better way around this | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add a item to the roadmap for |
||
*/ | ||
User user = AccessController.doPrivileged((PrivilegedAction<User>) () -> { | ||
try { | ||
return DefaultObjectMapper.readValue(subjectNodeString, User.class); | ||
} catch (IOException e) { | ||
throw new RuntimeException(e); | ||
} | ||
}); | ||
|
||
internalUsersMap.put(primaryPrincipal, user); | ||
} | ||
} catch (IOException e) { | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -504,6 +504,7 @@ public ActionModule( | |||||||||
Stream.of(new RestHeaderDefinition(Task.X_OPAQUE_ID, false)) | ||||||||||
).collect(Collectors.toSet()); | ||||||||||
UnaryOperator<RestHandler> restWrapper = null; | ||||||||||
// Only one plugin is allowed to have a rest wrapper. i.e. Security plugin | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a helper comment for other developers to better understand this for loop There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you know if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like developers will hit the exception on 513 that has the relevant details. If you think this is an improvement please consider making a small PR onto main to add this documentation, no reason for it to stay in the feature branch, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cwperks From what I've understood there is no particular order in which plugins are loaded. And @peternied the method getRestHandlerWrapper does contain a note in its javadoc that says the same thing. OpenSearch/server/src/main/java/org/opensearch/plugins/ActionPlugin.java Lines 148 to 151 in 7dc137f
I added it here to help quickly understand the purpose of the for loop. If you still think that it is worthwhile adding this comment to main I can create a PR |
||||||||||
for (ActionPlugin plugin : actionPlugins) { | ||||||||||
UnaryOperator<RestHandler> newRestWrapper = plugin.getRestHandlerWrapper(threadPool.getThreadContext()); | ||||||||||
if (newRestWrapper != null) { | ||||||||||
|
@@ -514,6 +515,7 @@ public ActionModule( | |||||||||
restWrapper = newRestWrapper; | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
mappingRequestValidators = new RequestValidators<>( | ||||||||||
actionPlugins.stream().flatMap(p -> p.mappingRequestValidators().stream()).collect(Collectors.toList()) | ||||||||||
); | ||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
/* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* The OpenSearch Contributors require contributions made to | ||
* this file be licensed under the Apache-2.0 license or a | ||
* compatible open source license. | ||
*/ | ||
|
||
package org.opensearch.identity; | ||
|
||
import org.apache.logging.log4j.LogManager; | ||
import org.apache.logging.log4j.Logger; | ||
import org.apache.shiro.authc.AuthenticationToken; | ||
import org.apache.shiro.authc.UsernamePasswordToken; | ||
import org.opensearch.authn.HttpHeaderToken; | ||
|
||
import java.nio.charset.StandardCharsets; | ||
import java.util.Base64; | ||
|
||
/** | ||
* Extracts Shiro's {@link AuthenticationToken} from different types of auth headers | ||
* | ||
* @opensearch.experimental | ||
*/ | ||
public class AuthenticationTokenHandler { | ||
|
||
private static final Logger logger = LogManager.getLogger(AuthenticationTokenHandler.class); | ||
|
||
/** | ||
* Extracts shiro auth token from the given header token | ||
* @param authenticationToken the token from which to extract | ||
* @return the extracted shiro auth token to be used to perform login | ||
*/ | ||
public static AuthenticationToken extractShiroAuthToken(org.opensearch.authn.AuthenticationToken authenticationToken) { | ||
AuthenticationToken authToken = null; | ||
|
||
if (authenticationToken instanceof HttpHeaderToken) { | ||
final HttpHeaderToken headerToken = (HttpHeaderToken) authenticationToken; | ||
|
||
if (headerToken.getHeaderValue().contains("Basic")) { | ||
authToken = handleBasicAuth(headerToken); | ||
} | ||
|
||
// TODO: check for other type of Headers | ||
} | ||
// TODO: Handle other type of auths, and see if we can use switch case here | ||
|
||
return authToken; | ||
} | ||
|
||
/** | ||
* Returns auth token extracted from basic auth header | ||
* @param token the basic auth token | ||
* @return the extracted auth token | ||
*/ | ||
private static AuthenticationToken handleBasicAuth(final HttpHeaderToken token) { | ||
|
||
final byte[] decodedAuthHeader = Base64.getDecoder().decode(token.getHeaderValue().substring("Basic".length()).trim()); | ||
String decodedHeader = new String(decodedAuthHeader, StandardCharsets.UTF_8); | ||
|
||
final String[] decodedUserNamePassword = decodedHeader.split(":"); | ||
|
||
// Malformed AuthHeader strings | ||
if (decodedUserNamePassword.length != 2) return null; | ||
|
||
logger.info("Logging in as: " + decodedUserNamePassword[0]); | ||
|
||
return new UsernamePasswordToken(decodedUserNamePassword[0], decodedUserNamePassword[1]); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
/* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* The OpenSearch Contributors require contributions made to | ||
* this file be licensed under the Apache-2.0 license or a | ||
* compatible open source license. | ||
*/ | ||
package org.opensearch.identity.internal; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets move this package into |
||
|
||
import org.opensearch.authn.AccessToken; | ||
import org.opensearch.identity.AccessTokenManager; | ||
|
||
/** | ||
* Implementation of access token manager that does not enforce authentication | ||
* | ||
* This class and related classes in this package will not return nulls or fail permissions checks | ||
* | ||
* @opensearch.internal | ||
*/ | ||
public class InternalAccessTokenManager implements AccessTokenManager { | ||
|
||
@Override | ||
public void expireAllTokens() { | ||
// Tokens cannot be expired | ||
} | ||
|
||
@Override | ||
public AccessToken generate() { | ||
return new AccessToken(); | ||
} | ||
|
||
@Override | ||
public AccessToken refresh(final AccessToken token) { | ||
return new AccessToken(); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
/* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* The OpenSearch Contributors require contributions made to | ||
* this file be licensed under the Apache-2.0 license or a | ||
* compatible open source license. | ||
*/ | ||
|
||
package org.opensearch.identity.internal; | ||
|
||
import org.opensearch.authn.realm.InternalRealm; | ||
import org.opensearch.identity.AccessTokenManager; | ||
import org.opensearch.identity.AuthenticationManager; | ||
import org.opensearch.authn.Subject; | ||
import org.apache.shiro.SecurityUtils; | ||
import org.apache.shiro.mgt.DefaultSecurityManager; | ||
import org.apache.shiro.mgt.SecurityManager; | ||
|
||
/** | ||
* Implementation of authentication manager that does not enforce authentication | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't this class support authentication? Should be cleaned up at some point |
||
* | ||
* This class and related classes in this package will not return nulls or fail permissions checks | ||
* | ||
* @opensearch.internal | ||
*/ | ||
public class InternalAuthenticationManager implements AuthenticationManager { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maye add a note or two about what this class does? I am personally not sure what the security manager does after it is created here. |
||
|
||
public InternalAuthenticationManager() { | ||
final SecurityManager securityManager = new DefaultSecurityManager(InternalRealm.INSTANCE); | ||
SecurityUtils.setSecurityManager(securityManager); | ||
} | ||
|
||
public InternalAuthenticationManager(SecurityManager securityManager) { | ||
SecurityUtils.setSecurityManager(securityManager); | ||
} | ||
|
||
@Override | ||
public Subject getSubject() { | ||
return new InternalSubject(SecurityUtils.getSubject()); | ||
} | ||
|
||
@Override | ||
public AccessTokenManager getAccessTokenManager() { | ||
return null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you aren't returning at There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's keep it for now..We might need it for when we implement token vending |
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will let you access internal dependencies of shiro, is that the intention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this change is needed, Could you provide the why of switching from
implementation
toapi
dependency type?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to set it as
api
otherwise it throwserror: package org.apache.shiro.authc does not exist
inside RestController.java#L38For core to be able to use Shiro module imported via authn package, shiro package needs to be public (i.e.
api
). If it is changed toimplementation
shiro import becomes private and we will have to import it separately inserver/build.gradle
to use it.If I change it back to
implementation
here and add a line server/build.gradle likeimplementation 'org.apache.shiro:shiro-core:1.9.1'
the gradle works fine.IMO there is no need to import Shiro twice as it is not a big build performance issue in using
api
vsimplementation
here.I found this helpful: https://stackoverflow.com/questions/44413952/gradle-implementation-vs-api-configuration