xmidt-org / argus Goto Github PK

Submitting a PUT request to non-existent endpoint with the correct auth results in a 403, instead of a 404 as expected.
Example:
curl -X PUT https://$argus_server:443/v10 -H "Authorization:$basicauth" -d '{"identifier" : "safepayload","data": {"year":1967,"words": ["What", "a", "Wonderful", "World"]},"ttl" : 300}' -i
HTTP/1.1 403 Forbidden
X-Midt-Server: argus
X-Midt-Version: 0.3.4
Date: Wed, 05 Aug 2020 22:13:54 GMT
Content-Length: 0
Connection: close

Ideally it would great to be able to have a directory of configuration files that are loaded and used for configuration. In this way, we can more easily separate out the configuration from the secrets.

This includes decoders, encoders as well as endpoint and handler constructors.

The client should be able to POST to store/{bucket}.

If the uuid is set then use PUT on store/{bucket}/{id}.

Our testing configuration uses dynamodb. I submitted a nested json object (512 deep) to argus. This results in a 500 error as dynamodb supports up to 32 deep nested objects.
The error returned is:
Request Failed: "ValidationException: Nesting Levels have exceeded supported limits: Attributes in the item have nested levels beyond supported limit\n\tstatus code: 400, request id: [redacted]"
dynamodb returns a 400, we should probably return a 400 to the user rather than a 500 as this is an error in the user request.

For example, these counters https://github.com/xmidt-org/argus/blob/main/store/db/metric/metrics.go#L32 can be one counter with multiple labels.

After storing an item the ttl can be negative when retrieved

1. store an item with a short ttl
   {"identifier":"payload3","data":{"qa3":"qa3"},"ttl":5}

2. query repeatedly for the item

3. the item returned can hav a negative ttl
   {"identifier":"payload3","data":{"qa3":"qa3"},"ttl":-3}

Update the Push() to take in map[string]interface{}{} as first parameter instead of webhook.W to allow other types to get stored.
https://github.com/xmidt-org/argus/blob/44bccbc01b54e93a3853df736ecec33acb89e55a/webhookclient/client.go#L175

Currently, the value coming out of the X-Midt-Owner is treated as case sensitive.

This includes doing as much clean up work of functions and structs that are no longer relevant.

Let's focus on the main three exported methods (getItems, removeItem and pushItem).

Also have to change references to the branch in .travis.yml, README, and CONTRIBUTING. Double check any other markdown files as well - sometimes links have the branch name in them.

The authorized() check allows super users to bypass a check if the item owner provided in the request matches the owner stored:

In the set endpoint, we push this item:

For normal users, the owners have to match so it's not an issue. But because super users bypass this check, it allows them to modify the owner field. My suggested fix is to check or overwrite any fields we don't want to change after getting the item already stored.

A few questions:

• If a super user provides an owner and it doesn't match the item there, do we fix the item they want to store or return a non-200 status?
• Are there any other fields that shouldn't be modified?

This includes decoders, encoders as well as endpoint and handler constructors

The code is introduced in #35

This includes decoders, encoders as well as endpoint and handler constructors.

We need to test the endpoints https://github.com/xmidt-org/argus/blob/main/store/endpoint.go

Themis might be a good example to follow: https://github.com/xmidt-org/themis/tree/main/token

There are a few cases we should add for completeness to

Argus directly returns raw errors from the underlying db. Our test configuration uses dynamodb. One testcase submitted a 512 deep nested object which exceeded dynamodb's 32 deep limit. The error header returned was:
X-Xmidt-Error: Request Failed: "ValidationException: Nesting Levels have exceeded supported limits: Attributes in the item have nested levels beyond supported limit\n\tstatus code: 400, request id: [redacted]"
We probably shouldn't return the error messages directly as we could accidentally expose important information from the underlying db (security) or other sensitive information. We would be relying on the underlying db to filter its error messages.

argus will accept many different characters as bucket names. For example \ is considered valid.
e.g
curl -H "Authorization:$basicauth" 'https://$argusserver:443/api/v1/store/\' -d '{"identifier" : "payload3","data":{"qa3":"qa3"},"ttl":300}' -i -X PUT
returns:
{"bucket":"\\","id":"tPO9_4P78gMPbmDA6_XxhJRrhIVUguLgK0Mgf0ztHrc"}

Also note that the bucket name returned is \\ instead of \
To retrieve you still need \ as in
curl -X GET -i -H "Authorization:$basicauth" 'https://$argusserver:443/api/v1/store/\'

A couple of things we could change:

• argus/model/model.go

  Line 22 in 5da527f

  // Bucket is a collection of items.

  
  "Bucket is the name of the grouping to which this item belongs" might be better

• Review comments and ensure they align with the different storage options we offer (inmem, cassandra, dynamo)

Clean up the code using this field.

Part of #78

We need to test the handlers https://github.com/xmidt-org/argus/blob/main/store/handler.go

Themis might be a good example to follow: https://github.com/xmidt-org/themis/tree/main/token where the encoders/decoders are placed in a transport.go file

argus no longer requires identifier as it should. In previous builds it was required and the documentations states that it should be.

curl -i -H "Authorization: $basicauth" https://[argus-server]:443/api/v1/store/Joel2222 -d '{"data": {"year":1967,"words": ["What", "a", "Wonderful", "World"]},"ttl" : 300}' -X PUT -H "X-Midt-Owner: joel"
HTTP/1.1 200 OK
Content-Type: application/json
X-Midt-Server: argus
X-Midt-Version: 0.3.6
Date: Tue, 06 Oct 2020 23:24:20 GMT
Content-Length: 72
Connection: close
{"bucket":"Joel2222","id":"47DEQpj8HBSa-_TImW-5JCeuQeRkm5NMpJWZG3hSuFU"}

