[WIP] Log database changes.

[Gary Lockyer via samba-technical]

I'm currently adding logging of Database changes, the attached patch set
is the work to date.

It still needs the the following done:
* separating out the common code with the auth logging
* cleaning up the code
* plumbing the remote IP through
- it's not currently available in all cases
* Add a session ID GUID and log that in the Authorization messages as
well
* Log/Audit group membership changes.
* Write unit and integration Tests.

Notes:
* Currently the attribute truncation limit is set artificially low to
aid development

Sample Human readable messages.
Values enclosed in {} are base64 encoded
Values ending with ... have been truncated.

Line breaks added for clarity

Samdb Change [Add] at [Thu, 05 Apr 2018 14:56:28.086708 NZST]
transaction id [d9184e5e-aeca-4793-b7a7-a8a558378421]
status [Success] remote host [Unknown]
SID [S-1-5-21-202143440-2159076023-2784540227-500]
DN [CN=krbtgt,CN=Users,DC=addom,DC=samba,DC=example,DC=com]
attributes [
accountExpires [9223372036854775807]
adminCount [1]
clearTextPassword [REDACTED SECRET ATTRIBUTE]
description [Key Distribution Cen...]
isCriticalSystemObject [TRUE]
objectClass [top] [person] [organizationalPerson] [user]
objectSid {AQUAAAAAAAUVAAAA0HYMDLfisIA=...}
sAMAccountName [krbtgt]
servicePrincipalName [kadmin/changepw]
showInAdvancedViewOnly [TRUE] userAccountControl [514]]

Password Change [Reset] at [Thu, 05 Apr 2018 14:56:28.086738 NZST]
transaction id [d9184e5e-aeca-4793-b7a7-a8a558378421]
status [Success] remote host [Unknown]
SID [S-1-5-21-202143440-2159076023-2784540227-500]
DN [CN=krbtgt,CN=Users,DC=addom,DC=samba,DC=example,DC=com]


Sample JSON messages:
JSON samdbChange: {
"timestamp": "2018-04-05T14:56:28.086749+1200",
"type": "samdbChange",
"samdbChange": {
"status": "Success",
"version": {"major": 1, "minor": 0},
"remoteAddress": "NULL",
"operation": "Add",
"userSid": "S-1-5-21-202143440-2159076023-2784540227-500",
"dn": "CN=krbtgt,CN=Users,DC=addom,DC=samba,DC=example,DC=com",
"transactionId": "d9184e5e-aeca-4793-b7a7-a8a558378421",
"attributes": {
"adminCount": {"values": [{"value": "1"}]},
"objectSid": {"values": [
{"truncated": true, "base64": true,
"value": "AQUAAAAAAAUVAAAA0HYMDLfisIA="}]},
"accountExpires": {"values": [{"value": "9223372036854775807"}]},
"objectClass": {"values": [
{"value": "top"},
{"value": "person"},
{"value": "organizationalPerson"},
{"value": "user"}
]},
"clearTextPassword": {"redacted": true},
"description": {"values": [
{"truncated": true, "value": "Key Distribution Cen"}
]},
"isCriticalSystemObject": {"values": [{"value": "TRUE"}]},
"sAMAccountName": {"values": [{"value": "krbtgt"}]},
"servicePrincipalName": {"values": [{"value": "kadmin/changepw"}]},
"showInAdvancedViewOnly": {"values": [{"value": "TRUE"}]},
"userAccountControl": {"values": [{"value": "514"}]}}}}

JSON passwordChange: {
"timestamp": "2018-04-05T14:56:28.086947+1200",
"type": "passwordChange",
"passwordChange": {
"status": "Success",
"version": {"major": 1, "minor": 0},
"remoteAddress": "NULL",
"userSid": "S-1-5-21-202143440-2159076023-2784540227-500",
"action": "Reset",
"dn": "CN=krbtgt,CN=Users,DC=addom,DC=samba,DC=example,DC=com",
"transactionId": "d9184e5e-aeca-4793-b7a7-a8a558378421"}}

Comments appreciated
Gary

[Volker Lendecke via samba-technical]

Post by Gary Lockyer via samba-technical
I'm currently adding logging of Database changes, the attached patch set
is the work to date.

What's the reason for the first patch? Complex macro magic should be
avoided IMHO.

Volker

[Gary Lockyer via samba-technical]

Post by Volker Lendecke via samba-technical

Post by Gary Lockyer via samba-technical
I'm currently adding logging of Database changes, the attached patch set
is the work to date.

What's the reason for the first patch? Complex macro magic should be
avoided IMHO.

I was just following the pattern used for DSDB_SECRET_ATTRIBUTES.
I'll replace this with a #define without the macro magic.

Post by Volker Lendecke via samba-technical
Volker

Gary

[Gary Lockyer via samba-technical]

Current state of this task.

Comments appreciated.

Thanks
Gary

