Discussion:
[WIP] Log database changes.
(too old to reply)
Gary Lockyer via samba-technical
2018-04-05 03:13:59 UTC
Permalink
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
2018-04-05 06:25:46 UTC
Permalink
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
--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:***@sernet.de
Gary Lockyer via samba-technical
2018-04-05 21:14:49 UTC
Permalink
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
2018-05-06 19:20:50 UTC
Permalink
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
2018-05-07 16:05:01 UTC
Permalink
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
2018-05-14 08:41:59 UTC
Permalink
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'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.
SACL support would still need the same infrastructure, it would just
provide a way to filter which events to audit, rather than the course-
grained filters we have here.
I see it as a version 2 kind of thing, we need to get this much in
first. So far the client requests have been for class-based logging
(the filtering happens on external log analysis tools).
I would also want to know clearly what the use case is for SACL
logging, because if it is only really valuable in conjunction with a
full Event Log and matching windows exactly, that would be much more
work.
As it stands, our ACLs are a pain to modify (outside the windows GUI),
so in the short time per-server smb.conf options, matching the audit
work done so far seem much more practical.
Ok.
metze
Thanks Metze.

Just to round this out: On a walk over the weekend I considered a
design for the v2 you requested.

In short, the modification audit code runs in the result callback and
can access controls passed back up on the result. We can pass back up
a control indicating that the SACL was fired, and a future version of
the module could additionally log it in an audit_sacl class or such.

This would build quite nicely on the existing infrastructure nicely, if
it is ever required. As always a big part of the task would be the
required testing (which is where Gary is spending his days right now).

Thanks,

Andrew Bartlett
--
Andrew Bartlett http://samba.org/~abartlet/
Authentication Developer, Samba Team http://samba.org
Samba Developer, Catalyst IT http://catalyst.net.nz/services/samba
Andrew Bartlett via samba-technical
2018-05-15 22:53:08 UTC
Permalink
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
--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team https://samba.org
Samba Development and Support, Catalyst IT
https://catalyst.net.nz/services/samba
Jeremy Allison via samba-technical
2018-05-15 22:58:43 UTC
Permalink
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
2018-05-15 23:46:29 UTC
Permalink
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
--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team https://samba.org
Samba Development and Support, Catalyst IT
https://catalyst.net.nz/services/samba
Jeremy Allison via samba-technical
2018-05-15 23:59:13 UTC
Permalink
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
2018-05-16 00:30:47 UTC
Permalink
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
--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team https://samba.org
Samba Development and Support, Catalyst IT
https://catalyst.net.nz/services/samba
Loading...