-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Allow Kibana to restrict the usage of JWT for a predefined set of routes only. #163806
Conversation
0a41a33
to
e7fa68b
Compare
// The `api` tag ensures that unauthenticated calls receive a 401 rather than a 302 redirect to login page. | ||
// The `security:acceptJWT` tag allows route to be accessed with JWT credentials. It points to | ||
// ROUTE_TAG_ACCEPT_JWT from '@kbn/security-plugin/server' that cannot be imported here directly. | ||
tags: ['api', 'security:acceptJWT'], |
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.
note: I couldn't find a way to not hard-code string defined in x-pack security plugin here apart from moving these constants to a dedicated package or common
(but logically it's not common
so ..)?
e7fa68b
to
120de73
Compare
Hey @legrego, I don't have integration tests yet, so it's not ready for a full review. However, it would be great to hear your thoughts on the approach in general. Thank you! |
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.
General approach LGTM, I left a couple of questions below
// If Kibana is configured to restrict JWT authentication only to selected routes, ensure that the route is marked | ||
// with the `ROUTE_TAG_ACCEPT_JWT` tag to bypass that restriction. | ||
if ( | ||
user.authentication_realm.type === JWT_REALM_TYPE && | ||
this.jwt?.taggedRoutesOnly && | ||
!request.route.options.tags.includes(ROUTE_TAG_ACCEPT_JWT) | ||
) { | ||
this.logger.error( | ||
`Attempted to authenticate with JWT credentials against ${request.url.pathname}${request.url.search}, but it's not allowed. ` + | ||
`Ensure that the route is defined with the "${ROUTE_TAG_ACCEPT_JWT}" tag.` | ||
); | ||
return AuthenticationResult.notHandled(); | ||
} |
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'm torn on the location & timing of this check. This will allow a user to attempt JWT auth for disallowed routes, incurring unnecessary overhead (which may or may not matter). It also gives us different behavior in the failure scenario where the user presents an invalid JWT (catch
block below), as we are relying on the successful auth attempt to detect if it should be allowed or not.
Performing this check earlier in the lifecycle would require us to inspect the shape of the Bearer
token that we receive, to see if it looks like a JWT or not. That feels more fragile to me, but it eliminates the round-trip to ES. The fragility might be tolerable since this is scoped only to serverless, where we have full control over the types of values we accept in the Authorization
header; In serverless, it'll only ever be one of [API Key, ES Token, JWT], whereas on-prem may be configured with a custom realm that we aren't equipped to detect.
I don't have a definitive opinion at this point, but I wanted to start a discussion to see how you/the team felt.
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 will allow a user to attempt JWT auth for disallowed routes, incurring unnecessary overhead (which may or may not matter)
I'm not particularly concerned about this aspect. If someone truly wishes to introduce overhead to the cluster, they can already achieve that by using a custom SAMLResponse
XML document, which could be large and/or expensive to decode. Unfortunately, there's not much we can do to mitigate this.
In fact, I can even see the benefit. This way, we'll catch all (expected and unexpected) authentication attempts in the Elasticsearch audit logs, together with all other authentication related audit logs. It might help us to spot this suspicious activity early on. What do you think?
It also gives us different behavior in the failure scenario where the user presents an invalid JWT (catch block below), as we are relying on the successful auth attempt to detect if it should be allowed or not.
I believe we'll only have different logs, the user will get the same 401 Unauthorized
in both cases, or do you mean something else?
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 fact, I can even see the benefit. This way, we'll catch all (expected and unexpected) authentication attempts in the Elasticsearch audit logs, together with all other authentication related audit logs. It might help us to spot this suspicious activity early on. What do you think?
I think you've convinced me. Sounds good!
I believe we'll only have different logs, the user will get the same 401 Unauthorized in both cases, or do you mean something else?
Correct, just different logs
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 think you've convinced me. Sounds good!
Great, thanks!
`Attempted to authenticate with JWT credentials against ${request.url.pathname}${request.url.search}, but it's not allowed. ` + | ||
`Ensure that the route is defined with the "${ROUTE_TAG_ACCEPT_JWT}" tag.` |
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.
question What do you think about logging a small portion of the JWT in this message? Thinking about supportability, it might be nice to know if a specific token is being used, and having a fragment of that token in the log message could help with this.
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.
Yeah, it's a good idea, I can log some port of the signature (n-last characters, since n-first characters, header, will be the same most of the time).
@@ -594,7 +594,7 @@ packages/kbn-search-api-panels @elastic/enterprise-search-frontend | |||
examples/search_examples @elastic/kibana-data-discovery | |||
packages/kbn-search-response-warnings @elastic/kibana-data-discovery | |||
x-pack/plugins/searchprofiler @elastic/platform-deployment-management | |||
x-pack/test/security_api_integration/packages/helpers @elastic/kibana-core | |||
x-pack/test/security_api_integration/packages/helpers @elastic/kibana-security |
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.
note: not sure why the Core team owned this initially...
// Log a portion of the JWT signature to make debugging easier. | ||
const jwtExcerpt = authorizationHeader.credentials.slice(-10); | ||
this.logger.error( | ||
`Attempted to authenticate with JWT credentials (${jwtExcerpt}…) against ${request.url.pathname}${request.url.search}, but it's not allowed. ` + |
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.
note to myself: it should be
`Attempted to authenticate with JWT credentials (${jwtExcerpt}…) against ${request.url.pathname}${request.url.search}, but it's not allowed. ` + | |
`Attempted to authenticate with JWT credentials (…${jwtExcerpt}) against ${request.url.pathname}${request.url.search}, but it's not allowed. ` + |
describe('#security/authentication/http', function () { | ||
it('allows JWT HTTP authentication only for selected routes', async () => { | ||
const jsonWebToken = | ||
'eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJodHRwczovL2tpYmFuYS5lbGFzdGljLmNvL2p3dC8iLCJzdWIiOiJlbGFzdGljLWFnZW50IiwiYXVkIjoiZWxhc3RpY3NlYXJjaCIsIm5hbWUiOiJFbGFzdGljIEFnZW50IiwiaWF0Ijo5NDY2ODQ4MDAsImV4cCI6NDA3MDkwODgwMH0.P7RHKZlLskS5DfVRqoVO4ivoIq9rXl2-GW6hhC9NvTSkwphYivcjpTVcyENZvxTTvJJNqcyx6rF3T-7otTTIHBOZIMhZauc5dob-sqcN_mT2htqm3BpSdlJlz60TBq6diOtlNhV212gQCEJMPZj0MNj7kZRj_GsECrTaU7FU0A3HAzkbdx15vQJMKZiFbbQCVI7-X2J0bZzQKIWfMHD-VgHFwOe6nomT-jbYIXtCBDd6fNj1zTKRl-_uzjVqNK-h8YW1h6tE4xvZmXyHQ1-9yNKZIWC7iEaPkBLaBKQulLU5MvW3AtVDUhzm6--5H1J85JH5QhRrnKYRon7ZW5q1AQ'; |
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.
note: this test token will expire on Jan 01, 2099
(you can check at https://jwt.io/).
'/api/status', | ||
'/api/stats', | ||
'/api/task_manager/_background_task_utilization', | ||
'/internal/task_manager/_background_task_utilization', | ||
'/api/task_manager/metrics', |
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.
question: @elastic/kibana-core @elastic/response-ops are these the only endpoints that need to be accessed in MKI by our o11y services?
Pinging @elastic/kibana-security (Team:Security) |
'xpack.security.authc.realms.file.file1.order=-100', | ||
|
||
'xpack.security.authc.realms.native.native1.order=-99', | ||
|
||
'xpack.security.authc.realms.jwt.jwt1.order=-98', |
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.
note: copied order
from the actual order
we use for these realms in MKI, just in case.
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.
tags on core owned routes LGTM
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.
Serverless test area changes LGTM
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.
ResponseOps changes LGTM
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.
LGTM! Just one JWT question, but my test may be invalid
x-pack/test_serverless/api_integration/test_suites/common/security_authentication_http.ts
Outdated
Show resolved
Hide resolved
💚 Build Succeeded
Metrics [docs]Canvas Sharable Runtime
History
To update your PR or re-run it, just comment with: cc @azasypkin |
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.
LGTM!
…t of routes only. (elastic#163806)" This reverts commit 5aee5da.
…fined set of routes only. (elastic#163806)"" This reverts commit 033fbbd.
Summary
Allow Kibana to restrict the usage of JWT for a predefined set of routes only in Serverless environment by default. This capability is not available in non-Serverless environment.
Any route that needs to be accessed in Serverless environemnt using JWT as a means of authentication should include
security:acceptJWT
tag.How to test
If you'd like to generate your own JWT to test the PR, please follow the steps outlined in #159117 (comment) or just run functional test server and use static JWT from the Serverless test.
This PR also generated a Serverless Docker image that you can use in your Dev/QA MKI cluster.
security:acceptJWT
tagsecurity:acceptJWT
Fixes: #162632