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

The SSO integration does not consider HTTP_PROXY env vars when making requests #9259

Closed
3 tasks done
sarahhenkens opened this issue Jul 29, 2022 · 9 comments · Fixed by #9760 or #10046
Closed
3 tasks done

The SSO integration does not consider HTTP_PROXY env vars when making requests #9259

sarahhenkens opened this issue Jul 29, 2022 · 9 comments · Fixed by #9760 or #10046
Labels
area/sso-rbac P1 High priority. All bugs with >=5 thumbs up that aren’t P0, plus: Any other bugs deemed high priority type/bug

Comments

@sarahhenkens
Copy link
Contributor

Checklist

  • Double-checked my configuration.
  • Tested using the latest version.
  • Used the Emissary executor.

Summary

The github.com/coreos/go-oidc/v3/oidc library by default uses an http client that is not reading the env vars for HTTP_PROXY variables. This results in SSO failing to initialize at startup.

Diagnostics

I added several log.Print statements in my fork of the release-3.3 branch and I discovered that the root cause is occuring within the oidc.NewProvider call:

func providerFactoryOIDC(ctx context.Context, issuer string) (providerInterface, error) {
	log2.Print("Attemping to create new OIDC provider")
	provider, err := oidc.NewProvider(ctx, issuer)
	log2.Print("Finished oidc.NewProvider(ctx, issuer)")
	return provider, err
}

By setting the context myself with the native http client of go, it works correctly:

func providerFactoryOIDC(ctx context.Context, issuer string) (providerInterface, error) {
	log2.Print("Attemping to create new OIDC provider")
	// Set the client context explicitly in order to use proxy configuration from environment(if any)
	// See https://github.com/coreos/go-oidc/blob/8d771559cf6e5111c9b9159810d0e4538e7cdc82/oidc.go#L43-L53
	oidc_ctx := oidc.ClientContext(ctx, &http.Client{})
	provider, err := oidc.NewProvider(oidc_ctx, issuer)
	log2.Print("Finished oidc.NewProvider(ctx, issuer)")
	return provider, err
}

Message from the maintainers:

Impacted by this bug? Give it a 👍. We prioritise the issues with the most 👍.

@sarahhenkens
Copy link
Contributor Author

The HandleCallback function seems to be impacted by this bug as well

@stale
Copy link

stale bot commented Aug 13, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is a mentoring request, please provide an update here. Thank you for your contributions.

@stale stale bot added the problem/stale This has not had a response in some time label Aug 13, 2022
@sarahhenkens
Copy link
Contributor Author

Still pending

@sarabala1979 sarabala1979 removed the problem/stale This has not had a response in some time label Aug 23, 2022
@sarabala1979
Copy link
Member

@sarahhenkens Do you like to submit the PR for the above change?

@simox-83
Copy link

We are impacted by the same issue. When can we get a resolution please?

@tvalasek
Copy link
Contributor

Hello. For us this is a blocker for upgrading.

@Evalle
Copy link

Evalle commented Sep 19, 2022

This is blocking us as well, thx.

@sarabala1979 sarabala1979 added the P1 High priority. All bugs with >=5 thumbs up that aren’t P0, plus: Any other bugs deemed high priority label Sep 24, 2022
@alexec
Copy link
Contributor

alexec commented Oct 6, 2022

@simox-83 @tvalasek @Evalle would you like to submit fix please?

alexec pushed a commit that referenced this issue Oct 10, 2022
…ixes #9259 (#9760)

fix: SSO integration not considering HTTP_PROXY env vars when making requests

Signed-off-by: Rohan Kumar <[email protected]>

Signed-off-by: Rohan Kumar <[email protected]>
juchaosong pushed a commit to juchaosong/argo-workflows that referenced this issue Nov 3, 2022
…ixes argoproj#9259 (argoproj#9760)

fix: SSO integration not considering HTTP_PROXY env vars when making requests

Signed-off-by: Rohan Kumar <[email protected]>

Signed-off-by: Rohan Kumar <[email protected]>
Signed-off-by: juchao <[email protected]>
@sarahhenkens
Copy link
Contributor Author

@alexec @rohankmr414, this is still broken due to the custom http.Transport created inside newSso. All the OAuth2 related API calls are missing the proxy information because of:

// Create http client with TLSConfig to allow skipping of CA validation if InsecureSkipVerify is set.
httpClient := &http.Client{Transport: &http.Transport{TLSClientConfig: &tls.Config{InsecureSkipVerify: c.InsecureSkipVerify}}}

Which is used in:

func (s *sso) HandleCallback(w http.ResponseWriter, r *http.Request)

...

oauth2Context := context.WithValue(ctx, oauth2.HTTPClient, s.httpClient)
oauth2Token, err := s.config.Exchange(oauth2Context, r.URL.Query().Get("code"), redirectOption)

The DefaultTransport of http sets the Proxy. And since s.httpClient was created with a custom Transport, its missing the proxy config. And as a result, the s.config.Exchange fails to use the http proxies configured.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/sso-rbac P1 High priority. All bugs with >=5 thumbs up that aren’t P0, plus: Any other bugs deemed high priority type/bug
Projects
None yet
6 participants