cloud-sdk-java HttpClientWrapper in non-public

Issue Description

HttpClientWrapper used by cloud sdk is a non-public class. This class is returned by HttpClientAccessor and used by http clinet cache.

The issue is that I am trying to implement a custom CSRF token management logic to avoid multiple parallel CSRF token requests which are not necessary (the token can be safely cached). And sometimes there is race condition between tokens that leads to invalid tokens being sent (as each request produces own token but tokens are bound to a session cookie and session cookie is ovewritten with every request for a token). And CSRF token should be bound to an instance of http client as the target system binds token to a session cookie and cookie store is bound to an http client. One of the possible solutions is to extend existing http client and add csrf token field to it (and provide custom HttpClientFactory), so that csrf token can be cached along with the client. But the fact that HttpClientWrapper is non-public prevents me from extending it.... In addition AbstractHttpClientCache checks specifically for HttpClientWrapper class to execute withDestination method before returning the client. It means if I provide custom implementation of teh client that is not a subclass of HttpClientWrapper, the method withDestination will not be executed. And potentially there can be some further logic added by cloud sdk that checks specifically for HttpClientWrapper.

So, it would be very useful to have HttpClientWrapper as a public class.

Impact / Priority

Affected development phase: Production

Impact: Inconvenience/Impaired (cannot proceed with implementation)

Error Message

Project Details

SDK Version: 5.8.0

Project type, for example:
- [x] CAP Project
- [ ] SDK Maven Archetype
- [ ] None of the above:
Platform:
- [ ] Cloud Foundry
- [x] Deploy with Confidence (Cloud Foundry)
- [ ] None of the above:

Checklist

[ ] Checked out the documentation and Stack Overflow
[ ] Description provided with all relevant information
[ ] Exception and stack trace provided
[ ] Attached debug logs
[ ] Attached dependency tree
[ ] Provided Cloud SDK version & link to relevant source code

May 15 '24 13:05 ivan-karlovich-sap

(a) This sounds rough. Is it possible to tell the service maintainers to stop (ab)using CSRF token handling that way? This seems very strange to me.

(b) HttpClientWrapper is non-public because of poor API quality. Extending or polishing it will require some alignment with the core development team. This is especially difficult to decide since Apache HttpClient 4 (or 5) are not that popular anymore in "modern" Java development, and we were asked to support other clients instead.

(c) Having AbstractHttpClientCache directly instance-check and cast the provided http client object is indeed weak, and jeopardizes extensibility - as you described. Unfortunate. https://github.com/SAP/cloud-sdk-java/blob/f0dd44016892ef1f1fbd758b83c84d00948f9854/cloudplatform/connectivity-apache-httpclient4/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/AbstractHttpClientCache.java#L115-L117

We'll check in the team, whether it makes sense to offer a public interface, at least.

(d) If I were you, I would've tried to provide a custom CsrfTokenRetriever instead. Maybe it could check whether a session cookie is already stored with the client and try to reuse the previous CSRF token instead of querying a new one. To me this would sound easier than developing a custom SDK-based HttpClient implementation. However it's still sketchy.

May 15 '24 14:05 newtork

a) Unfortunately, it is not possible to change CSRF-token-related behavior in the target system. Parallel HEAD requests without or with invalid session cookie will produce new (and different) session cookies and (new and different) corresponding tokens.

b) Could you please suggest what other clients are provided by Cloud SDK?

d) As for checking the value of the session cookie stored by http client: the issue is that we should not interpret cookies. The client should just send the cookies back to the server (respecting expiration time and path) but should not try to interpret cookies names or content. In general, the name of the session cookie is not guaranteed.

May 15 '24 15:05 ivan-karlovich-sap

Please correct me if I understood the situation incorrectly:

There's an OData service, referenced by Destination.
The target system requires a CSRF token for some or all endpoint interactions.
The target system requires a session cookie. It will send a new cookie...
- if none was provided previously
- if there's a request header x-csrf-token: fetch
The target system responds with an error...
- if no CSRF token is provided (for some or all endpoint interactions)
- if the provided cookie does not match the provided CSRF token

