On 12/7/2012 5:46 AM, Marek Potociar wrote:
> Hi Bill,
> Thank you for writing this up. It's been sitting on my plate for too long.This is a great starting point. A few thoughts/questions:
>
I'm open to anything here as I'm not sure what the optimal solution is yet.
> IIUC, you suggest we re-introduce a "client builder" concept that would be represented by ClientFactory. I'm fine with that.
Ya, I'm sorry you had something similar way back when and I fought
against it. I fought mainly because we had no configuration to build
the client with.
> With KeyStore/TrustManager ClientFactory builder methods in place, should we expose similar methods via client-side configuration? Or does the proposal mean that users would not be able to modify keystore/truststore etc. on any other level than Client (i.e. not in WebTarget or Invocation.Builder)? Are there any common use cases that we would be ignoring? IOW, do you see a need to change keystore/truststore configuration for different resource targets? Also, I think we need to expose a method that would accept a custom SecureRandom instance.
>
I don't know about Jersey, but Resteasy's client defaults to Apache Http
Client 4.x and it is by far the most popular. The reason I put this
stuff in a ClientBuilder is because of how Apache Http Client works.
You need the truststore and client-cert keystore available when you
create a SocketFactory in AHC 4.1. If we allowed SSL configuration at
the Client and WebTarget level, you'd have to manage the lifecycle of
multiple HttpClients (and socket factories). java.io.* implementations
pretty much have a one-to-one relationship between stores and
SocketFactorys. I don't think java.nio.* has the same problems, but I
haven't done a lot of SSL + NIO.
Also, I was thinking of having these additional methods:
Probably need a way to set properties on the Client builder/factory.
property(String name, Object value);
disableTrustManager(). Basically will not do cert checks for HTTPS
calls. This is useful when testing or for clients that don't care about
trust. IMO, though, trust should not be disabled by default as
disabling it is a bad thing usually.
hostNameVerification(HostNameVerificationPolicy verification);
enum HostNameVerificationPolicy
{
ANY,
BROWSER, // default
STRICT
}
This checks the CN attribute of the X509PrincipalName of the server's
CERT to make sure it is the host you are actually querying. ANY will
not do the check, BROWSER allows wildcards *.foo.com. Strict requires
the CN to be exactly the same as the requested host.
> As for the Authentication, I cannot really decide. On one hand I am inclined to prefer a composition-based solution, e.g. something like
>
> target.register(new BasicAuthFilter(user, pwd)) // or BasicAuthFeature....
>
Again, Resteasy at least is based on Apache HTTP Client. While
BasicAuth is very easy to implement, Digest is not as trivial so we
delegate to Apache HTTP Client for the implementation of this.
We could also just ditch authentication() and have defined properties:
webTarget.configuration().setProperty(SecurityProperties.USERNAME,
"bburke");
webTarget.configuration().setProperty(SecurityProperties.PASSWORD,
"geheim");
In this case, we require client implementations to support basic and
digest auth. How they implement it is up to them.
> but at the same time, your proposal works well with the fluent API and with careful javadocing can be extensible in the future too. The disadvantage of the above is also the need to provide the implementation in the spec and fixing any bugs can become a nightmare after we release. So I guess I would like to hear from others what they think before I make up my mind.
>
For authentication, is there a possibility of a bug? Basic and Digest
auth just need username/password and thats basically all we're doing at
the moment.
> The one problem I'd like to point out though is that it is a big security hole to force users to represent passwords as plain Strings. Strings in Java are internalized and are not garbage-collected in the same way as other objects. They may stay in the heap long after they're not used. A memory dump may thus reveal passwords stored as strings. I guess it's hard to completely avoid this issue, but I was thinking whether we should also expose overloaded methods that take e.g. byte[] or CharSequence as a password input for users who want to avoid working with plain-string passwords in their code.
>
FYI, The above argument is for passwords that will only temporarily be
accessed. The idea is, if you provide a char[] array you can zero out
the array after you're done using the password, *hopefully* before
somebody does a memory dump (or the OS does a SWAP). This doesn't work
so well when the password has to remain in memory so that subsequent
requests can use it :)
--
Bill Burke
JBoss, a division of Red Hat
http://bill.burkecentral.com