Adding osctrl-api component #28
Conversation
| @@ -3,18 +3,20 @@ | |||
|
|
|||
| VAGRANTFILE_API_VERSION = "2" | |||
|
|
|||
| IP_ADDRESS = "10.10.10.6" | |||
There was a problem hiding this comment.
Is hardcoding this ok or should we use hostnames (or configurable values)?
There was a problem hiding this comment.
This is the IP address for the vagrant machine. There is no way (that I know of) to configure this via a command and is better to have the IP set to something known. Since the flow is to run vagrant up this should be fine.
| } | ||
|
|
||
| // Automigrate of tables | ||
| func automigrateDB() error { |
There was a problem hiding this comment.
The automigrateDB() function is used by the ORM to create the schema based on the data stored. It is not doing much now, but if we decide to store something in the DB, this will come handy. I am thinking keeping requests per token, so we can enable/enforce some rate limiting?
cmd/api/handlers-environments.go
Outdated
| "github.com/jmpsec/osctrl/pkg/utils" | ||
| ) | ||
|
|
||
| // GET Handler for single JSON environment |
There was a problem hiding this comment.
Do you mean this API is used to validate if an environment exists in the backend?
There was a problem hiding this comment.
Correct, and returns all data for that environment. Does it make more sense to have a verb to check on a specific environment, passing the name, and return just a boolean?
cmd/api/handlers-environments.go
Outdated
| incMetric(metricAPIOK) | ||
| } | ||
|
|
||
| // GET Handler for multiple JSON environments |
cmd/api/handlers-nodes.go
Outdated
| incMetric(metricAPIErr) | ||
| if err.Error() == "record not found" { | ||
| log.Printf("node not found: %s", uuid) | ||
| apiHTTPResponse(w, JSONApplicationUTF8, http.StatusNotFound, ApiErrorResponse{Error: "node not found"}) |
There was a problem hiding this comment.
Is what this returns a similar JSON format to successful run? If so that'll make it easier to parse in prospective clients.
There was a problem hiding this comment.
When the requested node exists, it does return the structure for OsqueryNode{}, with all its fields in it. Does it make more sense to return always a JSON response with one field for message, and another for data?
There was a problem hiding this comment.
No that should be fine, a client can easily check that the fields are empty :)
cmd/api/handlers-queries.go
Outdated
| newQuery := queries.DistributedQuery{ | ||
| Query: q.Query, | ||
| Name: queryName, | ||
| Creator: "API", |
There was a problem hiding this comment.
This should be the username from the authorization mechanism, not API
There was a problem hiding this comment.
Totally, but this is just a placeholder until I get the actual tokens part finished.
cmd/api/handlers-queries.go
Outdated
| Active: true, | ||
| Completed: false, | ||
| Deleted: false, | ||
| Repeat: 0, |
There was a problem hiding this comment.
I'm not sure how this value should work. In my eyes a distributed query should run only once. If it runs anymore times than that, then when you fetch something by queryName what does that actually mean? Is it original data? The most recent data? Both?
There was a problem hiding this comment.
This is some legacy stuff, that I thought about having distributed queries in a way that can be repeated, but then again, those queries should be scheduled. I can probably clean this up since it is dead code...
| } | ||
| // Prepare and create new query | ||
| queryName := "query_" + generateQueryName() | ||
| newQuery := queries.DistributedQuery{ |
There was a problem hiding this comment.
We should have an accelerate flag in here as well. We should set it if a query targets only a single host.
There was a problem hiding this comment.
Let's get that done!
cmd/api/handlers-queries.go
Outdated
| for _, e := range q.Environments { | ||
| if (e != "") && envs.Exists(e) { | ||
| if err := queriesmgr.CreateTarget(queryName, queries.QueryTargetEnvironment, e); err != nil { | ||
| apiErrorResponse(w, "error creating query environment target", err) |
There was a problem hiding this comment.
Same question about sensitive errors getting passed to the client.
| incMetric(metricAPIOK) | ||
| } | ||
|
|
||
| // GET Handler to return multiple queries in JSON |
There was a problem hiding this comment.
This is a super powerful API and should probably be locked to a certain subset of users because that allows visibility into all queries, including ones that might relate to on going investigations.
Update DEB build files
Overview
osctrl-apicomponent as a new service. It will listen locally in port9002and remotely in8444, but those are default values and can be customized.I will add more details once I am done, since there will be likely changes to this implementation.
cc @obelisk