Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use new class ProxyConfig for proxy selection in OIDC configuration #466

Merged
merged 1 commit into from Feb 27, 2023

Conversation

zgael
Copy link

@zgael zgael commented Feb 13, 2023

Hi,

following this (closed/not merged) issue #422 , I create this new one to make use of the newly created ProxyConfig class in order to have OIDC configuration use proxy if declared in configuration.

Thanks @nscuro for the work on ProxyConfig, looks good to me !

HTTPRequest httpRequest = new UserInfoRequest(configuration.getUserInfoEndpointUri(), new BearerAccessToken(accessToken)).toHTTPRequest();
final ProxyConfig proxyCfg = ProxyUtil.getProxyConfig();

if (proxyCfg.shouldProxy(configuration.getUserInfoEndpointUri().toURL())) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

18% of developers fix this issue

NULL_DEREFERENCE: object proxyCfg last assigned on line 51 could be null and is dereferenced at line 53.


ℹ️ 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 ]

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zgael A check for proxyCfg being null would be good here. getProxyConfig will return null when no proxy config was found at all.

Copy link
Collaborator

@nscuro nscuro Feb 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, was already addressed. Never mind then, please mark this conversation as resolved.

Copy link
Collaborator

@nscuro nscuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @zgael!

Code looks good to me, but I'd like to test it with DT to make sure it works as intended.

@zgael
Copy link
Author

zgael commented Feb 20, 2023

Depending on how you plan to test it, I might be able to help :

  • if that means you will build a docker image for DT (with a specific tag) and push it somewhere public, I could pull it and try to use it as I originally intended to (behind corporate proxy) with minimal effort (as I would reinvest that time when deploying it).
  • if you were by some way planning to build and test things locally, as I'm in no way familiar with building DT, I'm not sure I will be fit for this task. But once a newer version of DT is released, I would perform the test anyway, so you'd have feedback.

Keep me informed!

@stevespringett
Copy link
Owner

@nscuro Have you tested this with DT?

@nscuro
Copy link
Collaborator

nscuro commented Feb 26, 2023

@stevespringett Not yet, sorry. I'll try to get it done tomorrow.

Copy link
Collaborator

@nscuro nscuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with Dependency-Track by proxying requests to a local Keycloak server through ZAP. Both config resolution and UserInfo requests are proxied correctly for the ALPINE_HTTP_PROXY_* and "standard" HTTP_PROXY way. Including the Keycloak address in NO_PROXY bypasses the proxy as expected.

Screenshot 2023-02-26 at 12 57 55

This is good to merge from my side @stevespringett. Thanks for the PR @zgael! 🚀

@stevespringett stevespringett merged commit 3793e56 into stevespringett:master Feb 27, 2023
nscuro added a commit to nscuro/Alpine that referenced this pull request Apr 25, 2023
Fixes an oversight of stevespringett#466

Relates to DependencyTrack/dependency-track#2696

Signed-off-by: nscuro <nscuro@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants