Skip to content
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

feat: [1122]-ldap and user extension + AD profile #1136

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

PavelJurka
Copy link
Contributor

@PavelJurka PavelJurka commented Jul 2, 2024

Related Issue: .

#1122

Description of changes:

This issue is about adding a few useful fields to LDAP user, User object and adding a way how to map Azure ID to it.

dictionary.json Outdated Show resolved Hide resolved
dictionary.json Outdated Show resolved Hide resolved
@floydtree floydtree added the v1.3.0 Changes marked for v1.3.0 of OCSF label Jul 2, 2024
@mikeradka mikeradka added enhancement New feature or request non_breaking Non Breaking, backwards compatible changes labels Jul 2, 2024
@PavelJurka PavelJurka requested a review from Aniak5 July 3, 2024 09:16
@@ -154,6 +167,17 @@
}
}
},
"allowed_to_act_on_behalf_of_other_identity": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very long name. Could this be made more concise? Also should this be a boolean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's long but name and type is consistent with AD. Can we keep it? thank you!

Copy link
Contributor

@mikeradka mikeradka Jul 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have some examples of how this looks in AD?

We may still want to consider looking how to make this more concise, if it is possible to do so. Why not something along the lines of act_on_behalf, acts_on_behalf, or on_behalf_of with the details about the indication that the account is allowed to act on behalf of another identity within the attribute's description?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the native AD name. There is an LDAP display name, and a CN name. adminCount (which doesn't have an internal separator) is a LDAP display name. Admin-Count is the CN name.

msDS-Allowed-To-Act-On-Behalf-Of-Other-Identity is the CN name and msDSAllowedToActOnBehalfOfOtherIdentity is the LDAP display name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another common name for this is 'impersonation.' e.g. can_impersonate. If it is a boolean, then we would have is_* has_* and can_* conventions. However this proposed attribute seems to be a string_t - not sure why.

Similarly, allowed_to_delegate_to could be can_delegate. Again as boolean but this is also an array.

@mikeradka
Copy link
Contributor

mikeradka commented Jul 24, 2024

  • force_change_password_next_sign_in
  • force_change_password_next_sign_in_with_mfa
  • on_premises_distinguished_name
  • user_password_expiry_computed_time

These are a few that have pretty long names. With OCSF we try our best to keep attribute names as concise as possible. Can we look a bit deeper into ways to shorten these attribute names?

While I understand there is an intent to keep these consistent with AD, OCSF and AD still have separate conventions and I am hesitant to deviate from OCSF by introducing large attribute names unless absolutely needed.

  • force_change_password_next_sign_in and force_change_password_next_sign_in_with_mfa
    • the common element with these two is a force_pw_change. Maybe there is a way to consolidate them.
  • on_premises_distinguished_name
    • on_prem_dn? (note that the abbreviation dn is already used in the ldap_dn dictionary attribute)
  • user_password_expiry_computed_time
    • looks like an expiry time. What about something along the lines of pw_expiry?

@PavelJurka PavelJurka force-pushed the 1122-User-LdapPerson-Asure-AD-support branch from bc354a3 to f0c170a Compare July 26, 2024 18:05
@mikeradka mikeradka added v1.4.0 or later Changes marked for versions beyond v1.3.0 of OCSF and removed v1.3.0 Changes marked for v1.3.0 of OCSF labels Jul 26, 2024
mikeradka pushed a commit that referenced this pull request Aug 13, 2024
Expands the `user` object to add relevant data that comes from various
Identity Providers or Directories while keep relevance with LDAP and
MITRE D3FEND.

- Add Observable `type_id` 31-35 for User UID, Group Name, Group UID,
Account Name, Account UID
- Add `phone_number` to `user` and to `ldap_person` - this attribute can
be assigned to both or one or the other depending on the upstream
system. For instance Entra ID or Okta
- ~~Add `state_id` and `state` to `user` to represent the various states
of a user record in a directory or IDP such as their provisioning
status, (de)activation. This is 1:1 with Okta with an extra `Deleted`
enum added for Google Workspace~~ Removed as #1136 already has a
solution
- Add `has_mfa` Boolean to Dictionary and `user` object as a quick way
to tell if a `user` has MFA/2FA enabled/assigned to them

