You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2021/02/24 20:38:36 UTC
[GitHub] [lucene-solr] epugh opened a new pull request #2430: SOLR-15194: relax requirements and allow http urls.
epugh opened a new pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430
# Description
Allow `http` urls to work, albeit with a warning.
# Solution
Add the `http` protocol, and log a warning about this not being super secure!
# Tests
Tweaked the unit tests, and tested manually with Keycloak (which out of the box docker image version doesn't support https!).
# Checklist
Please review the following and check all that apply:
- [X ] 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.
- [ X] I have created a Jira issue and added the issue ID to my pull request title.
- [ X] 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)
- [ X] I have developed this patch against the `master` branch.
- [ X] I have run `./gradlew check`.
- [ X] I have added tests for my changes.
- [ X] I have added documentation for the [Ref Guide](https://github.com/apache/lucene-solr/tree/master/solr/solr-ref-guide) (for Solr changes only).
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] epugh commented on a change in pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
epugh commented on a change in pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#discussion_r583262966
##########
File path: solr/core/src/java/org/apache/solr/security/JWTIssuerConfig.java
##########
@@ -349,12 +349,18 @@ public HttpsJwksFactory(long jwkCacheDuration, long refreshReprieveThreshold) {
this.jwkCacheDuration = jwkCacheDuration;
this.refreshReprieveThreshold = refreshReprieveThreshold;
}
-
+
+ /**
+ * While the class name is HttpsJwks, it actually works with plain http formatted url as well.
+ * @param url the Url to connect to for JWK details.
+ */
private HttpsJwks create(String url) {
try {
URL jwksUrl = new URL(url);
- if (!"https".equalsIgnoreCase(jwksUrl.getProtocol())) {
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, PARAM_JWKS_URL + " must use HTTPS");
+ if ("http".equalsIgnoreCase(jwksUrl.getProtocol())) {
+ if (log.isWarnEnabled()) {
Review comment:
there are 9 places in non test code where we do this ;-) SIgh.
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] epugh commented on a change in pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
epugh commented on a change in pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#discussion_r583567245
##########
File path: solr/core/src/java/org/apache/solr/security/JWTIssuerConfig.java
##########
@@ -68,6 +68,11 @@
private WellKnownDiscoveryConfig wellKnownDiscoveryConfig;
private String clientId;
private String authorizationEndpoint;
+
+ public static boolean ALLOW_OUTBOUND_HTTP = Boolean.parseBoolean(System.getProperty("solr.auth.jwt.allowOutboundHttp", "false"));
+ public static final String ALLOW_OUTBOUND_HTTP_ERR_MSG = "Outbound non SSL protected JWT authentication urls are not enabled, start your nodes with -Dsolr.auth.jwt.allowOutboundHttp=true.";
Review comment:
I like!
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#issuecomment-786956265
Could you bring the or up to date with master? Lots of unrelated stuff in the diff now.
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] epugh commented on pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#issuecomment-787059727
I believe I brought it up to date with master. I do struggle with those unrelated commits sneaking in.
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] epugh commented on pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#issuecomment-786284092
I've added in the environment variable that @janhoy suggested. I don't know if the warning actually is needed anymore, since presumably if you set the environment variable you know what the right thing? that you are bypassing htings? I left it, and if someone has an opionon, I'm super easily swayed.
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] epugh commented on pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#issuecomment-786817645
Okay, I've spent quite a few hours attempting to a) Mock up the call to open the URL, or b) Introduce a builder pattern that I could then mock out, and had no joy. Refactoring this to mock up the call is out of my depth right now.
I'd love another LGTM and then I'll merge and backport.
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] epugh commented on pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#issuecomment-786645918
I think you are right, so I'll yank out the WARN (and the related test code), and then I think we are ready for merging!
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] HoustonPutman commented on pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#issuecomment-786009511
Very weird precommit failure, given that its suggesting you do exactly what you have already 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: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] madrob commented on a change in pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#discussion_r583268336
##########
File path: solr/core/src/test/org/apache/solr/security/JWTIssuerConfigTest.java
##########
@@ -125,6 +128,31 @@ public void parseIssuerConfigExplicit() {
assertEquals("https://host/jwk", issuerConfig.getJwksUrls().get(0));
}
+ @Test
Review comment:
So another option would be to create a mock URL object, that returns a FileInputStream when called openConnection.
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] epugh commented on a change in pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
epugh commented on a change in pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#discussion_r583225058
##########
File path: solr/core/src/test/org/apache/solr/security/JWTIssuerConfigTest.java
##########
@@ -144,13 +172,29 @@ public void wellKnownConfigFromString() throws IOException {
assertEquals(Arrays.asList("code", "code id_token", "code token", "code id_token token", "token", "id_token", "id_token token"), config.getResponseTypesSupported());
}
- @Test(expected = SolrException.class)
- public void wellKnownConfigNotHttps() {
- JWTIssuerConfig.WellKnownDiscoveryConfig.parse("http://127.0.0.1:45678/.well-known/config");
+ @Test
+ public void wellKnownConfigWithHttpLogsAWarning() {
+
+ LogWatcherConfig watcherCfg = new LogWatcherConfig(true, null, "WARN", 100);
+ @SuppressWarnings({"rawtypes"})
+ LogWatcher watcher = LogWatcher.newRegisteredLogWatcher(watcherCfg, null);
+
+ watcher.reset();
+
+ // currently we throw an exception because we can't open a connection to this fake url and can't mock it.
+ expectThrows(SolrException.class, () -> JWTIssuerConfig.WellKnownDiscoveryConfig.parse("http://127.0.0.1:45678/.well-known/config"));
Review comment:
This is different than `wellKnownConfigNotREachable`, in that test we confirm we get an excpetion when you open a connection a not reachable url. This one is JUST supposed to test the `http` logs a warning. However, the code path then opens a connection to the url, triggering the SolrException (just like in `wellKnownConfigNotReachable`). I really wanted a mock for the actually http call, and I tried gettign soemthign to work with mockito, but it started me down refactorign a lot of code, so I stopped....
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] madrob commented on a change in pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#discussion_r583170235
##########
File path: solr/core/src/test/org/apache/solr/security/JWTIssuerConfigTest.java
##########
@@ -125,6 +128,31 @@ public void parseIssuerConfigExplicit() {
assertEquals("https://host/jwk", issuerConfig.getJwksUrls().get(0));
}
+ @Test
+ public void jwksUrlwithHttpLogsaWarning() {
+ HashMap<String, Object> issuerConfigMap = new HashMap<>();
+ issuerConfigMap.put("name", "myName");
+ issuerConfigMap.put("iss", "myIss");
+ issuerConfigMap.put("jwksUrl", "http://host/jwk");
+
+ JWTIssuerConfig issuerConfig = new JWTIssuerConfig(issuerConfigMap);
+
+ LogWatcherConfig watcherCfg = new LogWatcherConfig(true, null, "WARN", 100);
+ @SuppressWarnings({"rawtypes"})
+ LogWatcher watcher = LogWatcher.newRegisteredLogWatcher(watcherCfg, null);
+
+ assertEquals(1, issuerConfig.getHttpsJwks().size());
+ assertEquals("http://host/jwk", issuerConfig.getHttpsJwks().get(0).getLocation());
+
+
+ SolrDocumentList history = watcher.getHistory(-1, null);
+
+ if (history.stream().filter(d -> "org.apache.solr.security.JWTIssuerConfig".equals(d.getFirstValue("logger"))).count() == 0) {
Review comment:
`assertTrue(msg, history.stream().findFirst(d -> ...).isPresent())`
##########
File path: solr/core/src/test/org/apache/solr/security/JWTIssuerConfigTest.java
##########
@@ -144,13 +172,29 @@ public void wellKnownConfigFromString() throws IOException {
assertEquals(Arrays.asList("code", "code id_token", "code token", "code id_token token", "token", "id_token", "id_token token"), config.getResponseTypesSupported());
}
- @Test(expected = SolrException.class)
- public void wellKnownConfigNotHttps() {
- JWTIssuerConfig.WellKnownDiscoveryConfig.parse("http://127.0.0.1:45678/.well-known/config");
+ @Test
+ public void wellKnownConfigWithHttpLogsAWarning() {
+
+ LogWatcherConfig watcherCfg = new LogWatcherConfig(true, null, "WARN", 100);
+ @SuppressWarnings({"rawtypes"})
+ LogWatcher watcher = LogWatcher.newRegisteredLogWatcher(watcherCfg, null);
+
+ watcher.reset();
+
+ // currently we throw an exception because we can't open a connection to this fake url and can't mock it.
+ expectThrows(SolrException.class, () -> JWTIssuerConfig.WellKnownDiscoveryConfig.parse("http://127.0.0.1:45678/.well-known/config"));
Review comment:
Store the exception returned and assert something about it, there are lots of different solr exceptions that can get thrown. Is this the same test as `wellKnownConfigNotReachable`?
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] epugh commented on a change in pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
epugh commented on a change in pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#discussion_r583223688
##########
File path: solr/core/src/test/org/apache/solr/security/JWTIssuerConfigTest.java
##########
@@ -125,6 +128,31 @@ public void parseIssuerConfigExplicit() {
assertEquals("https://host/jwk", issuerConfig.getJwksUrls().get(0));
}
+ @Test
+ public void jwksUrlwithHttpLogsaWarning() {
+ HashMap<String, Object> issuerConfigMap = new HashMap<>();
+ issuerConfigMap.put("name", "myName");
+ issuerConfigMap.put("iss", "myIss");
+ issuerConfigMap.put("jwksUrl", "http://host/jwk");
+
+ JWTIssuerConfig issuerConfig = new JWTIssuerConfig(issuerConfigMap);
+
+ LogWatcherConfig watcherCfg = new LogWatcherConfig(true, null, "WARN", 100);
+ @SuppressWarnings({"rawtypes"})
+ LogWatcher watcher = LogWatcher.newRegisteredLogWatcher(watcherCfg, null);
+
+ assertEquals(1, issuerConfig.getHttpsJwks().size());
+ assertEquals("http://host/jwk", issuerConfig.getHttpsJwks().get(0).getLocation());
+
+
+ SolrDocumentList history = watcher.getHistory(-1, null);
+
+ if (history.stream().filter(d -> "org.apache.solr.security.JWTIssuerConfig".equals(d.getFirstValue("logger"))).count() == 0) {
Review comment:
I am struggling to get this.. I can get a .findFirst(), but not with the filter.... I'd be happy for you to push up an edit ;-) This is at my outer edges of experinece in Java! the whole `->` weirds me out!
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] epugh commented on a change in pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
epugh commented on a change in pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#discussion_r583262322
##########
File path: solr/core/src/java/org/apache/solr/security/JWTIssuerConfig.java
##########
@@ -349,12 +349,18 @@ public HttpsJwksFactory(long jwkCacheDuration, long refreshReprieveThreshold) {
this.jwkCacheDuration = jwkCacheDuration;
this.refreshReprieveThreshold = refreshReprieveThreshold;
}
-
+
+ /**
+ * While the class name is HttpsJwks, it actually works with plain http formatted url as well.
+ * @param url the Url to connect to for JWK details.
+ */
private HttpsJwks create(String url) {
try {
URL jwksUrl = new URL(url);
- if (!"https".equalsIgnoreCase(jwksUrl.getProtocol())) {
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, PARAM_JWKS_URL + " must use HTTPS");
+ if ("http".equalsIgnoreCase(jwksUrl.getProtocol())) {
+ if (log.isWarnEnabled()) {
Review comment:
I think I had it puking, and tried to fix this. Do we have a pattern?
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] epugh commented on a change in pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
epugh commented on a change in pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#discussion_r583565911
##########
File path: solr/core/src/java/org/apache/solr/security/JWTIssuerConfig.java
##########
@@ -68,6 +68,11 @@
private WellKnownDiscoveryConfig wellKnownDiscoveryConfig;
private String clientId;
private String authorizationEndpoint;
+
+ public static boolean ALLOW_OUTBOUND_HTTP = Boolean.parseBoolean(System.getProperty("solr.auth.jwt.allowOutboundHttp", "false"));
+ public static final String ALLOW_OUTBOUND_HTTP_ERR_MSG = "Outbound non SSL protected JWT authentication urls are not enabled, start your nodes with -Dsolr.auth.jwt.allowOutboundHttp=true.";
+
+
Review comment:
"Jan-Lint-Bot" ;-) Where is my Solr Java Lint checker ;-)
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] madrob commented on a change in pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#discussion_r583268708
##########
File path: solr/core/src/test/org/apache/solr/security/JWTIssuerConfigTest.java
##########
@@ -125,6 +128,31 @@ public void parseIssuerConfigExplicit() {
assertEquals("https://host/jwk", issuerConfig.getJwksUrls().get(0));
}
+ @Test
Review comment:
Wait, that wouldn't work because we don't create the URL in the test, it's done in the main code. Well, maybe you could still thread this through, I'm not sure.
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] epugh commented on a change in pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
epugh commented on a change in pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#discussion_r583265431
##########
File path: solr/core/src/test/org/apache/solr/security/JWTIssuerConfigTest.java
##########
@@ -125,6 +128,31 @@ public void parseIssuerConfigExplicit() {
assertEquals("https://host/jwk", issuerConfig.getJwksUrls().get(0));
}
+ @Test
Review comment:
This feels like using a sledgehammer on a fly... Thoughts on just coming up with a mock in the right place? How mcuh change would it be to introduce a mock object to return the content instead of a full blown server. Plus, this feels like it might be slow ;-)
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] epugh commented on pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#issuecomment-786585570
@janhoy I think we all know the answer to "is it worth all the extra code lines" since we both have the same feeling ;-). Do you think the WARN logic is EVEN useful since we have a parameter now that you have to intentionally set. I'm wondering what your thought is on removing the warning in the log?
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on a change in pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#discussion_r583296977
##########
File path: solr/core/src/java/org/apache/solr/security/JWTIssuerConfig.java
##########
@@ -68,6 +68,11 @@
private WellKnownDiscoveryConfig wellKnownDiscoveryConfig;
private String clientId;
private String authorizationEndpoint;
+
+ public static boolean ALLOW_OUTBOUND_HTTP = Boolean.parseBoolean(System.getProperty("solr.auth.jwt.allowOutboundHttp", "false"));
+ public static final String ALLOW_OUTBOUND_HTTP_ERR_MSG = "Outbound non SSL protected JWT authentication urls are not enabled, start your nodes with -Dsolr.auth.jwt.allowOutboundHttp=true.";
Review comment:
Hard to read sentence - dobule negation. What about
>HTTPS required for IDP communication. Please use SSL or start your nodes with -Dsolr.auth.jwt.allowOutboundHttp=true to allow HTTP for test purposes.
##########
File path: solr/core/src/java/org/apache/solr/security/JWTIssuerConfig.java
##########
@@ -68,6 +68,11 @@
private WellKnownDiscoveryConfig wellKnownDiscoveryConfig;
private String clientId;
private String authorizationEndpoint;
+
+ public static boolean ALLOW_OUTBOUND_HTTP = Boolean.parseBoolean(System.getProperty("solr.auth.jwt.allowOutboundHttp", "false"));
+ public static final String ALLOW_OUTBOUND_HTTP_ERR_MSG = "Outbound non SSL protected JWT authentication urls are not enabled, start your nodes with -Dsolr.auth.jwt.allowOutboundHttp=true.";
+
+
Review comment:
Unneccessary spaces
##########
File path: solr/solr-ref-guide/src/jwt-authentication-plugin.adoc
##########
@@ -161,6 +161,10 @@ Let's comment on this config:
<12> Configure the audience claim. A token's 'aud' claim must match 'aud' for one of the configured issuers.
<13> This issuer is auto configured through discovery, so 'iss' and JWK settings are not required
+=== Using non SSL URLs
+In Production you should always use SSL protected HTTPS connections since Solr is making an outbound connection,
Review comment:
Production -> "production environments"
Remove "since Solr is making an outbound connection"?
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] madrob commented on a change in pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#discussion_r583259647
##########
File path: solr/core/src/java/org/apache/solr/security/JWTIssuerConfig.java
##########
@@ -349,12 +349,18 @@ public HttpsJwksFactory(long jwkCacheDuration, long refreshReprieveThreshold) {
this.jwkCacheDuration = jwkCacheDuration;
this.refreshReprieveThreshold = refreshReprieveThreshold;
}
-
+
+ /**
+ * While the class name is HttpsJwks, it actually works with plain http formatted url as well.
+ * @param url the Url to connect to for JWK details.
+ */
private HttpsJwks create(String url) {
try {
URL jwksUrl = new URL(url);
- if (!"https".equalsIgnoreCase(jwksUrl.getProtocol())) {
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, PARAM_JWKS_URL + " must use HTTPS");
+ if ("http".equalsIgnoreCase(jwksUrl.getProtocol())) {
+ if (log.isWarnEnabled()) {
Review comment:
You generally don't need guard statements around warn logging - it is almost always enabled, so we don't need to check on it like we do for more detailed levels.
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#issuecomment-786317911
It's difficult to review since it suddenly contains unrelated edits. Think you should merge in latest master again...
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#issuecomment-786612766
> Do you think the WARN logic is EVEN useful since we have a parameter now that you have to intentionally set. I'm wondering what your thought is on removing the warning in the log?
I'd probably skip testing that the logger actually logs. And given how this is documented, users should be aware of the risk. The only reason for keeping the WARN log would be in cases where settings from dev/test environments sneak into production unaware. But then again, if the production IDP server is setup with HTTPS, it would immediately fail if trying a HTTP URL...
My gut feeling is keep it securey by default, make it easy to switch to insecure for dev and that's it.
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] madrob commented on a change in pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#discussion_r583244417
##########
File path: solr/core/src/test/org/apache/solr/security/JWTIssuerConfigTest.java
##########
@@ -125,6 +128,31 @@ public void parseIssuerConfigExplicit() {
assertEquals("https://host/jwk", issuerConfig.getJwksUrls().get(0));
}
+ @Test
+ public void jwksUrlwithHttpLogsaWarning() {
+ HashMap<String, Object> issuerConfigMap = new HashMap<>();
+ issuerConfigMap.put("name", "myName");
+ issuerConfigMap.put("iss", "myIss");
+ issuerConfigMap.put("jwksUrl", "http://host/jwk");
+
+ JWTIssuerConfig issuerConfig = new JWTIssuerConfig(issuerConfigMap);
+
+ LogWatcherConfig watcherCfg = new LogWatcherConfig(true, null, "WARN", 100);
+ @SuppressWarnings({"rawtypes"})
+ LogWatcher watcher = LogWatcher.newRegisteredLogWatcher(watcherCfg, null);
+
+ assertEquals(1, issuerConfig.getHttpsJwks().size());
+ assertEquals("http://host/jwk", issuerConfig.getHttpsJwks().get(0).getLocation());
+
+
+ SolrDocumentList history = watcher.getHistory(-1, null);
+
+ if (history.stream().filter(d -> "org.apache.solr.security.JWTIssuerConfig".equals(d.getFirstValue("logger"))).count() == 0) {
Review comment:
Ah, you're right, I should have checked an IDE before commenting. You want `assertTrue(msg, history.stream().anyMatch(d -> ...)`
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] madrob commented on a change in pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430#discussion_r583262258
##########
File path: solr/core/src/test/org/apache/solr/security/JWTIssuerConfigTest.java
##########
@@ -125,6 +128,31 @@ public void parseIssuerConfigExplicit() {
assertEquals("https://host/jwk", issuerConfig.getJwksUrls().get(0));
}
+ @Test
Review comment:
If you want to stand up a lightweight server, you can do something like this. You'll probably want to randomize the port (maybe you can pass 0 to always get an open port?)
```
@SuppressForbidden(reason="using httpserver to mock a response")
@Test
public void wellKnownConfigWithHttpLogsAWarning() throws IOException {
com.sun.net.httpserver.HttpServer server = HttpServer.create(new InetSocketAddress("127.0.0.1", 45678), 1);
server.createContext("/", exchange -> {
Path configJson = TEST_PATH().resolve("security").resolve("jwt_well-known-config.json");
exchange.sendResponseHeaders(200, 0);
exchange.getResponseBody().write(Files.readAllBytes(configJson));
exchange.close();
});
try {
server.start();
// stuff
} finally {
server.stop(0);
}
}
```
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] epugh merged pull request #2430: SOLR-15194: relax requirements and allow http urls.
Posted by GitBox <gi...@apache.org>.
epugh merged pull request #2430:
URL: https://github.com/apache/lucene-solr/pull/2430
----------------------------------------------------------------
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@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org