This sounds rough. Is it possible to tell the service maintainers to stop (ab)using CSRF token handling that way? This seems very strange to me.

Unfortunately, it is not possible to change CSRF-token-related behavior in the target system. Parallel HEAD requests without or with invalid session cookie will produce new (and different) session cookies and (new and different) corresponding tokens.

Is it possible to ask the target system to disable CSRF token requirements? Effectively it doesn't offer any security benefits and instead unnecessarily increases complexity, computation and traffic.

Could you please suggest what other clients are provided by Cloud SDK?

None effectively. There are workarounds for RestTemplate and WebClient to leverage Apache HttpClient, but this will not be helpful to you. I meant, proper support for these/other HTTP clients are being requested, but we're lacking capacity at the moment. So please don't expect huge API or behavior changes.

As said earlier, we can certainly consider offering public interfaces, for unblocking extensibility.

As for checking the value of the session cookie stored by http client: the issue is that we should not interpret cookies. The client should just send the cookies back to the server (respecting expiration time and path) but should not try to interpret cookies names or content. In general, the name of the session cookie is not guaranteed.

I did not mention cookie names or content.

Currently the DefaultHttpClientCache stores instances of HttpClient for 1h after last access. If you can assure that this usually does not exceed cookie expiration, then you can assume the stored cookie will remain legit and you only need to make sure the client never queries a second CSRF token.

This is why I was suggesting a custom CsrfTokenRetriever implementation.

You could take the "DefaultCsrfTokenRetriever" as template and wrap a cache around the critical code.

// make sure to have the CSRF tokens live longer than HttpClient (and its contained Cookie)
private static Cache<HttpClient, CsrfToken> cache =
    Caffeine.newBuilder().expireAfterWrite(Duration.ofDays(1)).build();

@Override
@Nonnull
public CsrfToken retrieveCsrfToken(
    @Nonnull final HttpClient httpClient,
    @Nonnull final String servicePath,
    @Nonnull final Map<String, Collection<String>> headers )
{
    return cache.get(httpClient, (client) -> {
        try {
            // ...
        } catch (final NoSuchElementException e) {
            // ...
        } catch (final Exception e) {
            // ...
        }
    });
}

If the HttpClient lives longer than the cookie, I'm afraid we don't have a logic to evict it from the above mentioned cache yet.

Anyhow, I wouldn't recommend doing any of this productively and instead complain to the service maintainers to drop CSRF token requirements.

May 16 '24 08:05 newtork

One of the possible solutions is to extend existing http client and add csrf token field to it (and provide custom HttpClientFactory), so that csrf token can be cached along with the client.

Have you considered...?

Disable CSRF token retrieval for individual OData requests and...

Apply custom header field to the Destination after initialization.

Destination destinationBefore = DestinationAccessor.getDestination("blub");
String csrfToken; // your logic

Destination destinationAfter = ((DefaultHttpDestination) destinationBefore)
  .toBuilder()
  .header("x-csrf-token", csrfToken)
  .build();

Unfortunately the cookie will likely be lost between the destinations. Probably you would need to propagate it somehow.

Instead of casting and recreating a new destination (like above) you could also look into DestinationHeaderProvider API for setting the x-csrf-token in customized way. Of course you would need a cache/map to combine clients and tokens.
Use the Generic OData Client as it interacts directly with the provided HttpClient. There is no implicit Destination API. This way you may have better control over cookies and token headers. Conveniently you can cast all OData v2 / v4 requests to Generic OData API using the toRequest() method.

if I provide custom implementation of the client that is not a subclass of HttpClientWrapper, the method withDestination will not be executed.

That should not be a problem since, the method is "only" used for sanity checking. It may not be a requirement for your use case.

And potentially there can be some further logic added by cloud sdk that checks specifically for HttpClientWrapper.

There are no further checks or references to HttpClientWrapper than what you already found. Of course, a custom implementation introduces the risk that this may change over time. No one can give you a proper promise here.

