Allow access plugins to store per-request state with the auth server.

[fspmarshall]

Many access plugin implementations will end up needing to store small amounts of per-request information (e.g. API callbacks, tokens, timestamps, and the like). In order to support making plugins stateless, we can allow them to store arbitrary data against individual access requests.

Layout

Because multiple plugins may be running simultaneously, its important that some form of "namespacing" exist to ensure that plugins don't accidentally overwrite one-another's data. Some obvious potential mechanisms are:

1. Use a mapping of the form plugin-name -> blob. Each plugin can store an arbitrary blob with any relevant data in its assigned slot.

   • Pros: Super simple!
   • Cons: Plugins must rewrite all of their state to update one element.

2. Use a double-layered mapping (or prefixing convention) of the form plugin-name -> key -> blob. Each plugin can update keys within its mapping individually.

   • Pros: Plugins can individually update subsets of their state.
   • Cons: More complexity; benefit is potentially marginal.

I'm leaning towards 1 right now, mostly because I think it plays better with the operations described below:

Operations

Given that most plugins will leverage some amount of parallelism, its important that plugin authors have tools to protect themselves from getting their plugins into a bad state due to concurrent writes:

• API should distinguish between create and upsert operations so that authors don't accidentally overwrite unexpected state.

• API should allow authors to protect themselves from concurrent writes (e.g. via compare-and-swap).

Sketch

This is what I'm leaning towards right now:

metadata:
  kind: access_request
  # ...
spec:
  user: alice
  roles: [dictator]
  # add the `ext_data` field with a mapping of string -> string.
  ext_data:
    my-plugin: '{"data":[1,2,3]}'
  # ...

// example call demonstrating a plugin updating its stored state from
// `{"data":[1,2,3]}` to `{"data":[1,2,3,4,5]}`.
client.SetAccessRequestExtData(ExtDataSetter {
    RequestID: "abc123",
    Name: "my-plugin",
    Value: `{"data":[1,2,3,4,5]}`,  // Value deletes if nil
    Expect: `{"data":[1,2,3]}`,     // Expect states expected previous value for compare-and-swap
    Upsert: true,                   // Upsert toggles whether this can overwrite existing data
})

[fspmarshall]

ping: @marshall-lee

[klizhentas]

I agree with compare and swap approach, option 2 is a bit simpler, because allows to have keys/value stored easily, I wonder if we can make it easy to store primitive types of GRPC as well? E.g. what data will be stored by slack plugin initially?

Storing blobs will promote folks to think about all serialization/versioning etc, I wonder if we can make simple things simple here

Regarding method, is it possible to reuse SetAccessRequestData vs having a separate method, what are pros cons?

[fspmarshall]

I wonder if we can make it easy to store primitive types of GRPC as well?

There is google.protobuf.Struct which can represent any valid JSON. Between that and jsonpb, theres probably a lot we could do. I'll give it some thought.

That being said, I'm not sure if there is a significant benefit to supporting GRPC types over, say, providing a helper method that simply marshals/unmarshals plugin data from JSON.

what data will be stored by slack plugin initially?

Currently, just two string values that are required for later callbacks.

is it possible to reuse SetAccessRequestData vs having a separate method, what are pros cons?

There is no SetAccessRequestData, I just added the Ext as an indicator that this is an "extension" field. This field was initially discussed as UserData but I felt that would be slightly confusing given the fact that AccessRequest already has a User field whose functionality is orthogonal to this feature.

---

Idea:

In the interest of making this as simple as possible (at least for golang devs), we could provide helper methods which automatically marshal/unmarshal and use an opaque token to manage compare-and-swap or optimistic locking internally (optimistic locking probably makes more sense in this case). Something like this:

var data MyData

// unmarshal data from request, returns a correctly configured "update token"
token, err := req.UnmarshalExtData("my-plugin", &data)
if err != nil {
    return err
}

// if there was no pre-existing state, the token can encode that too
if token.IsInit() {
    data = NewMyData()
}

// make some changes to data...

// send updated data to auth server, and get back a "fresh" update token
token, err = client.UpdateExtData(token, &data)
if err != nil {
    return err
}

// token is just data... can be used across multiple client connections
token, err = client2.UpdateExtData(token, &data)
// ...

Downside of this is that it effectively pushes plugins towards a very specific workflow when it comes to dealing with their data... upside is that its pretty damn simple. Pass around your token to protect yourself from nasty surprises... if your update fails, reload your state and try again.

[marshall-lee]