Post by Gary Lockyer via samba-technical
I'm currently adding logging of Database changes, the attached patch set
is the work to date.
* separating out the common code with the auth logging
* cleaning up the code
* plumbing the remote IP through
- it's not currently available in all cases
* Add a session ID GUID and log that in the Authorization messages as
well
* Log/Audit group membership changes.
* Write unit and integration Tests.
* Currently the attribute truncation limit is set artificially low to
aid development
Sample Human readable messages.
Values enclosed in {} are base64 encoded
Values ending with ... have been truncated.
Line breaks added for clarity
Samdb Change [Add] at [Thu, 05 Apr 2018 14:56:28.086708 NZST]
transaction id [d9184e5e-aeca-4793-b7a7-a8a558378421]
status [Success] remote host [Unknown]
SID [S-1-5-21-202143440-2159076023-2784540227-500]
DN [CN=krbtgt,CN=Users,DC=addom,DC=samba,DC=example,DC=com]
attributes [
accountExpires [9223372036854775807]
adminCount [1]
clearTextPassword [REDACTED SECRET ATTRIBUTE]
description [Key Distribution Cen...]
isCriticalSystemObject [TRUE]
objectClass [top] [person] [organizationalPerson] [user]
objectSid {AQUAAAAAAAUVAAAA0HYMDLfisIA=...}
sAMAccountName [krbtgt]
servicePrincipalName [kadmin/changepw]
showInAdvancedViewOnly [TRUE] userAccountControl [514]]
Password Change [Reset] at [Thu, 05 Apr 2018 14:56:28.086738 NZST]
transaction id [d9184e5e-aeca-4793-b7a7-a8a558378421]
status [Success] remote host [Unknown]
SID [S-1-5-21-202143440-2159076023-2784540227-500]
DN [CN=krbtgt,CN=Users,DC=addom,DC=samba,DC=example,DC=com]
JSON samdbChange: {
"timestamp": "2018-04-05T14:56:28.086749+1200",
"type": "samdbChange",
"samdbChange": {
"status": "Success",
"version": {"major": 1, "minor": 0},
"remoteAddress": "NULL",
"operation": "Add",
"userSid": "S-1-5-21-202143440-2159076023-2784540227-500",
"dn": "CN=krbtgt,CN=Users,DC=addom,DC=samba,DC=example,DC=com",
"transactionId": "d9184e5e-aeca-4793-b7a7-a8a558378421",
"attributes": {
"adminCount": {"values": [{"value": "1"}]},
"objectSid": {"values": [
{"truncated": true, "base64": true,
"value": "AQUAAAAAAAUVAAAA0HYMDLfisIA="}]},
"accountExpires": {"values": [{"value": "9223372036854775807"}]},
"objectClass": {"values": [
{"value": "top"},
{"value": "person"},
{"value": "organizationalPerson"},
{"value": "user"}
]},
"clearTextPassword": {"redacted": true},
"description": {"values": [
{"truncated": true, "value": "Key Distribution Cen"}
]},
"isCriticalSystemObject": {"values": [{"value": "TRUE"}]},
"sAMAccountName": {"values": [{"value": "krbtgt"}]},
"servicePrincipalName": {"values": [{"value": "kadmin/changepw"}]},
"showInAdvancedViewOnly": {"values": [{"value": "TRUE"}]},
"userAccountControl": {"values": [{"value": "514"}]}}}}
JSON passwordChange: {
"timestamp": "2018-04-05T14:56:28.086947+1200",
"type": "passwordChange",
"passwordChange": {
"status": "Success",
"version": {"major": 1, "minor": 0},
"remoteAddress": "NULL",
"userSid": "S-1-5-21-202143440-2159076023-2784540227-500",
"action": "Reset",
"dn": "CN=krbtgt,CN=Users,DC=addom,DC=samba,DC=example,DC=com",
"transactionId": "d9184e5e-aeca-4793-b7a7-a8a558378421"}}
Comments appreciated
Gary

[Stefan Metzmacher via samba-technical]

Hi Gary,

Post by Gary Lockyer via samba-technical
Current state of this task.
Comments appreciated.

Most of the preparation like the session guid looks good.

I'm wondering if we want to implement the auditing of the directory
database similar to Windows using SACLs in the security descriptors
instead of having custom modules for various types of events.

metze

[Andrew Bartlett via samba-technical]

This post might be inappropriate. Click to display it.

[Andrew Bartlett via samba-technical]

On Fri, 2018-05-11 at 06:06 +1200, Andrew Bartlett via samba-technical

On Mon, 2018-05-07 at 18:05 +0200, Stefan Metzmacher via samba-

Post by Stefan Metzmacher via samba-technical
Hi Gary,

Post by Gary Lockyer via samba-technical
Current state of this task.
Comments appreciated.

Most of the preparation like the session guid looks good.

I've reviewed and pushed the preparation patches patches that I could
get past an autobuild on GitLab CI. The new common audit/json code
needs a talloc dep (fails to find talloc.h, a common trap for new
isolated subsystems), and I'll fix that next and push some more of the
prep code later.

Attached is the larger reviewed set of patches that I'm planning to put
into the next autobuild.

These are
Reviewed-by: Andrew Bartlett <***@samba.org>

Andrew Bartlett

[Jeremy Allison via samba-technical]

