You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2022/12/15 01:08:09 UTC
[GitHub] [zookeeper] li4wang opened a new pull request, #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
li4wang opened a new pull request, #1966:
URL: https://github.com/apache/zookeeper/pull/1966
Provide auth support including digest, x509 and IP for admin server commands.
Author: Li Wang [liwang@apple.com](mailto:liwang@apple.com)
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by "li4wang (via GitHub)" <gi...@apache.org>.
li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1116283087
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java:
##########
@@ -89,17 +90,20 @@ public JettyAdminServer() throws AdminServerException, IOException, GeneralSecur
System.getProperty("zookeeper.admin.commandURL", DEFAULT_COMMAND_URL),
Integer.getInteger("zookeeper.admin.httpVersion", DEFAULT_HTTP_VERSION),
Boolean.getBoolean("zookeeper.admin.portUnification"),
- Boolean.getBoolean("zookeeper.admin.forceHttps"));
+ Boolean.getBoolean("zookeeper.admin.forceHttps"),
+ Boolean.getBoolean("zookeeper.admin.needClientAuth"));
}
+ @SuppressWarnings("deprecation")
Review Comment:
Thanks for this. It's much simpler and clean now.
Updated.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] sonatype-lift[bot] commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by "sonatype-lift[bot] (via GitHub)" <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1116371322
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Command.java:
##########
@@ -45,18 +45,17 @@ public interface Command {
*/
String getPrimaryName();
- /**
- * A string documenting this command (e.g., what it does, any arguments it
- * takes).
- */
- String getDoc();
-
/**
* @return true if the command requires an active ZooKeeperServer or a
* synced peer in order to resolve
*/
boolean isServerRequired();
+ /**
+ * @return AuthRequest associated to the command. Null means auth check is not required.
Review Comment:
<picture><img alt="38% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/38/display.svg"></picture>
<b>*[MissingSummary](https://google.github.io/styleguide/javaguide.html#s7.2-summary-fragment):</b>* A summary fragment is required; consider using the value of the @return block as a summary fragment instead.
---
```suggestion
*Returns authRequest associated to the command. Null means auth check is not required.
```
---
<details><summary>âšī¸ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[â[đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=405524795&lift_comment_rating=1)â] - [â[đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=405524795&lift_comment_rating=2)â] - [â[đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=405524795&lift_comment_rating=3)â] - [â[đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=405524795&lift_comment_rating=4)â] - [â[đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=405524795&lift_comment_rating=5)â]
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Command.java:
##########
@@ -45,18 +45,17 @@ public interface Command {
*/
String getPrimaryName();
- /**
- * A string documenting this command (e.g., what it does, any arguments it
- * takes).
- */
- String getDoc();
-
/**
* @return true if the command requires an active ZooKeeperServer or a
Review Comment:
<picture><img alt="38% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/38/display.svg"></picture>
<b>*[MissingSummary](https://google.github.io/styleguide/javaguide.html#s7.2-summary-fragment):</b>* A summary fragment is required; consider using the value of the @return block as a summary fragment instead.
---
```suggestion
*Returns true if the command requires an active ZooKeeperServer or a
```
---
<details><summary>âšī¸ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[â[đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=405524819&lift_comment_rating=1)â] - [â[đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=405524819&lift_comment_rating=2)â] - [â[đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=405524819&lift_comment_rating=3)â] - [â[đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=405524819&lift_comment_rating=4)â] - [â[đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=405524819&lift_comment_rating=5)â]
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by "li4wang (via GitHub)" <gi...@apache.org>.
li4wang commented on PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#issuecomment-1441098414
@tisonkun thanks for reviewing the PR. Please let me know if you have any comments. Thanks.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by GitBox <gi...@apache.org>.
li4wang commented on PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#issuecomment-1352431449
This PR was created on top of admin snapshot PR https://github.com/apache/zookeeper/pull/1943 and admin restore PR https://github.com/apache/zookeeper/pull/1961, as they are currently blocked by the CI build pipeline issue.
The patch for this PR is in [322458e](https://github.com/apache/zookeeper/pull/1966/commits/322458ec920c23cb7bfe778250e1574565dce464)
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by GitBox <gi...@apache.org>.
li4wang commented on PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#issuecomment-1376105969
Thanks @eolivelli for reviewing it.
Can someone else please also take a look at it? Thanks.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by "li4wang (via GitHub)" <gi...@apache.org>.
li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1116270232
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Commands.java:
##########
@@ -532,23 +615,21 @@ public CommandResponse runGet(ZooKeeperServer zkServer, Map<String, String> kwar
*/
public static class RestoreCommand extends PostCommand {
static final String RESPONSE_DATA_LAST_ZXID = "last_zxid";
-
static final String ADMIN_RESTORE_ENABLED = "zookeeper.admin.restore.enabled";
-
private RateLimiter rateLimiter;
public RestoreCommand() {
- super(Arrays.asList("restore", "rest"));
- rateLimiter = new RateLimiter(1, rateLimiterInterval, TimeUnit.MICROSECONDS);
+ super(Arrays.asList("restore", "rest"), true, new AuthRequest(ZooDefs.Perms.ALL, ROOT_PATH));
+ rateLimiter = new RateLimiter(1, rateLimiterInterval, TimeUnit.MILLISECONDS);
Review Comment:
Yes, 300000 MICROSECONDS would be 5 seconds not 5 min.
https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Commands.java#L78
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by "li4wang (via GitHub)" <gi...@apache.org>.
li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1116268141
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Commands.java:
##########
@@ -76,6 +85,8 @@ public class Commands {
static final Logger LOG = LoggerFactory.getLogger(Commands.class);
static final String ADMIN_RATE_LIMITER_INTERVAL = "zookeeper.admin.rateLimiterIntervalInMS";
private static final long rateLimiterInterval = Integer.parseInt(System.getProperty(ADMIN_RATE_LIMITER_INTERVAL, "300000"));
+ static final String AUTH_INFO_SEPARATOR = " ";
+ static final String ROOT_PATH = "/";
Review Comment:
Yes, added comment to make it more clear.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1055081235
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java:
##########
@@ -257,17 +263,84 @@ protected void doGet(
for (Map.Entry<String, String[]> entry : parameterMap.entrySet()) {
kwargs.put(entry.getKey(), entry.getValue()[0]);
}
+ final String authInfo = request.getHeader(HttpHeader.AUTHORIZATION.asString());
// Run the command
- CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs);
+ final CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs, null, authInfo, request);
+ response.setStatus(cmdResponse.getStatusCode());
- // Format and print the output of the command
- CommandOutputter outputter = new JsonOutputter();
- response.setStatus(HttpServletResponse.SC_OK);
+ final Map<String, String> headers = cmdResponse.getHeaders();
+ for (final Map.Entry<String, String> header : headers.entrySet()) {
+ response.addHeader(header.getKey(), header.getValue());
+ }
+ final String clientIP = IPAuthenticationProvider.getClientIPAddress(request);
+ if (cmdResponse.getInputStream() == null) {
+ // Format and print the output of the command
+ CommandOutputter outputter = new JsonOutputter(clientIP);
+ response.setContentType(outputter.getContentType());
+ outputter.output(cmdResponse, response.getWriter());
+ } else {
+ // Stream out the output of the command
+ CommandOutputter outputter = new StreamOutputter(clientIP);
+ response.setContentType(outputter.getContentType());
+ outputter.output(cmdResponse, response.getOutputStream());
+ }
+ }
+
+ /**
+ * Serves HTTP POST requests. It reads request payload as raw data.
+ * It's up to each command to process the payload accordingly.
+ * For example, RestoreCommand uses the payload InputStream directly
+ * to read snapshot data.
+ */
+ @Override
+ protected void doPost(final HttpServletRequest request,
+ final HttpServletResponse response) throws ServletException, IOException {
+ final String cmdName = extractCommandNameFromURL(request, response);
+ if (cmdName != null) {
+ final String authInfo = request.getHeader(HttpHeader.AUTHORIZATION.asString());
+ final CommandResponse cmdResponse = Commands.runCommand(cmdName, zkServer, null, request.getInputStream(), authInfo, request);
Review Comment:
@sonatype-lift ignore
Although http header is user-supplied data, but we don't call out to OS commands in the code base, so it can be ignored.
ProviderRegistry.addOrUpdateProvider(...) is an existing API before this PR.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by "li4wang (via GitHub)" <gi...@apache.org>.
li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1116285563
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/StreamOutputter.java:
##########
@@ -46,7 +46,7 @@ public void output(final CommandResponse response, final OutputStream os) {
try (final InputStream is = response.getInputStream()){
IOUtils.copyBytes(is, os, 1024, true);
} catch (final IOException e) {
- LOG.error("Exception occurred when streaming out data to {}", clientIP, e);
+ LOG.warn("Exception streaming out data to {}", clientIP, e);
Review Comment:
The log level was changed to be consistent with what's in the `JsonOutputter`.
https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JsonOutputter.java#L64
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by "li4wang (via GitHub)" <gi...@apache.org>.
li4wang commented on PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#issuecomment-1430025321
@eolivelli I have added an admin guide for snapshot and restore operations and a test for restoring a cluster from quorum lost. Would you mind take a quick look at it? Thanks.
https://github.com/li4wang/zookeeper/blob/ZOOKEEPER-4639/zookeeper-docs/src/main/resources/markdown/zookeeperSnapshotAndRestore.md
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#issuecomment-1441779306
@tisonkun ping
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by "li4wang (via GitHub)" <gi...@apache.org>.
li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1116290975
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/AuthenticationProvider.java:
##########
@@ -49,6 +52,19 @@ public interface AuthenticationProvider {
*/
KeeperException.Code handleAuthentication(ServerCnxn cnxn, byte[] authData);
+ /**
+ * This method is called when admin server command passes authentication data for this
+ * scheme.
+ *
+ * @param request
+ * the request that contains the authentication information.
+ * @param authData
+ * the authentication data received.
+ * @return Ids
+ * the list of Id. Empty list means not authenticated
+ */
+ List<Id> handleAuthentication(HttpServletRequest request, byte[] authData);
Review Comment:
Yep, good idea. Changed.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by "li4wang (via GitHub)" <gi...@apache.org>.
li4wang commented on PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#issuecomment-1444232930
Thanks @eolivelli and @tisonkun for help reviewing and providing feedback.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1054743602
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java:
##########
@@ -257,17 +263,84 @@ protected void doGet(
for (Map.Entry<String, String[]> entry : parameterMap.entrySet()) {
kwargs.put(entry.getKey(), entry.getValue()[0]);
}
+ final String authInfo = request.getHeader(HttpHeader.AUTHORIZATION.asString());
// Run the command
- CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs);
+ final CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs, null, authInfo, request);
+ response.setStatus(cmdResponse.getStatusCode());
- // Format and print the output of the command
- CommandOutputter outputter = new JsonOutputter();
- response.setStatus(HttpServletResponse.SC_OK);
+ final Map<String, String> headers = cmdResponse.getHeaders();
+ for (final Map.Entry<String, String> header : headers.entrySet()) {
+ response.addHeader(header.getKey(), header.getValue());
+ }
+ final String clientIP = IPAuthenticationProvider.getClientIPAddress(request);
+ if (cmdResponse.getInputStream() == null) {
+ // Format and print the output of the command
+ CommandOutputter outputter = new JsonOutputter(clientIP);
+ response.setContentType(outputter.getContentType());
+ outputter.output(cmdResponse, response.getWriter());
+ } else {
+ // Stream out the output of the command
+ CommandOutputter outputter = new StreamOutputter(clientIP);
+ response.setContentType(outputter.getContentType());
+ outputter.output(cmdResponse, response.getOutputStream());
+ }
+ }
+
+ /**
+ * Serves HTTP POST requests. It reads request payload as raw data.
+ * It's up to each command to process the payload accordingly.
+ * For example, RestoreCommand uses the payload InputStream directly
+ * to read snapshot data.
+ */
+ @Override
+ protected void doPost(final HttpServletRequest request,
+ final HttpServletResponse response) throws ServletException, IOException {
+ final String cmdName = extractCommandNameFromURL(request, response);
+ if (cmdName != null) {
+ final String authInfo = request.getHeader(HttpHeader.AUTHORIZATION.asString());
+ final CommandResponse cmdResponse = Commands.runCommand(cmdName, zkServer, null, request.getInputStream(), authInfo, request);
+ final String clientIP = IPAuthenticationProvider.getClientIPAddress(request);
+ sendJSONResponse(response, cmdResponse, clientIP);
+ }
+ }
+
+ /**
+ * Extracts the command name from URL if it exists otherwise null
+ */
+ private String extractCommandNameFromURL(final HttpServletRequest request,
+ final HttpServletResponse response) throws IOException {
+ String cmd = request.getPathInfo();
+ if (cmd == null || cmd.equals("/")) {
+ printCommandLinks(response);
+ return null;
+ }
+ // Strip leading "/"
+ return cmd.substring(1);
+ }
+
+ /**
+ * Prints the list of URLs to each registered command as response.
+ */
+ private void printCommandLinks(final HttpServletResponse response) throws IOException {
+ for (final String link : commandLinks()) {
+ response.getWriter().println(link);
Review Comment:
@sonatype-lift ignore
the link string is defined in the zk code base, not user defined data, so there should not be vulnerable to XSS.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by "li4wang (via GitHub)" <gi...@apache.org>.
li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1116285563
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/StreamOutputter.java:
##########
@@ -46,7 +46,7 @@ public void output(final CommandResponse response, final OutputStream os) {
try (final InputStream is = response.getInputStream()){
IOUtils.copyBytes(is, os, 1024, true);
} catch (final IOException e) {
- LOG.error("Exception occurred when streaming out data to {}", clientIP, e);
+ LOG.warn("Exception streaming out data to {}", clientIP, e);
Review Comment:
Change the log level so it's consistent with `JsonOutputter`.
https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JsonOutputter.java#L64
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by "li4wang (via GitHub)" <gi...@apache.org>.
li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1116285563
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/StreamOutputter.java:
##########
@@ -46,7 +46,7 @@ public void output(final CommandResponse response, final OutputStream os) {
try (final InputStream is = response.getInputStream()){
IOUtils.copyBytes(is, os, 1024, true);
} catch (final IOException e) {
- LOG.error("Exception occurred when streaming out data to {}", clientIP, e);
+ LOG.warn("Exception streaming out data to {}", clientIP, e);
Review Comment:
The log level was changed to consistent with what's in the `JsonOutputter`.
https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JsonOutputter.java#L64
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1054752855
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java:
##########
@@ -620,6 +623,43 @@ public void deserializeSnapshot(InputArchive ia) throws IOException {
initialized = true;
}
+ /**
+ * Deserialize a snapshot that contains FileHeader from an input archive. It is used by
+ * the admin restore command.
+ *
+ * @param ia the input archive to deserialize from
+ * @param is the CheckInputStream to check integrity
+ *
+ * @throws IOException
+ */
+ public void deserializeSnapshot(final InputArchive ia, final CheckedInputStream is) throws IOException {
+ clear();
Review Comment:
@sonatype-lift ignore
This API is called from a thread-safe context, as the zkDatabase is a local variable in ZookeeperServer.restoreFromSnapshot() method.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1054844144
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java:
##########
@@ -257,17 +263,84 @@ protected void doGet(
for (Map.Entry<String, String[]> entry : parameterMap.entrySet()) {
kwargs.put(entry.getKey(), entry.getValue()[0]);
}
+ final String authInfo = request.getHeader(HttpHeader.AUTHORIZATION.asString());
// Run the command
- CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs);
+ final CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs, null, authInfo, request);
Review Comment:
@sonatype-lift ignore
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by "li4wang (via GitHub)" <gi...@apache.org>.
li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1116258350
##########
zookeeper-docs/src/main/resources/markdown/zookeeperSnapshotAndRestore.md:
##########
@@ -0,0 +1,67 @@
+<!--
+Copyright 2002-2004 The Apache Software Foundation
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+//-->
+
+# ZooKeeper Snapshot and Restore Guide
+
+Zookeeper is designed to withstand machine failures. A Zookeeper cluster can automatically recover
+from temporary failures such as machine reboot. It can also tolerate up to (N-1)/2 permanent
+failures for a cluster of N members due to hardware failures or disk corruption, etc. When a member
+permanently fails, it loses access to the cluster. If the cluster permanently loses more than
+(N-1)/2 members, it disastrously fails and loses quorum. Once the quorum is lost, the cluster
+cannot reach consensus and therefore cannot continue to accept updates.
+
+To recover from such disastrous failures, Zookeeper provides snapshot and restore functionalities to
+restore a cluster from a snapshot.
+
+1. Snapshot and restore operate on the connected server via Admin Server APIs
+1. Snapshot and restore are rate limited to protect the server from being overloaded
+1. Snapshot and restore require authentication and authorization on the root path with ALL permission.
+The supported auth schemas are digest, x509 and IP.
+
+* [Snapshot](#zookeeper_snapshot)
+* [Restore](#zookeeper_restore)
+
+<a name="zookeeper_snapshot"></a>
+
+## Snapshot
+Recovering a cluster needs a snapshot from a ZooKeeper cluster. Users can periodically take
+snapshots from a live server which has the highest zxid and stream out data to a local
+or external storage/file system (e.g., S3).
+
+ ```bash
+ # The snapshot command takes snapshot from the server it connects to and rate limited to once every 5 mins by default
+ curl -H 'Authorization: digest root:root_passwd' http://hostname:adminPort/commands/snapshot?streaming=true --output snapshotFileName
+ ```
+
+<a name="zookeeper_restore"></a>
+## Restore
+
+Restoring a cluster needs a single snapshot as input stream. Restore can be used for recovering a
+cluster for quorum lost or building a brand-new cluster with seed data.
+
+All members should restore using the same snapshot. The following are the recommended steps:
+
+- Blocking client traffic before restore starts
+- For each server
+ - Moving the files in dataDir and dataLogDir to different location to prevent the restored database
Review Comment:
> Is it better to take a snapshot of the latest state?
Yes, added.
> I think blocking client traffic doesn't prohibit admin request?
Yes, updated the doc to be more precise.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by "li4wang (via GitHub)" <gi...@apache.org>.
li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1116268141
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Commands.java:
##########
@@ -76,6 +85,8 @@ public class Commands {
static final Logger LOG = LoggerFactory.getLogger(Commands.class);
static final String ADMIN_RATE_LIMITER_INTERVAL = "zookeeper.admin.rateLimiterIntervalInMS";
private static final long rateLimiterInterval = Integer.parseInt(System.getProperty(ADMIN_RATE_LIMITER_INTERVAL, "300000"));
+ static final String AUTH_INFO_SEPARATOR = " ";
+ static final String ROOT_PATH = "/";
Review Comment:
yes, added comment.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1054844144
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java:
##########
@@ -257,17 +263,84 @@ protected void doGet(
for (Map.Entry<String, String[]> entry : parameterMap.entrySet()) {
kwargs.put(entry.getKey(), entry.getValue()[0]);
}
+ final String authInfo = request.getHeader(HttpHeader.AUTHORIZATION.asString());
// Run the command
- CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs);
+ final CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs, null, authInfo, request);
Review Comment:
@sonatype-lift ignore
ProviderRegistry.addOrUpdateProvider() is not new code from this PR.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] sonatype-lift[bot] commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1049123156
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java:
##########
@@ -257,17 +263,84 @@ protected void doGet(
for (Map.Entry<String, String[]> entry : parameterMap.entrySet()) {
kwargs.put(entry.getKey(), entry.getValue()[0]);
}
+ final String authInfo = request.getHeader(HttpHeader.AUTHORIZATION.asString());
// Run the command
- CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs);
+ final CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs, null, authInfo, request);
+ response.setStatus(cmdResponse.getStatusCode());
- // Format and print the output of the command
- CommandOutputter outputter = new JsonOutputter();
- response.setStatus(HttpServletResponse.SC_OK);
+ final Map<String, String> headers = cmdResponse.getHeaders();
+ for (final Map.Entry<String, String> header : headers.entrySet()) {
+ response.addHeader(header.getKey(), header.getValue());
+ }
+ final String clientIP = IPAuthenticationProvider.getClientIPAddress(request);
+ if (cmdResponse.getInputStream() == null) {
+ // Format and print the output of the command
+ CommandOutputter outputter = new JsonOutputter(clientIP);
+ response.setContentType(outputter.getContentType());
+ outputter.output(cmdResponse, response.getWriter());
+ } else {
+ // Stream out the output of the command
+ CommandOutputter outputter = new StreamOutputter(clientIP);
+ response.setContentType(outputter.getContentType());
+ outputter.output(cmdResponse, response.getOutputStream());
+ }
+ }
+
+ /**
+ * Serves HTTP POST requests. It reads request payload as raw data.
+ * It's up to each command to process the payload accordingly.
+ * For example, RestoreCommand uses the payload InputStream directly
+ * to read snapshot data.
+ */
+ @Override
+ protected void doPost(final HttpServletRequest request,
+ final HttpServletResponse response) throws ServletException, IOException {
+ final String cmdName = extractCommandNameFromURL(request, response);
+ if (cmdName != null) {
+ final String authInfo = request.getHeader(HttpHeader.AUTHORIZATION.asString());
+ final CommandResponse cmdResponse = Commands.runCommand(cmdName, zkServer, null, request.getInputStream(), authInfo, request);
+ final String clientIP = IPAuthenticationProvider.getClientIPAddress(request);
+ sendJSONResponse(response, cmdResponse, clientIP);
+ }
+ }
+
+ /**
+ * Extracts the command name from URL if it exists otherwise null
+ */
+ private String extractCommandNameFromURL(final HttpServletRequest request,
+ final HttpServletResponse response) throws IOException {
+ String cmd = request.getPathInfo();
+ if (cmd == null || cmd.equals("/")) {
+ printCommandLinks(response);
+ return null;
+ }
+ // Strip leading "/"
+ return cmd.substring(1);
+ }
+
+ /**
+ * Prints the list of URLs to each registered command as response.
+ */
+ private void printCommandLinks(final HttpServletResponse response) throws IOException {
+ for (final String link : commandLinks()) {
+ response.getWriter().println(link);
Review Comment:
*[XSS_SERVLET](https://find-sec-bugs.github.io/bugs.htm#XSS_SERVLET):* This use of java/io/PrintWriter.println(Ljava/lang/String;)V could be vulnerable to XSS in the Servlet
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=361419525&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=361419525&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=361419525&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=361419525&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=361419525&lift_comment_rating=5) ]
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java:
##########
@@ -620,6 +623,43 @@ public void deserializeSnapshot(InputArchive ia) throws IOException {
initialized = true;
}
+ /**
+ * Deserialize a snapshot that contains FileHeader from an input archive. It is used by
+ * the admin restore command.
+ *
+ * @param ia the input archive to deserialize from
+ * @param is the CheckInputStream to check integrity
+ *
+ * @throws IOException
+ */
+ public void deserializeSnapshot(final InputArchive ia, final CheckedInputStream is) throws IOException {
+ clear();
Review Comment:
<picture><img alt="8% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/8/display.svg"></picture>
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `ZKDatabase.deserializeSnapshot(...)` indirectly writes to field `this.dataTree` outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=361419637&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=361419637&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=361419637&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=361419637&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=361419637&lift_comment_rating=5) ]
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java:
##########
@@ -257,17 +263,84 @@ protected void doGet(
for (Map.Entry<String, String[]> entry : parameterMap.entrySet()) {
kwargs.put(entry.getKey(), entry.getValue()[0]);
}
+ final String authInfo = request.getHeader(HttpHeader.AUTHORIZATION.asString());
// Run the command
- CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs);
+ final CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs, null, authInfo, request);
+ response.setStatus(cmdResponse.getStatusCode());
- // Format and print the output of the command
- CommandOutputter outputter = new JsonOutputter();
- response.setStatus(HttpServletResponse.SC_OK);
+ final Map<String, String> headers = cmdResponse.getHeaders();
+ for (final Map.Entry<String, String> header : headers.entrySet()) {
+ response.addHeader(header.getKey(), header.getValue());
+ }
+ final String clientIP = IPAuthenticationProvider.getClientIPAddress(request);
+ if (cmdResponse.getInputStream() == null) {
+ // Format and print the output of the command
+ CommandOutputter outputter = new JsonOutputter(clientIP);
+ response.setContentType(outputter.getContentType());
+ outputter.output(cmdResponse, response.getWriter());
+ } else {
+ // Stream out the output of the command
+ CommandOutputter outputter = new StreamOutputter(clientIP);
+ response.setContentType(outputter.getContentType());
+ outputter.output(cmdResponse, response.getOutputStream());
+ }
+ }
+
+ /**
+ * Serves HTTP POST requests. It reads request payload as raw data.
+ * It's up to each command to process the payload accordingly.
+ * For example, RestoreCommand uses the payload InputStream directly
+ * to read snapshot data.
+ */
+ @Override
+ protected void doPost(final HttpServletRequest request,
+ final HttpServletResponse response) throws ServletException, IOException {
+ final String cmdName = extractCommandNameFromURL(request, response);
+ if (cmdName != null) {
+ final String authInfo = request.getHeader(HttpHeader.AUTHORIZATION.asString());
+ final CommandResponse cmdResponse = Commands.runCommand(cmdName, zkServer, null, request.getInputStream(), authInfo, request);
Review Comment:
*SHELL_INJECTION:* UserControlledString(HttpServletRequest.getHeader(...)) at line 301 ~> ClassLoading(ClassLoader.loadClass(...)) in procedure ProviderRegistry.addOrUpdateProvider(...) at line 69.
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=361420208&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=361420208&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=361420208&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=361420208&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=361420208&lift_comment_rating=5) ]
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java:
##########
@@ -257,17 +263,84 @@ protected void doGet(
for (Map.Entry<String, String[]> entry : parameterMap.entrySet()) {
kwargs.put(entry.getKey(), entry.getValue()[0]);
}
+ final String authInfo = request.getHeader(HttpHeader.AUTHORIZATION.asString());
// Run the command
- CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs);
+ final CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs, null, authInfo, request);
Review Comment:
*SHELL_INJECTION:* UserControlledString(HttpServletRequest.getHeader(...)) at line 266 ~> ClassLoading(ClassLoader.loadClass(...)) in procedure ProviderRegistry.addOrUpdateProvider(...) at line 69.
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=361420264&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=361420264&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=361420264&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=361420264&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=361420264&lift_comment_rating=5) ]
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Commands.java:
##########
@@ -92,24 +112,97 @@ public static void registerCommand(Command command) {
*
* @param cmdName
* @param zkServer
- * @param kwargs String-valued keyword arguments to the command
+ * @param kwargs String-valued keyword arguments to the command from HTTP GET request
* (may be null if command requires no additional arguments)
+ * @param inputStream inputStream from HTTP POST request
+ * (null if it's HTTP GET request)
+ * @param authInfo auth info for auth check
+ * (null if command requires no auth check)
+ * @param request HTTP request
* @return Map representing response to command containing at minimum:
* - "command" key containing the command's primary name
* - "error" key containing a String error message or null if no error
*/
public static CommandResponse runCommand(
- String cmdName,
- ZooKeeperServer zkServer,
- Map<String, String> kwargs) {
+ String cmdName,
+ ZooKeeperServer zkServer,
+ Map<String, String> kwargs,
+ InputStream inputStream,
+ String authInfo,
+ HttpServletRequest request) {
Command command = getCommand(cmdName);
if (command == null) {
- return new CommandResponse(cmdName, "Unknown command: " + cmdName);
+ // set the status code to 200 to keep the current behavior of existing commands
+ LOG.warn("Unknown command: {}", cmdName);
+ return new CommandResponse(cmdName, "Unknown command: " + cmdName, HttpServletResponse.SC_OK);
}
if (command.isServerRequired() && (zkServer == null || !zkServer.isRunning())) {
- return new CommandResponse(cmdName, "This ZooKeeper instance is not currently serving requests");
+ // set the status code to 200 to keep the current behavior of existing commands
+ LOG.warn("This ZooKeeper instance is not currently serving requests for command: {}", cmdName);
+ return new CommandResponse(cmdName, "This ZooKeeper instance is not currently serving requests", HttpServletResponse.SC_OK);
}
- return command.run(zkServer, kwargs);
+
+ final AuthRequest authRequest = command.getAuthRequest();
+ if (authRequest != null) {
+ if (authInfo == null) {
+ LOG.warn("Auth info is missing for command: {}", cmdName);
+ return new CommandResponse(cmdName, "Auth info is missing for the command", HttpServletResponse.SC_UNAUTHORIZED);
+ }
+ try {
+ final List<Id> ids = handleAuthentication(request, authInfo);
+ handleAuthorization(zkServer, ids, authRequest.getPermission(), authRequest.getPath());
+ } catch (final KeeperException.AuthFailedException e) {
+ return new CommandResponse(cmdName, "Not authenticated", HttpServletResponse.SC_UNAUTHORIZED);
+ } catch (final KeeperException.NoAuthException e) {
+ return new CommandResponse(cmdName, "Not authorized", HttpServletResponse.SC_FORBIDDEN);
+ } catch (final Exception e) {
+ LOG.warn("Error occurred during auth for command: {}", cmdName, e);
+ return new CommandResponse(cmdName, "Error occurred during auth", HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+ }
+ }
+ return command.run(zkServer, kwargs, inputStream);
+ }
+
+ private static List<Id> handleAuthentication(final HttpServletRequest request, final String authInfo) throws KeeperException.AuthFailedException {
+ final String[] authData = authInfo.split(AUTH_INFO_SEPARATOR);
+ // for IP and x509, auth info only contains the schema and Auth Id will be extracted from HTTP request
+ if (authData.length != 1 && authData.length != 2) {
+ LOG.warn("Invalid auth info: {}", authInfo);
+ throw new KeeperException.AuthFailedException();
+ }
+
+ final String schema = authData[0];
+ final ServerAuthenticationProvider authProvider = ProviderRegistry.getServerProvider(schema);
+ if (authProvider != null) {
+ try {
+ final byte[] auth = authData.length == 2 ? authData[1].getBytes(StandardCharsets.UTF_8) : null;
+ final List<Id> ids = authProvider.handleAuthentication(request, auth);
+ if (ids.isEmpty()) {
+ LOG.warn("Auth Id list is empty: {}", authInfo);
Review Comment:
<picture><img alt="20% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/20/display.svg"></picture>
đŦ 5 similar findings have been found in this PR
---
*[CRLF_INJECTION_LOGS](https://find-sec-bugs.github.io/bugs.htm#CRLF_INJECTION_LOGS):* This use of org/slf4j/Logger.warn(Ljava/lang/String;Ljava/lang/Object;)V might be used to include CRLF characters into log messages
---
<details><summary><b>đ Expand here to view all instances of this finding</b></summary><br/>
<div align=\"center\">
| **File Path** | **Line Number** |
| ------------- | ------------- |
| zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Commands.java | [170](https://github.com/apache/zookeeper/blob/322458ec920c23cb7bfe778250e1574565dce464/zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Commands.java#L170) |
| zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Commands.java | [190](https://github.com/apache/zookeeper/blob/322458ec920c23cb7bfe778250e1574565dce464/zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Commands.java#L190) |
| zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Commands.java | [148](https://github.com/apache/zookeeper/blob/322458ec920c23cb7bfe778250e1574565dce464/zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Commands.java#L148) |
| zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Commands.java | [141](https://github.com/apache/zookeeper/blob/322458ec920c23cb7bfe778250e1574565dce464/zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Commands.java#L141) |
| zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Commands.java | [136](https://github.com/apache/zookeeper/blob/322458ec920c23cb7bfe778250e1574565dce464/zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Commands.java#L136) |
<p><a href="https://lift.sonatype.com/results/github.com/apache/zookeeper/01GM9N64NEZK9Q7F2RCV474B5V?t=FindSecBugs|CRLF_INJECTION_LOGS" target="_blank">Visit the Lift Web Console</a> to find more details in your report.</p></div></details>
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=361419498&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=361419498&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=361419498&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=361419498&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=361419498&lift_comment_rating=5) ]
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by "li4wang (via GitHub)" <gi...@apache.org>.
li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1116270232
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Commands.java:
##########
@@ -532,23 +615,21 @@ public CommandResponse runGet(ZooKeeperServer zkServer, Map<String, String> kwar
*/
public static class RestoreCommand extends PostCommand {
static final String RESPONSE_DATA_LAST_ZXID = "last_zxid";
-
static final String ADMIN_RESTORE_ENABLED = "zookeeper.admin.restore.enabled";
-
private RateLimiter rateLimiter;
public RestoreCommand() {
- super(Arrays.asList("restore", "rest"));
- rateLimiter = new RateLimiter(1, rateLimiterInterval, TimeUnit.MICROSECONDS);
+ super(Arrays.asList("restore", "rest"), true, new AuthRequest(ZooDefs.Perms.ALL, ROOT_PATH));
+ rateLimiter = new RateLimiter(1, rateLimiterInterval, TimeUnit.MILLISECONDS);
Review Comment:
Yes, 300000 MICROSECONDS would be 5 seconds not 5 mins.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by "li4wang (via GitHub)" <gi...@apache.org>.
li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1116262437
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java:
##########
@@ -2069,7 +2069,7 @@ public void dumpMonitorValues(BiConsumer<String, Object> response) {
/**
* Grant or deny authorization to an operation on a node as a function of:
- * @param cnxn : the server connection
+ * @param cnxn : the server connection or null for admin server commands
Review Comment:
Yes, you are right. the cnxn is not required, as the `serverObjs` is not used at all.
We can clean it up separately.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1054858542
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Commands.java:
##########
@@ -92,24 +112,97 @@ public static void registerCommand(Command command) {
*
* @param cmdName
* @param zkServer
- * @param kwargs String-valued keyword arguments to the command
+ * @param kwargs String-valued keyword arguments to the command from HTTP GET request
* (may be null if command requires no additional arguments)
+ * @param inputStream inputStream from HTTP POST request
+ * (null if it's HTTP GET request)
+ * @param authInfo auth info for auth check
+ * (null if command requires no auth check)
+ * @param request HTTP request
* @return Map representing response to command containing at minimum:
* - "command" key containing the command's primary name
* - "error" key containing a String error message or null if no error
*/
public static CommandResponse runCommand(
- String cmdName,
- ZooKeeperServer zkServer,
- Map<String, String> kwargs) {
+ String cmdName,
+ ZooKeeperServer zkServer,
+ Map<String, String> kwargs,
+ InputStream inputStream,
+ String authInfo,
+ HttpServletRequest request) {
Command command = getCommand(cmdName);
if (command == null) {
- return new CommandResponse(cmdName, "Unknown command: " + cmdName);
+ // set the status code to 200 to keep the current behavior of existing commands
+ LOG.warn("Unknown command: {}", cmdName);
+ return new CommandResponse(cmdName, "Unknown command: " + cmdName, HttpServletResponse.SC_OK);
}
if (command.isServerRequired() && (zkServer == null || !zkServer.isRunning())) {
- return new CommandResponse(cmdName, "This ZooKeeper instance is not currently serving requests");
+ // set the status code to 200 to keep the current behavior of existing commands
+ LOG.warn("This ZooKeeper instance is not currently serving requests for command: {}", cmdName);
+ return new CommandResponse(cmdName, "This ZooKeeper instance is not currently serving requests", HttpServletResponse.SC_OK);
}
- return command.run(zkServer, kwargs);
+
+ final AuthRequest authRequest = command.getAuthRequest();
+ if (authRequest != null) {
+ if (authInfo == null) {
+ LOG.warn("Auth info is missing for command: {}", cmdName);
+ return new CommandResponse(cmdName, "Auth info is missing for the command", HttpServletResponse.SC_UNAUTHORIZED);
+ }
+ try {
+ final List<Id> ids = handleAuthentication(request, authInfo);
+ handleAuthorization(zkServer, ids, authRequest.getPermission(), authRequest.getPath());
+ } catch (final KeeperException.AuthFailedException e) {
+ return new CommandResponse(cmdName, "Not authenticated", HttpServletResponse.SC_UNAUTHORIZED);
+ } catch (final KeeperException.NoAuthException e) {
+ return new CommandResponse(cmdName, "Not authorized", HttpServletResponse.SC_FORBIDDEN);
+ } catch (final Exception e) {
+ LOG.warn("Error occurred during auth for command: {}", cmdName, e);
+ return new CommandResponse(cmdName, "Error occurred during auth", HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+ }
+ }
+ return command.run(zkServer, kwargs, inputStream);
+ }
+
+ private static List<Id> handleAuthentication(final HttpServletRequest request, final String authInfo) throws KeeperException.AuthFailedException {
+ final String[] authData = authInfo.split(AUTH_INFO_SEPARATOR);
+ // for IP and x509, auth info only contains the schema and Auth Id will be extracted from HTTP request
+ if (authData.length != 1 && authData.length != 2) {
+ LOG.warn("Invalid auth info: {}", authInfo);
+ throw new KeeperException.AuthFailedException();
+ }
+
+ final String schema = authData[0];
+ final ServerAuthenticationProvider authProvider = ProviderRegistry.getServerProvider(schema);
+ if (authProvider != null) {
+ try {
+ final byte[] auth = authData.length == 2 ? authData[1].getBytes(StandardCharsets.UTF_8) : null;
+ final List<Id> ids = authProvider.handleAuthentication(request, auth);
+ if (ids.isEmpty()) {
+ LOG.warn("Auth Id list is empty: {}", authInfo);
Review Comment:
fixed
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] sonatype-lift[bot] commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1054844182
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java:
##########
@@ -257,17 +263,84 @@ protected void doGet(
for (Map.Entry<String, String[]> entry : parameterMap.entrySet()) {
kwargs.put(entry.getKey(), entry.getValue()[0]);
}
+ final String authInfo = request.getHeader(HttpHeader.AUTHORIZATION.asString());
// Run the command
- CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs);
+ final CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs, null, authInfo, request);
Review Comment:
I've recorded this as ignored for this pull request.
If you change your mind, just comment `@sonatype-lift unignore`.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by "li4wang (via GitHub)" <gi...@apache.org>.
li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1116265331
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/CommandBase.java:
##########
@@ -28,22 +28,31 @@ public abstract class CommandBase implements Command {
private final Set<String> names;
private final String doc;
private final boolean serverRequired;
+ private final AuthRequest authRequest;
/**
* @param names The possible names of this command, with the primary name first.
*/
protected CommandBase(List<String> names) {
- this(names, true, null);
+ this(names, true);
}
protected CommandBase(List<String> names, boolean serverRequired) {
this(names, serverRequired, null);
}
- protected CommandBase(List<String> names, boolean serverRequired, String doc) {
+ protected CommandBase(List<String> names, boolean serverRequired, AuthRequest authRequest) {
+ this(names, serverRequired, null, authRequest);
+ }
+
+ protected CommandBase(List<String> names, boolean serverRequired, String doc, AuthRequest authRequest) {
Review Comment:
Doc is always null and not be used. Removed.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tisonkun commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1115748846
##########
zookeeper-docs/src/main/resources/markdown/zookeeperSnapshotAndRestore.md:
##########
@@ -0,0 +1,67 @@
+<!--
+Copyright 2002-2004 The Apache Software Foundation
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+//-->
+
+# ZooKeeper Snapshot and Restore Guide
+
+Zookeeper is designed to withstand machine failures. A Zookeeper cluster can automatically recover
+from temporary failures such as machine reboot. It can also tolerate up to (N-1)/2 permanent
+failures for a cluster of N members due to hardware failures or disk corruption, etc. When a member
+permanently fails, it loses access to the cluster. If the cluster permanently loses more than
+(N-1)/2 members, it disastrously fails and loses quorum. Once the quorum is lost, the cluster
+cannot reach consensus and therefore cannot continue to accept updates.
+
+To recover from such disastrous failures, Zookeeper provides snapshot and restore functionalities to
+restore a cluster from a snapshot.
+
+1. Snapshot and restore operate on the connected server via Admin Server APIs
+1. Snapshot and restore are rate limited to protect the server from being overloaded
+1. Snapshot and restore require authentication and authorization on the root path with ALL permission.
+The supported auth schemas are digest, x509 and IP.
+
+* [Snapshot](#zookeeper_snapshot)
+* [Restore](#zookeeper_restore)
+
+<a name="zookeeper_snapshot"></a>
+
+## Snapshot
+Recovering a cluster needs a snapshot from a ZooKeeper cluster. Users can periodically take
+snapshots from a live server which has the highest zxid and stream out data to a local
+or external storage/file system (e.g., S3).
+
+ ```bash
+ # The snapshot command takes snapshot from the server it connects to and rate limited to once every 5 mins by default
+ curl -H 'Authorization: digest root:root_passwd' http://hostname:adminPort/commands/snapshot?streaming=true --output snapshotFileName
+ ```
+
+<a name="zookeeper_restore"></a>
+## Restore
+
+Restoring a cluster needs a single snapshot as input stream. Restore can be used for recovering a
+cluster for quorum lost or building a brand-new cluster with seed data.
+
+All members should restore using the same snapshot. The following are the recommended steps:
+
+- Blocking client traffic before restore starts
+- For each server
+ - Moving the files in dataDir and dataLogDir to different location to prevent the restored database
Review Comment:
Is it better to take a snapshot of the latest state? I think blocking client traffic doesn't prohibit admin request?
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java:
##########
@@ -2069,7 +2069,7 @@ public void dumpMonitorValues(BiConsumer<String, Object> response) {
/**
* Grant or deny authorization to an operation on a node as a function of:
- * @param cnxn : the server connection
+ * @param cnxn : the server connection or null for admin server commands
Review Comment:
Actually, I found `cnxn` is not required for this method since `ServerObjs serverObjs` is not effectively used in the method call.
But it can be out of the scope so this comment is just a hint.
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/CommandBase.java:
##########
@@ -28,22 +28,31 @@ public abstract class CommandBase implements Command {
private final Set<String> names;
private final String doc;
private final boolean serverRequired;
+ private final AuthRequest authRequest;
/**
* @param names The possible names of this command, with the primary name first.
*/
protected CommandBase(List<String> names) {
- this(names, true, null);
+ this(names, true);
}
protected CommandBase(List<String> names, boolean serverRequired) {
this(names, serverRequired, null);
}
- protected CommandBase(List<String> names, boolean serverRequired, String doc) {
+ protected CommandBase(List<String> names, boolean serverRequired, AuthRequest authRequest) {
+ this(names, serverRequired, null, authRequest);
+ }
+
+ protected CommandBase(List<String> names, boolean serverRequired, String doc, AuthRequest authRequest) {
Review Comment:
nit: It seems `doc` is always null and can be safely removed.
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/StreamOutputter.java:
##########
@@ -46,7 +46,7 @@ public void output(final CommandResponse response, final OutputStream os) {
try (final InputStream is = response.getInputStream()){
IOUtils.copyBytes(is, os, 1024, true);
} catch (final IOException e) {
- LOG.error("Exception occurred when streaming out data to {}", clientIP, e);
+ LOG.warn("Exception streaming out data to {}", clientIP, e);
Review Comment:
Why do you make this change?
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/AuthenticationProvider.java:
##########
@@ -49,6 +52,19 @@ public interface AuthenticationProvider {
*/
KeeperException.Code handleAuthentication(ServerCnxn cnxn, byte[] authData);
+ /**
+ * This method is called when admin server command passes authentication data for this
+ * scheme.
+ *
+ * @param request
+ * the request that contains the authentication information.
+ * @param authData
+ * the authentication data received.
+ * @return Ids
+ * the list of Id. Empty list means not authenticated
+ */
+ List<Id> handleAuthentication(HttpServletRequest request, byte[] authData);
Review Comment:
`default` to empty list and thus reduce some impls?
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java:
##########
@@ -89,17 +90,20 @@ public JettyAdminServer() throws AdminServerException, IOException, GeneralSecur
System.getProperty("zookeeper.admin.commandURL", DEFAULT_COMMAND_URL),
Integer.getInteger("zookeeper.admin.httpVersion", DEFAULT_HTTP_VERSION),
Boolean.getBoolean("zookeeper.admin.portUnification"),
- Boolean.getBoolean("zookeeper.admin.forceHttps"));
+ Boolean.getBoolean("zookeeper.admin.forceHttps"),
+ Boolean.getBoolean("zookeeper.admin.needClientAuth"));
}
+ @SuppressWarnings("deprecation")
Review Comment:
Here is a patch to reduce complex:
```diff
diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java
index 2c3bb723d..a237e4c3b 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java
@@ -94,7 +94,6 @@ public JettyAdminServer() throws AdminServerException, IOException, GeneralSecur
Boolean.getBoolean("zookeeper.admin.needClientAuth"));
}
- @SuppressWarnings("deprecation")
public JettyAdminServer(
String address,
int port,
@@ -148,15 +147,12 @@ public JettyAdminServer(
throw e;
}
- SslContextFactory sslContextFactory = new SslContextFactory.Server();
+ SslContextFactory.Server sslContextFactory = new SslContextFactory.Server();
sslContextFactory.setKeyStore(keyStore);
sslContextFactory.setKeyStorePassword(privateKeyPassword);
sslContextFactory.setTrustStore(trustStore);
sslContextFactory.setTrustStorePassword(certAuthPassword);
- if (needClientAuth) {
- // setNeedClientAuth() of SslContextFactory.Server is not a deprecated API
- sslContextFactory.setNeedClientAuth(true);
- }
+ sslContextFactory.setNeedClientAuth(needClientAuth);
if (forceHttps) {
connector = new ServerConnector(server,
```
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Commands.java:
##########
@@ -532,23 +615,21 @@ public CommandResponse runGet(ZooKeeperServer zkServer, Map<String, String> kwar
*/
public static class RestoreCommand extends PostCommand {
static final String RESPONSE_DATA_LAST_ZXID = "last_zxid";
-
static final String ADMIN_RESTORE_ENABLED = "zookeeper.admin.restore.enabled";
-
private RateLimiter rateLimiter;
public RestoreCommand() {
- super(Arrays.asList("restore", "rest"));
- rateLimiter = new RateLimiter(1, rateLimiterInterval, TimeUnit.MICROSECONDS);
+ super(Arrays.asList("restore", "rest"), true, new AuthRequest(ZooDefs.Perms.ALL, ROOT_PATH));
+ rateLimiter = new RateLimiter(1, rateLimiterInterval, TimeUnit.MILLISECONDS);
Review Comment:
And here we fix a bug?
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Commands.java:
##########
@@ -76,6 +85,8 @@ public class Commands {
static final Logger LOG = LoggerFactory.getLogger(Commands.class);
static final String ADMIN_RATE_LIMITER_INTERVAL = "zookeeper.admin.rateLimiterIntervalInMS";
private static final long rateLimiterInterval = Integer.parseInt(System.getProperty(ADMIN_RATE_LIMITER_INTERVAL, "300000"));
+ static final String AUTH_INFO_SEPARATOR = " ";
+ static final String ROOT_PATH = "/";
Review Comment:
VisibleForTesting?
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#issuecomment-1443838406
merge to master branch
great work @li4wang
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli merged pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli merged PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org