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 ?
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...
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
Cheers, Sergey
>
>> 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 :)
>
>
>
--
Sergey Beryozkin
Talend Community Coders
http://coders.talend.com/
Blog: http://sberyozkin.blogspot.com