http91 bearer token support

[davidradl]

Description

Add support for bearer tokens.

Resolves HTTP91

PR Checklist

• [x ] Tests added
• [x ] Changelog updated

[kristoffSC]

Happy to see this change.
Left some comments/questions.

Apart from them, there is a code formater check failing on build.

[davidradl]

@kristoffSC The fix isn't quite ready so I let the pr in draft. I was going to get the code clean before making the pr ready for review. But it is bonus that you have given me feedback anyway - that is really helpful - thanks :-)

• I was looking at this and thought that it would be better to move this to a new OIDCAuthHeaderValuePreprocessor that implements HeaderValuePreprocessor, we can then look at the config and drive this HeaderValuePreprocessor instead of the basic one. With:

 @Override
    public String preprocessHeaderValue(String rawValue) {
        OidcAccessTokenManager auth = new OidcAccessTokenManager(
                HttpClient.newBuilder().build(),
                oidcTokenRequest.get(),
                oidcAuthURL.get(),
                oidcExpiryReduction.get()
        );
        // apply the OIDC authentication by adding the dynamically calculated header value.
        return "BEARER " + auth.authenticate();
    } 

In this way we preprocess the basic and OIDC authorization header content prior to calling the request. Also the change is minimal for the main code base, as it just tweaks the authorization header leaving the request processing unchanged.

Before this change, the basic auth content could have been processed then we override it with an OIDC header.

I am reworking the fix in this design. WDYT?

[davidradl]

@kristoffSC pushed up the new code addressing most of the code review comments - testing now.

[davidradl]

@kristoffSC Hi I have been testing against the watsonx APIs, the latest fix is not working (I will sort that out).

During this testing I found that the SSL handshake was failing. I had assumed that it would pickup the default JAVA certificates, but it does not.
1)
https://github.com/getindata/flink-http-connector/blob/05d3b474f403ff68ab6e7abe396[…]ta/connectors/http/internal/utils/JavaNetHttpClientFactory.java .

We can see that the SSLContext is created.

2. From https://github.com/getindata/flink-http-connector/blob/05d3b474f403ff68ab6e7abe396[…]etindata/connectors/http/internal/security/SecurityContext.java it does something when there is no certifcate supplied. The SSL handshake fails.
3. If I remove the SSLContext then is uses the certificates in java and the SSL handshake works.

In

flink-http-connector/src/main/java/com/getindata/connectors/http/internal/utils/JavaNetHttpClientFactory.java

Line 145 in 05d3b47

return SecurityContext.create();

it seems to create a Keystote when one has not been supplied. Do we need to do this?

If not we could just not put the SSLContext on the httpRequest when there is no supplied keystore.
if we need this logic, then maybe we should have new option to say use Default java certificates.
Raised #119 for this.
WDYT?

My actions at this stage are to:

• test specifying the public cert information in flink and config.
• debug why the bearer token logic is not working

[davidradl]

@kristoffSC the new design is not working because the preprocessing only works if there is a header to preprocess. In my case it was not working as I had not defined a Authentication header in the config - if I do it works. I see the following options:
1- document that an Authentication header needs to be specified in the Config
2- amend the code to add a dummy Authentication header if we are dealing with OIDC. The value will then be supplied by the OIDC header pre processor
3- Go with the original design.

I am looking at option 2 as 1 seems unnecessary extra considerations for the user, 3 seems a bit of a hack. WDYT?

[davidradl]

I have included the public certs and it works. By this I mean while testing I put the public server cert in an ssl folder and referred to it in the config. Ideally we should not have to do this as public certs should just work as they are in the JVM- as per previous updates

[kristoffSC]

Hi @davidradl
Regarding public certs -> I've replayed in #119 (comment)

Regarding this Pr here,
I also think option 3 is good, but could you describe what "the dummy header" would be?

[davidradl]

Hi @kristoffSC, In response to your comment

Regarding public certs -> I've replayed in #119 (comment)
Regarding this Pr here, I also think option 3 is good, but could you describe what "the dummy header" would be?

I have coded up option 2, by dummy header - if you search for dummy in the pr code you will see what I mean.
This allows me to add an oidc header pre processor. I think adding another header pre processor like the basic auth one, seems a good place to put this. Do you have a strong view as to why this is not a good idea? Option 2 changes the main code path as you previously noted.

[grzegorz8]

I have coded up option 2, by dummy header - if you search for dummy in the pr code you will see what I mean.
This allows me to add an oidc header pre processor. I think adding another header pre processor like the basic auth one, seems a good place to put this. Do you have a strong view as to why this is not a good idea? Option 2 changes the main code path as you previously noted.

For me the dummy header looks fine. We can always refactor if we come up with better solution in the future.

I left only a few minor comments.

[davidradl]

@grzegorz8 Hi there I have fixed up the requested changes- please could you review and ee if there is anything else you can spot and GTM if you are good with the changes.

[grzegorz8]

@grzegorz8 Hi there I have fixed up the requested changes- please could you review and ee if there is anything else you can spot and GTM if you are good with the changes.

Thanks. I'll review it soon.
I have one request, please do not amend commits and force push, because now it is hard to see what was actually changed :)

[davidradl]

@grzegorz8 Hi there I have fixed up the requested changes- please could you review and ee if there is anything else you can spot and GTM if you are good with the changes.

Thanks. I'll review it soon. I have one request, please do not amend commits and force push, because now it is hard to see what was actually changed :)

@grzegorz8 sorry - good point- I forgot.

[davidradl]

@grzegorz8 On the review comment , saying it was not addressed; I do not see any format changes now introduced by the fix. I see :

[grzegorz8]

@grzegorz8 On the review comment , saying it was not addressed; I do not see any format changes now introduced by the fix. I see :

This is what I see:

Probably you have "Hide whitespaces" enabled.

[davidradl]

@grzegorz8 On the review comment , saying it was not addressed; I do not see any format changes now introduced by the fix. I see :

This is what I see: Probably you have "Hide whitespaces" enabled.

@grzegorz8 I see - I thought you were referring to line 82. I will make this change.

[davidradl]

@grzegorz8 I fixed the format of the one you pointed out and also some in the test files. Let me know if you need anything else changing.

All suggestions have been applied.