---------

Signed-off-by: Jonathan Rau <[email protected]>
Co-authored-by: Rajas <[email protected]>
},
"allowed_to_delegate_to": {
"requirement": "optional"
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these AD specific attributes included directly in the ldap_person object rather than included via the AD profile which is also applied? That is, these two will always be in the object even if the profile is not applied.

@@ -154,6 +167,17 @@
}
}
},
"allowed_to_act_on_behalf_of_other_identity": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another common name for this is 'impersonation.' e.g. can_impersonate. If it is a boolean, then we would have is_* has_* and can_* conventions. However this proposed attribute seems to be a string_t - not sure why.

Similarly, allowed_to_delegate_to could be can_delegate. Again as boolean but this is also an array.

"description": "The number of times the user tried to log on to the account using an incorrect password."
},
"bad_password_time": {
"caption": "Bad Password Time",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming again. Not sure if these are native AD names. I would prefer 'failed_loginsornum_failed_logins. Also last_failed_loginorlast_failed_login_time`.

"caption": "Consistency Guid",
"type": "string_t",
"description": "Is used to check consistency between the directory and another object, database, or application by comparing GUIDs."
},
Copy link
Contributor

@pagbabian-splunk pagbabian-splunk Aug 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one seems very specific, maybe to AD only? Should be type uuid_t.

