use system props when creating the RestTransport#539
use system props when creating the RestTransport#539fer-marino wants to merge 10 commits intoweaviate:mainfrom
Conversation
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Infrastructure as Code | View in Orca | ||
| SAST | View in Orca | ||
| Secrets | View in Orca | ||
| Vulnerabilities | View in Orca |
|
To avoid any confusion in the future about your contribution to Weaviate, we work with a Contributor License Agreement. If you agree, you can simply add a comment to this PR that you agree with the CLA so that we can merge. |
|
Nice addition, thank you @fer-marino |
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Infrastructure as Code | View in Orca | ||
| SAST | View in Orca | ||
| Secrets | View in Orca | ||
| Vulnerabilities | View in Orca |
🛡️ The following SAST misconfigurations have been detected
| NAME | FILE | ||
|---|---|---|---|
| Prevent Server-Side Request Forgery (SSRF) via User Input in URLs | ...ltGrpcTransport.java | View in code |
Note: The scan should have failed if no policies were configured in warn-only mode.
|
oh sorry. I've added a lot more stuff. Using an http proxy proved much more difficult than I thought, so here is the implementation. I've also added an extra method for the OIDC authentication to use resourceOwnerPassword providing also a client secret id (needed if anon login is disabled) |
|
@fer-marino it's been a busy week, but I'm going to get back to this PR first thing Monday. Appreciate you taking time to contribute!
Could you please elaborate on the difficulties you ran into with configuring http proxy via system properties? I guess one limitation of that approach would be that it doesn't allow using a different proxy per client. Was there something else you encountered? |
bevzzz
left a comment
There was a problem hiding this comment.
I suggest focusing this PR on adding proxy support and introduce ROPC authentication in a separate PR.
src/main/java/io/weaviate/client6/v1/internal/oidc/OidcUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/io/weaviate/client6/v1/internal/oidc/nimbus/NimbusTokenProvider.java
Outdated
Show resolved
Hide resolved
src/main/java/io/weaviate/client6/v1/internal/oidc/nimbus/NimbusTokenProvider.java
Show resolved
Hide resolved
|
hi, I've implemented most of your suggestions. About my difficulties in implementing the proxy, is the different http libraries used by the different interfaces. My initial solution worked only for the rest transport, leaving out gRPC and OIDC authentication. |
|
Thanks! Could you please rebase your branch on the latest |
bevzzz
left a comment
There was a problem hiding this comment.
This is shaping up really well. Here are a couple more things I noticed.
Is there any simple test we could write for the HTTP / gRPC proxy configs? We're already using MockServer for some tests, can we extend the test suite to also verify request are being proxied correctly? e.g. using MockServer's proxying utility
src/main/java/io/weaviate/client6/v1/internal/oidc/nimbus/Flow.java
Outdated
Show resolved
Hide resolved
src/main/java/io/weaviate/client6/v1/internal/oidc/nimbus/NimbusTokenProvider.java
Outdated
Show resolved
Hide resolved
| return () -> grant; // Reuse cached authorization grant | ||
| } | ||
|
|
||
| static Flow resourceOwnerPassword(String clientId, String clientSecret, String username, String password) { |
There was a problem hiding this comment.
Rename to resourceOwnerPasswordCredentials
src/main/java/io/weaviate/client6/v1/internal/oidc/OidcConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/io/weaviate/client6/v1/internal/TokenProvider.java
Outdated
Show resolved
Hide resolved
src/main/java/io/weaviate/client6/v1/internal/oidc/nimbus/NimbusTokenProvider.java
Outdated
Show resolved
Hide resolved
Makes sense, yes. Thanks a lot for taking these two aspects into consideration. |
Co-authored-by: dyma solovei <53943884+bevzzz@users.noreply.github.com>
….java Co-authored-by: dyma solovei <53943884+bevzzz@users.noreply.github.com>
…usTokenProvider.java Co-authored-by: dyma solovei <53943884+bevzzz@users.noreply.github.com>
Co-authored-by: dyma solovei <53943884+bevzzz@users.noreply.github.com>
…java Co-authored-by: dyma solovei <53943884+bevzzz@users.noreply.github.com>
…usTokenProvider.java Co-authored-by: dyma solovei <53943884+bevzzz@users.noreply.github.com>
|
added some more test units, including one for the proxy. I've also modified the pom, as in my setup the lombok annotation processor was not getting invoked during unit tests execution. |
| // Since WeaviateClient performs REST calls in constructor, we want to ensure | ||
| // that we separate the verification. | ||
| // ProxyTest's setUp already created a client which did REST calls via proxy. | ||
|
|
There was a problem hiding this comment.
This comment looks misplaced -- we've already verified the proxy works before (or failed the test)
| // In this test, we verify that the client has the proxy configured. | ||
| org.junit.Assert.assertNotNull(client.getConfig().proxy()); | ||
| org.junit.Assert.assertEquals((long) proxyServer.getLocalPort(), (long) client.getConfig().proxy().port()); |
There was a problem hiding this comment.
Should this be its own test case? Seems like these assertions aren't directly related to gRPC transport. Additionally, let's stick to assertj.Assertions for test assertions.
| try { | ||
| // Perform a gRPC call. | ||
| client.collections.use("Test").size(); | ||
| } catch (Exception e) { | ||
| // If we reach here, it means the client (and its gRPC transport) | ||
| // was successfully initialized and attempted a call. | ||
| // The "Network closed" error in logs confirms that gRPC tried to connect. | ||
| } |
There was a problem hiding this comment.
Is there a way we can narrow down this assertion? Right now any exception thrown by CollectionHandle::size is interpreted as "the test succeeded"; we should distill some condition to tell us if the exception was thrown for the right reason. I included a suggestion below.
The "Network closed" error in logs confirms that gRPC tried to connect.
Are these logs visible anywhere and/or can be asserted? For example:
Assertions.assertThatThrownBy(() -> client.collections.use("Test").size())
.isInstanceOf(WeaviateTransportException.class)
.hasMessageContaining("Network closed");If not, then I suggest we remove that comment as it is not clear which logs its referring to.
| public static OidcProxy from(Proxy proxy) { | ||
| return proxy == null ? null : new OidcProxy(proxy.host(), proxy.port(), proxy.scheme()); | ||
| } |
There was a problem hiding this comment.
| public static OidcProxy from(Proxy proxy) { | |
| return proxy == null ? null : new OidcProxy(proxy.host(), proxy.port(), proxy.scheme()); | |
| } | |
| public OidcProxy(Proxy proxy) { | |
| this(requireNonNull(proxy, "proxy is null").scheme(), proxy.host(), proxy.port()); | |
| } |
Feels like proxy == null ? null will be causing ambiguity at the call site. IMO a less ambiguous contract would be "create a non-null OidcProxy from a non-null Proxy".
See another comment about the ordering of parameters.
| public record OidcProxy( | ||
| String host, | ||
| int port, | ||
| String scheme) { |
There was a problem hiding this comment.
nit: similarly to Proxy, can we order the parameters as scheme, host, port?
|
|
||
| static Flow resourceOwnerPasswordCredentials(String clientId, String clientSecret, String username, String password) { | ||
| return new Flow() { | ||
| private final AuthorizationGrant GRANT = new ResourceOwnerPasswordCredentialsGrant(username, new Secret(password)); |
There was a problem hiding this comment.
| private final AuthorizationGrant GRANT = new ResourceOwnerPasswordCredentialsGrant(username, new Secret(password)); | |
| private static final AuthorizationGrant GRANT = new ResourceOwnerPasswordCredentialsGrant(username, new Secret(password)); |
sorry, I must've missed that in my original suggestion
| private final OIDCProviderMetadata metadata; | ||
| private final AuthorizationServerMetadata metadata; |
There was a problem hiding this comment.
Why is this change necessary?
| this(ep == null ? "NULL" : ep.method(req), | ||
| ep == null ? "NULL" : ep.requestUrl(req), | ||
| ep == null ? "NULL" : ep.body(req), | ||
| ep == null ? null : ep.queryParameters(req)); |
There was a problem hiding this comment.
When would endpoint be null? The endpoint describes how to execute the request.
| } | ||
|
|
||
| private List<Request<?>> requests = new ArrayList<>(); | ||
| private Object nextResponse = null; |
There was a problem hiding this comment.
Seems like nextResponse is only ever set to null. If so, the condition on line 52 if (nextResponse != null) {...} is never going to fire. What do we need this field for?
| <configuration> | ||
| <annotationProcessorPaths> | ||
| <path> | ||
| <groupId>org.projectlombok</groupId> | ||
| <artifactId>lombok</artifactId> | ||
| <version>${lombok.version}</version> | ||
| </path> | ||
| </annotationProcessorPaths> | ||
| </configuration> |
There was a problem hiding this comment.
This project doesn't use Lombok for generating getters/setters. If you apply the suggestion about getConfig() in WeaviateClient.java then this configuration will probably become redundant.
Thank you 👍 |
this is a simple pull request that enable the usage of system properties when creating a RestTransport client. This is particularly useful when you need to configure the HTTP client for edge cases, eg. using an http proxy:
-Dhttp.proxyHost=10.250.0.1 -Dhttp.proxyPort=33128