client.SetAccessRequestExtData(ExtDataSetter {
    RequestID: "abc123",
    Name: "my-plugin",
    Value: `{"data":[1,2,3,4,5]}`,  // Value deletes if nil

Name name is confusing. It looks like I'm updating attribute named "my-plugin".

[marshall-lee]

Currently, for slackbot both approaches are good enough.

Lets go with a simpler blobby solution. Anyway safe partial updates, versioning, etc could be expressed on top of that.

[marshall-lee]

What's data type will be used for blob? Is it a string or []byte? I proposed a byte array, since someone would want to store something non-json. Though it will require base64 serialization of ext data when representing access request in YAML so I don't know if it's really worth.

[klizhentas]

The idea with token is not very clear to me, the previous version with just a setter is much easier as it is only one step to understand vs having tokens and some optimistic locking, I suggest we go back to compare and swap and then just make it easy to add key value string pairs

[fspmarshall]

Alright, based on the above feedback, I think I've got a design in mind that walks a good balance:

Have ext_data be a mapping of the form namespace -> key -> val (i.e. map[string]map[string]string).

This is our new ExtDataSetter:

type ExtDataSetter struct {
    RequestID string
    Namespace string
    Set map[string]string
    Expect map[string]string
}

The Set mapping indicates what keys should be set to what values within the provided namespace. If Set includes keys with empty ("") values, those keys are removed. Keys that don't appear in Set are unmodified (making this like a shallow patch operation).

The Expect mapping is optional, and allows for per-key compare-and-swap style logic. Meaning you can do something like { Set: [count:2], Expect: [count:1], ... } which will cause count to be updated to 2 if and only if it is currently 1. We can also expect a field to not exist by explicitly providing an empty value ("").

Ex:

// An update with no `Expect` value behaves like a normal upsert.
client.UpdateExtData(ExtDataSetter {
    RequestID: req.ID,
    Namespace: config.Namespace,
    Set: map[string]string {
        "hello":"world",
        "spam":"eggs",
    }
})

// Delete values, or expect fields to not exist using ""
client.UpdateExtData(ExtDataSetter {
    RequestID: req.ID,
    Namespace: config.Namespace,
    Set: map[string]string {
        "unwanted-key":"",
    },
    Expect: map[string]string {
        "unexpected-key":"",
    }
})

// Treat it like a patch + optimistic locking system if you want to!
client.UpdateExtData(ExtDataSetter {
    RequestID: req.ID,
    Namespace: config.Namespace,
    Set: map[string]string {
        "spam":"eggs",
        "version":"3",
    },
    Expect: map[string]string {
        "version":"2",
    }
})

[klizhentas]

Looks beautiful, however more polish on the naming of ExtDataSetter and UpdateExtData is required - I find ext and setter especially unclear, can you find a form that makes perfect sense without those predicates?

[fspmarshall]

I find ext and setter especially unclear, can you find a form that makes perfect sense without those predicates?

How about something like this:

client.UpdateRequestExtension(ExtensionUpdateParams {
    // ...
})

I'm not sure this achieves perfect sense, but I think its closer to the mark. Extension is definitely less jargon-y than ExtData, and I think UpdateParams gives a pretty good intuition that the struct is going to hold parameters used by some kind of update operation (as opposed to Setter which is definitely a lot more generic).

An alternative to the Extension terminology is to use something like Attributes, but I find that naming slightly less intuitive. I think Extension better communicates the fact that this is an "add-on" and not necessary something central to how teleport processes access requests (though perhaps Extension creates a false expectation that it is extending/expanding the scope of the access request... I'm not sure).

[klizhentas]

sorry for all the exchange, I think this method is quite important though as all plugins will be using it. I think to make it more user friendly we have to de-generalize it a bit. In essence, what we are doing is setting plugin metadata? Can we reflect that in the name somehow?

[fspmarshall]

Haha, no problem. I have a real issue erring on the side of generalization, but you're right...

How about PluginData:

client.UpdateRequestPluginData(PluginDataUpdateParams {
    // ...
}) 

This feels pretty on the nose. Its the per-request data that belongs to the plugin.

EDIT: In this case, it might be worth changing the Namespace parameter to Plugin or PluginName as well...

[gravitational-jenkins]

perfect!

…

On Fri, Jan 17, 2020 at 5:40 PM Forrest Marshall ***@***.***> wrote: Haha, no problem. I have a real issue erring on the side of generalization, but you're right... How about PluginData: client.UpdateRequestPluginData(PluginDataUpdateParams { // ... }) This feels pretty on the nose. Its the data that belongs to the plugin. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3286?email_source=notifications&email_token=AD5GI6WJIOBQPLVSFM5GCWLQ6JMZHA5CNFSM4KH2FNY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJJNKDY#issuecomment-575853839>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD5GI6SLIJEBW45I2WNNJ5DQ6JMZHANCNFSM4KH2FNYQ> . -- You received this message because you are subscribed to the Google Groups "Tech Operations" group. To unsubscribe from this group and stop receiving emails from it, send an email to ***@***.***

[klizhentas]

@fspmarshall looks good to me, also agree on updating the Namespace to Plugin. Here is how I imagine plugin developers would think:

"Hey, how do I set plugin data?.... Ctrl-F -> UpdateRequestPluginData`

[marshall-lee]

@fspmarshall Perhaps rename Set: and Expect: to New: and Old: accordingly? They're the same length and also New: says explicetely that all the old value will be overwritten.

And Set: could be for raw partial update without compare-and-swap if someone would need it.

[klizhentas]

Expect communicates the fact that it's an expected value, prerequisite, so I would keep Set and Expect

[fspmarshall]

Added in #3295