icefury71 commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r592742130
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/fetcher/BaseSegmentFetcher.java
##########
@@ -36,6 +35,7 @@
public static final String RETRY_COUNT_CONFIG_KEY = "retry.count";
public static final String RETRY_WAIT_MS_CONFIG_KEY = "retry.wait.ms";
public static final String RETRY_DELAY_SCALE_FACTOR_CONFIG_KEY =
"retry.delay.scale.factor";
+ public static final String AUTH_TOKEN = "auth.token";
Review comment:
Looks like this is already defined in CommonConstants ?
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/segment/generation/SegmentGenerationUtils.java
##########
@@ -87,7 +94,12 @@ public static Schema getSchema(String schemaURIString) {
}
}
+ @Deprecated
Review comment:
Is it necessary to deprecate this ? For folks who don't care about
security, they will have to keep passing null here ?
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * 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.controller.api.access;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import javax.ws.rs.core.HttpHeaders;
+import org.apache.pinot.core.auth.BasicAuthPrincipal;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the
"controller.admin.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * controller.admin.access.control.principals=admin123,user456
+ * controller.admin.access.control.principals.admin123.password=verysecret
+ * controller.admin.access.control.principals.user456.password=kindasecret
+ *
controller.admin.access.control.principals.user456.tables=stuff,lessImportantStuff
+ *
controller.admin.access.control.principals.user456.permissions=read,update
+ * </pre>
+ */
+public class BasicAuthAccessControlFactory implements AccessControlFactory {
+ private static final String PREFIX =
"controller.admin.access.control.principals";
+
+ private static final String HEADER_AUTHORIZATION = "Authorization";
+
+ private AccessControl _accessControl;
+
+ public void init(PinotConfiguration configuration) {
+ _accessControl = new
BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration,
PREFIX));
+ }
+
+ @Override
+ 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 hasDataAccess(HttpHeaders httpHeaders, String tableName) {
+ return getPrincipal(httpHeaders).filter(p ->
p.hasTable(tableName)).isPresent();
+ }
+
+ @Override
+ public boolean hasAccess(String tableName, AccessType accessType,
HttpHeaders httpHeaders, String endpointUrl) {
Review comment:
Should all these methods be defined in some interface ? (hasDataAccess,
has Access , blah)
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * 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.controller.api.access;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import javax.ws.rs.core.HttpHeaders;
+import org.apache.pinot.core.auth.BasicAuthPrincipal;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the
"controller.admin.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * controller.admin.access.control.principals=admin123,user456
+ * controller.admin.access.control.principals.admin123.password=verysecret
+ * controller.admin.access.control.principals.user456.password=kindasecret
+ *
controller.admin.access.control.principals.user456.tables=stuff,lessImportantStuff
+ *
controller.admin.access.control.principals.user456.permissions=read,update
+ * </pre>
+ */
+public class BasicAuthAccessControlFactory implements AccessControlFactory {
+ private static final String PREFIX =
"controller.admin.access.control.principals";
+
+ private static final String HEADER_AUTHORIZATION = "Authorization";
+
+ private AccessControl _accessControl;
+
+ public void init(PinotConfiguration configuration) {
+ _accessControl = new
BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration,
PREFIX));
+ }
+
+ @Override
Review comment:
Not sure if we need Override here?
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java
##########
@@ -0,0 +1,75 @@
+/**
+ * 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.controller.api.resources;
+
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiParam;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;
+import javax.inject.Inject;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.MediaType;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.controller.api.access.AccessControl;
+import org.apache.pinot.controller.api.access.AccessControlFactory;
+import org.apache.pinot.controller.api.access.AccessType;
+
+
+@Api(tags = "Auth")
+@Path("/")
+public class PinotControllerAuthResource {
+
+ @Inject
+ private AccessControlFactory _accessControlFactory;
+
+ @Context
+ HttpHeaders _httpHeaders;
+
+ @GET
+ @Path("auth/verify")
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Check whether authentication is enabled")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Verification
result provided"), @ApiResponse(code = 500, message = "Verification error")})
Review comment:
Should we use 401 instead of 500 ?
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -64,4 +67,32 @@ default boolean hasAccess(String tableName, AccessType
accessType, HttpHeaders h
default boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders,
String endpointUrl) {
return true;
}
+
+ /**
+ * Return workflow info for authenticating users. Not all workflows may be
supported by the pinot UI implementation.
+ *
+ * @return workflow info for user authentication
+ */
+ default AuthWorkflowInfo getAuthWorkflowInfo() {
+ return new AuthWorkflowInfo(WORKFLOW_NONE);
+ }
+
+ /**
+ * Container for authentication workflow info. May be extended by
implementations.
+ */
+ class AuthWorkflowInfo {
+ String workflow;
+
+ public AuthWorkflowInfo(String workflow) {
+ this.workflow = workflow;
+ }
+
+ public String getWorkflow() {
+ return workflow;
+ }
+
+ public void setWorkflow(String workflow) {
Review comment:
Do we need set here ? Once the auth workflow is created, would it ever
change ?
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/fetcher/SegmentFetcherFactory.java
##########
@@ -34,29 +34,40 @@
public class SegmentFetcherFactory {
- private SegmentFetcherFactory() {
- }
+ private final static SegmentFetcherFactory instance = new
SegmentFetcherFactory();
static final String SEGMENT_FETCHER_CLASS_KEY_SUFFIX = ".class";
private static final String PROTOCOLS_KEY = "protocols";
+ private static final String AUTH_TOKEN_KEY = "auth.token";
Review comment:
Same here
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java
##########
@@ -0,0 +1,55 @@
+/**
+ * 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.core.auth;
+
+import java.util.Set;
+
+
+/**
+ * Container object for basic auth principal
+ */
+public class BasicAuthPrincipal {
+ private final String _name;
+ private final String _token;
+ private final Set<String> _tables;
+ private final Set<String> _permissions;
Review comment:
Should this be of type String or better to have "AccessType" ?
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * 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.controller.api.access;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import javax.ws.rs.core.HttpHeaders;
+import org.apache.pinot.core.auth.BasicAuthPrincipal;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the
"controller.admin.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * controller.admin.access.control.principals=admin123,user456
+ * controller.admin.access.control.principals.admin123.password=verysecret
+ * controller.admin.access.control.principals.user456.password=kindasecret
+ *
controller.admin.access.control.principals.user456.tables=stuff,lessImportantStuff
+ *
controller.admin.access.control.principals.user456.permissions=read,update
+ * </pre>
+ */
+public class BasicAuthAccessControlFactory implements AccessControlFactory {
+ private static final String PREFIX =
"controller.admin.access.control.principals";
+
+ private static final String HEADER_AUTHORIZATION = "Authorization";
+
+ private AccessControl _accessControl;
+
+ public void init(PinotConfiguration configuration) {
+ _accessControl = new
BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration,
PREFIX));
+ }
+
+ @Override
+ 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
Review comment:
What are these Overrides for ?
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * 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.controller.api.access;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import javax.ws.rs.core.HttpHeaders;
+import org.apache.pinot.core.auth.BasicAuthPrincipal;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the
"controller.admin.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * controller.admin.access.control.principals=admin123,user456
+ * controller.admin.access.control.principals.admin123.password=verysecret
+ * controller.admin.access.control.principals.user456.password=kindasecret
+ *
controller.admin.access.control.principals.user456.tables=stuff,lessImportantStuff
+ *
controller.admin.access.control.principals.user456.permissions=read,update
+ * </pre>
+ */
+public class BasicAuthAccessControlFactory implements AccessControlFactory {
+ private static final String PREFIX =
"controller.admin.access.control.principals";
+
+ private static final String HEADER_AUTHORIZATION = "Authorization";
+
+ private AccessControl _accessControl;
+
+ public void init(PinotConfiguration configuration) {
+ _accessControl = new
BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration,
PREFIX));
+ }
+
+ @Override
+ public AccessControl create() {
+ return _accessControl;
Review comment:
So the caller has to invoke init before create ? Instead, should we
check if _accessControl is initialized and take action accordingly ?
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * 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.controller.api.access;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import javax.ws.rs.core.HttpHeaders;
+import org.apache.pinot.core.auth.BasicAuthPrincipal;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the
"controller.admin.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * controller.admin.access.control.principals=admin123,user456
+ * controller.admin.access.control.principals.admin123.password=verysecret
+ * controller.admin.access.control.principals.user456.password=kindasecret
+ *
controller.admin.access.control.principals.user456.tables=stuff,lessImportantStuff
+ *
controller.admin.access.control.principals.user456.permissions=read,update
+ * </pre>
+ */
+public class BasicAuthAccessControlFactory implements AccessControlFactory {
+ private static final String PREFIX =
"controller.admin.access.control.principals";
+
+ private static final String HEADER_AUTHORIZATION = "Authorization";
+
+ private AccessControl _accessControl;
+
+ public void init(PinotConfiguration configuration) {
+ _accessControl = new
BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration,
PREFIX));
+ }
+
+ @Override
+ 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;
Review comment:
nit: rename to "_tokenToPrincipalsMap" just for ease of readability ?
----------------------------------------------------------------
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
