Can you rename the interface to GetSolarisAuditSessionData or something similar?  Audit is pretty much an overloaded word these days and unless Linux Audit and Solaris Audit return the same data it doesn't make sense to have a generic API.   

Created attachment 16260 [details] [review]
patch to add "GetAdtAuditSessionData" method

Renamed interface name and removed unsuitable compiling-time macro "HAVE_ADT" in public header file "dbus/dbus-connection.h".

(In reply to comment #1)
> Can you rename the interface to GetSolarisAuditSessionData or something
> similar?  Audit is pretty much an overloaded word these days and unless Linux
> Audit and Solaris Audit return the same data it doesn't make sense to have a
> generic API.   
> 

Sure. Renamed to "GetAdtAuditSessionData". Because the Solaris API may be
adopted by other platforms in the future, "Adt" would be more generic than
"Solaris".

I would #ifdef this less; notice how the windows SID is done, where it has most of the API even on Unix, it's just that the windows SID is never set in the credentials struct.

Inside the bus daemon (driver.c), you could always export the method, but just have it return an error if the audit data isn't available. Which should happen naturally if dbus_connection_get_adt_audit_session_data() fails.

In general only the sysdeps-unix.c part looks like it truly, definitely needs the #ifdef (and the adt header should not be included anywhere else, for example including bsm/adt.h in driver.c is not correct; direct use of the system API should all be in the sysdeps-* files.

Created attachment 16525 [details] [review]
enhanced patch to add "GetAdtAuditSessionData"

Havoc, thank you for review.

Removed "#ifdef" and "#include <bsm/adt.h>" and let "GetAdtAuditSessionData" return error as long as the audit data isn't available. Could you have a look again?

Looks reasonable.

Created attachment 16838 [details] [review]
rename more to use _adt, static for internal func, fix one or two spaces

Simon, can you have a look at this updated patch which tweaks yours:

o Rename more bits to use _ADT
o Fix a function to be static that was only used internally
o Add spaces between function name and call in one or two places

(In reply to comment #7)
> Created an attachment (id=16838) [details]
> rename more to use _adt, static for internal func, fix one or two spaces
> 
> Simon, can you have a look at this updated patch which tweaks yours:
> 
> o Rename more bits to use _ADT
> o Fix a function to be static that was only used internally
> o Add spaces between function name and call in one or two places
> 

Colin,

Really appreciate your help. 

Only one minor issue. Because _dbus_credentials_add_adt_audit_data() is also called in file dbus/dbus-sysdeps-unix.c, it would be good to mask it as export function rather than static.

Could you help to push upstream if patch is fine?

I removed the errant "static" and have committed this upstream.  If you have a chance to update to git HEAD and test and report success, it would be appreciated!

commit ab1eb1fd5a26affa2383b0eb7e292efd83ec2546
Author: Colin Walters <walters@verbum.org>
Date:   Thu Jun 5 17:24:34 2008 -0400

    Bug 15740: Solaris/ADT auditing support (simon zheng)
    
    	* bus/driver.c: Add GetAdtAuditSessionData method
    	which returns audit data for a connection.
    	* configure.in: Detect ADT auditing support
    	* dbus/dbus-auth.c: Read ADT auditing creds.
    	* dbus/dbus-connection.c: Implement
    	dbus_connection_get_adt_audit_session_data.
    	* dbus/dbus-connection.h: Export it.
    	* dbus/dbus-credentials.c: Add support for
    	gathering adt_audit_data and retrieving it
    	via _dbus_credentials_get_adt_audit_data.
    	* dbus/dbus-credentials.h: Add
    	DBUS_CREDENTIAL_ADT_AUDIT_DATA_ID.
    	* dbus/dbus-protocol.h: New error
    	DBUS_ERROR_ADT_AUDIT_DATA_UNKNOWN.
    	* dbus/dbus-sysdeps.c: Support for reading
    	audit credentials via ADT API.
    	* dbus/dbus-transport.c: New function
    	_dbus_transport_get_adt_audit_session_data
    	to retrieve credentials.
    	* dbus/dbus-transport.h: Export it.