hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alejandro Abdelnur (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HADOOP-10756) KMS audit log should consolidate successful similar requests
Date Thu, 24 Jul 2014 16:50:39 GMT

    [ https://issues.apache.org/jira/browse/HADOOP-10756?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14073375#comment-14073375
] 

Alejandro Abdelnur commented on HADOOP-10756:
---------------------------------------------

Nice, I like it much better using Guava Cache.

*kms-log4j.properties*:
* name of appender should be 'aggregated-audit', 'delayed-audit' gives the wrong impression.

*KMSAudit.java*:
* the {{op()}} method should receive as param the AuditOpStatus and that should be set in
the {{MDC}} within the {{op()}} method instead in the 2 caller methods. Then all is in one
place, it is more symmetric.

*KMSAuditAppender.java*:
* the {{AtomicInteger}} can be created in the constructor of {{DelayedEvent}}
* the {{executor}} should be created in the {{initialize()}}, at the very end (if something
fails after construction or early init, you don’t leave a dangling thread running.
* for testings, you should have a method to stop the executor to get rid of the executor’s
thread.
* default delay should be the same that in the kms-log4j.properties, 10000 (currently is 5000)
* {{append()}} method, I don’t think we need to check {{mdcStatus}} is an AuditOptStatus
instance, we can safely assume it is.
* {{append()}} you can directly use {{status == AuditOpStatus.UNAUTHORIZED}}, the beauty of
enum.
* {{append()}}, why we don’t leverage the Cache entry creation logic on get here, then we
can avoid the race condition of creating/putting 2 events. having the {{accessCount}} set
to -1 on creation would be the indicator that is new. you would do an incrementAndGet, if
you get zero it means it is first occurrence (more on this a bit later)
* how about having a {{dispatchAppend()}} method doing all the event’s property setting
and calling {{super.append()}}, and using it from {{handleUnAuthorizedAccess()}} & {{append()}}

*TestKMSAuditAppender.java*:
* we should, somehow, shutdown the appender executor (see comment above)
* the current test should be used to simply verify the append is properly wired. you could
use Mockito to easier test all code paths in the appender.

A couple of things we missed:

1. On unauthorized access and on cache expiration we are removing the Delayed Event from the
cache. This will make the next authorized access log to flush with accessCount 1 immediately.
I think we should instead:

* On expiry/unauthorized, if accessCount is greater than 0, flush to log, set accessCount
to 0, set entry in cache again.
* On expiry/unauthorized, if acccessCount is zero, remove from cache.

please check if this makes sense.

2. on flushing we should make sure we set the event time to the flushing time. We could also
log the time interval of the aggregated count (currentTime - (timeOfFirstAccess or timeOfLastFlush)).


> KMS audit log should consolidate successful similar requests
> ------------------------------------------------------------
>
>                 Key: HADOOP-10756
>                 URL: https://issues.apache.org/jira/browse/HADOOP-10756
>             Project: Hadoop Common
>          Issue Type: Bug
>          Components: security
>    Affects Versions: 3.0.0
>            Reporter: Alejandro Abdelnur
>            Assignee: Arun Suresh
>         Attachments: HADOOP-10756.1.patch, HADOOP-10756.2.patch, HADOOP-10756.3.patch,
HADOOP-10756.4.patch, HADOOP-10756.5.patch
>
>
> Every rejected access should be audited, but successful accesses should be consolidated
within a given amount of time if the request is from the same user for he same key. 



--
This message was sent by Atlassian JIRA
(v6.2#6252)

Mime
View raw message