[
https://issues.apache.org/jira/browse/CALCITE-2294?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16482759#comment-16482759
]
ASF GitHub Bot commented on CALCITE-2294:
-----------------------------------------
Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/calcite-avatica/pull/48#discussion_r189658596
--- Diff:
server/src/main/java/org/apache/calcite/avatica/server/HttpServer.java ---
@@ -239,18 +239,8 @@ protected void internalStart() {
server.setConnectors(new Connector[] { connector });
// Default to using the handler that was passed in
- final HandlerList handlerList = new HandlerList();
- Handler avaticaHandler = handler;
-
- // Wrap the provided handler for security if we made one
- if (null != securityHandler) {
- securityHandler.setHandler(handler);
- avaticaHandler = securityHandler;
- }
+ configureHandlers(securityHandler);
--- End diff --
Agreed, we can add a clause that `CUSTOM` config has to be used with
appropriate `ServerCustomizer` to create the server. I can document that
appropriately. We can also restrict users to use `ServerCustomizer` with
`CUSTOM` type for the following reason.
`ServerCustomizer` was added to provide additional options for
configuration to the server. Since we pass in the `Jetty server` object
directly, one can directly remove all the connectors , security handlers and
other relevant properties from the server, which can potentially lead to
unexpected bugs.
Here (at SFDC), we need a way to customize the servers so that we can
configure our own `SSLContextFactory` and `ServerConnector` on the handler
(based off some internal code for MTLS auth). Having customizers allow us to do
that, however we don't have a mechanism to bypass all default
`AvaticaServerConfiguration` logic with the methods provided. We also need to
support impersonation, which is only allowed with `buildSpnegoConfiguration()`
method, however we don't want to use SPNEGO. This `CUSTOM` method of
authentication opens up a lot of doors to customize both authZ and authN as per
requirements. As shown in example, we can also use this up for adding
impersonation with BasicAuth, which is currently not supported. We still want
to use the same handlers as the core logic remains same.
Let me know your thoughts on this one!
I agree that this is not the _best_ way of doing this. I believe that
`AvaticaServerConfiguration` interface should provide methods to authenticate
the user. For example, the method `isUserPermitted()` is called first inside
the `AvaticaProtobufHandler` as well as `AvaticaJsonHandler` classes, where the
authentication happens only for the case of SPNEGO. Ideally this method should
be wrapped around every handler that is implemented, since I can potentially
add a new Handler and forget to call this method at the start. Also, if we add
new auth methods, we need to add if-else statements over here to ensure that we
cover all the cases. I feel that in such a case `AvaticaServerConfiguration`
should be self-sufficient to provide an interface method that can handle the
part based on its type. We can use that wrapper to also handle common pieces of
code in these handlers like exception handling or impersonation allowance based
on serverConfig.
If you can point me to the code that helps in such wrapping, I am happy to
do the refactoring.
> Allow customization for AvaticaServerConfiguration for plugging new
> authentication mechanisms
> ---------------------------------------------------------------------------------------------
>
> Key: CALCITE-2294
> URL: https://issues.apache.org/jira/browse/CALCITE-2294
> Project: Calcite
> Issue Type: Improvement
> Components: avatica
> Reporter: Karan Mehta
> Priority: Major
>
> {{AvaticaServerConfiguration}} is currently only created if authentication
> mechanism such as {{BASIC, DIGEST or SPNEGO}} is provided. We can change it
> to a builder pattern to create this object and provide a way for users to
> plugin their own security configuration.
> An example here can be using it for custom config that supports MTLS.
> Thanks [~alexaraujo] for suggesting this approach.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
