You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/02/06 00:14:50 UTC
[GitHub] [incubator-pinot] apucher opened a new pull request #6552: add optional http basic auth to pinot broker
apucher opened a new pull request #6552:
URL: https://github.com/apache/incubator-pinot/pull/6552
## Description
Added support for an optional HTTP basic auth provider to pinot-broker, which enables user- and table-level authentication of incoming queries.
HTTP basic auth configuration uses pinot's regular properties format. For example:
> pinot.broker.access.control.class=org.apache.pinot.broker.broker.BasicAuthAccessControlFactory
> pinot.broker.access.control.principals=admin,user
> pinot.broker.access.control.principals.admin.password=verysecret
> pinot.broker.access.control.principals.user.password=secret
> pinot.broker.access.control.principals.user.tables=myUserTable,myOtherUserTable
## Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
* [ ] Yes (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR fix a zero-downtime upgrade introduced earlier?
* [ ] Yes (Please label this as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR otherwise need attention when creating release notes? Things to consider:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
* [x] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
## Release Notes
Added support for an optional HTTP basic auth provider to pinot-broker, which enables user- and table-level authentication of incoming queries.
## Documentation
If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6552: add optional http basic auth to pinot broker
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6552:
URL: https://github.com/apache/incubator-pinot/pull/6552#discussion_r572546437
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,159 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+package org.apache.pinot.broker.broker;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.broker.api.RequesterIdentity;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+import java.util.*;
+import java.util.stream.Collectors;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the "pinot.broker.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * pinot.broker.access.control.principals=admin123,user456
+ * pinot.broker.access.control.principals.admin123.password=verysecret
+ * pinot.broker.access.control.principals.user456.password=kindasecret
+ * pinot.broker.access.control.principals.user456.tables=stuff,lessImportantStuff
+ * </pre>
+ */
+public class BasicAuthAccessControlFactory extends AccessControlFactory {
+ private static final String PRINCIPALS = "principals";
+ private static final String PASSWORD = "password";
+ private static final String TABLES = "tables";
+ private static final String TABLES_ALL = "*";
+
+ private static final String HEADER_AUTHORIZATION = "authorization";
+
+ private AccessControl _accessControl;
+
+ public BasicAuthAccessControlFactory() {
+ // left blank
+ }
+
+ public void init(PinotConfiguration configuration) {
+ String principalNames = configuration.getProperty(PRINCIPALS);
+ Preconditions.checkArgument(StringUtils.isNotBlank(principalNames), "must provide principals");
+
+ List<BasicAuthPrincipal> principals = Arrays.stream(principalNames.split(",")).map(rawName -> {
+ String name = rawName.trim();
+ Preconditions.checkArgument(StringUtils.isNotBlank(name), "%s is not a valid name", name);
+
+ String password = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, PASSWORD));
+ Preconditions.checkArgument(StringUtils.isNotBlank(password), "must provide a password for %s", name);
+
+ Set<String> tables = new HashSet<>();
+ String tableNames = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, TABLES));
+ if (StringUtils.isNotBlank(tableNames) && !TABLES_ALL.equals(tableNames)) {
+ tables.addAll(Arrays.asList(tableNames.split(",")));
+ }
+
+ return new BasicAuthPrincipal(name, toToken(name, password), tables);
+ }).collect(Collectors.toList());
+
+ _accessControl = new BasicAuthAccessControl(principals);
+ }
+
+ public AccessControl create() {
+ return _accessControl;
+ }
+
+ /**
+ * Access Control using header-based basic http authentication
+ */
+ private static class BasicAuthAccessControl implements AccessControl {
+ private final Map<String, BasicAuthPrincipal> _principals;
+
+ public BasicAuthAccessControl(Collection<BasicAuthPrincipal> principals) {
+ this._principals = principals.stream().collect(Collectors.toMap(BasicAuthPrincipal::getToken, p -> p));
+ }
+
+ @Override
+ public boolean hasAccess(RequesterIdentity requesterIdentity, BrokerRequest brokerRequest) {
+ Preconditions.checkArgument(requesterIdentity instanceof HttpRequesterIdentity, "HttpRequesterIdentity required");
+ HttpRequesterIdentity identity = (HttpRequesterIdentity) requesterIdentity;
+
+ Collection<String> tokens = identity.getHttpHeaders().get(HEADER_AUTHORIZATION);
+ Optional<BasicAuthPrincipal> principalOpt = tokens.stream().map(BasicAuthAccessControlFactory::normalizeToken)
+ .map(_principals::get).filter(Objects::nonNull).findFirst();
+
+ if (!principalOpt.isPresent()) {
+ // no matching token? reject
+ return false;
+ }
+
+ BasicAuthPrincipal principal = principalOpt.get();
+ if (principal.getTables().isEmpty()
+ || !brokerRequest.isSetQuerySource()
+ || !brokerRequest.getQuerySource().isSetTableName()) {
+ // no table restrictions? accept
+ return true;
+ }
+
+ return principal.getTables().contains(brokerRequest.getQuerySource().getTableName());
+ }
+ }
+
+ /**
+ * Container object for basic auth principal
+ */
+ private static class BasicAuthPrincipal {
+ private final String _name;
+ private final String _token;
+ private final Set<String> _tables;
+
+ public BasicAuthPrincipal(String name, String token, Set<String> tables) {
+ this._name = name;
+ this._token = token;
+ this._tables = tables;
+ }
+
+ public String getName() {
+ return _name;
+ }
+
+ public Set<String> getTables() {
+ return _tables;
+ }
+
+ public String getToken() {
+ return _token;
+ }
+ }
+
+ private static String toToken(String name, String password) {
+ String identifier = String.format("%s:%s", name, password);
+ return normalizeToken(String.format("Basic %s", Base64.getEncoder().encodeToString(identifier.getBytes())));
+ }
+
+ private static String normalizeToken(String token) {
+ if (token == null) {
Review comment:
technically, null-value is possible in header. see line 102.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on pull request #6552: add optional http basic auth to pinot broker
Posted by GitBox <gi...@apache.org>.
apucher commented on pull request #6552:
URL: https://github.com/apache/incubator-pinot/pull/6552#issuecomment-776178335
re the first comment (which I somehow can't respond to inline):
`.map(_principals::get)` is fine if the key, i.e. the token, is null. There's even a unit tests for this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6552: add optional http basic auth to pinot broker
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6552:
URL: https://github.com/apache/incubator-pinot/pull/6552#discussion_r573240061
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,168 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+package org.apache.pinot.broker.broker;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.broker.api.RequesterIdentity;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+import java.nio.charset.StandardCharsets;
Review comment:
Weird.. The setting should not split the imports into 2 parts
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6552: add optional http basic auth to pinot broker
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6552:
URL: https://github.com/apache/incubator-pinot/pull/6552#discussion_r573264456
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,168 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+package org.apache.pinot.broker.broker;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.broker.api.RequesterIdentity;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+import java.nio.charset.StandardCharsets;
Review comment:
meh. 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6552: add optional http basic auth to pinot broker
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6552:
URL: https://github.com/apache/incubator-pinot/pull/6552#discussion_r572556178
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,159 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+package org.apache.pinot.broker.broker;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.broker.api.RequesterIdentity;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+import java.util.*;
+import java.util.stream.Collectors;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the "pinot.broker.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * pinot.broker.access.control.principals=admin123,user456
+ * pinot.broker.access.control.principals.admin123.password=verysecret
+ * pinot.broker.access.control.principals.user456.password=kindasecret
+ * pinot.broker.access.control.principals.user456.tables=stuff,lessImportantStuff
+ * </pre>
+ */
+public class BasicAuthAccessControlFactory extends AccessControlFactory {
+ private static final String PRINCIPALS = "principals";
+ private static final String PASSWORD = "password";
+ private static final String TABLES = "tables";
+ private static final String TABLES_ALL = "*";
+
+ private static final String HEADER_AUTHORIZATION = "authorization";
+
+ private AccessControl _accessControl;
+
+ public BasicAuthAccessControlFactory() {
+ // left blank
+ }
+
+ public void init(PinotConfiguration configuration) {
+ String principalNames = configuration.getProperty(PRINCIPALS);
+ Preconditions.checkArgument(StringUtils.isNotBlank(principalNames), "must provide principals");
+
+ List<BasicAuthPrincipal> principals = Arrays.stream(principalNames.split(",")).map(rawName -> {
+ String name = rawName.trim();
+ Preconditions.checkArgument(StringUtils.isNotBlank(name), "%s is not a valid name", name);
+
+ String password = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, PASSWORD));
+ Preconditions.checkArgument(StringUtils.isNotBlank(password), "must provide a password for %s", name);
+
+ Set<String> tables = new HashSet<>();
+ String tableNames = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, TABLES));
+ if (StringUtils.isNotBlank(tableNames) && !TABLES_ALL.equals(tableNames)) {
+ tables.addAll(Arrays.asList(tableNames.split(",")));
+ }
+
+ return new BasicAuthPrincipal(name, toToken(name, password), tables);
+ }).collect(Collectors.toList());
+
+ _accessControl = new BasicAuthAccessControl(principals);
+ }
+
+ public AccessControl create() {
+ return _accessControl;
+ }
+
+ /**
+ * Access Control using header-based basic http authentication
+ */
+ private static class BasicAuthAccessControl implements AccessControl {
+ private final Map<String, BasicAuthPrincipal> _principals;
+
+ public BasicAuthAccessControl(Collection<BasicAuthPrincipal> principals) {
+ this._principals = principals.stream().collect(Collectors.toMap(BasicAuthPrincipal::getToken, p -> p));
+ }
+
+ @Override
+ public boolean hasAccess(RequesterIdentity requesterIdentity, BrokerRequest brokerRequest) {
+ Preconditions.checkArgument(requesterIdentity instanceof HttpRequesterIdentity, "HttpRequesterIdentity required");
+ HttpRequesterIdentity identity = (HttpRequesterIdentity) requesterIdentity;
+
+ Collection<String> tokens = identity.getHttpHeaders().get(HEADER_AUTHORIZATION);
+ Optional<BasicAuthPrincipal> principalOpt = tokens.stream().map(BasicAuthAccessControlFactory::normalizeToken)
+ .map(_principals::get).filter(Objects::nonNull).findFirst();
+
+ if (!principalOpt.isPresent()) {
+ // no matching token? reject
+ return false;
+ }
+
+ BasicAuthPrincipal principal = principalOpt.get();
+ if (principal.getTables().isEmpty()
+ || !brokerRequest.isSetQuerySource()
+ || !brokerRequest.getQuerySource().isSetTableName()) {
+ // no table restrictions? accept
+ return true;
+ }
+
+ return principal.getTables().contains(brokerRequest.getQuerySource().getTableName());
+ }
+ }
+
+ /**
+ * Container object for basic auth principal
+ */
+ private static class BasicAuthPrincipal {
+ private final String _name;
+ private final String _token;
+ private final Set<String> _tables;
+
+ public BasicAuthPrincipal(String name, String token, Set<String> tables) {
+ this._name = name;
+ this._token = token;
+ this._tables = tables;
+ }
+
+ public String getName() {
+ return _name;
+ }
+
+ public Set<String> getTables() {
+ return _tables;
+ }
+
+ public String getToken() {
+ return _token;
+ }
+ }
+
+ private static String toToken(String name, String password) {
+ String identifier = String.format("%s:%s", name, password);
+ return normalizeToken(String.format("Basic %s", Base64.getEncoder().encodeToString(identifier.getBytes())));
+ }
+
+ private static String normalizeToken(String token) {
+ if (token == null) {
+ return null;
+ }
+ return token.trim().replace("=", "");
Review comment:
btw StringUtils literally calls replace :P
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6552: add optional http basic auth to pinot broker
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6552:
URL: https://github.com/apache/incubator-pinot/pull/6552#discussion_r573240997
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,170 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+package org.apache.pinot.broker.broker;
+
+import com.google.common.base.Preconditions;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.broker.api.RequesterIdentity;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+import java.nio.charset.StandardCharsets;
+import java.util.*;
+import java.util.stream.Collectors;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the "pinot.broker.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * pinot.broker.access.control.principals=admin123,user456
+ * pinot.broker.access.control.principals.admin123.password=verysecret
+ * pinot.broker.access.control.principals.user456.password=kindasecret
+ * pinot.broker.access.control.principals.user456.tables=stuff,lessImportantStuff
+ * </pre>
+ */
+public class BasicAuthAccessControlFactory extends AccessControlFactory {
+ private static final String PRINCIPALS = "principals";
+ private static final String PASSWORD = "password";
+ private static final String TABLES = "tables";
+ private static final String TABLES_ALL = "*";
+
+ private static final String HEADER_AUTHORIZATION = "authorization";
+
+ private AccessControl _accessControl;
+
+ public BasicAuthAccessControlFactory() {
+ // left blank
+ }
+
+ public void init(PinotConfiguration configuration) {
+ String principalNames = configuration.getProperty(PRINCIPALS);
+ Preconditions.checkArgument(StringUtils.isNotBlank(principalNames), "must provide principals");
+
+ List<BasicAuthPrincipal> principals = Arrays.stream(principalNames.split(",")).map(rawName -> {
+ String name = rawName.trim();
+ Preconditions.checkArgument(StringUtils.isNotBlank(name), "%s is not a valid name", name);
+
+ String password = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, PASSWORD));
+ Preconditions.checkArgument(StringUtils.isNotBlank(password), "must provide a password for %s", name);
+
+ Set<String> tables = new HashSet<>();
+ String tableNames = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, TABLES));
+ if (StringUtils.isNotBlank(tableNames) && !TABLES_ALL.equals(tableNames)) {
+ tables.addAll(Arrays.asList(tableNames.split(",")));
+ }
+
+ return new BasicAuthPrincipal(name, toToken(name, password), tables);
+ }).collect(Collectors.toList());
+
+ _accessControl = new BasicAuthAccessControl(principals);
+ }
+
+ public AccessControl create() {
+ return _accessControl;
+ }
+
+ /**
+ * Access Control using header-based basic http authentication
+ */
+ private static class BasicAuthAccessControl implements AccessControl {
+ private final Map<String, BasicAuthPrincipal> _principals;
+
+ public BasicAuthAccessControl(Collection<BasicAuthPrincipal> principals) {
+ _principals = principals.stream().collect(Collectors.toMap(BasicAuthPrincipal::getToken, p -> p));
+ }
+
+ @Override
+ public boolean hasAccess(RequesterIdentity requesterIdentity, BrokerRequest brokerRequest) {
+ Preconditions.checkArgument(requesterIdentity instanceof HttpRequesterIdentity, "HttpRequesterIdentity required");
+ HttpRequesterIdentity identity = (HttpRequesterIdentity) requesterIdentity;
+
+ Collection<String> tokens = identity.getHttpHeaders().get(HEADER_AUTHORIZATION);
+ Optional<BasicAuthPrincipal> principalOpt =
+ tokens.stream().map(BasicAuthAccessControlFactory::normalizeToken).map(_principals::get)
+ .filter(Objects::nonNull).findFirst();
+
+ if (!principalOpt.isPresent()) {
+ // no matching token? reject
+ return false;
+ }
+
+ BasicAuthPrincipal principal = principalOpt.get();
+ if (principal.getTables().isEmpty() || !brokerRequest.isSetQuerySource() || !brokerRequest.getQuerySource()
+ .isSetTableName()) {
+ // no table restrictions? accept
+ return true;
+ }
+
+ return principal.getTables().contains(brokerRequest.getQuerySource().getTableName());
+ }
+ }
+
+ /**
+ * Container object for basic auth principal
+ */
+ private static class BasicAuthPrincipal {
+ private final String _name;
+ private final String _token;
+ private final Set<String> _tables;
+
+ public BasicAuthPrincipal(String name, String token, Set<String> tables) {
+ _name = name;
+ _token = token;
+ _tables = tables;
+ }
+
+ public String getName() {
+ return _name;
+ }
+
+ public Set<String> getTables() {
+ return _tables;
+ }
+
+ public String getToken() {
+ return _token;
+ }
+ }
+
+ private static String toToken(String name, String password) {
+ String identifier = String.format("%s:%s", name, password);
+ return normalizeToken(
+ String.format("Basic %s", Base64.getEncoder().encodeToString(identifier.getBytes(StandardCharsets.UTF_8))));
+ }
+
+ /**
+ * Implementations of base64 encoding vary and may generate different numbers of padding characters "=". We normalize
+ * these by removing any padding.
+ *
+ * @param token raw token
+ * @return normalized token
+ */
+ @Nullable
+ private static String normalizeToken(String token) {
+ if (token == null) {
+ return null;
+ }
+ return StringUtils.remove(token.trim(), "=");
Review comment:
```suggestion
return StringUtils.remove(token.trim(), '=');
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6552: add optional http basic auth to pinot broker
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6552:
URL: https://github.com/apache/incubator-pinot/pull/6552#discussion_r573145470
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,159 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+package org.apache.pinot.broker.broker;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.broker.api.RequesterIdentity;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+import java.util.*;
+import java.util.stream.Collectors;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the "pinot.broker.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * pinot.broker.access.control.principals=admin123,user456
+ * pinot.broker.access.control.principals.admin123.password=verysecret
+ * pinot.broker.access.control.principals.user456.password=kindasecret
+ * pinot.broker.access.control.principals.user456.tables=stuff,lessImportantStuff
+ * </pre>
+ */
+public class BasicAuthAccessControlFactory extends AccessControlFactory {
+ private static final String PRINCIPALS = "principals";
+ private static final String PASSWORD = "password";
+ private static final String TABLES = "tables";
+ private static final String TABLES_ALL = "*";
+
+ private static final String HEADER_AUTHORIZATION = "authorization";
+
+ private AccessControl _accessControl;
+
+ public BasicAuthAccessControlFactory() {
+ // left blank
+ }
+
+ public void init(PinotConfiguration configuration) {
+ String principalNames = configuration.getProperty(PRINCIPALS);
+ Preconditions.checkArgument(StringUtils.isNotBlank(principalNames), "must provide principals");
+
+ List<BasicAuthPrincipal> principals = Arrays.stream(principalNames.split(",")).map(rawName -> {
+ String name = rawName.trim();
+ Preconditions.checkArgument(StringUtils.isNotBlank(name), "%s is not a valid name", name);
+
+ String password = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, PASSWORD));
+ Preconditions.checkArgument(StringUtils.isNotBlank(password), "must provide a password for %s", name);
+
+ Set<String> tables = new HashSet<>();
+ String tableNames = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, TABLES));
+ if (StringUtils.isNotBlank(tableNames) && !TABLES_ALL.equals(tableNames)) {
+ tables.addAll(Arrays.asList(tableNames.split(",")));
+ }
+
+ return new BasicAuthPrincipal(name, toToken(name, password), tables);
+ }).collect(Collectors.toList());
+
+ _accessControl = new BasicAuthAccessControl(principals);
+ }
+
+ public AccessControl create() {
+ return _accessControl;
+ }
+
+ /**
+ * Access Control using header-based basic http authentication
+ */
+ private static class BasicAuthAccessControl implements AccessControl {
+ private final Map<String, BasicAuthPrincipal> _principals;
+
+ public BasicAuthAccessControl(Collection<BasicAuthPrincipal> principals) {
+ this._principals = principals.stream().collect(Collectors.toMap(BasicAuthPrincipal::getToken, p -> p));
+ }
+
+ @Override
+ public boolean hasAccess(RequesterIdentity requesterIdentity, BrokerRequest brokerRequest) {
+ Preconditions.checkArgument(requesterIdentity instanceof HttpRequesterIdentity, "HttpRequesterIdentity required");
+ HttpRequesterIdentity identity = (HttpRequesterIdentity) requesterIdentity;
+
+ Collection<String> tokens = identity.getHttpHeaders().get(HEADER_AUTHORIZATION);
+ Optional<BasicAuthPrincipal> principalOpt = tokens.stream().map(BasicAuthAccessControlFactory::normalizeToken)
+ .map(_principals::get).filter(Objects::nonNull).findFirst();
+
+ if (!principalOpt.isPresent()) {
+ // no matching token? reject
+ return false;
+ }
+
+ BasicAuthPrincipal principal = principalOpt.get();
+ if (principal.getTables().isEmpty()
+ || !brokerRequest.isSetQuerySource()
+ || !brokerRequest.getQuerySource().isSetTableName()) {
+ // no table restrictions? accept
+ return true;
+ }
+
+ return principal.getTables().contains(brokerRequest.getQuerySource().getTableName());
+ }
+ }
+
+ /**
+ * Container object for basic auth principal
+ */
+ private static class BasicAuthPrincipal {
+ private final String _name;
+ private final String _token;
+ private final Set<String> _tables;
+
+ public BasicAuthPrincipal(String name, String token, Set<String> tables) {
+ this._name = name;
+ this._token = token;
+ this._tables = tables;
+ }
+
+ public String getName() {
+ return _name;
+ }
+
+ public Set<String> getTables() {
+ return _tables;
+ }
+
+ public String getToken() {
+ return _token;
+ }
+ }
+
+ private static String toToken(String name, String password) {
+ String identifier = String.format("%s:%s", name, password);
+ return normalizeToken(String.format("Basic %s", Base64.getEncoder().encodeToString(identifier.getBytes())));
+ }
+
+ private static String normalizeToken(String token) {
+ if (token == null) {
Review comment:
If it returns `null`, will `.map(_principals::get)` on line 103 throw NPE?
If we allow `null` here, suggest annotate token with `@Nullable`
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java
##########
@@ -29,18 +29,22 @@
import java.net.HttpURLConnection;
import java.net.URL;
import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
Review comment:
Some imports are unused?
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,168 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+package org.apache.pinot.broker.broker;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.broker.api.RequesterIdentity;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+import java.nio.charset.StandardCharsets;
Review comment:
Can you follow the instructions here to set up the IDE and auto-reformat the code: https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6552: add optional http basic auth to pinot broker
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6552:
URL: https://github.com/apache/incubator-pinot/pull/6552#discussion_r572543502
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,159 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+package org.apache.pinot.broker.broker;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.broker.api.RequesterIdentity;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+import java.util.*;
+import java.util.stream.Collectors;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the "pinot.broker.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * pinot.broker.access.control.principals=admin123,user456
+ * pinot.broker.access.control.principals.admin123.password=verysecret
+ * pinot.broker.access.control.principals.user456.password=kindasecret
+ * pinot.broker.access.control.principals.user456.tables=stuff,lessImportantStuff
+ * </pre>
+ */
+public class BasicAuthAccessControlFactory extends AccessControlFactory {
+ private static final String PRINCIPALS = "principals";
+ private static final String PASSWORD = "password";
+ private static final String TABLES = "tables";
+ private static final String TABLES_ALL = "*";
+
+ private static final String HEADER_AUTHORIZATION = "authorization";
+
+ private AccessControl _accessControl;
+
+ public BasicAuthAccessControlFactory() {
+ // left blank
+ }
+
+ public void init(PinotConfiguration configuration) {
+ String principalNames = configuration.getProperty(PRINCIPALS);
+ Preconditions.checkArgument(StringUtils.isNotBlank(principalNames), "must provide principals");
+
+ List<BasicAuthPrincipal> principals = Arrays.stream(principalNames.split(",")).map(rawName -> {
+ String name = rawName.trim();
+ Preconditions.checkArgument(StringUtils.isNotBlank(name), "%s is not a valid name", name);
+
+ String password = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, PASSWORD));
+ Preconditions.checkArgument(StringUtils.isNotBlank(password), "must provide a password for %s", name);
+
+ Set<String> tables = new HashSet<>();
+ String tableNames = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, TABLES));
+ if (StringUtils.isNotBlank(tableNames) && !TABLES_ALL.equals(tableNames)) {
+ tables.addAll(Arrays.asList(tableNames.split(",")));
+ }
+
+ return new BasicAuthPrincipal(name, toToken(name, password), tables);
+ }).collect(Collectors.toList());
+
+ _accessControl = new BasicAuthAccessControl(principals);
+ }
+
+ public AccessControl create() {
+ return _accessControl;
+ }
+
+ /**
+ * Access Control using header-based basic http authentication
+ */
+ private static class BasicAuthAccessControl implements AccessControl {
+ private final Map<String, BasicAuthPrincipal> _principals;
+
+ public BasicAuthAccessControl(Collection<BasicAuthPrincipal> principals) {
+ this._principals = principals.stream().collect(Collectors.toMap(BasicAuthPrincipal::getToken, p -> p));
+ }
+
+ @Override
+ public boolean hasAccess(RequesterIdentity requesterIdentity, BrokerRequest brokerRequest) {
+ Preconditions.checkArgument(requesterIdentity instanceof HttpRequesterIdentity, "HttpRequesterIdentity required");
+ HttpRequesterIdentity identity = (HttpRequesterIdentity) requesterIdentity;
+
+ Collection<String> tokens = identity.getHttpHeaders().get(HEADER_AUTHORIZATION);
+ Optional<BasicAuthPrincipal> principalOpt = tokens.stream().map(BasicAuthAccessControlFactory::normalizeToken)
+ .map(_principals::get).filter(Objects::nonNull).findFirst();
+
+ if (!principalOpt.isPresent()) {
+ // no matching token? reject
+ return false;
+ }
+
+ BasicAuthPrincipal principal = principalOpt.get();
+ if (principal.getTables().isEmpty()
+ || !brokerRequest.isSetQuerySource()
+ || !brokerRequest.getQuerySource().isSetTableName()) {
+ // no table restrictions? accept
+ return true;
+ }
+
+ return principal.getTables().contains(brokerRequest.getQuerySource().getTableName());
+ }
+ }
+
+ /**
+ * Container object for basic auth principal
+ */
+ private static class BasicAuthPrincipal {
+ private final String _name;
+ private final String _token;
+ private final Set<String> _tables;
+
+ public BasicAuthPrincipal(String name, String token, Set<String> tables) {
+ this._name = name;
+ this._token = token;
+ this._tables = tables;
+ }
+
+ public String getName() {
+ return _name;
+ }
+
+ public Set<String> getTables() {
+ return _tables;
+ }
+
+ public String getToken() {
+ return _token;
+ }
+ }
+
+ private static String toToken(String name, String password) {
+ String identifier = String.format("%s:%s", name, password);
+ return normalizeToken(String.format("Basic %s", Base64.getEncoder().encodeToString(identifier.getBytes())));
+ }
+
+ private static String normalizeToken(String token) {
+ if (token == null) {
+ return null;
+ }
+ return token.trim().replace("=", "");
Review comment:
the implementation of base64 encoding across different libraries varies. "=" is a placeholder to align the number of output bytes, but some libs strip it or align differently, hence, for the same input we may end up with "aaa", "aaa=", "aaa==" and so on. They're ultimately equivalent, but hash-matching may fail if it isn't normalized.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6552: add optional http basic auth to pinot broker
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6552:
URL: https://github.com/apache/incubator-pinot/pull/6552#discussion_r573162779
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,168 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+package org.apache.pinot.broker.broker;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.broker.api.RequesterIdentity;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+import java.nio.charset.StandardCharsets;
Review comment:
I did this already. Auto-formatted the code again, just added a few line breaks. What are you looking for, specifically?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6552: add optional http basic auth to pinot broker
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6552:
URL: https://github.com/apache/incubator-pinot/pull/6552#issuecomment-774367851
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6552?src=pr&el=h1) Report
> Merging [#6552](https://codecov.io/gh/apache/incubator-pinot/pull/6552?src=pr&el=desc) (d217688) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.45%`.
> The diff coverage is `40.74%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6552?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6552 +/- ##
===========================================
- Coverage 66.44% 43.99% -22.46%
===========================================
Files 1075 1341 +266
Lines 54773 65930 +11157
Branches 8168 9616 +1448
===========================================
- Hits 36396 29006 -7390
- Misses 15700 34484 +18784
+ Partials 2677 2440 -237
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `43.99% <40.74%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6552?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...t/broker/broker/BasicAuthAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0Jhc2ljQXV0aEFjY2Vzc0NvbnRyb2xGYWN0b3J5LmphdmE=) | `0.00% <0.00%> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/interval/Interval.java](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsLmphdmE=) | `0.00% <0.00%> (ø)` | |
| ... and [1352 more](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6552?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6552?src=pr&el=footer). Last update [e62addb...d217688](https://codecov.io/gh/apache/incubator-pinot/pull/6552?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher merged pull request #6552: add optional http basic auth to pinot broker
Posted by GitBox <gi...@apache.org>.
apucher merged pull request #6552:
URL: https://github.com/apache/incubator-pinot/pull/6552
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6552: add optional http basic auth to pinot broker
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6552:
URL: https://github.com/apache/incubator-pinot/pull/6552#discussion_r572489919
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,159 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+package org.apache.pinot.broker.broker;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.broker.api.RequesterIdentity;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+import java.util.*;
+import java.util.stream.Collectors;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the "pinot.broker.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * pinot.broker.access.control.principals=admin123,user456
+ * pinot.broker.access.control.principals.admin123.password=verysecret
+ * pinot.broker.access.control.principals.user456.password=kindasecret
+ * pinot.broker.access.control.principals.user456.tables=stuff,lessImportantStuff
+ * </pre>
+ */
+public class BasicAuthAccessControlFactory extends AccessControlFactory {
+ private static final String PRINCIPALS = "principals";
+ private static final String PASSWORD = "password";
+ private static final String TABLES = "tables";
+ private static final String TABLES_ALL = "*";
+
+ private static final String HEADER_AUTHORIZATION = "authorization";
+
+ private AccessControl _accessControl;
+
+ public BasicAuthAccessControlFactory() {
+ // left blank
+ }
+
+ public void init(PinotConfiguration configuration) {
+ String principalNames = configuration.getProperty(PRINCIPALS);
+ Preconditions.checkArgument(StringUtils.isNotBlank(principalNames), "must provide principals");
+
+ List<BasicAuthPrincipal> principals = Arrays.stream(principalNames.split(",")).map(rawName -> {
+ String name = rawName.trim();
+ Preconditions.checkArgument(StringUtils.isNotBlank(name), "%s is not a valid name", name);
+
+ String password = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, PASSWORD));
+ Preconditions.checkArgument(StringUtils.isNotBlank(password), "must provide a password for %s", name);
+
+ Set<String> tables = new HashSet<>();
+ String tableNames = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, TABLES));
+ if (StringUtils.isNotBlank(tableNames) && !TABLES_ALL.equals(tableNames)) {
+ tables.addAll(Arrays.asList(tableNames.split(",")));
+ }
+
+ return new BasicAuthPrincipal(name, toToken(name, password), tables);
+ }).collect(Collectors.toList());
+
+ _accessControl = new BasicAuthAccessControl(principals);
+ }
+
+ public AccessControl create() {
+ return _accessControl;
+ }
+
+ /**
+ * Access Control using header-based basic http authentication
+ */
+ private static class BasicAuthAccessControl implements AccessControl {
+ private final Map<String, BasicAuthPrincipal> _principals;
+
+ public BasicAuthAccessControl(Collection<BasicAuthPrincipal> principals) {
+ this._principals = principals.stream().collect(Collectors.toMap(BasicAuthPrincipal::getToken, p -> p));
Review comment:
(nit) remove `this.` Same for other places
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,159 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+package org.apache.pinot.broker.broker;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.broker.api.RequesterIdentity;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+import java.util.*;
+import java.util.stream.Collectors;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the "pinot.broker.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * pinot.broker.access.control.principals=admin123,user456
+ * pinot.broker.access.control.principals.admin123.password=verysecret
+ * pinot.broker.access.control.principals.user456.password=kindasecret
+ * pinot.broker.access.control.principals.user456.tables=stuff,lessImportantStuff
+ * </pre>
+ */
+public class BasicAuthAccessControlFactory extends AccessControlFactory {
+ private static final String PRINCIPALS = "principals";
+ private static final String PASSWORD = "password";
+ private static final String TABLES = "tables";
+ private static final String TABLES_ALL = "*";
+
+ private static final String HEADER_AUTHORIZATION = "authorization";
+
+ private AccessControl _accessControl;
+
+ public BasicAuthAccessControlFactory() {
+ // left blank
+ }
+
+ public void init(PinotConfiguration configuration) {
+ String principalNames = configuration.getProperty(PRINCIPALS);
+ Preconditions.checkArgument(StringUtils.isNotBlank(principalNames), "must provide principals");
+
+ List<BasicAuthPrincipal> principals = Arrays.stream(principalNames.split(",")).map(rawName -> {
+ String name = rawName.trim();
+ Preconditions.checkArgument(StringUtils.isNotBlank(name), "%s is not a valid name", name);
+
+ String password = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, PASSWORD));
+ Preconditions.checkArgument(StringUtils.isNotBlank(password), "must provide a password for %s", name);
+
+ Set<String> tables = new HashSet<>();
+ String tableNames = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, TABLES));
+ if (StringUtils.isNotBlank(tableNames) && !TABLES_ALL.equals(tableNames)) {
+ tables.addAll(Arrays.asList(tableNames.split(",")));
+ }
+
+ return new BasicAuthPrincipal(name, toToken(name, password), tables);
+ }).collect(Collectors.toList());
+
+ _accessControl = new BasicAuthAccessControl(principals);
+ }
+
+ public AccessControl create() {
+ return _accessControl;
+ }
+
+ /**
+ * Access Control using header-based basic http authentication
+ */
+ private static class BasicAuthAccessControl implements AccessControl {
+ private final Map<String, BasicAuthPrincipal> _principals;
+
+ public BasicAuthAccessControl(Collection<BasicAuthPrincipal> principals) {
+ this._principals = principals.stream().collect(Collectors.toMap(BasicAuthPrincipal::getToken, p -> p));
+ }
+
+ @Override
+ public boolean hasAccess(RequesterIdentity requesterIdentity, BrokerRequest brokerRequest) {
+ Preconditions.checkArgument(requesterIdentity instanceof HttpRequesterIdentity, "HttpRequesterIdentity required");
+ HttpRequesterIdentity identity = (HttpRequesterIdentity) requesterIdentity;
+
+ Collection<String> tokens = identity.getHttpHeaders().get(HEADER_AUTHORIZATION);
+ Optional<BasicAuthPrincipal> principalOpt = tokens.stream().map(BasicAuthAccessControlFactory::normalizeToken)
+ .map(_principals::get).filter(Objects::nonNull).findFirst();
+
+ if (!principalOpt.isPresent()) {
+ // no matching token? reject
+ return false;
+ }
+
+ BasicAuthPrincipal principal = principalOpt.get();
+ if (principal.getTables().isEmpty()
+ || !brokerRequest.isSetQuerySource()
+ || !brokerRequest.getQuerySource().isSetTableName()) {
+ // no table restrictions? accept
+ return true;
+ }
+
+ return principal.getTables().contains(brokerRequest.getQuerySource().getTableName());
+ }
+ }
+
+ /**
+ * Container object for basic auth principal
+ */
+ private static class BasicAuthPrincipal {
+ private final String _name;
+ private final String _token;
+ private final Set<String> _tables;
+
+ public BasicAuthPrincipal(String name, String token, Set<String> tables) {
+ this._name = name;
Review comment:
(nit) Remove `this.`
##########
File path: pinot-broker/src/test/java/org/apache/pinot/broker/broker/BasicAuthAccessControlTest.java
##########
@@ -0,0 +1,152 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+package org.apache.pinot.broker.broker;
+
+import com.google.common.collect.ArrayListMultimap;
+import com.google.common.collect.Multimap;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.QuerySource;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.util.HashMap;
+import java.util.Map;
+
+public class BasicAuthAccessControlTest {
+ private static final String TOKEN_USER = "Basic dXNlcjpzZWNyZXQ"; // user:secret
Review comment:
Fix the indentation and the import
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,159 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+package org.apache.pinot.broker.broker;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.broker.api.RequesterIdentity;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+import java.util.*;
+import java.util.stream.Collectors;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the "pinot.broker.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * pinot.broker.access.control.principals=admin123,user456
+ * pinot.broker.access.control.principals.admin123.password=verysecret
+ * pinot.broker.access.control.principals.user456.password=kindasecret
+ * pinot.broker.access.control.principals.user456.tables=stuff,lessImportantStuff
+ * </pre>
+ */
+public class BasicAuthAccessControlFactory extends AccessControlFactory {
+ private static final String PRINCIPALS = "principals";
+ private static final String PASSWORD = "password";
+ private static final String TABLES = "tables";
+ private static final String TABLES_ALL = "*";
+
+ private static final String HEADER_AUTHORIZATION = "authorization";
+
+ private AccessControl _accessControl;
+
+ public BasicAuthAccessControlFactory() {
+ // left blank
+ }
+
+ public void init(PinotConfiguration configuration) {
+ String principalNames = configuration.getProperty(PRINCIPALS);
+ Preconditions.checkArgument(StringUtils.isNotBlank(principalNames), "must provide principals");
+
+ List<BasicAuthPrincipal> principals = Arrays.stream(principalNames.split(",")).map(rawName -> {
+ String name = rawName.trim();
+ Preconditions.checkArgument(StringUtils.isNotBlank(name), "%s is not a valid name", name);
+
+ String password = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, PASSWORD));
+ Preconditions.checkArgument(StringUtils.isNotBlank(password), "must provide a password for %s", name);
+
+ Set<String> tables = new HashSet<>();
+ String tableNames = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, TABLES));
+ if (StringUtils.isNotBlank(tableNames) && !TABLES_ALL.equals(tableNames)) {
+ tables.addAll(Arrays.asList(tableNames.split(",")));
+ }
+
+ return new BasicAuthPrincipal(name, toToken(name, password), tables);
+ }).collect(Collectors.toList());
+
+ _accessControl = new BasicAuthAccessControl(principals);
+ }
+
+ public AccessControl create() {
+ return _accessControl;
+ }
+
+ /**
+ * Access Control using header-based basic http authentication
+ */
+ private static class BasicAuthAccessControl implements AccessControl {
+ private final Map<String, BasicAuthPrincipal> _principals;
+
+ public BasicAuthAccessControl(Collection<BasicAuthPrincipal> principals) {
+ this._principals = principals.stream().collect(Collectors.toMap(BasicAuthPrincipal::getToken, p -> p));
+ }
+
+ @Override
+ public boolean hasAccess(RequesterIdentity requesterIdentity, BrokerRequest brokerRequest) {
+ Preconditions.checkArgument(requesterIdentity instanceof HttpRequesterIdentity, "HttpRequesterIdentity required");
+ HttpRequesterIdentity identity = (HttpRequesterIdentity) requesterIdentity;
+
+ Collection<String> tokens = identity.getHttpHeaders().get(HEADER_AUTHORIZATION);
+ Optional<BasicAuthPrincipal> principalOpt = tokens.stream().map(BasicAuthAccessControlFactory::normalizeToken)
+ .map(_principals::get).filter(Objects::nonNull).findFirst();
+
+ if (!principalOpt.isPresent()) {
+ // no matching token? reject
+ return false;
+ }
+
+ BasicAuthPrincipal principal = principalOpt.get();
+ if (principal.getTables().isEmpty()
+ || !brokerRequest.isSetQuerySource()
+ || !brokerRequest.getQuerySource().isSetTableName()) {
+ // no table restrictions? accept
+ return true;
+ }
+
+ return principal.getTables().contains(brokerRequest.getQuerySource().getTableName());
+ }
+ }
+
+ /**
+ * Container object for basic auth principal
+ */
+ private static class BasicAuthPrincipal {
+ private final String _name;
+ private final String _token;
+ private final Set<String> _tables;
+
+ public BasicAuthPrincipal(String name, String token, Set<String> tables) {
+ this._name = name;
+ this._token = token;
+ this._tables = tables;
+ }
+
+ public String getName() {
+ return _name;
+ }
+
+ public Set<String> getTables() {
+ return _tables;
+ }
+
+ public String getToken() {
+ return _token;
+ }
+ }
+
+ private static String toToken(String name, String password) {
+ String identifier = String.format("%s:%s", name, password);
+ return normalizeToken(String.format("Basic %s", Base64.getEncoder().encodeToString(identifier.getBytes())));
+ }
+
+ private static String normalizeToken(String token) {
+ if (token == null) {
Review comment:
token should never be `null`?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java
##########
@@ -210,7 +214,14 @@ public String getQueryResponse(String query, String traceEnabled, String queryOp
String url = getQueryURL(protocol, hostNameWithPrefix.substring(hostNameWithPrefix.indexOf("_") + 1),
String.valueOf(port), querySyntax);
ObjectNode requestJson = getRequestJson(query, traceEnabled, queryOptions, querySyntax);
- return sendRequestRaw(url, query, requestJson);
+
+ // forward client-supplied headers
+ Map<String, String> headers = httpHeaders.getRequestHeaders().entrySet().stream()
+ .filter(entry -> !entry.getValue().isEmpty())
+ .map(entry -> Pair.of(entry.getKey(), entry.getValue().get(0)))
Review comment:
Is it okay to only keep the first value?
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,159 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+package org.apache.pinot.broker.broker;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.broker.api.RequesterIdentity;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+import java.util.*;
+import java.util.stream.Collectors;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the "pinot.broker.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * pinot.broker.access.control.principals=admin123,user456
+ * pinot.broker.access.control.principals.admin123.password=verysecret
+ * pinot.broker.access.control.principals.user456.password=kindasecret
+ * pinot.broker.access.control.principals.user456.tables=stuff,lessImportantStuff
+ * </pre>
+ */
+public class BasicAuthAccessControlFactory extends AccessControlFactory {
+ private static final String PRINCIPALS = "principals";
+ private static final String PASSWORD = "password";
+ private static final String TABLES = "tables";
+ private static final String TABLES_ALL = "*";
+
+ private static final String HEADER_AUTHORIZATION = "authorization";
+
+ private AccessControl _accessControl;
+
+ public BasicAuthAccessControlFactory() {
+ // left blank
+ }
+
+ public void init(PinotConfiguration configuration) {
+ String principalNames = configuration.getProperty(PRINCIPALS);
+ Preconditions.checkArgument(StringUtils.isNotBlank(principalNames), "must provide principals");
+
+ List<BasicAuthPrincipal> principals = Arrays.stream(principalNames.split(",")).map(rawName -> {
+ String name = rawName.trim();
+ Preconditions.checkArgument(StringUtils.isNotBlank(name), "%s is not a valid name", name);
+
+ String password = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, PASSWORD));
+ Preconditions.checkArgument(StringUtils.isNotBlank(password), "must provide a password for %s", name);
+
+ Set<String> tables = new HashSet<>();
+ String tableNames = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, TABLES));
+ if (StringUtils.isNotBlank(tableNames) && !TABLES_ALL.equals(tableNames)) {
+ tables.addAll(Arrays.asList(tableNames.split(",")));
+ }
+
+ return new BasicAuthPrincipal(name, toToken(name, password), tables);
+ }).collect(Collectors.toList());
+
+ _accessControl = new BasicAuthAccessControl(principals);
+ }
+
+ public AccessControl create() {
+ return _accessControl;
+ }
+
+ /**
+ * Access Control using header-based basic http authentication
+ */
+ private static class BasicAuthAccessControl implements AccessControl {
+ private final Map<String, BasicAuthPrincipal> _principals;
+
+ public BasicAuthAccessControl(Collection<BasicAuthPrincipal> principals) {
+ this._principals = principals.stream().collect(Collectors.toMap(BasicAuthPrincipal::getToken, p -> p));
+ }
+
+ @Override
+ public boolean hasAccess(RequesterIdentity requesterIdentity, BrokerRequest brokerRequest) {
+ Preconditions.checkArgument(requesterIdentity instanceof HttpRequesterIdentity, "HttpRequesterIdentity required");
+ HttpRequesterIdentity identity = (HttpRequesterIdentity) requesterIdentity;
+
+ Collection<String> tokens = identity.getHttpHeaders().get(HEADER_AUTHORIZATION);
+ Optional<BasicAuthPrincipal> principalOpt = tokens.stream().map(BasicAuthAccessControlFactory::normalizeToken)
+ .map(_principals::get).filter(Objects::nonNull).findFirst();
+
+ if (!principalOpt.isPresent()) {
+ // no matching token? reject
+ return false;
+ }
+
+ BasicAuthPrincipal principal = principalOpt.get();
+ if (principal.getTables().isEmpty()
+ || !brokerRequest.isSetQuerySource()
+ || !brokerRequest.getQuerySource().isSetTableName()) {
+ // no table restrictions? accept
+ return true;
+ }
+
+ return principal.getTables().contains(brokerRequest.getQuerySource().getTableName());
+ }
+ }
+
+ /**
+ * Container object for basic auth principal
+ */
+ private static class BasicAuthPrincipal {
+ private final String _name;
+ private final String _token;
+ private final Set<String> _tables;
+
+ public BasicAuthPrincipal(String name, String token, Set<String> tables) {
+ this._name = name;
+ this._token = token;
+ this._tables = tables;
+ }
+
+ public String getName() {
+ return _name;
+ }
+
+ public Set<String> getTables() {
+ return _tables;
+ }
+
+ public String getToken() {
+ return _token;
+ }
+ }
+
+ private static String toToken(String name, String password) {
+ String identifier = String.format("%s:%s", name, password);
+ return normalizeToken(String.format("Basic %s", Base64.getEncoder().encodeToString(identifier.getBytes())));
+ }
+
+ private static String normalizeToken(String token) {
+ if (token == null) {
+ return null;
+ }
+ return token.trim().replace("=", "");
Review comment:
Why do we need to remove the `=`?
Besides, `StringUtils.remove(token.trim(), '=')` is faster.
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,159 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+package org.apache.pinot.broker.broker;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.broker.api.RequesterIdentity;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+import java.util.*;
+import java.util.stream.Collectors;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the "pinot.broker.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * pinot.broker.access.control.principals=admin123,user456
+ * pinot.broker.access.control.principals.admin123.password=verysecret
+ * pinot.broker.access.control.principals.user456.password=kindasecret
+ * pinot.broker.access.control.principals.user456.tables=stuff,lessImportantStuff
+ * </pre>
+ */
+public class BasicAuthAccessControlFactory extends AccessControlFactory {
+ private static final String PRINCIPALS = "principals";
+ private static final String PASSWORD = "password";
+ private static final String TABLES = "tables";
+ private static final String TABLES_ALL = "*";
+
+ private static final String HEADER_AUTHORIZATION = "authorization";
+
+ private AccessControl _accessControl;
+
+ public BasicAuthAccessControlFactory() {
+ // left blank
+ }
+
+ public void init(PinotConfiguration configuration) {
+ String principalNames = configuration.getProperty(PRINCIPALS);
+ Preconditions.checkArgument(StringUtils.isNotBlank(principalNames), "must provide principals");
+
+ List<BasicAuthPrincipal> principals = Arrays.stream(principalNames.split(",")).map(rawName -> {
+ String name = rawName.trim();
+ Preconditions.checkArgument(StringUtils.isNotBlank(name), "%s is not a valid name", name);
+
+ String password = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, PASSWORD));
+ Preconditions.checkArgument(StringUtils.isNotBlank(password), "must provide a password for %s", name);
+
+ Set<String> tables = new HashSet<>();
+ String tableNames = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, TABLES));
+ if (StringUtils.isNotBlank(tableNames) && !TABLES_ALL.equals(tableNames)) {
+ tables.addAll(Arrays.asList(tableNames.split(",")));
+ }
+
+ return new BasicAuthPrincipal(name, toToken(name, password), tables);
+ }).collect(Collectors.toList());
+
+ _accessControl = new BasicAuthAccessControl(principals);
+ }
+
+ public AccessControl create() {
+ return _accessControl;
+ }
+
+ /**
+ * Access Control using header-based basic http authentication
+ */
+ private static class BasicAuthAccessControl implements AccessControl {
+ private final Map<String, BasicAuthPrincipal> _principals;
+
+ public BasicAuthAccessControl(Collection<BasicAuthPrincipal> principals) {
+ this._principals = principals.stream().collect(Collectors.toMap(BasicAuthPrincipal::getToken, p -> p));
+ }
+
+ @Override
+ public boolean hasAccess(RequesterIdentity requesterIdentity, BrokerRequest brokerRequest) {
+ Preconditions.checkArgument(requesterIdentity instanceof HttpRequesterIdentity, "HttpRequesterIdentity required");
+ HttpRequesterIdentity identity = (HttpRequesterIdentity) requesterIdentity;
+
+ Collection<String> tokens = identity.getHttpHeaders().get(HEADER_AUTHORIZATION);
+ Optional<BasicAuthPrincipal> principalOpt = tokens.stream().map(BasicAuthAccessControlFactory::normalizeToken)
+ .map(_principals::get).filter(Objects::nonNull).findFirst();
+
+ if (!principalOpt.isPresent()) {
+ // no matching token? reject
+ return false;
+ }
+
+ BasicAuthPrincipal principal = principalOpt.get();
+ if (principal.getTables().isEmpty()
+ || !brokerRequest.isSetQuerySource()
+ || !brokerRequest.getQuerySource().isSetTableName()) {
+ // no table restrictions? accept
+ return true;
+ }
+
+ return principal.getTables().contains(brokerRequest.getQuerySource().getTableName());
+ }
+ }
+
+ /**
+ * Container object for basic auth principal
+ */
+ private static class BasicAuthPrincipal {
+ private final String _name;
+ private final String _token;
+ private final Set<String> _tables;
+
+ public BasicAuthPrincipal(String name, String token, Set<String> tables) {
+ this._name = name;
+ this._token = token;
+ this._tables = tables;
+ }
+
+ public String getName() {
+ return _name;
+ }
+
+ public Set<String> getTables() {
+ return _tables;
+ }
+
+ public String getToken() {
+ return _token;
+ }
+ }
+
+ private static String toToken(String name, String password) {
+ String identifier = String.format("%s:%s", name, password);
+ return normalizeToken(String.format("Basic %s", Base64.getEncoder().encodeToString(identifier.getBytes())));
Review comment:
Should we use `UTF-8` here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6552: add optional http basic auth to pinot broker
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6552:
URL: https://github.com/apache/incubator-pinot/pull/6552#discussion_r572559652
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,159 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+package org.apache.pinot.broker.broker;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.broker.api.RequesterIdentity;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+import java.util.*;
+import java.util.stream.Collectors;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the "pinot.broker.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * pinot.broker.access.control.principals=admin123,user456
+ * pinot.broker.access.control.principals.admin123.password=verysecret
+ * pinot.broker.access.control.principals.user456.password=kindasecret
+ * pinot.broker.access.control.principals.user456.tables=stuff,lessImportantStuff
+ * </pre>
+ */
+public class BasicAuthAccessControlFactory extends AccessControlFactory {
+ private static final String PRINCIPALS = "principals";
+ private static final String PASSWORD = "password";
+ private static final String TABLES = "tables";
+ private static final String TABLES_ALL = "*";
+
+ private static final String HEADER_AUTHORIZATION = "authorization";
+
+ private AccessControl _accessControl;
+
+ public BasicAuthAccessControlFactory() {
+ // left blank
+ }
+
+ public void init(PinotConfiguration configuration) {
+ String principalNames = configuration.getProperty(PRINCIPALS);
+ Preconditions.checkArgument(StringUtils.isNotBlank(principalNames), "must provide principals");
+
+ List<BasicAuthPrincipal> principals = Arrays.stream(principalNames.split(",")).map(rawName -> {
+ String name = rawName.trim();
+ Preconditions.checkArgument(StringUtils.isNotBlank(name), "%s is not a valid name", name);
+
+ String password = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, PASSWORD));
+ Preconditions.checkArgument(StringUtils.isNotBlank(password), "must provide a password for %s", name);
+
+ Set<String> tables = new HashSet<>();
+ String tableNames = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, TABLES));
+ if (StringUtils.isNotBlank(tableNames) && !TABLES_ALL.equals(tableNames)) {
+ tables.addAll(Arrays.asList(tableNames.split(",")));
+ }
+
+ return new BasicAuthPrincipal(name, toToken(name, password), tables);
+ }).collect(Collectors.toList());
+
+ _accessControl = new BasicAuthAccessControl(principals);
+ }
+
+ public AccessControl create() {
+ return _accessControl;
+ }
+
+ /**
+ * Access Control using header-based basic http authentication
+ */
+ private static class BasicAuthAccessControl implements AccessControl {
+ private final Map<String, BasicAuthPrincipal> _principals;
+
+ public BasicAuthAccessControl(Collection<BasicAuthPrincipal> principals) {
+ this._principals = principals.stream().collect(Collectors.toMap(BasicAuthPrincipal::getToken, p -> p));
Review comment:
done
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,159 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+package org.apache.pinot.broker.broker;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.broker.api.RequesterIdentity;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+import java.util.*;
+import java.util.stream.Collectors;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the "pinot.broker.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * pinot.broker.access.control.principals=admin123,user456
+ * pinot.broker.access.control.principals.admin123.password=verysecret
+ * pinot.broker.access.control.principals.user456.password=kindasecret
+ * pinot.broker.access.control.principals.user456.tables=stuff,lessImportantStuff
+ * </pre>
+ */
+public class BasicAuthAccessControlFactory extends AccessControlFactory {
+ private static final String PRINCIPALS = "principals";
+ private static final String PASSWORD = "password";
+ private static final String TABLES = "tables";
+ private static final String TABLES_ALL = "*";
+
+ private static final String HEADER_AUTHORIZATION = "authorization";
+
+ private AccessControl _accessControl;
+
+ public BasicAuthAccessControlFactory() {
+ // left blank
+ }
+
+ public void init(PinotConfiguration configuration) {
+ String principalNames = configuration.getProperty(PRINCIPALS);
+ Preconditions.checkArgument(StringUtils.isNotBlank(principalNames), "must provide principals");
+
+ List<BasicAuthPrincipal> principals = Arrays.stream(principalNames.split(",")).map(rawName -> {
+ String name = rawName.trim();
+ Preconditions.checkArgument(StringUtils.isNotBlank(name), "%s is not a valid name", name);
+
+ String password = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, PASSWORD));
+ Preconditions.checkArgument(StringUtils.isNotBlank(password), "must provide a password for %s", name);
+
+ Set<String> tables = new HashSet<>();
+ String tableNames = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, TABLES));
+ if (StringUtils.isNotBlank(tableNames) && !TABLES_ALL.equals(tableNames)) {
+ tables.addAll(Arrays.asList(tableNames.split(",")));
+ }
+
+ return new BasicAuthPrincipal(name, toToken(name, password), tables);
+ }).collect(Collectors.toList());
+
+ _accessControl = new BasicAuthAccessControl(principals);
+ }
+
+ public AccessControl create() {
+ return _accessControl;
+ }
+
+ /**
+ * Access Control using header-based basic http authentication
+ */
+ private static class BasicAuthAccessControl implements AccessControl {
+ private final Map<String, BasicAuthPrincipal> _principals;
+
+ public BasicAuthAccessControl(Collection<BasicAuthPrincipal> principals) {
+ this._principals = principals.stream().collect(Collectors.toMap(BasicAuthPrincipal::getToken, p -> p));
+ }
+
+ @Override
+ public boolean hasAccess(RequesterIdentity requesterIdentity, BrokerRequest brokerRequest) {
+ Preconditions.checkArgument(requesterIdentity instanceof HttpRequesterIdentity, "HttpRequesterIdentity required");
+ HttpRequesterIdentity identity = (HttpRequesterIdentity) requesterIdentity;
+
+ Collection<String> tokens = identity.getHttpHeaders().get(HEADER_AUTHORIZATION);
+ Optional<BasicAuthPrincipal> principalOpt = tokens.stream().map(BasicAuthAccessControlFactory::normalizeToken)
+ .map(_principals::get).filter(Objects::nonNull).findFirst();
+
+ if (!principalOpt.isPresent()) {
+ // no matching token? reject
+ return false;
+ }
+
+ BasicAuthPrincipal principal = principalOpt.get();
+ if (principal.getTables().isEmpty()
+ || !brokerRequest.isSetQuerySource()
+ || !brokerRequest.getQuerySource().isSetTableName()) {
+ // no table restrictions? accept
+ return true;
+ }
+
+ return principal.getTables().contains(brokerRequest.getQuerySource().getTableName());
+ }
+ }
+
+ /**
+ * Container object for basic auth principal
+ */
+ private static class BasicAuthPrincipal {
+ private final String _name;
+ private final String _token;
+ private final Set<String> _tables;
+
+ public BasicAuthPrincipal(String name, String token, Set<String> tables) {
+ this._name = name;
Review comment:
done
##########
File path: pinot-broker/src/test/java/org/apache/pinot/broker/broker/BasicAuthAccessControlTest.java
##########
@@ -0,0 +1,152 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+package org.apache.pinot.broker.broker;
+
+import com.google.common.collect.ArrayListMultimap;
+import com.google.common.collect.Multimap;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.QuerySource;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.util.HashMap;
+import java.util.Map;
+
+public class BasicAuthAccessControlTest {
+ private static final String TOKEN_USER = "Basic dXNlcjpzZWNyZXQ"; // user:secret
Review comment:
done
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,159 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+package org.apache.pinot.broker.broker;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.broker.api.RequesterIdentity;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+import java.util.*;
+import java.util.stream.Collectors;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the "pinot.broker.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * pinot.broker.access.control.principals=admin123,user456
+ * pinot.broker.access.control.principals.admin123.password=verysecret
+ * pinot.broker.access.control.principals.user456.password=kindasecret
+ * pinot.broker.access.control.principals.user456.tables=stuff,lessImportantStuff
+ * </pre>
+ */
+public class BasicAuthAccessControlFactory extends AccessControlFactory {
+ private static final String PRINCIPALS = "principals";
+ private static final String PASSWORD = "password";
+ private static final String TABLES = "tables";
+ private static final String TABLES_ALL = "*";
+
+ private static final String HEADER_AUTHORIZATION = "authorization";
+
+ private AccessControl _accessControl;
+
+ public BasicAuthAccessControlFactory() {
+ // left blank
+ }
+
+ public void init(PinotConfiguration configuration) {
+ String principalNames = configuration.getProperty(PRINCIPALS);
+ Preconditions.checkArgument(StringUtils.isNotBlank(principalNames), "must provide principals");
+
+ List<BasicAuthPrincipal> principals = Arrays.stream(principalNames.split(",")).map(rawName -> {
+ String name = rawName.trim();
+ Preconditions.checkArgument(StringUtils.isNotBlank(name), "%s is not a valid name", name);
+
+ String password = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, PASSWORD));
+ Preconditions.checkArgument(StringUtils.isNotBlank(password), "must provide a password for %s", name);
+
+ Set<String> tables = new HashSet<>();
+ String tableNames = configuration.getProperty(String.format("%s.%s.%s", PRINCIPALS, name, TABLES));
+ if (StringUtils.isNotBlank(tableNames) && !TABLES_ALL.equals(tableNames)) {
+ tables.addAll(Arrays.asList(tableNames.split(",")));
+ }
+
+ return new BasicAuthPrincipal(name, toToken(name, password), tables);
+ }).collect(Collectors.toList());
+
+ _accessControl = new BasicAuthAccessControl(principals);
+ }
+
+ public AccessControl create() {
+ return _accessControl;
+ }
+
+ /**
+ * Access Control using header-based basic http authentication
+ */
+ private static class BasicAuthAccessControl implements AccessControl {
+ private final Map<String, BasicAuthPrincipal> _principals;
+
+ public BasicAuthAccessControl(Collection<BasicAuthPrincipal> principals) {
+ this._principals = principals.stream().collect(Collectors.toMap(BasicAuthPrincipal::getToken, p -> p));
+ }
+
+ @Override
+ public boolean hasAccess(RequesterIdentity requesterIdentity, BrokerRequest brokerRequest) {
+ Preconditions.checkArgument(requesterIdentity instanceof HttpRequesterIdentity, "HttpRequesterIdentity required");
+ HttpRequesterIdentity identity = (HttpRequesterIdentity) requesterIdentity;
+
+ Collection<String> tokens = identity.getHttpHeaders().get(HEADER_AUTHORIZATION);
+ Optional<BasicAuthPrincipal> principalOpt = tokens.stream().map(BasicAuthAccessControlFactory::normalizeToken)
+ .map(_principals::get).filter(Objects::nonNull).findFirst();
+
+ if (!principalOpt.isPresent()) {
+ // no matching token? reject
+ return false;
+ }
+
+ BasicAuthPrincipal principal = principalOpt.get();
+ if (principal.getTables().isEmpty()
+ || !brokerRequest.isSetQuerySource()
+ || !brokerRequest.getQuerySource().isSetTableName()) {
+ // no table restrictions? accept
+ return true;
+ }
+
+ return principal.getTables().contains(brokerRequest.getQuerySource().getTableName());
+ }
+ }
+
+ /**
+ * Container object for basic auth principal
+ */
+ private static class BasicAuthPrincipal {
+ private final String _name;
+ private final String _token;
+ private final Set<String> _tables;
+
+ public BasicAuthPrincipal(String name, String token, Set<String> tables) {
+ this._name = name;
+ this._token = token;
+ this._tables = tables;
+ }
+
+ public String getName() {
+ return _name;
+ }
+
+ public Set<String> getTables() {
+ return _tables;
+ }
+
+ public String getToken() {
+ return _token;
+ }
+ }
+
+ private static String toToken(String name, String password) {
+ String identifier = String.format("%s:%s", name, password);
+ return normalizeToken(String.format("Basic %s", Base64.getEncoder().encodeToString(identifier.getBytes())));
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io commented on pull request #6552: add optional http basic auth to pinot broker
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6552:
URL: https://github.com/apache/incubator-pinot/pull/6552#issuecomment-774367851
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6552?src=pr&el=h1) Report
> Merging [#6552](https://codecov.io/gh/apache/incubator-pinot/pull/6552?src=pr&el=desc) (e79b1f2) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `1.41%`.
> The diff coverage is `60.60%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6552?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6552 +/- ##
==========================================
- Coverage 66.44% 65.03% -1.42%
==========================================
Files 1075 1337 +262
Lines 54773 65871 +11098
Branches 8168 9610 +1442
==========================================
+ Hits 36396 42841 +6445
- Misses 15700 19968 +4268
- Partials 2677 3062 +385
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `65.03% <60.60%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6552?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
| [...pinot/broker/api/resources/PinotClientRequest.java](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdENsaWVudFJlcXVlc3QuamF2YQ==) | `0.00% <0.00%> (-27.28%)` | :arrow_down: |
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <ø> (+9.52%)` | :arrow_up: |
| [...n/java/org/apache/pinot/client/BrokerResponse.java](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Jyb2tlclJlc3BvbnNlLmphdmE=) | `100.00% <ø> (ø)` | |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <ø> (-13.29%)` | :arrow_down: |
| [...n/java/org/apache/pinot/client/ExecutionStats.java](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0V4ZWN1dGlvblN0YXRzLmphdmE=) | `15.55% <ø> (ø)` | |
| [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `10.90% <ø> (-51.10%)` | :arrow_down: |
| ... and [1200 more](https://codecov.io/gh/apache/incubator-pinot/pull/6552/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6552?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6552?src=pr&el=footer). Last update [20ff276...e79b1f2](https://codecov.io/gh/apache/incubator-pinot/pull/6552?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6552: add optional http basic auth to pinot broker
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6552:
URL: https://github.com/apache/incubator-pinot/pull/6552#discussion_r573162893
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java
##########
@@ -29,18 +29,22 @@
import java.net.HttpURLConnection;
import java.net.URL;
import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
Review comment:
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6552: add optional http basic auth to pinot broker
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6552:
URL: https://github.com/apache/incubator-pinot/pull/6552#discussion_r572542570
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java
##########
@@ -210,7 +214,14 @@ public String getQueryResponse(String query, String traceEnabled, String queryOp
String url = getQueryURL(protocol, hostNameWithPrefix.substring(hostNameWithPrefix.indexOf("_") + 1),
String.valueOf(port), querySyntax);
ObjectNode requestJson = getRequestJson(query, traceEnabled, queryOptions, querySyntax);
- return sendRequestRaw(url, query, requestJson);
+
+ // forward client-supplied headers
+ Map<String, String> headers = httpHeaders.getRequestHeaders().entrySet().stream()
+ .filter(entry -> !entry.getValue().isEmpty())
+ .map(entry -> Pair.of(entry.getKey(), entry.getValue().get(0)))
Review comment:
For our purposes, yes. Pinot's handling of http headers overall isn't too glorious.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org