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

Openshift oauth discover authorization_endpoint #26

Merged
merged 2 commits into from
Jan 20, 2021

Conversation

csturiale
Copy link

@csturiale csturiale commented Jan 11, 2021

I previously faced issues in OCP4 where oauth server url differs from kubernetes API url.
This PR add the possibility to discover oauth authorization_endpoint and solve previous issue.

Copy link
Member

@Vlatombe Vlatombe left a comment

Choose a reason for hiding this comment

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

Looks good to me, works on OCP4 for me.

One small detail, the added org.json:json dependency can be removed and replaced with the net.sf.json lib that comes from Jenkins core

final HttpClientBuilder builder = HttpClientWithTLSOptionsFactory.getBuilder(uri, caCertData, skipTLSVerify);
HttpGet discover = new HttpGet(apiServerURL + "/.well-known/oauth-authorization-server");
final CloseableHttpResponse response = builder.build().execute(discover);
return new JSONObject(EntityUtils.toString(response.getEntity())).getString("authorization_endpoint");
Copy link
Member

Choose a reason for hiding this comment

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

Replace org.json with net.sf.json which is provided by the Jenkins ecosystem

Suggested change
return new JSONObject(EntityUtils.toString(response.getEntity())).getString("authorization_endpoint");
return JSONObject.fromObject(EntityUtils.toString(response.getEntity())).getString("authorization_endpoint");

Copy link
Author

Choose a reason for hiding this comment

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

added a new commit , removed org.json.JSONObject from pom.xml and replace JSSONObject with the suggested code

@Vlatombe
Copy link
Member

@maxlaverse this looks important

@@ -35,7 +36,8 @@

@Before
public void prepareFakeOAuthServer() throws Exception {
server = new Server(0);
InetSocketAddress addr = new InetSocketAddress("localhost", 0);

Choose a reason for hiding this comment

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

Just to understand, what does this change ?

Copy link
Author

Choose a reason for hiding this comment

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

It change nothing in normal situation, I make the change because I was connected via VPN and maybe also due to proxy settings I was experiencing issues reaching the assigned IP (192.* ) . If needed , I can revert this change.

private String getOauthServerUrl(String apiServerURL, String caCertData, boolean skipTLSVerify) throws URISyntaxException, IOException, HttpClientWithTLSOptionsFactory.TLSConfigurationError {
URI uri = new URI(apiServerURL);
final HttpClientBuilder builder = HttpClientWithTLSOptionsFactory.getBuilder(uri, caCertData, skipTLSVerify);
HttpGet discover = new HttpGet(apiServerURL + "/.well-known/oauth-authorization-server");

Choose a reason for hiding this comment

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

Do all API servers answer to such request in OpenShift ? Or do we need to fallback to the apiServerURL + /oauth/authorize when it's not the case ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I tested this api in OCP3 and OCP4 , it should be a standard for oauth authentication as reported here

@maxlaverse
Copy link

Alright. Thanks for the additional information

@maxlaverse maxlaverse merged commit 03955e1 into jenkinsci:master Jan 20, 2021
@maxlaverse
Copy link

Released as 0.8.0

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.

3 participants