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

remove public access to metrics and updated uri metrics output #114

Merged
merged 3 commits into from Oct 4, 2021

Conversation

CathalOConnorRH
Copy link
Contributor

Motivation

Metrics are accessible outside of an openshift cluster,
I've added a parameter to disable external access once the 'x-forwarded-host' header has data into as added by ha proxy on cluster.

Updated metrics to replace client id with '{id}' when uri metrics are enabled and not detailed.

@Rajagopalan-Ranganathan

@CathalOConnorRH have we tested with keycloak on kubernetes? if it doesnt work there, we need to explicitly mention that this would only for openshift deployments.

@CathalOConnorRH
Copy link
Contributor Author

@CathalOConnorRH have we tested with keycloak on kubernetes? if it doesnt work there, we need to explicitly mention that this would only for openshift deployments.

@Rajagopalan-Ranganathan We have tested but this doesn't work on kubernetes out of the box, it may if there is a proxy in front of the pod that sets the 'x-forwarded-host' header

@Rajagopalan-Ranganathan

@pb82 can we have this reviewed and merged? we are waiting for this change to be rolled out , so that we can promote our changes to prod.

public Response get(@Context HttpServletRequest servletRequest, @Context HttpHeaders headers) {

if (DISABLE_OPENSHIFT_EXTERNAL_ACCESS){
if ( !headers.getRequestHeader("x-forwarded-host").isEmpty() ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@CathalOConnorRH Can we rename the flag to DISABLE_EXTERNAL_ACCESS? The spi itself, nor the x-forwarded-host are not OpenShift specific.

The value of the header is not checked. So any requester who sets the header would be permitted. Is that ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated env var and readme

@pb82
Copy link
Contributor

pb82 commented Sep 30, 2021

@Rajagopalan-Ranganathan on it, sorry for the delay

@Rajagopalan-Ranganathan

@Rajagopalan-Ranganathan on it, sorry for the delay

thanks!

@pb82 pb82 merged commit f6c3f81 into aerogear:master Oct 4, 2021
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