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

Add a body_length field to http_request and http_response #1141

Closed
wants to merge 4 commits into from

Conversation

mavam
Copy link
Contributor

@mavam mavam commented Jul 15, 2024

This PR adds a new field to the http_[request|response] objects called body_length that captures the length of the HTTP request body. The existing field length describes the entire length of the HTTP request, including headers.

Delete once you have confirmed the following:

  1. Have you followed the contribution guidelines?
  2. Did you run a local instance of the ocsf-server and ensure it ran without any errors/warnings?

@mavam mavam force-pushed the topic/http-request-body-length branch 3 times, most recently from 5bb3ff4 to 3233da9 Compare July 15, 2024 19:57
@rmouritzen-splunk
Copy link
Contributor

This look good, but can we use content_length instead of body_length, matching the wording of the standard request and response Content-Length headers? Or is there a reason not use "content" here?

@mavam
Copy link
Contributor Author

mavam commented Jul 23, 2024

This look good, but can we use content_length instead of body_length, matching the wording of the standard request and response Content-Length headers? Or is there a reason not use "content" here?

The reason: A maliciously crafted HTTP requests can have a Content-Length value independent of the actual body length.

However, your proposal made me thought: should we just add an array of key-value pairs that represent the header name and value?

@rmouritzen-splunk
Copy link
Contributor

rmouritzen-splunk commented Jul 23, 2024

The reason: A maliciously crafted HTTP requests can have a Content-Length value independent of the actual body length.

OK. Makes sense. Perhaps put that in the description, like "actual length of body, independent of Content-Length header, if any".

However, your proposal made me thought: should we just add an array of key-value pairs that represent the header name and value?

OCSF generally tries to avoid open-ended up definitions like this. Instead consider a fixed set of attributes. (Also as an aside, key-value pairs would more naturally fit in an object than in any array of objects, each with a single field.)

@floydtree
Copy link
Contributor

However, your proposal made me thought: should we just add an array of key-value pairs that represent the header name and value?

both http_request and http_response have a http_headers attribute which is an array of http_header object with name & value attributes.

@mikeradka mikeradka added the v1.4.0 Changes marked for the upcoming version 1.4.0 label Jul 26, 2024
mavam added 3 commits August 2, 2024 08:38
Signed-off-by: Matthias Vallentin <[email protected]>
@mavam mavam force-pushed the topic/http-request-body-length branch from 3233da9 to 597aa8b Compare August 2, 2024 06:41
@mavam mavam marked this pull request as ready for review August 2, 2024 06:41
@mavam
Copy link
Contributor Author

mavam commented Aug 2, 2024

@rmouritzen-splunk I've pushed a commit to clarify the relationship to the Content-Length header.

both http_request and http_response have a http_headers attribute which is an array of http_header object with name & value attributes.

Oh right, it's already there! 🙈

@mavam
Copy link
Contributor Author

mavam commented Aug 14, 2024

CI complains about a missing value for field "attributes.type". How would I go after this?

@mavam mavam changed the title Add a body_length field to http_request Add a body_length field to http_request and http_response Aug 14, 2024
@query-jeremy
Copy link
Contributor

query-jeremy commented Aug 14, 2024

CI complains about a missing value for field "attributes.type". How would I go after this?

Attributes require a type. This one should probably be integer_t or long_t.

body_length (or content_length) is a new attribute that isn't defined in dictionary.json.

Convention for adding attributes is to add them in the attributes object in dictionary.json. The ideal solution is to add body_length to dictionary.json and include a type property for the attribute.

A less desirable alternative would be to add type properties to both objects.

See the definition of length in the dictionary for an example here:

"length": {

@query-jeremy
Copy link
Contributor

Also: sorry for the confusing error message from the CI job. I'll think on it.

@rmouritzen-splunk
Copy link
Contributor

CI complains about a missing value for field "attributes.type". How would I go after this?

@mavam As @query-jeremy said, the body_length attribute needs to be defined in the dictionary. I'll give more detail to make this extra clear.

The core idea is that all attributes used anywhere in the schema need to be defined in a dictionary.

Since you're modifying objects in the base of the schema, add these to the base dictionary.json inside the attributes member. Something like this (without the comments):

{
  "caption": "Attribute Dictionary",
  "description": "The Attribute Dictionary ...",
  "name": "dictionary",
  "attributes": {
    // ...    
    "bios_ver": {
      "caption": "BIOS Version",
      "description": "The BIOS version. For example: <code>LENOVO G5ETA2WW (2.62)</code>.",
      "type": "string_t"
    },
    // We typically add attributes in sorted order, so add body_length here
    "body_length": {
      "caption": "Request Body Length",
      "description": "The actual length of the HTTP request body, in number of bytes, independent of a potentially existing Content-Length header.",
      "type": "long_t"
    },
    "boot_time": {
      "caption": "Boot Time",
      "description": "The time when the system was booted.",
      "type": "timestamp_t"
    },
    // ... 
}

I'm guessing long_t is the correct type, though length uses integer_t, which could be correct but of course is limited to lengths up to the 32-bit signed integer maximum value (2-billion-something).

This also defines the attribute's caption and description. You can override these in event classes and objects that pull in an attribute. In this case, the http objects aren't changing the values, so in the interest of maintenance you should remove caption and description from the body_length attributes in this objects.

@floydtree
Copy link
Contributor

This will be added as a part of a separate PR by @zschmerber-atlassian. Closing this one out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.4.0 Changes marked for the upcoming version 1.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants