dima767 / inspektr Goto Github PK

The scope of the try/catch blocks in AuditTrailManagementAspect#handleAuditTrail methods appears too large. The premise of this observation is based on my understanding of the following block:

    try {
        retVal = joinPoint.proceed();

        currentPrincipal = this.auditPrincipalResolver.resolveFrom(joinPoint, retVal);
        auditResource = auditResourceResolver.resolveFrom(joinPoint, retVal);
        action = auditActionResolver.resolveFrom(joinPoint, retVal, audit);

        return retVal;
    } catch (final Exception e) {
        currentPrincipal = this.auditPrincipalResolver.resolveFrom(joinPoint, e);
        auditResource = auditResourceResolver.resolveFrom(joinPoint, e);
        action = auditActionResolver.resolveFrom(joinPoint, e, audit);
        throw e;
    } finally {
        executeAuditCode(currentPrincipal, auditResource, joinPoint, retVal, action, audit);
    }

I understand the catch block's audit data resolution to be for an exception that is presumably thrown as a result of invoking the method at the join point. This is a faulty assumption for non-trivial resolvers. In our particular case principal resolution involves an LDAP query to normalize the principal, and this is very liable to breakage. As a result the principal resolution failure masks the audit entry for a successful authentication event. Since the same resolver fires for both the success and failure cases, it can throw in a way that leaves the audit resource null and the AuditActionContext complains. The end result is that an audit failure is masking an important application failure.

I would recommend the following:

• Narrow the scope of the try catch block to just the join point invocation.
• Insulate resolver invocations such that failure does not cause auditing to fail. Perhaps an internal logging facility would log these failures.
• Provide defaults such that AuditActionContext never receives values that fail assertion, or at least provide a configuration knob such that assertion failures do not cause audit recording to fail.

Saw this in providing some help with CAS today:

AuditActionContext constructor IllegalArgumentExceptions-out when arguments, e.g. resourceOperatedUpon, are null

assertNotNull(resourceOperatedUpon, "resourceOperatedUpon cannot be null");

The resulting IllegalArgumentException would be more helpful if it included the rest of the arguments to the constructor, to make it easier for the adopter encountering this exception to figure out what context gave rise to it.

Not sure whether and how to accomplish this. Catch the IllegalArgumentException and wrap in an IllegalArgumentException with the fuller context?

public AuditActionContext(final String principal, final String resourceOperatedUpon, final String actionPerformed, final String applicationCode, final Date whenActionWasPerformed, final String clientIpAddress, final String serverIpAddress) {

  try {
    assertNotNull(principal, "principal cannot be null");
    assertNotNull(resourceOperatedUpon, "resourceOperatedUpon cannot be null");
    assertNotNull(actionPerformed, "actionPerformed cannot be null.");
    assertNotNull(applicationCode, "applicationCode cannot be null.");
    assertNotNull(whenActionWasPerformed, "whenActionPerformed cannot be null.");
    assertNotNull(clientIpAddress, "clientIpAddress cannot be null.");
    assertNotNull(serverIpAddress, "serverIpAddress cannot be null.");
  } catch (IllegalArgumentException illegalArgException) {

    throw new IllegalArgumentException("One or more arguments to AuditActionContext constructor were invalid: " +
     "principal: " + principal +
     "resourceOperatedUpon: " + resourceOperatedUpon +
     "actionPerformed: " + actionPerformed +
     "applicationCode: " + applicationCode +
     "whenActionWasPerformed: " + whenActionWasPerformed + 
     "clientIpAddress: " + clientIpAddress + 
     "serverIpAddress: " + serverIpAddress,
     illegalArgException );

  }
  ...
}

Enhance the message in each assertNotNull ?

Something else?

Allow for column lengths longer than 100

I've been experiencing checksum failures for the inspektr-audit JAR file on 'repo1'. The relevant log file entries are below. Did someone make a mistake when they pushed the artifacts?

2010-09-09 13:58:10,765 [qtp-jetty7-1984526644-20073] [INFO ](o.a.r.HttpRepo :176) - repo1: Downloading 'http://repo1.maven.org/maven2/com/github/inspektr/inspektr-audit/1.0.0.GA/inspektr-audit-1.0.0.GA.jar'...
2010-09-09 13:58:10,780 [qtp-jetty7-1984526644-20073] [INFO ](o.a.r.HttpRepo :176) - repo1: Downloading 'http://repo1.maven.org/maven2/com/github/inspektr/inspektr-audit/1.0.0.GA/inspektr-audit-1.0.0.GA.jar.sha1'...
2010-09-09 13:58:10,781 [qtp-jetty7-1984526644-20073] [INFO ](o.a.r.HttpRepo :193) - repo1: Downloaded 'http://repo1.maven.org/maven2/com/github/inspektr/inspektr-audit/1.0.0.GA/inspektr-audit-1.0.0.GA.jar.sha1' with return code: 200.
2010-09-09 13:58:10,797 [qtp-jetty7-1984526644-20073] [INFO ](o.a.r.HttpRepo :176) - repo1: Downloading 'http://repo1.maven.org/maven2/com/github/inspektr/inspektr-audit/1.0.0.GA/inspektr-audit-1.0.0.GA.jar.md5'...
2010-09-09 13:58:10,797 [qtp-jetty7-1984526644-20073] [INFO ](o.a.r.HttpRepo :193) - repo1: Downloaded 'http://repo1.maven.org/maven2/com/github/inspektr/inspektr-audit/1.0.0.GA/inspektr-audit-1.0.0.GA.jar.md5' with return code: 200.
2010-09-09 13:58:10,801 [qtp-jetty7-1984526644-20073] [INFO ](o.a.r.HttpRepo :193) - repo1: Downloaded 'http://repo1.maven.org/maven2/com/github/inspektr/inspektr-audit/1.0.0.GA/inspektr-audit-1.0.0.GA.jar' with return code: 200.
2010-09-09 13:58:10,802 [qtp-jetty7-1984526644-20073] WARN - Sending HTTP error code 409: Checksum policy 'GEN_IF_ABSENT' rejected the artifact 'inspektr-audit-1.0.0.GA.jar'. Checksums info: [ChecksumInfo{type=MD5, original='1a50947189bc4317de2a7cae65fdb8c3', actual='929912996bbb3f42878cd178a071108d'}, ChecksumInfo{type=SHA-1, original='08a5f462a15036b59c4fbbfd595697f2f72f79ce', actual='14f9e03583c7b5c1350c3bffb0f7d562e3730297'}].

In our environment we only want to keep audit and statistics data around for 30 days. It would be very nice if Inspektr provided at least the infrastructure for creating these if not some default implementations for the storage backends that are supported. I would imagine this is much more important for the JDBC storage layer than file system.

Currently, Inspektr asserts that its arguments not be null. When these assertions fail, the action being audited fails.

Instead, failure in the audit trail system should not induce failure in the system being audited. Failure should be logged and control return from the Aspect to the functional code wrapped.

Cf. Issue #13 , which helped but didn't go far enough.

Cf. https://groups.google.com/d/topic/cas-user/e2FRrJj0BFU/discussion

I was wondering when you plan on rolling another release of inspektr? I'd like to use the single line logger and without the jar in the maven repo I'm finding this incredibly difficult.

I was trying to release the 1.0.6.GA using 'mvn release:prepare' and 'mvn release:perform' and during perform goal I get the following error:

Failed to execute goal org.apache.maven.plugins:maven-release-plugin:2.1:perform (default-cli) on project inspektr: Maven execution failed, exit code: '1' -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-release-plugin:2.1:perform (default-cli) on project inspektr: Maven execution failed, exit code: '1'

....

Caused by: org.apache.maven.shared.release.exec.MavenExecutorException: Maven execution failed, exit code: '1'

...

Any insight, pointers, help will be appreciated.

Hi,

I use CAS SSO server which uses Inspektr internally for auditing purposes.
After I configured store into DB using JdbcAuditTrailManager server is not able to finish correctly.
https://github.com/dima767/inspektr/blob/master/inspektr-support-spring/src/main/java/com/github/inspektr/audit/support/JdbcAuditTrailManager.java

Reason is default java.util.concurrent.ExecutorService instantiated in this class, which is not finished correctly in any way. This service holds internal nondeamon thread which prevents JVM from shutdown (and also prevents correct web application redeploy in many application servers).

One solution is to implement own java.util.concurrent.ThreadFactory which produces deamon threads and use it for ExecutorService creation.
Second solution is to call ExecutorService.shutdown correctly, but hard to say how to hook it on some reasonable shutdown event from JVM/JEE AS.

Thanks for patching this problem which makes Inspektr hard to use in production env.

We utilize a BigIP F5 load balancer in front of many of our applciations. This has the effect that the originating IP in the packets are re-written to originate from the F5. We have the ability to inject the x-forwarded-for header to include the original IP of the connection.

Currently when running JASIG CAS, this means that the F5 is logged into the COM_AUDIT_TRAIL table as the AUD_CLIENT_IP.

Would it be possible to create a configuration option so that the x-forwarded-for header is used if it is present?

Thanks,
Jeff

It appears that an 'audit begin' block can be written inside of another block in the log file. This creates a problem when trying to parse the log files. Perhaps a one-line entry in the log file instead of an entire block of lines would be easier, like Apache, et al.

Actual sample:

2010-04-29 09:01:00,462 - Audit trail record BEGIN
2010-04-29 09:01:00,462 - =============================================================
2010-04-29 09:01:00,462 - WHO: [username: xxxxxxx3]
2010-04-29 09:01:00,462 - WHAT: supplied credentials: [username: xxxxxxx3]
2010-04-29 09:01:00,462 - ACTION: AUTHENTICATION_SUCCESS
2010-04-29 09:01:00,462 - APPLICATION: CAS
2010-04-29 09:01:00,465 - WHEN: Thu Apr 29 09:01:00 PDT 2010
2010-04-29 09:01:00,472 - Audit trail record BEGIN
2010-04-29 09:01:00,473 - =============================================================
2010-04-29 09:01:00,473 - WHO: xxxxxxx7
2010-04-29 09:01:00,473 - WHAT: ST-3161-oaSDnrBC4b15M1XHQfWh-cas for https://www.google.com/a/mail.csuchico.edu/acs
2010-04-29 09:01:00,473 - ACTION: SERVICE_TICKET_CREATED
2010-04-29 09:01:00,473 - APPLICATION: CAS
2010-04-29 09:01:00,473 - WHEN: Thu Apr 29 09:01:00 PDT 2010
2010-04-29 09:01:00,473 - CLIENT IP ADDRESS: 132.241.237.248
2010-04-29 09:01:00,473 - SERVER IP ADDRESS: 132.241.251.22
2010-04-29 09:01:00,474 - =============================================================
2010-04-29 09:01:00,474 -

2010-04-29 09:01:00,488 - CLIENT IP ADDRESS: 174.26.197.203
2010-04-29 09:01:00,489 - SERVER IP ADDRESS: 132.241.251.22
2010-04-29 09:01:00,489 - =============================================================
2010-04-29 09:01:00,489 -

AUD_RESOURCE VARCHAR2(100) NOT NULL,

Please increase the limit to 255 or even higher, 100 is too low for a long URL.

When an audited method throws an exception with a null message, this method ReturnValueAsStringResourceResolver.resolveFrom(JoinPoint, Exception) returns an array containing null.

this array is then passed into AuditTrailManagementAspect.executeAuditCode() which attempts to generate an AuditActionContext object using the null vale, which throws the IllegalArgumentException.

The Auditable annotation currently uses class names to identify a resource and action resolution strategy:

Class <? extends org.inspektr.audit.spi.AuditableActionResolver> actionResolverClass() default DefaultAuditableActionResolver.class;

Class <? extends AuditableResourceResolver> resourceResolverClass();

This is problematic when you want to change resource or action resolution strategies in a project that uses inspektr logging (e.g. CAS) since it requires editing the annotations themselves.

The Spring Security @secured annotation provides a good alternative model for configuring action and resource resolution strategies. The value of @secured is simply a string ("config attribute") that can have arbitrary meaning that is assigned via external configuration. While this may complicate the simple case where the default class is sufficient, supporting the string-based configuration scheme would greatly facilitate more complex configuration cases where extension is required.

Allow a different ExecutorService to be injected into the JdbcAuditTrailManager