"creator_sid": {
"caption": "Creator SID",
"type": "string_t",
"description": "The security ID of the creator of the object that contains this attribute."
Copy link
Contributor

@pagbabian-splunk pagbabian-splunk Aug 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows specific - sid. Do we use strings or uids for sids? Can we use creator.uid already in dictionary? We can use this Windows name in the desc.

"description": "roxy address of a user. For example [SMTP: [email protected], smtp: [email protected]]",
"type": "string_t",
"is_array": true
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this always be an email address? If so, we can use the email_t type. Also, a typo in the description of proxy.

"caption": "Is Recycled",
"type": "boolean_t",
"description": "Identifies if the object has been added to the Recycle bin in Active Directory."
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a generic name with a specific to AD description.

"caption": "Resultant PSO",
"type": "string_t",
"description": "The effective password policy applied on this object."
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a Policy data type? Is this specific to AD? What is a PSO?

"caption": "SAM Account Type",
"type": "long_t",
"description": "This attribute contains information about every account type object."
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows specific sam_ prefixes. (Just keeping notes of these).

"type": "string_t",
"description": "The unique identifiers of a service instance in Active Directory.",
"is_array": true
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually sort these attributes even if they are related (e.g. is_ stays together) but not absolute.

"caption": "Last Known Parent",
"type": "string_t",
"description": "The Distinguished Name (DN) of the last known parent of an orphaned object."
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May need ldap_ because it is a DN or an object. A convention around objects like AD could be retrofitted to LDAP too (we would need to deprecate).

"caption": "On Premises Distinguished Name",
"description": "The distinguished name of the user in the on-premises Active Directory.",
"type": "string_t"
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a difference between AD and Azure Entra ID?

@pagbabian-splunk
Copy link
Contributor

We may need to break this PR down into a few smaller PRs - it is quite large with many new attributes, but is also very important to get right.

Copy link
Contributor

@floydtree floydtree left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I echo @pagbabian-splunk's comment, these additions are much needed for the framework, but we need to break the PR down in smaller chunks. Perhaps you can target additions/enhancements to just one object at a time in a single PR.

Other things to consider -

  1. If you have sample events that you want to normalize, it will help us quite a bit to review.
  2. Consider re-using existing attributes before adding new ones. (Refer to Paul's inline comments)
  3. Consider all the specific data-types as you add new attributes, instead of defaulting to string_t
  4. Consider object creation to avoid redundancy in naming and re-usability of attributes. (e.g on_premises_* vs on on_premises { * })
  5. Consider if you can create an object to group AD specific attributes together (thinking about re-usability and portability across the framework)

@pagbabian-splunk @mikeradka @zschmerber Feel free to add, if I missed anything.

"type": "string_t",
"description": "Specifies all the groups in which the object is a member, directly or indirectly. See specific usage.",
"is_array": true
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can the caption be made clearer?

query-jeremy pushed a commit to query-ai/ocsf-schema that referenced this pull request Aug 20, 2024
Expands the `user` object to add relevant data that comes from various
Identity Providers or Directories while keep relevance with LDAP and
MITRE D3FEND.

- Add Observable `type_id` 31-35 for User UID, Group Name, Group UID,
Account Name, Account UID
- Add `phone_number` to `user` and to `ldap_person` - this attribute can
be assigned to both or one or the other depending on the upstream
system. For instance Entra ID or Okta
- ~~Add `state_id` and `state` to `user` to represent the various states
of a user record in a directory or IDP such as their provisioning
status, (de)activation. This is 1:1 with Okta with an extra `Deleted`
enum added for Google Workspace~~ Removed as ocsf#1136 already has a
solution
- Add `has_mfa` Boolean to Dictionary and `user` object as a quick way
to tell if a `user` has MFA/2FA enabled/assigned to them

---------

Signed-off-by: Jonathan Rau <[email protected]>
Co-authored-by: Rajas <[email protected]>
query-jeremy pushed a commit to query-ai/ocsf-schema that referenced this pull request Aug 22, 2024
Expands the `user` object to add relevant data that comes from various
Identity Providers or Directories while keep relevance with LDAP and
MITRE D3FEND.

- Add Observable `type_id` 31-35 for User UID, Group Name, Group UID,
Account Name, Account UID
- Add `phone_number` to `user` and to `ldap_person` - this attribute can
be assigned to both or one or the other depending on the upstream
system. For instance Entra ID or Okta
- ~~Add `state_id` and `state` to `user` to represent the various states
of a user record in a directory or IDP such as their provisioning
status, (de)activation. This is 1:1 with Okta with an extra `Deleted`
enum added for Google Workspace~~ Removed as ocsf#1136 already has a
solution
- Add `has_mfa` Boolean to Dictionary and `user` object as a quick way
to tell if a `user` has MFA/2FA enabled/assigned to them

---------

Signed-off-by: Jonathan Rau <[email protected]>
Co-authored-by: Rajas <[email protected]>
srotsinha pushed a commit to query-ai/ocsf-schema that referenced this pull request Sep 3, 2024
Expands the `user` object to add relevant data that comes from various
Identity Providers or Directories while keep relevance with LDAP and
MITRE D3FEND.

- Add Observable `type_id` 31-35 for User UID, Group Name, Group UID,
Account Name, Account UID
- Add `phone_number` to `user` and to `ldap_person` - this attribute can
be assigned to both or one or the other depending on the upstream
system. For instance Entra ID or Okta
- ~~Add `state_id` and `state` to `user` to represent the various states
of a user record in a directory or IDP such as their provisioning
status, (de)activation. This is 1:1 with Okta with an extra `Deleted`
enum added for Google Workspace~~ Removed as ocsf#1136 already has a
solution
- Add `has_mfa` Boolean to Dictionary and `user` object as a quick way
to tell if a `user` has MFA/2FA enabled/assigned to them

---------

Signed-off-by: Jonathan Rau <[email protected]>
Co-authored-by: Rajas <[email protected]>
@PavelJurka PavelJurka marked this pull request as draft September 3, 2024 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request non_breaking Non Breaking, backwards compatible changes v1.4.0 or later Changes marked for versions beyond v1.3.0 of OCSF
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants