Use proxy (if provided) for OIDC configuration

[zgael]

As discussed in Dependency Track boards, proxies are not supported for OIDC configuration.

As the feature involves multiple projects :

1. Dependency Track
2. Alpine (this one)
3. Oauth-sdk OIDC

there is not one obivous place where the fix should be done.

I chose to perform everything inside this project as it is closely related to Dependency Track (where the issue is blocking).

Change summary : Instead of calling directly oauth-sdk's resolve OIDC config method, I chose to split it in two steps (that are actually performed internally by the function) :

• Fetch the config (using basic HTTPRequest with default parameters, where I added the proxy support)
• Call oauth-sdk's parse method

Feel free to discuss, ask questions, request changes if necessary.

[sonatype-lift]

⚠️ 5 God Classes were detected by Lift in this project. Visit the Lift web console for more details.

[zgael]

Hi @stevespringett, I don't really know how you usually handle your PRs, so allow me to notify you on this one.
Do you need anything more on this topic, explanations, informations ?
I believe there could be a "patched" version of Alpine with this to have it released faster than waiting for a full iteration, but that again depends on your workflow.

Happy to help if needed, don't hesitate to ask.

[stevespringett]

@nscuro any input?

[nscuro]

Thanks for the PR @zgael!

The same will need to be done for the OidcUserInfoAuthenticator, as it is reaching out to the auth server to validate the token.

We should also consider ALPINE_NO_PROXY. It is very common to have a company-internal auth server like Keycloak, for which no proxy must be used.

Moreover, I think we need to support the standard HTTP_PROXY / HTTPS_PROXY / NO_PROXY environment variables too. Dependency-Track has logic for this already that we can take inspiration from. @stevespringett, maybe a good opportunity to move the proxy logic to Alpine?

[stevespringett]

maybe a good opportunity to move the proxy logic to Alpine?

Certainly. If its common then it makes sense to move it here.

[zgael]

Hi @nscuro, thanks for pointing out what I missed in OidcUserInfoAuthenticator, nice catch.

Took me some time, but I just pushed another version to take your remarks into account :

1. Added the same proxy logic to OidcUserInfoAuthenticator
2. Factored the proxy logic for the 2 changes I made into a single class (OidcProxyHelper)
3. Took into account NO_PROXY logic, following curl's documentation (since there is no real specification on this).

I tried writing unit tests to check the proxy logic was working as intended, but it seemed to me it was a bit annoying to mock the config while I was not passing it as parameter of my new class (OidcProxyHelper).
Feel free to give tips on what I could improve if you feel the need to !

[zgael]

@nscuro Any news on this topic ?

[nscuro]

@zgael Apologies for the long delay, I'll have a look in the next few days.

[nscuro]

Overall this looks good, but I'd like to get a consistent proxy behavior with how DT does it. I don't think we should maintain such logic in both DT and Alpine.

[nscuro]

This will only take the Alpine config into account (ALPINE_HTTP_PROXY_ADDRESS etc.), but not the "standard" way of configuring proxies (http_proxy etc.).

We need to support both ways, in a way that is consistent with how DT is doing it. I've gone ahead and ported the logic from DT to Alpine in #456, as I suggested in #422 (comment). Once merged, you'll be able to do this:

final var proxyCfg = ProxyUtil.getProxyConfig();
if (proxyCfg.shouldProxy(configURL)) {
    return new Proxy(Proxy.Type.HTTP, new InetSocketAddress(proxyCfg.getHost(), proxyCfg.getPort()));
}

[sonatype-lift]

YodaCondition: The non-constant portion of an equals check generally comes first.

---

Suggested change

	if ("*".equals(noProxyHost)) {
	if (Objects.equals(noProxyHost, "*")) {

---

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command	Usage
@sonatype-lift ignore	Leave out the above finding from this PR
@sonatype-lift ignoreall	Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool>	Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

---

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

[sonatype-lift]

🛠 Lift Auto-fix

Some of the Lift findings in this PR can be automatically fixed. You can download and apply these changes in your local project directory of your branch to review the suggestions before committing.¹

# Download the patch
curl https://lift.sonatype.com/api/patch/github.com/stevespringett/Alpine/422.diff -o lift-autofixes.diff

# Apply the patch with git
git apply lift-autofixes.diff

# Review the changes
git diff

Want it all in a single command? Open a terminal in your project's directory and copy and paste the following command:

curl https://lift.sonatype.com/api/patch/github.com/stevespringett/Alpine/422.diff | git apply

Once you're satisfied, commit and push your changes in your project.

Footnotes

1. You can preview the patch by opening the patch URL in the browser. ↩