oxide-auth
oxide-auth copied to clipboard
HeroicKatora
Gate dependencies behind feature flags
This PR gates the following dependencies behind feature flags:
-
rust-argon2 -
rmp-serde
These dependencies are less common dependencies for some web services, since:
a) not everyone uses rust-argon2 (personally, I use the RustCrypto argon2 crate for pretty much all my projects)
b) not everyone uses MessagePack in their projects
In total, this makes the dependency tree 16 dependencies lighter with all features disabled.
Whether these features should be enabled by default or not can definitely be discussed.
All the features are documented on docs.rs using the unstable doc_auto_cfg feature
I license past and future contributions under the dual MIT/Apache-2.0 license, allowing licensees to chose either at their option.
Makes sense. Will try to refactor towards generalizing the implementations over different inner implementations.
Okay, the abstraction over different de-/serializers is.. a mess. Unless we pull in erased_serde to erase the actual types of the machinery inside using some dynamic dispatch and a box.
The dispatch need not be byte-compatible with previous version, imo, since in-memory data should be treated as ephemeral. Well, any new design could be validated by trying to swap the current encoding scheme for something like BER or even JWT but those can always live as separate implementations as well. This opens the possibility of having a public wrapper type (with private fields, implementing Serialize) which the policy is to turn into a byte vector. Or rather two methods since there are two different instances of encoding happening. The AssertGrant type codifies essentially already one of those two, the other is currently ad-hoc tuples. Both could be public types, without public constructors.
Something like that? @HeroicKatora
That's a fully opaque representation that publicly implements Deserialize and Serialize and doesn't expose any other information to the outside.
Explicitly adds a disclaimer to the documentation that the internal representation is unstable and might change at any time.
The internal representation at the moment is just an enum.
The only problem here are auto-traits, i.e. Send and Sync should still work for the Assertion
Rewrote the internals to dynamically dispatch instead. Works fine, added the auto traits as a requirement to the trait itself, so that's still fine.
Fyi, the bulk of this PR looks good. But, from the perspective of secure defaults, I don't yet feel comfortable with an interface doesnt provide some secure configuration of all interfaces for users by default. That means, there should be an easy choice and that one should be secure. In particular, new with a parameter seems off. It's congruent with that idiom if the new interface is not available in all configurations so long as it is available in the default one.
The serializer is not as relevant but still I'm not sold on the idea of making it more difficult to most people (I'm basing this on the fact that this is the first time that issue came up) and not having at least an optional dependency that fully implements the trait. Or, at the very least, there should be good guidance within the trait documentation on how to bind it to a generic serialization library.
Just some thoughts here, and why I'm not convinced yet that this PR is ready.
@HeroicKatora Well, do you think it would be sufficient if we provided a Default implementation (even if gated behind a cfg flag) that uses these secure defaults?
Such as the interfaces that use the argon2 password policy, they get a default impl.
And, of course, the Encoder also gets a default impl gated behind something like messagepack.
And all these features could be enabled by default to make the default experience secure by default.
@HeroicKatora Hey, sorry for the delay, I wanted to ask if what I proposed in the previous comment would be fine?
Meaning an API where the default features expose these secure defaults?
The approach is decent with the trait factoring, but I can't quite get over the modification to new and other constructors instead of a with_ method and new being dependent on (non-default) features. In particular note how that introduces an apparently necessity for (or maybe draw to) an improper implementation of the password store in tests and associated churn. Knowing users, this might well be the place used as a template for customized versions. And if this is motivated by being 'just simple' then I can definitely see real cases of it happening in Production where it is a huge security hole. The changes you've made since the initial version do contribute to security-by-default but I'm not fully convinced that the PR overall is positive. It's close though.
Do note that tests can have a dev-dependency on the encoding / hashing crates, maybe that is a direction to use?