May 16 '24 08:05 newtork

The target system requires a session cookie. It will send a new cookie.

The new session cookie will be issued either if no session cookie is provided with the request or if an invalid/expired cookie is provided. x-csrf-token: fetch header only asks to provide csrf token.

Is it possible to ask the target system to disable CSRF token requirements? Effectively it doesn't offer any security benefits and instead unnecessarily increases complexity, computation and traffic.

They say it is not possible.

As said earlier, we can certainly consider offering public interfaces, for unblocking extensibility.

It would be really helpful.

Currently the DefaultHttpClientCache stores instances of HttpClient for 1h after last access. If you can assure that this usually does not exceed cookie expiration, then you can assume the stored cookie will remain legit and you only need to make sure the client never queries a second CSRF token.

Yeah, I can do something like this. But the issue is that caching policy of HttpClient can change in future versions of cloud sdk. If I continue sending old csrf token after new http client is issued by the cache (with empty cookie store), I will just receive invalid token error (sometimes multiple errors in case of parallel requests). But I could avoid this if I could invalidate the token as soon as new client is provisioned. That's what I am trying to achieve but I see no way. I cannot check if internal instance of http client used by HttpClientWrapper was changed as I have no accees to the field. And I cannot provide custom wrapper over HttpClientWarpper as I will lose destinaton-related logic as there is no way to call withDestination method.

If the HttpClient lives longer than the cookie, I'm afraid we don't have a logic to evict it from the above mentioned cache yet.

Here I could implement custom HttpClient cache that would invalidate clients after 1h or so to lower probaility of expired session cookie. Also I could catch 403 or so error typical in this case. But if I implement custom cache of clients retruned by HttpClientAccessor, it will lead to caching destination along with the client. One way to avoid it is to call withDestination method before every request but it is not exposed.

Currently, I am sending requests as:

final HttpClient client = HttpClientAccessor.getHttpClient(destination);
// perform the HTTP operation:
final ODataRequestResultGeneric result = request.execute(client);

And I'm making sure that csrf token is retrieved once and then provided from cache in a manner similar to: https://sap.github.io/cloud-sdk/docs/java/features/odata/v4-vdm-client#solution

That should not be a problem since, the method is "only" used for sanity checking. It may not be a requirement for your use case.

Can you elaborate more on this? Why the client is not simply returned from cache but requires withDestination call? I see it as some destination headers that are not part of cache key could change but we still want to take them into account when making requests. Nevertheless, ideally, I would prefer this logic to be executed for custom client as well, otherwise it can lead to discrepancies in the future.

May 16 '24 09:05 ivan-karlovich-sap

Why the client is not simply returned from cache but requires withDestination call? I see it as some destination headers that are not part of cache key could change but we still want to take them into account when making requests.

The HttpClient withDestination(Destination) method makes sure that the HttpClient instance loaded from the cache for a given Destination, is actually really fitting the Destination.

Why could anything else be possible?

Because the cache keys are compared by equals and hashCode. It's surprisingly difficult to compare two instance of Destination without doubt. In the past we struggled with comparing TrustStore and KeyStore fields, however we found a reasonable workaround. Unfortunately we don't have a solution for the field storing instances of DestinationHeaderProvider; in the past we did not enforce proper hashCode and equals implementation on API or JavaDoc level. This is why today we fallback to comparing the Destination references, just to be sure. This sub-par quality is one of the reasons for us to not promote the classes to public :)

If you're not working with custom DestinationHeaderProvider in your application, I think you can reasonably ignore the method in question.

By the way, one other option for you could be to just use our package-namespace for custom implementations. I think this could at least unblock you development-wise. If you had a working solution using this workaround, this could simplify our discussion.

May 16 '24 13:05 newtork

It's been two weeks without update, I'm closing the issue for now. Please let me know if you want to continue here.

Anyways, please everyone feel encouraged to improve the situation. Please open a pull request, where we can discuss solutions and improvements to make SAP Cloud SDK a better product :)

May 30 '24 10:05 newtork