-
Notifications
You must be signed in to change notification settings - Fork 83
Authentication for metrics and version endpoint #9029
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
base: master
Are you sure you want to change the base?
Conversation
e5eddf2
to
cf5ccce
Compare
cf5ccce
to
53a145f
Compare
53a145f
to
fa24a14
Compare
fa24a14
to
f4993b1
Compare
6f9df66
to
a87519b
Compare
src/server/web_server.js
Outdated
try { | ||
jwt_utils.authenticate_jwt_token(req); | ||
} catch (err) { | ||
res.writeHead(403, { 'Content-Type': 'text/plain' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why text/plain
and not application/json
if the format is JSON? Same for prometheus reporting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First this error handling shouldn't be here. as noted below this should be in a function in http_utils.
Second - why are we authenticating the get_version call ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parent link for this epic: RHSTOR-7202 mentioned that version also needs to be authenticated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont know what is mentioned there and why, but I am afraid that it will fail the callers to this version route as they dont authenticate and just use plain curl. did you check who is calling it in the core and operator code bases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in operator I couldnt find the direct call to version endpoint, in core I could find one reference for version here, Is this code still in use?
Customer concern is about exposing the version, and vulnerabilities associated with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we always return the server version as a header - see endpoint_utils.set_noobaa_server_header
is this critical enough to require a change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We better add a config option that allow us to turn on/off the auth for both metrics and version routes (one config for each because different clients use it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, and for metrics and version default value for auth enabled?
config.METRICS_AUTH_ENABLED = true;
config.VERSION_AUTH_ENABLED = true;
src/server/web_server.js
Outdated
try { | ||
jwt_utils.authenticate_jwt_token(req); | ||
} catch (err) { | ||
res.writeHead(403, { 'Content-Type': 'text/plain' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First this error handling shouldn't be here. as noted below this should be in a function in http_utils.
Second - why are we authenticating the get_version call ??
41cf84e
to
2409ace
Compare
2409ace
to
53c677f
Compare
53c677f
to
5440be0
Compare
message: err.message, | ||
}, null, 2); | ||
res.end(reply); | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not enough, the caller should check for this return, and the normal path should return true. but i think maybe this is better be thrown instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all the methods ends with res.end() do not send anything back, simply end the method with res.end()
An I tested the flow with this change and its working without any issues
@@ -1118,6 +1118,9 @@ config.DEFAULT_REGION = 'us-east-1'; | |||
|
|||
config.VACCUM_ANALYZER_INTERVAL = 86400000; | |||
|
|||
config.METRICS_AUTH_ENABLED = true; | |||
config.VERSION_AUTH_ENABLED = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am not sure about the default being true. will that break anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldnt find issues during my testing.
Signed-off-by: naveenpaul1 <[email protected]>
5440be0
to
f84e8d8
Compare
Describe the Problem
Noobaa provides multiple metrics endpoints for customers. The first one is Noobaa management service and the other one is Noobaa endpoint service. In the case of Noobaa management metrics endpoint, Noobaa provides a route to fetch the metrics. And for Noobaa endpoint metrics, metrics can be fetched by loadbalancer IP and port. Anyone with a valid URL can access these metrics endpoints without any kind of authentication.
Explain the Changes
noobaa -core
metrics-auth
, with the role validation we can make sure other JWT tokens signed using the same secret won’t work.Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
containerized deployment
metrics-auth-secret
, secret can be used for accessing noobaa management and endpoint metrics/version.Design doc : https://ibm.ent.box.com/notes/1853310270159