Skip to content
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

chore(server): clear context after req done #2470

Merged
merged 28 commits into from
Mar 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
ae90955
chore(server): update swagger info for default server profile
SunnyBoy-WYH Jan 19, 2024
bb67693
chore(server): update swagger info for default server profile
SunnyBoy-WYH Jan 19, 2024
13f916f
fix(server): arthas default bind ip should not be 0.0.0.0
SunnyBoy-WYH Jan 27, 2024
30f1821
Merge branch 'master' into arthas-bind-ip
SunnyBoy-WYH Jan 27, 2024
2519b39
fix(server): arthas default bind ip should not be 0.0.0.0
SunnyBoy-WYH Jan 27, 2024
c787baf
Merge branch 'master' into arthas-bind-ip
SunnyBoy-WYH Feb 21, 2024
7d75e0d
fix(server): fix the bug which promtheus cant collect hg metric
SunnyBoy-WYH Feb 21, 2024
552dcb8
fix(server): fix the arthas default bind ip to 127.0.0.1
SunnyBoy-WYH Feb 27, 2024
ba74aaa
fix(server): clear auth context (TLS) after req done
SunnyBoy-WYH Mar 3, 2024
4f7fc0f
fix(server): clear auth context (TLS) after req done
SunnyBoy-WYH Mar 3, 2024
43288d9
fix: security bug
zyxxoo Jan 19, 2024
6104ed6
improve
zyxxoo Jan 19, 2024
906c0de
fix(server): white list change to fixed and flexable
SunnyBoy-WYH Mar 6, 2024
177e513
Merge branch 'arthas-bind-ip' into fix-context
SunnyBoy-WYH Mar 6, 2024
165b5c8
fix(server): white list change to fixed and flexable
SunnyBoy-WYH Mar 6, 2024
7eeda25
fix(server): white list change to fixed and flexable
SunnyBoy-WYH Mar 6, 2024
5aa1a40
fix(server): white list change to fixed and flexable
SunnyBoy-WYH Mar 6, 2024
6030882
Merge branch 'master' into pr/2470
imbajin Mar 8, 2024
41459f5
remove login from whiteList
imbajin Mar 8, 2024
1435e44
Merge branch 'master' into fix-context
imbajin Mar 9, 2024
3643116
Update AccessLogFilter.java
imbajin Mar 9, 2024
7484ab3
Update AuthenticationFilter.java
imbajin Mar 9, 2024
6a50b99
fix(server): white list change to fixed and flexable
SunnyBoy-WYH Mar 10, 2024
ed2b24a
fix(server): better code
SunnyBoy-WYH Mar 13, 2024
80c1634
fix(server): better code
SunnyBoy-WYH Mar 13, 2024
ed250f2
fix(server): better code
SunnyBoy-WYH Mar 13, 2024
2e3325c
fix(server): better code
SunnyBoy-WYH Mar 18, 2024
5817ccd
tiny improve
imbajin Mar 19, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,9 @@ public String login(@Context GraphManager manager, @PathParam("graph") String gr
checkCreatingBody(jsonLogin);

try {
String token = manager.authManager()
.loginUser(jsonLogin.name, jsonLogin.password);
String token = manager.authManager().loginUser(jsonLogin.name, jsonLogin.password);
HugeGraph g = graph(manager, graph);
return manager.serializer(g)
.writeMap(ImmutableMap.of("token", token));
return manager.serializer(g).writeMap(ImmutableMap.of("token", token));
} catch (AuthenticationException e) {
throw new NotAuthorizedException(e.getMessage(), e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,10 @@
import java.io.IOException;
import java.net.URI;

import org.apache.hugegraph.auth.HugeAuthenticator;
import org.apache.hugegraph.config.HugeConfig;
import org.apache.hugegraph.config.ServerOptions;
import org.apache.hugegraph.core.GraphManager;
import org.apache.hugegraph.metrics.MetricsUtil;
import org.apache.hugegraph.util.Log;
import org.slf4j.Logger;
Expand Down Expand Up @@ -55,6 +57,9 @@ public class AccessLogFilter implements ContainerResponseFilter {
@Context
private jakarta.inject.Provider<HugeConfig> configProvider;

@Context
private jakarta.inject.Provider<GraphManager> managerProvider;

public static boolean needRecordLog(ContainerRequestContext context) {
// TODO: add test for 'path' result ('/gremlin' or 'gremlin')
String path = context.getUriInfo().getPath();
Expand Down Expand Up @@ -114,6 +119,13 @@ public void filter(ContainerRequestContext requestContext,
executeTime, null, method, path, uri.getQuery());
}
}

// Unset the context in "HugeAuthenticator", need distinguish Graph/Auth server lifecycle
GraphManager manager = managerProvider.get();
// TODO: transfer Authorizer if we need after.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need TODO mark anymore since it's done

if (manager.requireAuthentication()) {
manager.unauthorize(requestContext.getSecurityContext());
}
}

private boolean statusOk(int status) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
import org.slf4j.Logger;

import com.alipay.remoting.util.StringUtils;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;

import jakarta.annotation.Priority;
import jakarta.ws.rs.BadRequestException;
Expand All @@ -71,15 +71,15 @@

private static final Logger LOG = Log.logger(AuthenticationFilter.class);

private static final List<String> WHITE_API_LIST = ImmutableList.of(
"graphs/*/auth/login",
private static final AntPathMatcher MATCHER = new AntPathMatcher();
private static final Set<String> FIXED_WHITE_API_SET = ImmutableSet.of(
"versions",
"openapi.json"
);
private static final AntPathMatcher MATCHER = new AntPathMatcher();

private static String whiteIpStatus;
/** Remove auth/login API from whitelist */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one * is ok

private static final Set<String> FLEXIBLE_WHITE_API_SET = ImmutableSet.of();

private static Boolean enabledWhiteIpCheck;
private static final String STRING_WHITE_IP_LIST = "whiteiplist";
private static final String STRING_ENABLE = "enable";

Expand All @@ -94,7 +94,7 @@

@Override
public void filter(ContainerRequestContext context) throws IOException {
if (AuthenticationFilter.isWhiteAPI(context)) {
if (isWhiteAPI(context)) {
return;
}
User user = this.authenticate(context);
Expand All @@ -107,7 +107,7 @@
E.checkState(manager != null, "Context GraphManager is absent");

if (!manager.requireAuthentication()) {
// Return anonymous user with admin role if disable authentication
// Return anonymous user with an admin role if disable authentication
return User.ANONYMOUS;
}

Expand All @@ -121,11 +121,12 @@
}

// Check whiteIp
if (whiteIpStatus == null) {
whiteIpStatus = this.configProvider.get().get(WHITE_IP_STATUS);
if (enabledWhiteIpCheck == null) {
String whiteIpStatus = this.configProvider.get().get(WHITE_IP_STATUS);
enabledWhiteIpCheck = Objects.equals(whiteIpStatus, STRING_ENABLE);
}

if (Objects.equals(whiteIpStatus, STRING_ENABLE) && request != null) {
if (enabledWhiteIpCheck && request != null) {
peer = request.getRemoteAddr() + ":" + request.getRemotePort();
path = request.getRequestURI();

Expand All @@ -134,38 +135,32 @@
boolean whiteIpEnabled = manager.authManager().getWhiteIpStatus();
if (!path.contains(STRING_WHITE_IP_LIST) && whiteIpEnabled &&
!whiteIpList.contains(remoteIp)) {
throw new ForbiddenException(
String.format("Remote ip '%s' is not permitted",
remoteIp));
throw new ForbiddenException(String.format("Remote ip '%s' is not permitted",

Check warning on line 138 in hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java

View check run for this annotation

Codecov / codecov/patch

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java#L138

Added line #L138 was not covered by tests
remoteIp));
}
}

Map<String, String> credentials = new HashMap<>();
// Extract authentication credentials
String auth = context.getHeaderString(HttpHeaders.AUTHORIZATION);
if (auth == null) {
throw new NotAuthorizedException(
"Authentication credentials are required",
"Missing authentication credentials");
throw new NotAuthorizedException("Authentication credentials are required",
"Missing authentication credentials");
}

if (auth.startsWith(BASIC_AUTH_PREFIX)) {
auth = auth.substring(BASIC_AUTH_PREFIX.length());
auth = new String(DatatypeConverter.parseBase64Binary(auth),
Charsets.ASCII_CHARSET);
auth = new String(DatatypeConverter.parseBase64Binary(auth), Charsets.ASCII_CHARSET);
String[] values = auth.split(":");
if (values.length != 2) {
throw new BadRequestException(
"Invalid syntax for username and password");
throw new BadRequestException("Invalid syntax for username and password");

Check warning on line 156 in hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java

View check run for this annotation

Codecov / codecov/patch

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java#L156

Added line #L156 was not covered by tests
}

final String username = values[0];
final String password = values[1];

if (StringUtils.isEmpty(username) ||
StringUtils.isEmpty(password)) {
throw new BadRequestException(
"Invalid syntax for username and password");
if (StringUtils.isEmpty(username) || StringUtils.isEmpty(password)) {
throw new BadRequestException("Invalid syntax for username and password");

Check warning on line 163 in hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java

View check run for this annotation

Codecov / codecov/patch

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java#L163

Added line #L163 was not covered by tests
}

credentials.put(HugeAuthenticator.KEY_USERNAME, username);
Expand All @@ -174,8 +169,7 @@
String token = auth.substring(BEARER_TOKEN_PREFIX.length());
credentials.put(HugeAuthenticator.KEY_TOKEN, token);
} else {
throw new BadRequestException(
"Only HTTP Basic or Bearer authentication is supported");
throw new BadRequestException("Only HTTP Basic or Bearer authentication is supported");

Check warning on line 172 in hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java

View check run for this annotation

Codecov / codecov/patch

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java#L172

Added line #L172 was not covered by tests
}

credentials.put(HugeAuthenticator.KEY_ADDRESS, peer);
Expand All @@ -185,8 +179,7 @@
try {
return manager.authenticate(credentials);
} catch (AuthenticationException e) {
throw new NotAuthorizedException("Authentication failed",
e.getMessage());
throw new NotAuthorizedException("Authentication failed", e.getMessage());
}
}

Expand Down Expand Up @@ -250,7 +243,7 @@
requiredPerm = RequiredPerm.fromPermission(required);

/*
* Replace owner value(it may be a variable) if the permission
* Replace owner value (it may be a variable) if the permission
* format like: "$owner=$graph $action=vertex_write"
*/
String owner = requiredPerm.owner();
Expand Down Expand Up @@ -316,7 +309,11 @@

public static boolean isWhiteAPI(ContainerRequestContext context) {
String path = context.getUriInfo().getPath();
for (String whiteApi : WHITE_API_LIST) {
if (FIXED_WHITE_API_SET.contains(path)) {
return true;

Check warning on line 313 in hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java

View check run for this annotation

Codecov / codecov/patch

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java#L313

Added line #L313 was not covered by tests
}

for (String whiteApi : FLEXIBLE_WHITE_API_SET) {
if (MATCHER.match(whiteApi, path)) {
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
import org.apache.hugegraph.util.E;
import org.apache.tinkerpop.gremlin.groovy.jsr223.dsl.credential.CredentialGraphTokens;

import jakarta.ws.rs.core.SecurityContext;

public class ConfigAuthenticator implements HugeAuthenticator {

public static final String KEY_USERNAME = CredentialGraphTokens.PROPERTY_USERNAME;
Expand Down Expand Up @@ -80,6 +82,10 @@
return new UserWithRole(IdGenerator.of(username), username, role);
}

@Override
public void unauthorize(SecurityContext context) {
}

Check warning on line 87 in hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/ConfigAuthenticator.java

View check run for this annotation

Codecov / codecov/patch

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/ConfigAuthenticator.java#L87

Added line #L87 was not covered by tests

@Override
public AuthManager authManager() {
throw new NotImplementedException("AuthManager is unsupported by ConfigAuthenticator");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@
import org.apache.tinkerpop.gremlin.server.auth.Authenticator;
import org.apache.tinkerpop.shaded.jackson.annotation.JsonProperty;

import jakarta.ws.rs.core.SecurityContext;

public interface HugeAuthenticator extends Authenticator {

String KEY_USERNAME = CredentialGraphTokens.PROPERTY_USERNAME;
Expand All @@ -64,6 +66,8 @@ public interface HugeAuthenticator extends Authenticator {

UserWithRole authenticate(String username, String password, String token);

void unauthorize(SecurityContext context);

AuthManager authManager();

HugeGraph graph();
Expand Down Expand Up @@ -103,10 +107,7 @@ default User authenticate(final Map<String, String> credentials)
}

HugeGraphAuthProxy.logUser(user, credentials.get(KEY_PATH));
/*
* Set authentication context
* TODO: unset context after finishing a request
*/
// TODO: Ensure context lifecycle in GraphServer & AuthServer(#AccessLogFilter)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need TODO mark anymore since it's done

HugeGraphAuthProxy.setContext(new Context(user));

return user;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1752,9 +1752,9 @@ public void apply(Traversal.Admin<?, ?> traversal) {
}

/*
* Verify gremlin-execute permission for user gremlin(in gremlin-
* server-exec worker) and gremlin job(in task worker).
* But don't check permission in rest worker, because the following
* Verify gremlin-execute permission for user gremlin (in gremlin-server-exec worker)
* and gremlin job(in task worker).
* But don't check permission in rest worker because the following
* places need to call traversal():
* 1.vertices/edges rest api
* 2.oltp rest api (like crosspointpath/neighborrank)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

import org.apache.commons.lang.StringUtils;
import org.apache.hugegraph.HugeGraph;
import org.apache.hugegraph.api.filter.AuthenticationFilter;
import org.apache.hugegraph.config.CoreOptions;
import org.apache.hugegraph.config.HugeConfig;
import org.apache.hugegraph.config.ServerOptions;
Expand All @@ -39,6 +40,8 @@
import org.apache.tinkerpop.gremlin.server.auth.AuthenticationException;
import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;

import jakarta.ws.rs.core.SecurityContext;

public class StandardAuthenticator implements HugeAuthenticator {

private static final String INITING_STORE = "initing_store";
Expand Down Expand Up @@ -192,6 +195,11 @@ public UserWithRole authenticate(String username, String password,
userWithRole.username(), role);
}

@Override
public void unauthorize(SecurityContext context) {
HugeGraphAuthProxy.resetContext();
}

@Override
public AuthManager authManager() {
return this.graph().authManager();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ public static synchronized ServerOptions instance() {
"arthas.ip",
"The IP provided by Arthas, it can be accessible from the outside.",
disallowEmpty(),
"0.0.0.0"
"127.0.0.1"
);

public static final ConfigOption<String> ARTHAS_DISABLED_COMMANDS =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@

import com.alipay.sofa.rpc.config.ServerConfig;

import jakarta.ws.rs.core.SecurityContext;

public final class GraphManager {

private static final Logger LOG = Log.logger(GraphManager.class);
Expand Down Expand Up @@ -263,6 +265,10 @@ public HugeAuthenticator.User authenticate(Map<String, String> credentials)
return this.authenticator().authenticate(credentials);
}

public void unauthorize(SecurityContext context) {
this.authenticator().unauthorize(context);
}

public AuthManager authManager() {
return this.authenticator().authManager();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ batch.max_write_threads=0
# configuration of arthas
arthas.telnet_port=8562
arthas.http_port=8561
arthas.ip=0.0.0.0
arthas.ip=127.0.0.1
arthas.disabled_commands=jad

# authentication configs
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,21 +38,35 @@ public void testArthasStart() {

@Test
public void testArthasApi() {
String body = "{\n" +
" \"action\": \"exec\",\n" +
" \"command\": \"version\"\n" +
"}";
// command exec
String execBody = "{\n" +
" \"action\": \"exec\",\n" +
" \"command\": \"version\"\n" +
"}";
RestClient arthasApiClient = new RestClient(ARTHAS_API_BASE_URL, false);
// If the request header contains basic auth,
// and if we are not set auth when arthas attach hg, arthas will auth it and return 401.
// ref:https://arthas.aliyun.com/en/doc/auth.html#configure-username-and-password
Response r = arthasApiClient.post(ARTHAS_API_PATH, body);
String result = assertResponseStatus(200, r);
Response execResponse = arthasApiClient.post(ARTHAS_API_PATH, execBody);
String result = assertResponseStatus(200, execResponse);
assertJsonContains(result, "state");
assertJsonContains(result, "body");

RestClient arthasApiClientWithAuth = new RestClient(ARTHAS_API_BASE_URL);
r = arthasApiClientWithAuth.post(ARTHAS_API_PATH, body);
assertResponseStatus(401, r);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer to add some negative test cases

// command session
String sessionBody = "{\n" +
" \"action\":\"init_session\"\n" +
"}";
Response sessionResponse = arthasApiClient.post(ARTHAS_API_PATH, sessionBody);
String sessionResult = assertResponseStatus(200, sessionResponse);
assertJsonContains(sessionResult, "sessionId");
assertJsonContains(sessionResult, "consumerId");
assertJsonContains(sessionResult, "state");

imbajin marked this conversation as resolved.
Show resolved Hide resolved
// join session: using invalid sessionId
String joinSessionBody = "{\n" +
" \"action\":\"join_session\",\n" +
" \"sessionId\" : \"xxx\"\n" +
"}";
Response joinSessionResponse = arthasApiClient.post(ARTHAS_API_PATH, joinSessionBody);
String joinSessionResult = assertResponseStatus(200, joinSessionResponse);
assertJsonContains(joinSessionResult, "message");
assertJsonContains(joinSessionResult, "state");
}
}
Loading
Loading