On 12/8/2012 5:45 PM, Sergey Beryozkin wrote:
> On 07/12/12 14:11, Bill Burke wrote:
>>
>>
>> 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.
>
> Can we restrict it .basic() only ?
>
IMO, we should support whatever the servlet spec requires: basic,
digest, client-cert, and login-form.
For Form i was thinking:
authentication().loginform(URI actionUri);
The servlet spec defines what the form parameter names should be. The
only problem is that the spec will return 200 with a error page on auth
failure. Also, the jax-rs client would have to keep track of session
cookies.
> Who even uses digest ? At least basic is a well-known mainstream scheme,
> digest is no more secure than digest without TLS and is more obscure I
> would say - so I wonder if it should promoted at the API level...
>
I agree not many use digest, but servlet spec supports Digest, so we
should too.
> Lets keep a basic shortcut for now, in terms of Authenticator.basic() only.
>
> One more note, would it make sense to somehow make setting client key
> store and non-TLS authentication mutually exclusive ? It does not make
> sense to use two-way TLS and say basic authentication, though may be it
> is not a big deal
>
I used to think the same, but it does make sense to do both two-way TLS
and basic-auth. A client-cert basically an auth of the machine you're
calling from. A password is an auth of the human that is using the
machine. So, if somebody gets your laptop, they still can't login
without your password, while the client-cert protects against MITM,
phishing, and redirect attacks.
Bill
--
Bill Burke
JBoss, a division of Red Hat
http://bill.burkecentral.com