Right now, a delete request without an owner header is able to delete resources that have an owner header. Is that what is wanted? Shouldn't it be that only the owner of the resource is allowed to delete it?

S is essentially the DAO for items being stored and so we should call it either Dao or ItemDao to be more descriptive.

Context

Currently, our services all share very similar config and setup code for authentication. However, the configuration keys are sometimes different and there's not an easy to tell how the keys relate to each other besides the comments we added to the sample yaml files. It also makes it a bit more challenging to have shared setup code that works for all different services.

Since Argus is still at an early stage, this might be a good time to try a new structure at least for all values related to authentication.

Suggested idea

The suggested structure stems from the idea that authentication can be both inbound (authenticating incoming requests) and outbound (obtaining credentials to access external services). For inbound authentication, we've mostly been working with basic and bearer types but there is no limitation in adding new ones. For outbound authentication, although it seems a bit open form, the tool or service we are trying to access often determines which credential needs to be used.

More concretely, take a look at the current and suggested structures:

Current configuration structure

Suggested structure:

Benefits:

• Our config files will be easier to visually digest and understand the relationship between different config values related to authentication. For example, one no longer needs to rely as much on the comments to understand that capabilityChecks, authHeader and jwtValidator are all part of the same authentication process.
• For inbound authentication, it might make it easier to write bascule helper methods to create validators that work across different services. It'll remove some duplicated code.

Challenges:

• The config is less flat than current one.
• It will be a big change if we wanted to propagate to all existing services. However, we could make it the standard for new services. @kristinaspring mentioned that this is also compatible with some of our newer internal services.

Once #97 is done, let's upgrade the super user feature to leverage bascule capabilities.

Chrysom, the client library, isn't mentioned in the argus readme. It would be nice if it had a readme of its own.

https://github.com/xmidt-org/httpaux/blob/main/error.go

This might save us from having to create our own structs https://github.com/xmidt-org/argus/blob/main/store/errors.go#L2 when we might not really need to.

We should validate this value before the time.Ticker uses it.

Fix in any other pages in the repo too.

Things to update:

• The codebase
• Integration test script

Part of #78

This includes decoders, encoders as well as endpoint and handler constructors

Currently, when generating a key for a push request, the identifier that the client provides is overridden by the hashed key in this line:

The user provided identifier should be saved as part of the resource. the push request object should probably include a Model.key as part of the struct instead. This way, when the user grabs all the resources or that specific resource, they can see the identifier they provided.

This includes a metric on successes and failures when polling:
https://github.com/xmidt-org/argus/blob/master/chrysom/client.go#L87

During the refactor, the instrumentation code was temporarily left out. We should add it back.

The super user feature is currently implemented through the X-Midt-Admin-Token header. That is, Argus knows whether a request is allowed to execute an operation based on whether the header value matches a configured secret.

Now, the team proposed to abandon this header approach and instead get the information from the existing authentication token (leveraging the bascule module).

At a high level, we want

• the ability to encode information in the JWT that indicates a request can run as super user (i.e. not having a capability might mean your request runs in "normal" mode while having a capability means you run as "super user").
• the ability to pass this information down to the handlers to use.

Using the current basculechecks, an AlwaysTrueCheck could be used along with having the handlers look at the capabilities to find if the request is in user mode or not but I wanted to hear your thoughts @kristinaspring @johnabass about if and how we could extend bascule to accommodate this use case.

The solution was already accepted in Talaria (xmidt-org/talaria@3db2d63) and we just need to port it over here

argus accepts json payloads with non-unique object names:
e.g.
{"identifier" : "minimal0","data": {"mini":"mouse"},"ttl":300,"identifier" : "minimal1","data": {"mini":"mouse"},"ttl":600}

which is stored as (ignore ttl decreasing value):
{"identifier":"minimal1","data":{"mini":"mouse"},"ttl":578}

It replaces the value of the first occurrence with the value of the last occurrence of the name. The json syntax doesn't require unique identifiers. The json lint validator flags this as invalid json.

It looks like this was the original handler, but now that the handlers have been split up, we don't use it. We do check if it's nil, but that doesn't seem necessary. Can we remove this field and the nil check?

1. Change UUID field name to ID: let's do this if we won't be enforcing this field to be an actual UUID.

2. Drop the Identifier field: unless there's a good reason for Argus to keep it, we should simply drop this and let users have it as part of their data payload.

3. Drop explicit use of an AdminToken header for super user feature: although the idea of a super user is still desired, the team proposed that information should be conveyed as part of the authentication token through bascule.

• JWTs: a capability could convey that information.
• Basic Auth: perhaps a configurable allowList could help.

Currently, Argus accepts json payloads with duplicate object names e.g.
{"identifier" : "minimal0","data": {"minnie":"mouse"},"ttl":300,"identifier" : "minimal1","data": {"mickey":"mouse"},"ttl":600}

This is what's stored:
{"identifier":"minimal1","data":{"mickey":"mouse","minnie":"mouse"},"ttl":572}

Non-unique names appeared to be allowed in json, but this results in inconsistent behavior. The value for identifier & ttl are replaced by the later duplicate, but data field values are merged.

@kristinaspring had the thought here https://github.com/xmidt-org/argus/pull/44/files#r490619697 that we may want to make values such as the header key for the Item Owner (currently X-Midt-Owner).

While it might be okay to leave this as is for simplicity and because Argus is a "Xmidt" product, making Argus less tied to hardcoded values related to XMiDT might make it more open source friendly.

The following should not be created:

The work here should include:

• Update to the inbound auth config structure
• Creating auth chains using uber/fx. We could start supporting the primary server.