Issue 6115 - Add client info to audit log

[mreynolds389]

There is a need to add being tracking info in the audit log. Right now it is not easy to match up audit events to the client in the access log.

This PR adds as comments to the audit event the client IP address, the connection id, and operation id.

fixes: #6115

[jchapma]

Works fine for me:

time: 20240306140041                       
dn: cn=config                              
##ip=10.0.0.1                              
##conn=2                                   
##op=2                                     
result: 0                                  
changetype: modify                         
replace: passwordMustChange                
passwordMustChange: on                     
-                                                                                     
replace: modifiersname                     
modifiersname: cn=directory manager        
-                                                                                     
replace: modifytimestamp                   
modifytimestamp: 20240306140041Z

[droideck]

Before the merge, I think we need to discuss the nature of the RFE for a bit, as it was not triaged at all, and it looks important to me.

My main concerns:

• Will it violate any security issue for users? Probably not, but it's something to consider as it can be an unexpected change for some setups.
• Will it break someone's tools for auditlog parsing? And our tools, of course...
• Do we need more info besides IP, conn and op?

[droideck]

And, of course, which releases to put it into.

[mreynolds389]

Before the merge, I think we need to discuss the nature of the RFE for a bit, as it was not triaged at all, and it looks important to me.

My main concerns:

* Will it violate any security issue for users? Probably not, but it's something to consider as it can be an unexpected change for some setups.

It's only adding information that is already available in the access log so I see no security risk. Of course it needs to be in the release notes/docs. Maybe an upstream announcement?

* Will it break someone's tools for auditlog parsing? And our tools, of course...

Well.... It's adding comments to an LDIF file which is legitimate and does not break ldapmodify. Of course it's always possible someone is parsing the LDIF in a way that might not expect comments in the ldif file.

There is a strong emerging need for better auditing in IDM, and it's only going to get stronger. IMHO the pros far outweigh the cons.

* Do we need more info besides IP, conn and op?

The purpose is to match up an audit log event with the access log. You could argue we don't need the IP address (I just threw it in there because it was easy to grab), but we definitely need the conn_id and op_id.

And, of course, which releases to put it into.

I believe the customer will be wanting this in RHEL 8, but product management is still talking to them about this. I'm just trying to get a head of it so we aren't scrambling to get it all done.

[mreynolds389]

Gentle reminder for a review please...

[progier389]

FYI: My concern is not really whether we should accept to merge this one in the main branch
but if we should cherry pick it on earlier version.

IMHO there is no good reason not to merge this one in the main branch:
As Mark mentioned: there is no security issue
Even if some custom tool is broken, as we just switched the version to 3.0 it is the right time for such change.
Do we need more data: maybe but we could always add them when we realize we need it. so it is not a blocker.

But I have concern to backport it in previous branches:
IMHO the risk of breaking something in existing production (even if it is small) means that we need a way
to disable that feature (i.e yet another config parameter. Sigh! )
Will we have the bandwidth to release these versions and When ? That is something that the team has to discuss.
@tbordaz, Have you any though about it ?

[mreynolds389]

Well improving the audit log tracking needs to be backported to RHEL 8/9, but it doesn't necessarily have to be this PR. Perhaps having the JSON log could be backported? Have two audit logs in RHEL 9 (not desirable but safer), and in 3.x just have the JSON log? Or have a audit log level to set it to LDIF or JSON?

Still waiting for customer requirements, but I know it's going to have to go into 8 or 9.

[tbordaz]

HI @mreynolds389 , first of all sorry for this late answer. I really like this idea and the patch.

From security pov I think we are safe as we are logging information that are already available in another file (access) in the same directory. Only root/admin can access those files and IMHO it is safe.

I tend to agree with @droideck that possibly bind DN could be also logged but it can be added later.

About the format, I agree it needs to start with a '#' . would it be helpful to write in a yaml format ?

Regarding compatibility I am a bit concern that by default we are changing audit content, even for CU who do not need it. I would like it to be configurable (like @progier389 a deep sight with a new config param: nsslapd-auditlog-extend-log?) and be disabled by default. It would facilitate the backport in previous versions.

[droideck]

As for the security part, I have a bit of worry about it: If the company, for some reason, has a security policy for audit logs that is less strict than for access logs (maybe sharing it with some additional list of employees for some reason?).
After the change, it will affect them because now the employees will see a bit more than they should.

I think the possibility of issues like these is pretty minor. "Disabled by default" and a careful warning from the release notes will negate them.

[mreynolds389]

HI @mreynolds389 , first of all sorry for this late answer. I really like this idea and the patch.

From security pov I think we are safe as we are logging information that are already available in another file (access) in the same directory. Only root/admin can access those files and IMHO it is safe.

I tend to agree with @droideck that possibly bind DN could be also logged but it can be added later.

Technically the bind dn is already there: creatorsName/modifiersName

About the format, I agree it needs to start with a '#' .

The entry attribute display audit log feature already uses '#', so we need to differentiate the values, that's why I chose '##'. Not exactly ideal, but since it's still a comment it should be "safer"

would it be helpful to write in a yaml format ?

Regarding compatibility I am a bit concern that by default we are changing audit content, even for CU who do not need it. I would like it to be configurable (like @progier389 a deep sight with a new config param: nsslapd-auditlog-extend-log?) and be disabled by default. It would facilitate the backport in previous versions.

I think we should add an audit log level (default is 1):

1 - default
2 - add client info
4 - JSON format
8 - ...

[Firstyear]

You need all the bitmask combinations to work together then though as a point :)

[mreynolds389]

You need all the bitmask combinations to work together then though as a point :)

Of course ;-)

[mreynolds389]

After having a team discussion we are not going to modify the existing audit log. Instead we will just add a JSON format option ("off" by default).

Closing PR...