Add "request.id" to file audit logs by tvernum · Pull Request #35536 · elastic/elasticsearch

tvernum · 2018-11-14T11:17:20Z

This generates a synthesized "id" for each incoming request that is
included in the audit logs (file only).
This id can be used to correlate events for the same request (e.g.
authentication success with access granted).

This request.id is specific to the audit logs and is not used for any
other purpose

This generates a synthesized "id" for each incoming request that is included in the audit logs (file only). This id can be used to correlate events for the same request (e.g. authentication success with access granted). This request.id is specific to the audit logs and is not used for any other purpose

Time based uuids are good for lucene ids, but are not great for audit request.id For a request.id we want a generation system where consequative ids arevisually distinct, so that it is easy to spot correlations.

elasticmachine · 2018-11-14T11:17:21Z

Pinging @elastic/es-security

albertzaharovits · 2018-11-19T12:55:04Z

...va/org/elasticsearch/xpack/security/action/interceptor/IndicesAliasesRequestInterceptor.java

                            if (Operations.subsetOf(aliasPermissions, indexPermissions) == false) {
                                // TODO we've already audited a access granted event so this is going to look ugly
-                                auditTrailService.accessDenied(authentication, action, request, userPermissions.names());
+                                auditTrailService.accessDenied(null, authentication, action, request, userPermissions.names());


I prefer us to be explicit here: threadContext.getTransient(AUDIT_REQUEST_ID).
It is a case of tamper request , if audit request Id is missing.

I think, in this case, it's better to be null, because it's already a denial.
I'd rather fail on the denied rather than fail on a tampered request.

I've expressed myself poorly. I agree failing with access_denied is better and we should not change this.

I meant that since these request interceptors are called after AuthorizationService#authorize

elasticsearch/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/filter/SecurityActionFilter.java

Line 174 in e179bd3

for (RequestInterceptor interceptor : requestInterceptors) {

we can be confident that they are assigned an audit request id already. If they don't have it, then it must be a case of tamper request. Does this sound reasonable?

Yes, it's reasonable, but the question is what should we do if there is not request.id?
We don't want to fail because then we'd lose the accessDenied audit, so I think we're better off logging it with no request.id
It certainly implies something is wrong, but I don't think there's any reasonable action to take about it.

albertzaharovits · 2018-11-19T12:55:22Z

.../main/java/org/elasticsearch/xpack/security/action/interceptor/ResizeRequestInterceptor.java

            if (Operations.subsetOf(targetIndexPermissions, sourceIndexPermissions) == false) {
                // TODO we've already audited a access granted event so this is going to look ugly
-                auditTrailService.accessDenied(authentication, action, request, userPermissions.names());
+                auditTrailService.accessDenied(null, authentication, action, request, userPermissions.names());


idem:

I prefer us to be explicit here: threadContext.getTransient(AUDIT_REQUEST_ID).
It is a case of tamper request , if audit request Id is missing.

As above - but let's chat if you disagree.

I've clarified my thoughts in the previous comment. Judging from the code flow, I believe we can be explicit about the id and not pass null.

albertzaharovits · 2018-11-19T12:57:33Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/AuditUtil.java

+
+    public static String getOrGenerateRequestId(ThreadContext threadContext) {
+        String requestId = extractRequestId(threadContext);
+        if (Strings.isEmpty(requestId)) {


I think we can get rid of this.

As in: be explicit when we generate these.
I know it would be a bummer if we hit an assertion about an event not assigned to a request, but I think there's value if we catch such an error, because such an event might reveal hidden bugs.

It's not really feasible to do that unfortunately.
When we process a transport action, we simply don't/can't know whether that action was spawned from an existing rest/transport request or whether it's standalone.

Well, I guess we could refactor a lot of stuff to make that explicit, but it's not right now.
Maybe, between system context, and withOrigin we could pull something together to know when a transport action is isolated from any originating request, but it's going to be fragile.

albertzaharovits · 2018-11-19T13:14:16Z

...security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java

        }

+        LogEntryBuilder withRequestId(String requestId, ThreadContext threadContext) {
+            if (Strings.isNullOrEmpty(requestId)) {


This is again a place where I think we should not generate if missing. Missing is an indication that we don't know how this request came to be which is a problem in itself.

We don't generate if missing - we just pull it from the thread context if it was passed in as null.
We can remove that (by making all callers do the extraction explicitly) if we want.

Ahhhh, yes, it makes sense now. I was mistaken in my belief that they're also generated here. All precautionary recommendations in the other comments assumed they're also generated here.

All good, sorry.

albertzaharovits · 2018-11-19T13:22:43Z

...gin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java

        Authenticator(RestRequest request, ActionListener<Authentication> listener) {
-            this(new AuditableRestRequest(auditTrail, failureHandler, threadContext, request), null, listener);
+            this(new AuditableRestRequest(auditTrail, failureHandler, threadContext, request,
+                AuditUtil.getOrGenerateRequestId(threadContext)), null, listener);


How do you feel if we move AuditUtil.getOrGenerateRequestId(threadContext) inside the AuditableRestRequest and AuditableTransportRequest ? This is oboviously equivalent, but I think the requestId fits better at the Request abstraction level, rather than Authenticator . The consistent abstraction argument, I believe, is more important when dealing with the threadContext since it is important when (on what thread) we pin down the request id. But, again, this is just a abstraction consistency type of argument.

I think you're right.
I think, originally the generate... method didn't exist, so I the generation was pushed up tothe Authenticator, but now that it's on AuditUtil it should be generated in the request.

albertzaharovits · 2018-11-19T13:23:24Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java

+        if (auditId == null) {
+            if (isInternalUser(authentication.getUser()) == false) {
+                logger.warn("Authorizing action without an existing request-id");
+                assert false : "Audit request.id should be generated during authentication";


I think this is a case for tampered request and authz failed ?

This is one of those cases where I'm worried about breaking something important to support a feature that is only relevant to some people.
On balance, I think you're correct here, but I still have some reservations.

Okay, I agree. Let's keep it as is.

albertzaharovits · 2018-11-19T13:26:01Z

...ty/src/main/java/org/elasticsearch/xpack/security/authz/SecuritySearchOperationListener.java

        final boolean sameUser = samePrincipal && sameRealmType;
        if (sameUser == false) {
-            auditTrailService.accessDenied(current, action, request, roleNames);
+            auditTrailService.accessDenied(null, current, action, request, roleNames);


Here should also be a request id present, otherwise tampered request?

As above, it's already a denied, which I think is the better failure than complaining about request ids.

albertzaharovits · 2018-11-19T13:32:43Z

I have left a bunch of comments, all about being more strict about when we generate the request id and not simply generating a new one for a request without it. I like though that you've chosen to pass the request_id as an argument instead of using the threadContext which feels like global variables type of cheating.

albertzaharovits · 2018-11-21T17:26:01Z

I've been mistaken about where you generated the ids, I though you generated them on each event if the parameter is null and if they're not in the context. Instead they are not generated at all, they are pulled from the context.

albertzaharovits

LGTM

tvernum · 2018-11-22T01:27:48Z

I actually changed a bunch of things based on your comments, but I had some unrelated test failures so it didn't get pushed.

- Explicitly generate request.id when we know we should - Explicitly extract request.id instead of doing this in the audit trail - Reject authz of external requests without a request.id

albertzaharovits · 2018-11-22T06:10:30Z

.../main/java/org/elasticsearch/xpack/security/action/interceptor/ResizeRequestInterceptor.java

            if (Operations.subsetOf(targetIndexPermissions, sourceIndexPermissions) == false) {
                // TODO we've already audited a access granted event so this is going to look ugly
-                auditTrailService.accessDenied(null, authentication, action, request, userPermissions.names());
+                auditTrailService.accessDenied(extractRequestId(threadContext), authentication, action, request, userPermissions.names());


albertzaharovits

LGTM Thank you!

albertzaharovits · 2018-11-22T06:10:54Z

...security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java

                    .with(REALM_FIELD_NAME, realm)
                    .withRestUri(request)
-                    .withRequestId(requestId, threadContext)
+                    .withRequestId(requestId)


tvernum · 2018-11-23T05:50:12Z

I plan to merge this as as (pending any additional review comments) but I also want to investigate passing the request.id between nodes so that it's possible to correlate when 1 request causes actions to run on different (search being the obvious example).

I don't expect any issues, but I'd like to keep it to its on PR.

tvernum · 2018-11-23T07:20:44Z

Scrap the comment above. It was a 2 line change to make it a header rather than transient, so I've added it into this PR.

tvernum · 2018-11-26T02:21:29Z

@elasticmachine test this please

tvernum · 2018-11-26T05:50:54Z

@elasticmachine: run gradle build tests 1

albertzaharovits

👍

This generates a synthesized "id" for each incoming request that is included in the audit logs (file only). This id can be used to correlate events for the same request (e.g. authentication success with access granted). This request.id is specific to the audit logs and is not used for any other purpose The request.id is consistent across nodes if a single request requires execution on multiple nodes (e.g. search acros multiple shards).

This generates a synthesized "id" for each incoming request that is included in the audit logs (file only). This id can be used to correlate events for the same request (e.g. authentication success with access granted). This request.id is specific to the audit logs and is not used for any other purpose The request.id is consistent across nodes if a single request requires execution on multiple nodes (e.g. search across multiple shards).

tvernum added 3 commits November 14, 2018 15:35

Explicitly require request.id for Authorization

c0c9639

Switch to random UUIDs for request.id

c79201a

Time based uuids are good for lucene ids, but are not great for audit request.id For a request.id we want a generation system where consequative ids arevisually distinct, so that it is easy to spot correlations.

tvernum added >enhancement v7.0.0 :Security/Audit X-Pack Audit logging v6.6.0 labels Nov 14, 2018

tvernum requested review from albertzaharovits and jkakavas November 14, 2018 11:17

jaymode self-requested a review November 16, 2018 16:06

albertzaharovits reviewed Nov 19, 2018

View reviewed changes

Merge branch 'master' into audit-request-id

cba24fb

albertzaharovits approved these changes Nov 21, 2018

View reviewed changes

Address feedback

bf400a9

- Explicitly generate request.id when we know we should - Explicitly extract request.id instead of doing this in the audit trail - Reject authz of external requests without a request.id

albertzaharovits reviewed Nov 22, 2018

View reviewed changes

albertzaharovits approved these changes Nov 22, 2018

View reviewed changes

Switch audit request.id to be a header, not transient

d21d79c

Merge branch 'master' into audit-request-id

5879bcc

albertzaharovits approved these changes Nov 26, 2018

View reviewed changes

jaymode approved these changes Nov 26, 2018

View reviewed changes

tvernum merged commit 5b427d4 into elastic:master Nov 27, 2018

tvernum added the backport pending label Nov 27, 2018

tvernum removed the backport pending label Nov 29, 2018

jaymode mentioned this pull request Dec 11, 2018

Propogate request id to internal actions or add as cause #36492

Open

albertzaharovits mentioned this pull request Jan 28, 2019

Audit request.id propagate to CRUD actions #37918

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Conversation

tvernum commented Nov 14, 2018

Uh oh!

elasticmachine commented Nov 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertzaharovits Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertzaharovits commented Nov 19, 2018

Uh oh!

albertzaharovits commented Nov 21, 2018

Uh oh!

albertzaharovits left a comment

Choose a reason for hiding this comment

Uh oh!

tvernum commented Nov 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertzaharovits left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvernum commented Nov 23, 2018

Uh oh!

tvernum commented Nov 23, 2018

Uh oh!

tvernum commented Nov 26, 2018

Uh oh!

tvernum commented Nov 26, 2018

Uh oh!

albertzaharovits left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

albertzaharovits Nov 19, 2018 •

edited

Loading