You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/06/09 17:23:34 UTC
[GitHub] [solr] thelabdude opened a new pull request #168: SOLR-15451: SolrSchema (for SQL) should use PKI principal for request to /admin/luke
thelabdude opened a new pull request #168:
URL: https://github.com/apache/solr/pull/168
https://issues.apache.org/jira/browse/SOLR-15451
# Description
When executing a SQL statement, Solr needs to hit the `/admin/luke` endpoint (for a specific collection). This PR executes that request in a background "server" thread that uses the PKI principal (instead of the principal for the user that initiated the SQL request).
# Solution
Pretty straight-forward solution to run the request in a background thread that is marked as server thread, so the PKI principal is used. I also added a check for 403 in the `LBSolrClient` error handling to propagate authz-related errors directly instead of reporting "No live servers available to service this request".
# Tests
Added a unit test that executes a SQL statement as a user that does not have explicit access to the `/admin/luke` endpoint to verify the SQL can be executed successfully.
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] thelabdude commented on pull request #168: SOLR-15451: SolrSchema (for SQL) should use PKI principal for request to /admin/luke
Posted by GitBox <gi...@apache.org>.
thelabdude commented on pull request #168:
URL: https://github.com/apache/solr/pull/168#issuecomment-858194341
Alternatively, we could have `PKIAuthenticationPlugin` just set the server token header for all server threads and do away with the flag I added to `SolrRequestInfo` ... this actually makes more sense to me than the current logic of preferring the Principal from `SolrRequestInfo` but I'm not sure if there is logic that depends on that?
--
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: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #168: SOLR-15451: SolrSchema (for SQL) should use PKI principal for request to /admin/luke
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #168:
URL: https://github.com/apache/solr/pull/168#discussion_r649247022
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java
##########
@@ -217,6 +217,15 @@ public synchronized String nextOrError(Exception previousEx) throws SolrServerEx
if (numServersTried > 0 && isTimeExceeded(timeAllowedNano, timeOutTime)) {
throw new SolrServerException("Time allowed to handle this request exceeded"+suffix, previousEx);
}
+
+ // is authz related error
+ if (previousEx instanceof BaseHttpSolrClient.RemoteSolrException) {
+ BaseHttpSolrClient.RemoteSolrException rse = (BaseHttpSolrClient.RemoteSolrException)previousEx;
+ if (rse.code() == 403) {
+ throw new SolrServerException(previousEx.getMessage(), previousEx);
+ }
+ }
Review comment:
Can we have a test case that exercises this?
##########
File path: solr/solr-ref-guide/src/parallel-sql-interface.adoc
##########
@@ -66,6 +66,14 @@ The `/sql` handler is the front end of the Parallel SQL interface. All SQL queri
By default, the `/sql` request handler is configured as an implicit handler, meaning that it is always enabled in every Solr installation and no further configuration is required.
+==== Authorization for SQL Requests
+
+If your Solr cluster is configured to use the <<rule-based-authorization-plugin.adoc#,Rule-based Authorization Plugin>>,
+then you need to grant `GET` and `POST` permission on the `/sql`, `/select`, and `/export` endpoints for all collections you intend to execute SQL queries against.
+
+Prior to Solr 9.0, you'll also need to grant `GET` permission on the `/admin/luke` endpoint for all collections queried using SQL; the JDBC driver uses this endpoint to get schema metadata for the collection.
+As of Solr 9.0, the Solr executes the request to the `/admin/luke` endpoint using the internal PKI principal.
Review comment:
@ctargett you were talking about the proper ways to document "known issues" previously, was wondering how you would handle this case 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: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] thelabdude commented on a change in pull request #168: SOLR-15451: SolrSchema (for SQL) should use PKI principal for request to /admin/luke
Posted by GitBox <gi...@apache.org>.
thelabdude commented on a change in pull request #168:
URL: https://github.com/apache/solr/pull/168#discussion_r649504387
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java
##########
@@ -217,6 +217,15 @@ public synchronized String nextOrError(Exception previousEx) throws SolrServerEx
if (numServersTried > 0 && isTimeExceeded(timeAllowedNano, timeOutTime)) {
throw new SolrServerException("Time allowed to handle this request exceeded"+suffix, previousEx);
}
+
+ // is authz related error
+ if (previousEx instanceof BaseHttpSolrClient.RemoteSolrException) {
+ BaseHttpSolrClient.RemoteSolrException rse = (BaseHttpSolrClient.RemoteSolrException)previousEx;
+ if (rse.code() == 403) {
+ throw new SolrServerException(previousEx.getMessage(), previousEx);
+ }
+ }
Review comment:
I'll revert this change, now that I fixed the LukeRequest issue, we're not hitting this code anyway. I think there's a problem here that should be improved but should be done as a separate change. For now it has no impact on this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #168: SOLR-15451: SolrSchema (for SQL) should use PKI principal for request to /admin/luke
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #168:
URL: https://github.com/apache/solr/pull/168#discussion_r648607552
##########
File path: solr/core/src/java/org/apache/solr/handler/sql/SolrSchema.java
##########
@@ -90,17 +95,34 @@ public boolean isClosed() {
return builder.build();
}
- private Map<String, LukeResponse.FieldInfo> getFieldInfo(String collection) {
- String zk = this.properties.getProperty("zk");
- CloudSolrClient cloudSolrClient = solrClientCache.getCloudSolrClient(zk);
+ @SuppressForbidden(reason = "Do not want the MDC to propagate the user principal, need PKI principal")
+ private Map<String, LukeResponse.FieldInfo> getFieldInfo(final String collection) {
+ final String zk = this.properties.getProperty("zk");
+ // Need to run this in a background server thread so the PKI principal is used to authn / authz the luke request
+ // Not using the MDC aware executor framework b/c that propagates the principal from this thread, which is what we don't want here
+ ExecutorService service = Executors.newSingleThreadExecutor(threadFactory);
Review comment:
Do we need to create this every time? I guess we don't want sql requests stacking on top of one another if this is a shared executor... There has to already be an internal handler for this that we can piggy back on, no?
--
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: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] thelabdude commented on a change in pull request #168: SOLR-15451: SolrSchema (for SQL) should use PKI principal for request to /admin/luke
Posted by GitBox <gi...@apache.org>.
thelabdude commented on a change in pull request #168:
URL: https://github.com/apache/solr/pull/168#discussion_r648734114
##########
File path: solr/core/src/java/org/apache/solr/handler/sql/SolrSchema.java
##########
@@ -90,17 +95,34 @@ public boolean isClosed() {
return builder.build();
}
- private Map<String, LukeResponse.FieldInfo> getFieldInfo(String collection) {
- String zk = this.properties.getProperty("zk");
- CloudSolrClient cloudSolrClient = solrClientCache.getCloudSolrClient(zk);
+ @SuppressForbidden(reason = "Do not want the MDC to propagate the user principal, need PKI principal")
+ private Map<String, LukeResponse.FieldInfo> getFieldInfo(final String collection) {
+ final String zk = this.properties.getProperty("zk");
+ // Need to run this in a background server thread so the PKI principal is used to authn / authz the luke request
+ // Not using the MDC aware executor framework b/c that propagates the principal from this thread, which is what we don't want here
+ ExecutorService service = Executors.newSingleThreadExecutor(threadFactory);
Review comment:
I took another approach for this, see updated code and PR description.
--
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: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] thelabdude commented on a change in pull request #168: SOLR-15451: SolrSchema (for SQL) should use PKI principal for request to /admin/luke
Posted by GitBox <gi...@apache.org>.
thelabdude commented on a change in pull request #168:
URL: https://github.com/apache/solr/pull/168#discussion_r648629727
##########
File path: solr/core/src/java/org/apache/solr/handler/sql/SolrSchema.java
##########
@@ -90,17 +95,34 @@ public boolean isClosed() {
return builder.build();
}
- private Map<String, LukeResponse.FieldInfo> getFieldInfo(String collection) {
- String zk = this.properties.getProperty("zk");
- CloudSolrClient cloudSolrClient = solrClientCache.getCloudSolrClient(zk);
+ @SuppressForbidden(reason = "Do not want the MDC to propagate the user principal, need PKI principal")
+ private Map<String, LukeResponse.FieldInfo> getFieldInfo(final String collection) {
+ final String zk = this.properties.getProperty("zk");
+ // Need to run this in a background server thread so the PKI principal is used to authn / authz the luke request
+ // Not using the MDC aware executor framework b/c that propagates the principal from this thread, which is what we don't want here
+ ExecutorService service = Executors.newSingleThreadExecutor(threadFactory);
Review comment:
I'm not aware of any "internal handler" that exists for this ... I'll add a cached thread pool service to the `PKIAuthenticationPlugin` to run the request in ... I just wasn't sure how common this problem is across the codebase to warrant adding something reusable.
--
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: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] thelabdude commented on a change in pull request #168: SOLR-15451: SolrSchema (for SQL) should use PKI principal for request to /admin/luke
Posted by GitBox <gi...@apache.org>.
thelabdude commented on a change in pull request #168:
URL: https://github.com/apache/solr/pull/168#discussion_r651965550
##########
File path: solr/solr-ref-guide/src/parallel-sql-interface.adoc
##########
@@ -66,6 +66,14 @@ The `/sql` handler is the front end of the Parallel SQL interface. All SQL queri
By default, the `/sql` request handler is configured as an implicit handler, meaning that it is always enabled in every Solr installation and no further configuration is required.
+==== Authorization for SQL Requests
+
+If your Solr cluster is configured to use the <<rule-based-authorization-plugin.adoc#,Rule-based Authorization Plugin>>,
+then you need to grant `GET` and `POST` permission on the `/sql`, `/select`, and `/export` endpoints for all collections you intend to execute SQL queries against.
+
+Prior to Solr 9.0, you'll also need to grant `GET` permission on the `/admin/luke` endpoint for all collections queried using SQL; the JDBC driver uses this endpoint to get schema metadata for the collection.
+As of Solr 9.0, the Solr executes the request to the `/admin/luke` endpoint using the internal PKI principal.
Review comment:
Your approach sounds good to me @ctargett ... I'll make some changes to branch_8x! Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org