Netflix / msl

Message Security Layer
Apache License 2.0
714 stars 79 forks source link

Provide data to configuration singletons #67

Closed wmiaw closed 8 years ago

wmiaw commented 8 years ago

There is a desire to provide some sort of non-global / constant data to the execution of what are otherwise singletons off MslContext: authentication factories, keyx factories, token factory, MSL store, etc.

Passing some sort of random Object throughout the entire MSL code base into every possible configuration implementation function seems both incredibly messy, wrong, and error prone in the same sense as globals. Additionally there's no guarantee the Object data would be thread-safe, neither of which is currently a requirement on the MslContext, MessageContext, and other interfaces. Sticking something on MessageContext would fundamentally change MessageContext which can currently be treated as a stateless data resource.

This request requires further thought before deciding it is a good idea and if so then how to best approach it.

wmiaw commented 8 years ago

After further thought and discussion, it is probably a bad idea to try and support this via code changes to the MSL core logic and interface changes (the random Object referred to above). Such an arbitrary state container would need to be provided as an input parameter on just about every function across the entire code base, from ICryptoContext.encrypt() to KeyExchangeFactory.generateResponse() to MslStore.getNewestMasterToken(). Likewise, repurposing MessageContext to serve this purpose would be equivalent to adding this new random data container.

There are several potential solutions that can support the use case described:

  1. ThreadLocal containers.
  2. Scoped data containers: e.g. set value X before calling into MslControl and make X available to the code that needs it.
  3. Scoped configuration implementation: e.g. provide a different EntityAuthenticationFactory or MslContext for each call into MslControl.

For the specific use cases described (externally) the third choice is probably what I would recommend. This allows multiple MslControl operations to be processed on a single thread, and still allows stateless singletons to be used whenever possible. It is also likely to have the smallest memory and processing footprint. It will work for trusted network servers without any problem as trusted network server MslContext and MslStore instances do not need to be persistent, shared, or store state.

quidryan commented 8 years ago

Options 1-3 aren't practical options. ThreadLocals are error-prone and impossible in a Reactive world. Setting value X before calling MslControl implies static values or static-equivalents when the schemes/factories are singletons. Creating factories on each call is might be possible, but it's onerous on the caller and breaks the model of having the Factories be exactly that, factories. We're using Guice to wire together all these objects because they are singletons by all accounts. They're singletons for a reason, they're meant to be stateless. Changing all factory/schemes to be non-singleton will make them more complicated and harder to test than they should be.

I think you're mischaracterizing what I'm asking for. I'm looking to be able to plumb context specific to the message through the system. MessageContext is a bit of misnomer in that it's used to construct a message instead of provide context about the message being received. The idea of plumbing request specific context is prevalent in Netflix code, it's a critical part of building production code. It's necessary for features like Dapper, internally called Salp, and request specific logging.

I'm not implying the implementation, MessageContext doesn't have to be how the data is plumbed through the system. But being able to plumb data through the system is a needed feature.

I'm not sure what you mean by a Random object, we'd be very explicit in creating this object and checking its values. It sounds like the problem is it "would need to be provided as an input parameter on just about every function across the entire code base". What's the problem with that?

wmiaw commented 8 years ago

I don't think adding "Object data" to every function signature in the MSL code base makes sense, to support operations that MSL does not care about or carry data that MSL has no need for. I am instead asking you to do something similar to other libraries where the "Object data" is provided externally to facilitate your logic. I don't think it is absolutely necessary to plumb the data through the entire code base to achieve your goals.

You could keep all of your auth + keyx factories singletons, and your token factory etc. singletons, but reference them from MslContext instances and then pull your "Object data" off of the MslContext. Or keep your base logic for each of the "singletons" as part of a base class or static class members, while providing instance members for the individual data.

On API, I think you currently use the Integration Context or similar things to accomplish this. For the backend services like MembershipClient, the APIs haven't been defined to so they all accept an arbitrary data container parameter that might be then used by some other service or implementation behind MembershipClient.