-
Notifications
You must be signed in to change notification settings - Fork 24.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce new audit record for security configuration changes via API #62916
Introduce new audit record for security configuration changes via API #62916
Conversation
@@ -51,7 +52,7 @@ appender.audit_rolling.layout.pattern = {\ | |||
# "user.run_by.realm" the realm name of the impersonating subject ("user.run_by.name") | |||
# "user.run_as.realm" if this "event.action" is of a run_as type, this is the realm name the impersonated user is looked up from | |||
# "user.roles" the roles array of the user; these are the roles that are granting privileges | |||
# "origin.type" it is "rest" if the event is originating (is in relation to) a REST request; possible other values are "transport" and "ip_filter" | |||
# "event.type" it is "rest" if the event is originating (is in relation to) a REST request; possible other values are "transport", "ip_filter" and "security_config_change" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The origin.type
and event.type
definitions have been reversed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: The word originating
throws me off a bit because the field is now event.type
instead of origin.type
. IIUC, even.type
says something about "at which layer the current request is being processed". This definition works fine for the existing values of rest
, transport
and ip_filter
, but falls short with the new security_config_change
value. I don't have a good suggestion, maybe something like "it is for the broader nature of the current request, the possible values are ...". Please feel free to come up with your own version.
@@ -37,12 +37,13 @@ appender.audit_rolling.layout.pattern = {\ | |||
%varsNotEmpty{, "transport.profile":"%enc{%map{transport.profile}}{JSON}"}\ | |||
%varsNotEmpty{, "rule":"%enc{%map{rule}}{JSON}"}\ | |||
%varsNotEmpty{, "event.category":"%enc{%map{event.category}}{JSON}"}\ | |||
%varsNotEmpty{, "config_change":%map{config_change}}\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new field that contains the configuration taking effect, as an un-escaped JSON object, for records with event.type: "security_config_change"
.
Pinging @elastic/es-security (:Security/Audit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I prefer this version better than the last one (#62740). One thing that I'd suggest is to introduce dedicated interfaces for these changes as detailed in below comments.
I also have some minor "philosophical" questions about the names:
- Whether we could name the event type to
config_change.security
? This might be more suitable if we need audit non-security config_change in future (though I am not sure whethere there would be such needs). - The
config_change
bit is repeated in field names. For the request body field, maybe could just call itdetails
.
As for the questions that you listed, here are my views:
Should we move the new event type to another output file?
I agree it looks fine in a single file and I prefer it this way. But if for some reason there are good chances that we need split them in future, how big would the BWC effort be? If the effort is too high , we may as well start with splitted files.
Do we need differentiated behaviour between the different types of credentials
For simplicity, we can just redact all of them for now. Also it is easier to "unredact" them later while adding a new redacting may be considered as breaking change.
should we audit tokens created for a successful SAML/OIDC authentication?
The PR currently audits token refreshing, which comes after the initial token creation by SAML/OIDC authentication. It feels a bit wierd that the initial creation is not auditing, but all subsequent refreshing is. It also creates a gap in the auditing events. With that being said, I think we can skip token related actions all together for the initial version. The initial token creation has some trickincess in terms of auditing. Also actions related to tokens can be much more often than other actions and there could be verbosity concerns. We could use some more discussions about them.
...e/src/main/java/org/elasticsearch/xpack/core/security/action/user/ChangePasswordRequest.java
Outdated
Show resolved
Hide resolved
String eventAction = null; | ||
if (msg instanceof PutUserRequest || | ||
msg instanceof PutRoleMappingRequest || | ||
msg instanceof PutRoleRequest || | ||
msg instanceof SetEnabledRequest || | ||
msg instanceof ChangePasswordRequest || | ||
msg instanceof CreateApiKeyRequest || | ||
msg instanceof GrantApiKeyRequest || | ||
msg instanceof CreateTokenRequest || | ||
msg instanceof PutPrivilegesRequest) { | ||
eventAction = "put"; | ||
} else if (msg instanceof DeleteUserRequest || | ||
msg instanceof DeleteRoleMappingRequest || | ||
msg instanceof DeleteRoleRequest || | ||
msg instanceof InvalidateTokenRequest || | ||
msg instanceof InvalidateApiKeyRequest || | ||
msg instanceof DeletePrivilegesRequest) { | ||
eventAction = "delete"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether we could introduce interfaces dedicated to these, something like the follows:
interface SecurityConfigAuditable {
String action();
String content(); // this method is equivalent to `ToXContent#toXContent`
}
interface PutSecurityConfigAuditable() {
default String action() {
return "put";
}
}
interface DeleteSecurityConfigAuditable() {
default String action() {
return "delete";
}
}
To me, it will make the code easier to read and write. I also think it is more preferable to have dedicated interfaces for serialising the request object for auditing purpose instead of the generic ToXContent
interface, because 1) the string representation it provides is specific to auditing; 2) security change auditing has pretty good business meaning and we may discover new things later that could add to the interfaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though we've discussed this before, I think you're raising a good point.
I believe the root question is to what extent is the auditing different from generic request serialisation. Given that the present audit approach is not designed around the concept of "requests" but rather centred on "changes", I think I can see the case for an audit-specific serialisation logic.
Here's a list for why I think that is the case:
- the refresh policy is not important for auditing
- the name of the changed object (role, user, etc) should be distinguished as a separate field, not as part of a serialised object
- the nature of the change (put, enable, clear) should also by distinguished as a separate field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ywangd After mulling over it some more, I've changed back to thinking that the code to print the request bodies should sit in the LoggingAuditTrail
class.
It's probably easier to show it in code than to talk about, but the reasoning is that the layout of it is particular to the logging audit trail format, and it's not reasonable to expect the reuse of it. It's also easier to ensure and maintain the formatting consistency, if it all sits together rather than be scattered across request methods. A simple example, is the consistency for the username
vs user.name
fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'll have to see the code to fully understand what you mean by "the code to print the request bodies". But I didn't suggest to move the "print" part to each individual class. The actual print should always be done in LoggingAuditTrail
and I agree with it. I was referring to the "serialization" part of the logging message, i.e. the toXContent
method, where the message is prepared. Also on the topic of resuing the auditing message, there were talks about auditing into an index (or maybe some other connectors). Would it be a valid possible future reuse?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is that even the toXContent
is not really suitable as a request object method.
The JSON object for the config change is part of the JSON object for the whole event so they must use the same conventions, eg for field names (username
vs user.name
), or plurals, or tenses (past/present). The objects must also be consistent among themselves. For example, check out the handling of the SetEnabledRequest
and GrantApiKeyRequest
requests.
In general, I've chosen the XContent field names in the context of the audit log that we've got; most commonly field names coincide with the XContent field names, but not always. Same for the nesting patterns.
Also on the topic of resuing the auditing message, there were talks about auditing into an index (or maybe some other connectors). Would it be a valid possible future reuse?
If, as initially planned, this feature would've been a particular case of "request body auditing" , then yes that would be a valid reuse case. But it's now a different thing. Audit records are a richer translation of requests in a friendlier format in the particular context of security config changes to be applied.
Slightly off-topic, the index connector is not a suitable connector for audits as the buffering on the local disk is really required (we had something like that and we've deprecated it).
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/AuditLevel.java
Outdated
Show resolved
Hide resolved
builder.field("password", password != null ? password.toString() : null); | ||
builder.field("access_token", accessToken != null ? accessToken.toString() : null); | ||
} else { | ||
builder.field("password", password != null ? "<redacted>" : null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are redacting, should we just always show <redacted>
regardless of whether the value is null or not? It is better from the information disclosure perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a good question, but I don't think we have to be that cautious, because the existence of a password doesn't give away much information. I think the audit log overall gives away much more details on the activity on the system.
IMO, the null
vs <redacted>
thing is not even something that we should document and test. I'm comfortable with only saying that "credentials and credential hashes are not printed in the audit log".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that in this case it doesn't make much difference. For instance, the only thing that the existence of the password
field ( redacted vs null) "discloses" is that the request is using the password grant type, as this is the only case where this is allowed as a parameter.
In general, from an audit perspective, as a legitimate administrator, I would like to know as much information as possible so I would prefer to know if a parameter was specified when the call to the API was made* and in that sense I prefer to have "" vs null.
* In the case of an investigation, I might want to know if an attacker had access to a valid password or if they figured out another way to compromise the system/api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, the null vs thing is not even something that we should document and test. I'm comfortable with only saying that "credentials and credential hashes are not printed in the audit log".
Err, except that the put user action is an example where null
vs <redacted>
IS significant. if the password_hash
field is <redacted>
then the put user action updated the password, whereas if it is null
then user password remains unchanged.
Therefore, the password/password_hash field will be null
if and only if the field is not present in the request body. Otherwise it will be <redacted>
. I'lll make sure to test for it.
I think I prefer
It's only repeated once between the field value
I don't see any particular problem wrt to BWC . If we split it in the future, the subsequent minors must default to the current way. Let's keep it in the same log file for now.
This is a good point. I'll take your advice and not audit tokens at all in this first version. |
I've now tried it and it works as expected. I believe a follow-up is hence not necessary? After indexing a couple of security config change events in an index with no explicit mapping defined:
I am able to filter the events concerning a specific user, with:
|
@tvernum I have resolved your comments. |
API Key request objects (`InvalidateApiKeyRequest` and `GetApiKeyRequest`) support multiple key-selection parameters such as realm-name, user-name, key-id and key-name. The specified behaviour is that if any of these are _blank_ then they act as a wildcard and do not restrict the search criteria. This change moves the "is blank" logic into the constructor for these requests so that there is a single consistent way to determine blank (`Strings.hasText(arg) == false`) and all usage of these fields can rely on the getter returning either `null` or a _real value_, and never a non-null blank. Relates: #62916
API Key request objects (`InvalidateApiKeyRequest` and `GetApiKeyRequest`) support multiple key-selection parameters such as realm-name, user-name, key-id and key-name. The specified behaviour is that if any of these are _blank_ then they act as a wildcard and do not restrict the search criteria. This change moves the "is blank" logic into the constructor for these requests so that there is a single consistent way to determine blank (`Strings.hasText(arg) == false`) and all usage of these fields can rely on the getter returning either `null` or a _real value_, and never a non-null blank. Relates: elastic#62916 Backport of: elastic#66240
I think it means we can wait and see what sort of feedback we get from real world usage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@elasticmachine update branch |
…elastic#62916) This PR introduces a new event.type category for audit records, namely the `security_config_change`, in the existing audit trail. Events in this category record that a security configuration has been set (eg user/role created/updated) or cleared (eg user/role deleted). The events are emitted by default, but can be explicitly toggled by the `security_config_changed` handler. The record contains all the change details, (e.g. the rules of the particular role mapping that has been created or updated), but all credentials are redacted out. The change details are formatted as a JSON object are are part of audit record structure (i.e. they are not JSON-escaped and put in a string field). Co-authored-by: Yang Wang <[email protected]> Co-authored-by: Tim Vernum <[email protected]>
…#62916) This PR introduces a new event.type category for audit records, namely the `security_config_change`, in the existing audit trail. Events in this category record that a security configuration has been set (eg user/role created/updated) or cleared (eg user/role deleted). The events are emitted by default, but can be explicitly toggled by the `security_config_changed` handler. The record contains all the change details, (e.g. the rules of the particular role mapping that has been created or updated), but all credentials are redacted out. The change details are formatted as a JSON object are are part of audit record structure (i.e. they are not JSON-escaped and put in a string field). Co-authored-by: Yang Wang <[email protected]> Co-authored-by: Tim Vernum <[email protected]>
Relates: #37914 |
Audit log doc changes about: * the new security_config_change event type (main scope of this PR) * remove mentions of the 6.5 audit format changes (the JSON format) * mention the new archiving and rotation by size (in v8 only) * mention the request.id event attribute used to correlate audit events * mention that audit is only available on certain subscription levels * add an exhaustive audit event example list (because schema became too complex to explain in words 😢 given the new security_config_change events) * move the ignore policies are explained on a separate page (it was collocated with the logfile output since we had multiple outputs and the policies were specific the the logfile only). Co-authored-by: Lisa Cawley [email protected] Relates #62916 Closes #29912
Audit log doc changes about: * the new security_config_change event type (main scope of this PR) * remove mentions of the 6.5 audit format changes (the JSON format) * mention the new archiving and rotation by size (in v8 only) * mention the request.id event attribute used to correlate audit events * mention that audit is only available on certain subscription levels * add an exhaustive audit event example list (because schema became too complex to explain in words 😢 given the new security_config_change events) * move the ignore policies are explained on a separate page (it was collocated with the logfile output since we had multiple outputs and the policies were specific the the logfile only). Co-authored-by: Lisa Cawley [email protected] Relates elastic#62916 Closes elastic#29912
Audit log doc changes about: * the new security_config_change event type (main scope of this PR) * remove mentions of the 6.5 audit format changes (the JSON format) * mention the new archiving and rotation by size (in v8 only) * mention the request.id event attribute used to correlate audit events * mention that audit is only available on certain subscription levels * add an exhaustive audit event example list (because schema became too complex to explain in words 😢 given the new security_config_change events) * move the ignore policies are explained on a separate page (it was collocated with the logfile output since we had multiple outputs and the policies were specific the the logfile only). Co-authored-by: Lisa Cawley [email protected] Relates #62916 Closes #29912
Audit log doc changes about: * the new security_config_change event type (main scope of this PR) * remove mentions of the 6.5 audit format changes (the JSON format) * mention the new archiving and rotation by size (in v8 only) * mention the request.id event attribute used to correlate audit events * mention that audit is only available on certain subscription levels * add an exhaustive audit event example list (because schema became too complex to explain in words 😢 given the new security_config_change events) * move the ignore policies are explained on a separate page (it was collocated with the logfile output since we had multiple outputs and the policies were specific the the logfile only). Co-authored-by: Lisa Cawley [email protected] Relates elastic#62916 Closes elastic#29912
Audit log doc changes about: * the new security_config_change event type (main scope of this PR) * remove mentions of the 6.5 audit format changes (the JSON format) * mention the new archiving and rotation by size (in v8 only) * mention the request.id event attribute used to correlate audit events * mention that audit is only available on certain subscription levels * add an exhaustive audit event example list (because schema became too complex to explain in words 😢 given the new security_config_change events) * move the ignore policies are explained on a separate page (it was collocated with the logfile output since we had multiple outputs and the policies were specific the the logfile only). Co-authored-by: Lisa Cawley [email protected] Relates #62916 Closes #29912
This PR introduces a new
event.type
category for audit records, namely thesecurity_config_change
, in the existing audit trail . Events in this category record that a security configuration has been set (eg user/role created/updated) or cleared (eg user/role deleted). The events are emitted by default, but can be explicitly toggled by thesecurity_config_changed
handler. The record contains all the change details, (e.g. the rules of the particular role mapping that has been created or updated), but all credentials are redacted out. The change details are formatted as a JSON object are are part of audit record structure (i.e. they are not JSON-escaped and put in a string field).Sample audit log output:
Notes:
name
is). I'll open a prerequisite PR to enable that, and then adjust this (or open a follow-up PR)request.id
field of the new audit record for the security change refers back to the security transport action, which is different from therequest.id
of the REST action that triggered it all. This will be addressed in another PR.Outstanding questions:
ES tokens
,kerberos tickets
,ES passwords
, andES password hashes
? Currently they are all redacted out (instead of the value, the audited field contains the<redacted>
placeholder.Overrides #62740