Post by Andrew Bartlett via samba-technical
On Fri, 2018-05-11 at 06:06 +1200, Andrew Bartlett via samba-technical

On Mon, 2018-05-07 at 18:05 +0200, Stefan Metzmacher via samba-

Post by Stefan Metzmacher via samba-technical
Hi Gary,

Post by Gary Lockyer via samba-technical
Current state of this task.
Comments appreciated.

Most of the preparation like the session guid looks good.

I've reviewed and pushed the preparation patches patches that I could
get past an autobuild on GitLab CI. The new common audit/json code
needs a talloc dep (fails to find talloc.h, a common trap for new
isolated subsystems), and I'll fix that next and push some more of the
prep code later.

Attached is the larger reviewed set of patches that I'm planning to put
into the next autobuild.

I do have one comment:

I really dislike the

* Error handling:
*
* The json_object structure contains a boolean 'error'. This is set whenever
* an error is detected. All the library functions check this flag and return
* immediately if it is set.
*
* if (object->error) {
* return;
* }
*
* This allows the operations to be sequenced naturally with out the clutter
* of error status checks.
*

idiom. This was used in the original asn.1
codebase in Samba and it took me a *LONG* time to get
all the errors out of that.

Please don't consider error status checks as "clutter".

They're *not*. I'd much rather have correctly
error checked code than an automatic assumption
that errors will be handled later.

Jeremy.

[Andrew Bartlett via samba-technical]

Post by Jeremy Allison via samba-technical
I really dislike the
*
* The json_object structure contains a boolean 'error'. This is set whenever
* an error is detected. All the library functions check this flag and return
* immediately if it is set.
*
* if (object->error) {
* return;
* }
*
* This allows the operations to be sequenced naturally with out the clutter
* of error status checks.
*
idiom. This was used in the original asn.1
codebase in Samba and it took me a *LONG* time to get
all the errors out of that.
Please don't consider error status checks as "clutter".
They're *not*. I'd much rather have correctly
error checked code than an automatic assumption
that errors will be handled later.

G'Day Jeremy.

Just to make sure I understand you. As I'm sure you have seen, this
patch just moves the existing helper functions (the work Gary did about
a year ago) into a mini-library and places it in another folder, which
is why it added the above documentation.

Are you asking that I not merge this series, or can I merge this much
and then you work with Gary to improve the API it as part of his
ongoing work stream?

Thanks,

Andrew Bartlett

[Jeremy Allison via samba-technical]

Post by Andrew Bartlett via samba-technical

Post by Jeremy Allison via samba-technical
I really dislike the
*
* The json_object structure contains a boolean 'error'. This is set whenever
* an error is detected. All the library functions check this flag and return
* immediately if it is set.
*
* if (object->error) {
* return;
* }
*
* This allows the operations to be sequenced naturally with out the clutter
* of error status checks.
*
idiom. This was used in the original asn.1
codebase in Samba and it took me a *LONG* time to get
all the errors out of that.
Please don't consider error status checks as "clutter".
They're *not*. I'd much rather have correctly
error checked code than an automatic assumption
that errors will be handled later.

G'Day Jeremy.
Just to make sure I understand you. As I'm sure you have seen, this
patch just moves the existing helper functions (the work Gary did about
a year ago) into a mini-library and places it in another folder, which
is why it added the above documentation.

"I'm sure you have seen" - nope. As I'm sure you
know I haven't seen :-).

Just to confirm, I don't see this code currently in
the tree so it is new stuff being added, yeah ?

I just *hate* that idiom for new API's, don't think
we should expand it.

Post by Andrew Bartlett via samba-technical
Are you asking that I not merge this series, or can I merge this much
and then you work with Gary to improve the API it as part of his
ongoing work stream?

No, I don't want to block things - so you can merge
if you need to. I would like to work with Gary to
get this fixed though, I don't want it left like
this long term.

Cheers,

Jeremy.

[Andrew Bartlett via samba-technical]

Post by Jeremy Allison via samba-technical

Post by Andrew Bartlett via samba-technical
G'Day Jeremy.
Just to make sure I understand you. As I'm sure you have seen, this
patch just moves the existing helper functions (the work Gary did about
a year ago) into a mini-library and places it in another folder, which
is why it added the above documentation.

"I'm sure you have seen" - nope. As I'm sure you
know I haven't seen :-).
Just to confirm, I don't see this code currently in
the tree so it is new stuff being added, yeah ?

It is currently in auth/auth_log.c. It was probably a good place to
hide, I know nobody reads auth code ;-)

Post by Jeremy Allison via samba-technical
I just *hate* that idiom for new API's, don't think
we should expand it.

OK.

Post by Jeremy Allison via samba-technical

Post by Andrew Bartlett via samba-technical
Are you asking that I not merge this series, or can I merge this much
and then you work with Gary to improve the API it as part of his
ongoing work stream?

No, I don't want to block things - so you can merge
if you need to. I would like to work with Gary to
get this fixed though, I don't want it left like
this long term.

Thanks. I saw Gary go through the phases of anger, grief and
resignation and he plans to go over it and make it return-based soon. 

:-)

Thanks,

Andrew Bartlett