-
-
Notifications
You must be signed in to change notification settings - Fork 15.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ability to pass along attributes via SslContext #6542
Comments
The SslContext object currently maintains a reference to the supported protocols [1]. Does it make sense to do something similar for OCSP? I'm not sure because of its "optional nature", but I'm also not sure if we need to support a generic map structure. How many other items do you envision needing (is it more than just the |
Nice, I see that you added the protocols just recently. 👍 It doesn't have to be generic map structure. If you're cool with adding an interface we can get away with something like this: interface Stapler {
Future<byte[]> get() throws Exception;
}
// only relevant for server contexts!
class OpenSslServerContext {
volatile Stapler stapler;
Stapler getStapler() {
return stapler;
}
void setStapler(Stapler stapler) {
this.stapler = stapler;
}
}
SslHandler sslHandler = sslContext.newHandler(ctx.alloc());
ReferenceCountedOpenSslEngine engine = (ReferenceCountedOpenSslEngine) sslHandler.engine();
Stapler stapler = sslContext.getStapler();
if (stapler != null) {
Future<byte[]> future = stapler.get();
if (future.isSuccess()) {
engine.setOcspResponse(future.getNow());
}
} The important thing and why it's mutable and returns a Future is that there are a few strategies for obtaining staples and more importantly managing them. A push based approach is simpler but possibly not viable because SslContext instances can outlive the validity of a staple (iirc the RFC says max of 10 days). A pull based approach is more involved but IMHO the way to go in a real production environment (at least that's what we do). |
Actually, I'm not sure about that snippet either. I think it carries too much biz logic weight and it's possibly awkward API-wise. It'd be nice if An another idea could be to break the direct dependence on interface SslContextHandle {
SslContext sslContext();
}
// Default behavior, user may implement their own
class DefaultSslContextHandle implements SslContextHandle {
private final SslContext sslContext;
// ctor omitted
@Override
public SslContext sslContext() {
return sslContext;
}
}
class SniHandler {
public SniHandler(DomainNameMapping<? extends SslContextHandle> mapping) {
}
} |
Can you elaborate on your concerns with using delegation? Is it primarily a general concern of "what if someone adds a method that I should override in the future, or overrides something they don't today and changes behavior"? It would be good to understand the specific concerns (you also mentioned reference counting was complicating things). Ideally we could avoid adding a new API, but maybe this is the best approach (not sure yet). |
Yes that would be my primary concern of using delegation that non-abstract methods get added and the user won't know about it until things start failing at runtime. The other problem is that... I.e. my delegate becomes this monstrosity of extending, overriding and implementing various interfaces of downstream objects just to cover the special needs of every actual implementation (JDK, OPENSSL, ...). In that sense I prefer interfaces because it allows a duck typing approach and keeps inheritance low. |
Do you have a proposal to introduce a interface(s) that will not break APIs? Wouldn't you also need to have special interfaces/implementations for the different types because they support different methods? |
The name interface ISslContext {
boolean isServer();
// ...
}
class SslContext implements ISslContext {
}
class SslContextBuilder {
public ISslContext build() {
}
}
class SniHandler {
SniHandler(AsyncMapping<? super String, ? extends ISslContext> mapping) {
}
} That should work without breaking the API?! |
Just to clarify the issue seems to be two fold.
Having the interfaces solves problem (1) (for public methods). However this doesn't seem to solve (2), right? You would still need different interfaces for each of the concrete types. An alternative approach to help mitigate (1) would be unit tests & reflection (does your delegate have the same methods as the concrete type). This is hacky and not saying its ideal but maybe a viable alternative to maintaining an new set of interfaces (plus the publicly visible classes). Either way for the next API breaking release we should consider exposing interfaces instead of concrete classes here. |
Correct regards 1 and 2. It doesn't solve (2) but it's IMHO more favorable to use interfaces than classes due to Java's lack of multiple inheritance. Technically speaking, my delegate is currently broken because it's only extending and overriding With that said and I'm still +1 on favoring interfaces. I think SniHandler's (using as an example) direct dependence on SslContext is possibly wrong/not-favorable. My first example (the |
I'm not necessarily against Interfaces and if we could break APIs (or starting from scratch) it would be an easier decision. I just wanted to explore alternatives and understand the motivation before supporting a new set of interfaces. My primary hesitations would be additional maintenance overhead (relatively minimal I hope), and extra interfaces which may confuse folks given the current concrete classes being exposed and widely used. Based upon the discussion I think I'm most in favor of the |
+1, I'm also in favor of the supplier approach and would solve my problems elegantly. I'll take a stab at it and see where it goes. |
Motivation There are use-cases (in particular with SniHandler+AsyncMapper) where it's desireable to be able to return more than just the SslContext from the mapper. It could be configuration data, debug information etc. The only way to do that right now is to employ a delegation pattern for SslContext but it's very maintanace intensive and always subject to breakage. Modifications Introduce a generic Supplier interface (may be replaced by JDK8's own version at a future point) and let SniHandler depend on Future<Supplier<SslContext>> instead of Future<SslContext>. The user can implement a custom Supplier that can be used to pass arbitary data beyond the SslContext itself into SniHandler. Result Fixes netty#6542
Motivation: Sometimes it is useful to be able to set attributes on a SslContext. Modifications: Add new method that will return a AttributeMap that is tied to a SslContext instance Result: Fixes #6542.
Motivation: Sometimes it is useful to be able to set attributes on a SslContext. Modifications: Add new method that will return a AttributeMap that is tied to a SslContext instance Result: Fixes #6542.
Motivation: Sometimes it is useful to be able to set attributes on a SslContext. Modifications: Add new method that will return a AttributeMap that is tied to a SslContext instance Result: Fixes #6542.
This is related to the #6158 OCSP pull request.
The #6158 PR is demonstrating the most simple way of obtaining and dealing with OCSP stapling but it gets a bit more complicated in practice. In particular when you look at how
SniHandler
andAsyncMapping
work (which we use).AsyncMapping
returnsFuture<SslContext>
SniHandler
usesFuture<SslContext>
to constructSslEngine
+SslHandler
... but how do I pass along the info that is needed to obtain a staple or the OCSP staple itself? I'm currently using the delegate pattern around
SslContext
but it gets quickly rather messy and error prone in conjunction with reference counting. It'd be great if it was possible to pass along arbitrary attributes viaSslContext
(e.g. viaAttributeMap
or simply as aMap<Object, Object>
).A more general way of looking at is the desire to pass configuration parameters from
SslContext
to "things" that use it. One scenario is described above but other use-cases could be other config parameters for theSslEngine
such assetEnabledProtocols()
or some custom behavior for theSslHandler
etc etc.Here's some pseudo code to illustrate it. WDYT - happy to PR it.
The text was updated successfully, but these errors were encountered: