Date   

Re: FYI, hashalgorithm branch locked

Arjan Tijms
 

Ok, thanks for the update Will!

Kind regards,
Arjan Tijms


FYI, hashalgorithm branch locked

Will Hopkins
 

I merged the hashalgorithm branch into master last night, for both the security-api and security-soteria repos, as we were basically finished with that project and I needed to publish matching artifacts for both API and RI.

The hashalgorithm branch is therefore not needed any more, so I've protected it to avoid any inadvertent submissions to it. There were to commits after I merged to master, those are now also in master.
-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803


Re: [jsr375-experts] EDR1 Review

Will Hopkins
 

Hi Darren,

Sorry for not resonding to this sooner, my filter was no longer set up to find "[jsr375-experts]" email ...

Will

On 07/06/2017 06:05 PM, Darran Lofthouse wrote:
On Thu, 6 Jul 2017 at 19:29 Will Hopkins <will.hopkins@...> wrote:
Hi Darren,

FYI, here are my responses to your original set of comments, which I incorporated/addressed addressed as described below in the PRD draft.

I thought I had sent this out earlier, but just found it sitting in my drafts folder.

Regards,

Will


On 03/20/2017 03:16 PM, Darran Lofthouse wrote:
Just having a pass through EDR1 and here are some comments so far.

* Section 2.1 *

The callers proof of identity is being described as a token or credential, I think it may be more accurate to somehow include response to a challenge in there.  As an example with HTTP Digest, the response is not a credential and I think describing it as a token is pushing the meaning of token.

I agree with your point that a challenge response isn't precisely a token or credential, but that particular passage (if I've got the right one), is meant to be descriptive in a general way, not prescriptive or limiting. I think trying to insert "response to a challenge" into the sentence would break up the flow of the paragraph and potentially make it less understandable.

Going through the later spec version I think this is now described differently in two locations, the first definition seems to be about credentials but the later description does go into more details about responses to challenges.
Do you think that's sufficient, or is there still a change you'd like to see (I plan to publish a PFD update shortly)?



* HttpAuthenticationMechanism *

I have raised this previously but I still think by including both request validation and challenge generation in a single method there is a missing option to support multiple authentication mechanisms concurrently - a common example I see is CLIENT_CERT with SPNEGO with one or more form of username /password authentication.
I wasn't involved in the previous discussions, and at this point in the JSR I don't think big changes are viable. That said, I think that aspect of HAMs largely derives from the way JASPIC works -- there's a single Server Auth Module invoked for any given request. The solution, as for JASPIC, would be to write a wrapper HAM that could invoke on a set of HAMs until one succeeded, remembering which one succeeded in the case that a challenge was issued (so that the correct HAM could be invoked to respond to the challenge). I agree that's not a completely satisfying solution, though.

The situation however is slightly more complex  - we would need multiple HAMs to be able to send a response concurrently - when handling the response in the event of a success we would need to clear any responses sent by the other HAMs.  Maybe wrapping the HttpServletResponse could be sufficient to cope with this.
It's inherently complex to support multiple methods that each require challenge/response, I think. For typical cases like BASIC and CERT, it's mostly a matter of choosing among credentials that may already be in the request, with perhaps one option that involves a challenge. But I think your suggestion of wrapping Request/Response is viable.


Outside of this we have needed to solve a number of additional issues related to HTTP authentication such as unsolicited authentication, caching of an authentication result without undermining the mechanism, having the authenticated available when unsecured resources subsequently accessed.
I may not completely understand all the implications of what you're suggesting, but HAMs should be able to handle unsolicited credentials. Not sure I understand the caching scenario. Having a previous authentication available when accessing unsecured resources I think is a session management problem, and the JSR doesn't really address session management, but I agree that setting up session cookies such that they are both secure and available to unsecured resources is both tricky and necessary.

I think the main point being that with the current API the HAM needs to be called for all resources so it can handle it's own caching.  The main point it deciding if a challenge needs to be sent of if a request can be allowed through without authentication.
I think JASPIC specifies that SAMs are called for every request (whether the resource is protected or not). JASPIC is really a message processing protocol that can perform authentication, rather than an authentication mechanism with a concept of protected and non-protected resources.



* Identity Store *

We have been working through some similar issues with the WildFly Elytron project we are currently integrating into WildFly.  So that we can support many different authentication mechanisms we have reached the point where our equivalent of an identity store can be used to both obtain a credential representation that a mechanism can use to verify the response that it has received and an alternative approach where a mechanism can pass in evidence for verification.

Looking at the current proposed API for IdentityStore I am not so sure it is a credential being verified, rather it is verifying either a response to a challenge or a token.

That first comment may be more about relating the API to terminology but the big risk with this is that the implementation details from the authentication mechanism now start to leak into the identity store implementation.  So in addition to being able to interact with the store of identities the store now needs to be aware of the requirements of different mechanisms if it was to be re-usable.

Related to this, as an application server vendor - to provide a default implementation of this interface I would assume a default set of supported credential types would need to be listed somewhere?

I think the most common case for IdentityStore will be verifying a UsernamePasswordCredential, as it's explicity dealing with a user store. IdentityStores can't really deal with challenge/response protocols; that would be handled by a HAM, which would invoke on the IdentityStore{Handler} only at the point that the dialog with the client is finished and the final token or credential that identifies the user is available.

I think as the IdentityStore is optional this can be handled. 
 
The only required credential to support would be UsernamePasswordCredential.
OK.


I think it feels to me the IdentityStore is something you would use just for clear text password verification, once your HAM moves on to more advanced mechanisms then it becomes more appropriate to inject something else.
Agree.

In my mind the distinction isn't between password vs. other (e.g., certificate) types of credentials, per se, it's the distinction between a credential that proves my identity, to the satisfaction of the identity provider with whom I have an account, and a token provided by some third party that does not, in itself, provide direct proof of my identity.

Credential validation as performed by an identity store necessarily involves me proving my identity to the store, and therefore requires a type of credential that can validate my identity, like a password or a 2FA token.

In contrast, a SAML assertion, or OAuth2 token, or even a kerberos TGT, is essentially an assertion by a third party that I have proved my identity to *them*, and the token presented contains no direct proof of my identity. Acceptance of the token is based on a trust relationship with the third party, and the proof element (signature, etc.) relates to proving the identity of the third party -- not me -- and the integrity of the token itself.


I will have another pass through the APIs but these are some of the main points that jumped out so far related to issues that we have needed to work through ourselves.

Thanks, Darran -- appreciate you taking the time to look at this.



Regards,
Darran Lofthouse.



-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803

-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803


Re: JetBrains s.r.o. feedback?

Will Hopkins
 

FYI, I heard back this morning from Trisha Gee. She didn't have any more detailed feedback, but said their working group was much happier with the updated spec and that she'd poke them and let us know if they had any further comments/concerns.

On 07/23/2017 05:18 PM, Will Hopkins wrote:
I wrote directly to their EC member, Trish Gee, right after the ballot concluded. I gave her a copy of the PFD spec, which I hoped would address some of their concerns, explained why we had chosen not to do OpenID Connect/OAuth2, and asked for a more detailed breakdown of their concerns.

She did respond, and said the concerns had been raised by an internal working group, and that she'd forward the updated spec to them and follow up to make sure we got more detailed feedback. I haven't heard anything further from them, but I just now pinged her to see if there was any further feedback.

There's not much we can do about the fact that we didn't to OIDC/OAuth2, but I'm optimistic that the recent updates will address most of their other concerns.

Will

On 07/23/2017 06:18 AM, Arjan Tijms wrote:
Hi,

In their voting comment for the Public Review Ballot, JetBrains s.r.o. mentioned working on a detailed response. We're about to post the Proposed Final Draft 2, but I haven't heard from them. Any concerns they may have will be increasingly hard to address at this point I think.

Has anyone seen their response or know what it's about other than what was mentioned in the comments?

Werner, Ivar, have you heard of anything?

Kind regards,
Arjan Tijms

-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803

-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803


Discovered tiny omission in @RememberMe

Arjan Tijms
 

Hi,

After going through the API one more time I just noticed that the @RememberMe annotation is missing one attribute, namely "isRememberMe" as typed version of "isRememberMeExpression".

Normally all non-string attributes have the XYZ and XYZExpression pairs, but here there's only the expression variant. It's easy to add though. Shall I add it?

Kind regards,
Arjan Tijms


Re: PasswordHash interface changes

Arjan Tijms
 

Hi Will, 

Thx for your changes. I'll look into my changes now, but they should be fairly trivial.

Kind regards,
Arjan Tijms


Re: PasswordHash interface changes

Will Hopkins
 

Arjan,

Signature changes are pushed to hashalgorithm branch in security-soteria.

I'll start implementing properties support for Pbkdf2PasswordHashImpl, but you should be able to make all your changes without waiting for any further updates, as long as you don't need the properties to do anything (they'll be ignored until I finish adding support for them).

Will

On 07/24/2017 04:57 PM, Will Hopkins wrote:
Hi Arjan,

I've pushed the interface change to the hashalgorithm branch in security-api. If you build that you should be able to start making the soteria changes to initialize with properties.

I'm going to update the two PasswordHash impl classes now, will push the signature change first so you can build, will let you know when the internal properties support is done.

Will
-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803

-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803


PasswordHash interface changes

Will Hopkins
 

Hi Arjan,

I've pushed the interface change to the hashalgorithm branch in security-api. If you build that you should be able to start making the soteria changes to initialize with properties.

I'm going to update the two PasswordHash impl classes now, will push the signature change first so you can build, will let you know when the internal properties support is done.

Will
-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803


Re: LDAP Annotation and Database Hashing Proposal

Guillermo González de Agüero
 

I see two options for the "one instance per identity store":
- Dynamically creating a qualifier for each instance. It is the more CDIish way and would make classes available for decoration, etc.
- Using the CDI Unmanaged class to make injection work on an arbitraty class. This is the way that most aligns with your "new Object()" proposal. Decoration and interception would work on those classes, and they would have no context (they'd be unmanaged for CDI) but that additional features might be provided as a vendor extension. A great article on the subject: http://www.next-presso.com/2017/06/non-contextual-instances-in-cdi/

For this time, I think just saying that "injection should work" at spec level should be enough. Not sure what Arjan will think though. Soteria could definitely provide them as managed beans.

For deferred EL expressions, I too think they don't need to be mandated. I even doubt how that feature should work, as I'd expect two subsequent calls to generate and verify methods to use the same values, which could not be the case for deferred expressions.

Btw, I see in your PR that you finally used classes instead of names to specify the algorithm to use. Weren't you working on a String version?


Regards,

Guillermo González de Agüero

El lun., 24 de julio de 2017 19:50, Will Hopkins <will.hopkins@...> escribió:
Not sure I care that much about the details of the implementation, although I'm starting to feel like CDI for this use case may be making things more complicated, not less, relative to simply newing up an instance of the type specified in the hashAlgorithm attribute and passing hashProperties to the constructor ...

What I think I'd like to see in terms of behavior is:
  • Hash algorithm and hash properties can be specified on the annotation.
  • Multiple instances of the annotation/idstore -- is that supported? -- could specify different hash algorithms, or, they could specify the same algorithm type, but different properties.
  • At runtime, each identity store instance gets its own instance of the hash algorithm type, even if other identity store instances are using the same hash algorithm.
  • The hash algorithm is initialized onced, up front, with the configured properties.
It's possible to make an argument for deferred evaluation of properties, etc., but that complicates things considerably and I think doing only the above provides plenty of flexibility. An implementation of HashAlgorithm could certainly do its own deferred evaluation internally.

My $0.02 (or maybe $2.00).

Will


On 07/24/2017 08:41 AM, Arjan Tijms wrote:
Hi,

On Mon, Jul 24, 2017 at 2:21 PM, Will Hopkins <will.hopkins@...> wrote:
>The one in the annotation is basically the base version of that. The
>producer could be dependent scoped indeed.

Not sure I follow what you mean.

Sorry, I meant the data that directly comes from the annotation instance and is converted into a Map here:


(ps, that's also an example showing the static import of unmodifiableMap)

 
I think the value of having it dependent scoped is that there could be more than one instance, each configured with different parameters. For any given instance, the parameters would never change, but if you had multiple identity stores, each one could have an instance of the algorithm that was configured differently, even if they all used the same algorithm. If it was application scoped, then every identity store in the app that wanted to use the same algorithm would have to use the same instance, and therefore the same parameters.

For the injection it would need an extra qualifier to get the right version of the parameters, which is indeed one of the extra complications. Since qualifier annotation attribute values can be binding, these qualifiers can be created more or less dynamically. I think it should be doable, but it's not super trivial.

Kind regards,
Arjan Tijms





-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803


Re: LDAP Annotation and Database Hashing Proposal

Will Hopkins
 

Not sure I care that much about the details of the implementation, although I'm starting to feel like CDI for this use case may be making things more complicated, not less, relative to simply newing up an instance of the type specified in the hashAlgorithm attribute and passing hashProperties to the constructor ...

What I think I'd like to see in terms of behavior is:
  • Hash algorithm and hash properties can be specified on the annotation.
  • Multiple instances of the annotation/idstore -- is that supported? -- could specify different hash algorithms, or, they could specify the same algorithm type, but different properties.
  • At runtime, each identity store instance gets its own instance of the hash algorithm type, even if other identity store instances are using the same hash algorithm.
  • The hash algorithm is initialized onced, up front, with the configured properties.
It's possible to make an argument for deferred evaluation of properties, etc., but that complicates things considerably and I think doing only the above provides plenty of flexibility. An implementation of HashAlgorithm could certainly do its own deferred evaluation internally.

My $0.02 (or maybe $2.00).

Will

On 07/24/2017 08:41 AM, Arjan Tijms wrote:
Hi,

On Mon, Jul 24, 2017 at 2:21 PM, Will Hopkins <will.hopkins@...> wrote:
>The one in the annotation is basically the base version of that. The
>producer could be dependent scoped indeed.

Not sure I follow what you mean.

Sorry, I meant the data that directly comes from the annotation instance and is converted into a Map here:


(ps, that's also an example showing the static import of unmodifiableMap)

 
I think the value of having it dependent scoped is that there could be more than one instance, each configured with different parameters. For any given instance, the parameters would never change, but if you had multiple identity stores, each one could have an instance of the algorithm that was configured differently, even if they all used the same algorithm. If it was application scoped, then every identity store in the app that wanted to use the same algorithm would have to use the same instance, and therefore the same parameters.

For the injection it would need an extra qualifier to get the right version of the parameters, which is indeed one of the extra complications. Since qualifier annotation attribute values can be binding, these qualifiers can be created more or less dynamically. I think it should be doable, but it's not super trivial.

Kind regards,
Arjan Tijms





-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803


Re: LDAP Annotation and Database Hashing Proposal

Arjan Tijms
 

Hi,

On Mon, Jul 24, 2017 at 2:21 PM, Will Hopkins <will.hopkins@...> wrote:
>The one in the annotation is basically the base version of that. The
>producer could be dependent scoped indeed.

Not sure I follow what you mean.

Sorry, I meant the data that directly comes from the annotation instance and is converted into a Map here:


(ps, that's also an example showing the static import of unmodifiableMap)

 
I think the value of having it dependent scoped is that there could be more than one instance, each configured with different parameters. For any given instance, the parameters would never change, but if you had multiple identity stores, each one could have an instance of the algorithm that was configured differently, even if they all used the same algorithm. If it was application scoped, then every identity store in the app that wanted to use the same algorithm would have to use the same instance, and therefore the same parameters.

For the injection it would need an extra qualifier to get the right version of the parameters, which is indeed one of the extra complications. Since qualifier annotation attribute values can be binding, these qualifiers can be created more or less dynamically. I think it should be doable, but it's not super trivial.

Kind regards,
Arjan Tijms





Re: LDAP Annotation and Database Hashing Proposal

Guillermo González de Agüero
 

I think application scoped with non-deferred expressions would work most of the time and it's simpler to understand and explain.

Even being application scoped, the container can create an instance for each one by using a custom qualifier (that could be an implementation detail).


Regards,

Guillermo González de Agüero


El lun., 24 de julio de 2017 14:21, Will Hopkins <will.hopkins@...> escribió:


On July 24, 2017 7:52:22 AM EDT, arjan tijms <arjan.tijms@...> wrote:
>Hi,
>
>On Mon, Jul 24, 2017 at 1:40 PM, Will Hopkins <will.hopkins@...>
>wrote:
>
>> Could we specify that it's not normal scoped, and inject a new
>instance
>> that we remember and use for the lifetime of the process?
>>
>
>The one in the annotation is basically the base version of that. The
>producer could be dependent scoped indeed.

Not sure I follow what you mean.

>> There's really no need for this to be fancy, it's a low level crypto
>> operation that will likely always operate one way for the lifetime of
>a
>> given deployment. Doesn't seem like there's a big need for deferred
>> evaluation, etc.
>>
>
>I'd be okay with that as well, although then we could just as well make
>the
>producer ApplicationScoped.

I think the value of having it dependent scoped is that there could be more than one instance, each configured with different parameters. For any given instance, the parameters would never change, but if you had multiple identity stores, each one could have an instance of the algorithm that was configured differently, even if they all used the same algorithm. If it was application scoped, then every identity store in the app that wanted to use the same algorithm would have to use the same instance, and therefore the same parameters.

>
>Kind regards,
>Arjan Tijms

--
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803




Re: LDAP Annotation and Database Hashing Proposal

Will Hopkins
 

On July 24, 2017 7:52:22 AM EDT, arjan tijms <arjan.tijms@gmail.com> wrote:
Hi,

On Mon, Jul 24, 2017 at 1:40 PM, Will Hopkins <will.hopkins@oracle.com>
wrote:

Could we specify that it's not normal scoped, and inject a new
instance
that we remember and use for the lifetime of the process?
The one in the annotation is basically the base version of that. The
producer could be dependent scoped indeed.
Not sure I follow what you mean.

There's really no need for this to be fancy, it's a low level crypto
operation that will likely always operate one way for the lifetime of
a
given deployment. Doesn't seem like there's a big need for deferred
evaluation, etc.
I'd be okay with that as well, although then we could just as well make
the
producer ApplicationScoped.
I think the value of having it dependent scoped is that there could be more than one instance, each configured with different parameters. For any given instance, the parameters would never change, but if you had multiple identity stores, each one could have an instance of the algorithm that was configured differently, even if they all used the same algorithm. If it was application scoped, then every identity store in the app that wanted to use the same algorithm would have to use the same instance, and therefore the same parameters.


Kind regards,
Arjan Tijms
--
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803


Re: LDAP Annotation and Database Hashing Proposal

Arjan Tijms
 

Hi,

On Mon, Jul 24, 2017 at 1:40 PM, Will Hopkins <will.hopkins@...> wrote:
Could we specify that it's not normal scoped, and inject a new instance that we remember and use for the lifetime of the process?

The one in the annotation is basically the base version of that. The producer could be dependent scoped indeed.

 
There's really no need for this to be fancy, it's a low level crypto operation that will likely always operate one way for the lifetime of a given deployment. Doesn't seem like there's a big need for deferred evaluation, etc.

I'd be okay with that as well, although then we could just as well make the producer ApplicationScoped.

Kind regards,
Arjan Tijms


Re: LDAP Annotation and Database Hashing Proposal

Will Hopkins
 

On July 24, 2017 7:24:58 AM EDT, Arjan Tijms <arjan.tijms@gmail.com> wrote:
Hi,

On Mon, Jul 24, 2017 at 10:56 AM, Will Hopkins
<will.hopkins@oracle.com>
wrote:

Well, let me ask this -- can we specify that the properties are
passed to
a constructor or init method somehow? (i.e., not to each method
call?)
The reason I passed them into each method was so they could have
deferred
values, and the runtime can then transparently resolve them.
Understood, but I wonder how often the flexibility will be needed. More critically, how do we ensure that the same parameters are passed to generate. I suppose we could drop generate -- but it's there so that impls of identity store can also update passwords, using any hash algorithm, which seems useful.


Also, since the CDI bean is fetched from CDI with each call (since we
don't
know the scope), if we use an init method, the implementation would
always
see two calls in succession:

init(parameters);
verify(...);

So that wouldn't make the difference we may think it makes.
Could we specify that it's not normal scoped, and inject a new instance that we remember and use for the lifetime of the process?

There's really no need for this to be fancy, it's a low level crypto operation that will likely always operate one way for the lifetime of a given deployment. Doesn't seem like there's a big need for deferred evaluation, etc.


I did experimented with another solution, and that's making a CDI
Producer
available so the parameters can be injected into the implementation.
This
producer could be either RequestScoped (so we can allow deferred values
still, albeit resolved once per request), or ApplicationScoped
(wouldn't
really allow for deferred values).

This is also possible, but it does add a little more spec text than a
simple extra parameter in an interface, which is why I went with the
latter
option initially.

Kind regards,
Arjan Tijms






One of my concerns is that we don't have control over the parameters
when
generateHash() is called, so unless the properies are passed to
constructor
or init, the generate() and verify() methods will have different
behavior.
That would provide slightly less flexibility, but the only case I can
think
of where it would really matter is multi-tenancy, which we aren't
really
claiming to support, and even then I suspect it would be rare.

On 07/24/2017 04:37 AM, Rudy De Busscher wrote:


- *Implementations can provide their own configuration mechanisms
-- a
standard config object that's injected, decorators, annotations of
their
own, abstract impls that developers extend to customize and
deploy, etc.
Those won't be standard, but neither would properties, because the
actual
property names and values would be specific to the implementation
in use.*

And that is what I want to avoid, that each vendor uses his own way
of
configuration.

The database access and hashing algorithms are then may be
standardized,
but when each vendor uses his own way of configuration, we are back
to
square one. No compatibility between the different servers8 So what
is the
point then of our effort?

Property names won't be standardized, I agree. But we have the same
situation for JPA. There you have also properties (the hibernate.xxxx
and
eclipselink.xxxx values)

But at least you need to specify them in the persistence.xml file and
not
within other areas (like environment variables, server xml files,
JNDI, ...
) for each server differently.

Rudy


On 24 July 2017 at 10:14, Will Hopkins <will.hopkins@oracle.com>
wrote:

All true.

A couple of counter-points to ponder:

- Property values will never be standard, even if we support
passing
them. At best they would be standard for the default PasswordHash
that is
supplied by the RI. A developer passing properties would have to
understand
the specifics of the implementation being used.


- Passing it to the method, rather than to the constructor or an
init
method. And we won't ever be in a position to pass them to
generateHash(),
so it can never behave consistently with verifyHash.


- Implementations can provide their own configuration mechanisms
-- a
standard config object that's injected, decorators, annotations
of their
own, abstract impls that developers extend to customize and
deploy, etc.
Those won't be standard, but neither would properties, because
the actual
property names and values would be specific to the implementation
in use.

Will

On 07/24/2017 02:54 AM, Rudy De Busscher wrote:

Guillermo,

We can't add additional methods to an interface later on. This would
make
WAR files packages and compiled against JAVA EE 8, break when they
are
deployed on a Java EE 9 server for example which has the additional
methods
in the interface.

So if we decide now (today?) that they are out, they are out
forever.

Or we have to define new interface classes.

Rudy


On 24 July 2017 at 07:46, Guillermo González de Agüero <
z06.guillermo@gmail.com> wrote:

I was going to raise the same concern, although I think properties
could
be supported in the future by providing a new method and specifying
that
the old one y to be used when there are no parameters defined, and
the new
one when there are.

If that is acceptable and can be done in the future, I think it's
not
such a big deal to leave them for now if the EG don't find
consensus.


Regards,

Guillermo González de Agüero


El lun., 24 de julio de 2017 6:49, Rudy De Busscher <
rdebusscher@gmail.com> escribió:

Will,

regarding the properties. Indeed when we do not support the
properties,
they don't need to be specified at the method.

But then, we will never be able to support them in any further
version
since it would break the backward compatibility of the interface
methods.
Once this release is out, we can't change anything anymore to
interface (no
change in the method parameters, method count, etc ... only we can
add
default implemented methods)

So I guess it is in now or leave out for ever.

Rudy



On 23 July 2017 at 23:31, Will Hopkins <will.hopkins@oracle.com>
wrote:

Hi Rudy,

Thanks for the feedback, responses inline.

Will

On 07/23/2017 08:05 AM, Rudy De Busscher wrote:

Will, See inline comments (hope it is clear enough, I'll put them
also
in a separate thread)


On 22 July 2017 at 23:17, Arjan Tijms <arjan.tijms@gmail.com>
wrote:

Hi,

On Sat, Jul 22, 2017 at 10:06 PM, Will Hopkins <
will.hopkins@oracle.com> wrote:

Arjan, et al.:

I've pushed some changes to the hashalgorithm branch in both
security-api and security-soteria; please have a look to see
what you think.

- I considered PasswordHashAlgorithm, but that seemed too
long,
especially for interfaces/classes that would extend it.

"PasswordHashAlgorithm" certainly doesn't feel too long for me.





- Also renamed IdentityHashAlgorithm to
PlaintextPasswordHash.

Ok




- Added generateHash() method.

Ok




- Removed hash parameters passed to verifyHash(). I did not
remove the DatabaseIdentityStoreDefinition hashProperties
attribute, nor did I remove the "plumbing" that passees the
properties down
to the hash implementation, in case we want to restore the
properties. But
they are not currently passed in to the PasswordHash.

Even if the standard Pbkdf2PasswordHashImpl doesn't use it, I
think
it doesn't hurt much to pass them in anyway.
The parameters are required. if the PasswordHash interface
doesn't
have methods with the parameters passed along, there is no
implemtattion
ever that can use them!


I assume what you mean is that the methods need the parameters
passed
along *if we retain the hashProperties attribute* ... correct?

The intent would be to either support properties, or remove the
hashProperties attribute on DatabaseIdentityStoreDefinition. My
change leaves them on the annotation because I didn't feel like
we'd made a
final decision yet, and didn't want to have to add all that back
if we
decided to keep them. If we don't, we would remove the annotation
attribute.








- Implemented a (more or less) constant time compare for
passwords/hashes, and updated the two PasswordHash impl
classes to use it.


- Re-wrote Pbkdf2PasswordHashImpl to:


- Generate hashes with a specific storage format (encoding),
and
parse that encoding when doing verifyHash(), such that
the verify operation
can use the algorithm/parameters/salt that were used to
hash the password
originally, instead of the currently configured
paramters.
- Added support for multiple flavors of PBKDF2 (i.e.,
"with"
different Hmac algorithms). Now supports all SHA1/2
variants, but not MD5
(which doesn't appear to be available, or at least not
enabled by default,
in JDK8).
- Changed the default parameters to be more in line with
current best practices.

Ok
I would also consider removing SHA1. Just as MD5, it is no longer
considered as really safe.

Good point, I'll do that. My thought in keeping it (and perhaps
MD5,
if it worked) was to support passwords hashed with legacy
algorithms, but
that doesn't make much sense since it's unlikely they'd be
encoded the same
way in the database, so the default PasswordHashAlgorithm
wouldn't work
anyway.





So, this is what I had in mind. It's really not so different
from
what you coded up, Arjan, except for some of the algorithm
implementation
details, the lack of properties support, and the flexible
encoding format
that allows default parameters to be changed while preserving
the ability
to verify passwords hashed with different parameters.

My reasoning behind removing the properties support is as
follows:

- The application developer already has complete control
because
he/she can provide a custom algorithm to meet any
requirement. Properties
may be convenient in some cases, but even without them the
developer can do
whatever they need to do.


- We are already introducing a very large change after PFD
publication. Properties mean more to specify and document,
and, more
crucially, they mean more test assertions and TCK test cases
to write.
There is already concern internally about our ability to
complete the TCK
on schedule.

Ok





- As is hopefully made more clear by my implementation code,
the
algorithm itself, and related parameters, are not really the
source of most
variation and complexity for password hashes -- it's the
content and format
of the encoded hashed password, as stored in the database,
that seems
likely to be infinitely more variable. There are likely a
number of fairly
common formats, but I suspect most are ad hoc, and practices
are changing
rapidly over time. In any event, we simply don't have time
to research that
question to figure out if there are any formats common
enough to
standardize on. (Nor do we have time to develop and
implement some kind of
expression syntax to specifies what the encoding should look
like.)

As I mentioned, the encoding step is very likely common enough
to
warrant having its own method in the interface, so developers
can e.g.
decorate the default algorithm and only change the encoding.




-

As a result, I think it's highly unlikely that the ability to
supply
properties is going to be enough to allow developers to
customize
everything they'll want or need to

Like the entire concept of the DatabaseIdentityStore, it's
indeed
unlikely to ever cover everything everyone ever wants, but the
80% rule
comes in play here; it's okay if we design for the 80% and then
the 20% can
do whatever they want in a custom implementation.

I personally still feel parameters with reasonable defaults are
still
a good intermediate solution between using the standard
algorithm as is,
and having to write something from scratch.

That said, if there's no time, there's no time. Or in other
words, it
is what it is ;)

But it needs to be useable. Otherwise it is better to deliver
nothing
then something which isn't working properly and useable!
If the perception of Java EE security API is not good during the
first
months, it will never be used by anyone, even if we come up with
a better
version later on.

Sorry to be this hard, but that is the thruth.

I agree that it needs to be usable, but, to me, the properties/no
properties question doesn't seem to be a fatal flaw -- properties
would be
convenient, in some cases, but lack of properties won't prevent a
developer
(or platform vendor) from implementing any algorithm(s) they
need. Do you
see it differently?



Kind regards,
Arjan Tijms




, even when they're creating a new identity store and have
complete
control over the format. In the case that they need to develop
for an
existing identity store, with existing password hash formats,
the chances
are close to zero that a generic password hash implementation
could be
adequately customized with properties.

With all of that in mind, I do think properties are a nice
feature,
but they don't seem absolutely necessary -- developers have
full control
without them -- and it's not clear they'd really be useful in
very many
scenarios. I suspect they would mostly be used to fiddle with
parameters in
cases where the default storage encoding is deemed to be
acceptable, and
that, in most cases, the default algorithm/parameters would
probably be
deemed acceptable as well, if properties weren't available. As
such, I'm
reluctant to take on the extra complexity and work required to
support
properties at this point.

Anyway, that's my proposal. Thoughts?

Will

A small detail anout the code, I would make it more Java 8 alike
(
with diamond operator, foreach method, ....)

If you like, I can make a PR for that.

Sure, if you want, but let's wait until we're fully decided on
the
approach. Also, did you sign the OCA yet? I don't understand why
that's
suddenly a requirement, but apparently it is.


Rudy



On 07/20/2017 03:25 PM, Arjan Tijms wrote:

Hi,

The adaptability concern is basically the main concern of the
entire
DataBaseIdentityStore. Indeed, having a query to get the
(hashed) password
from the caller name and another to get the groups covers maybe
80% of use
cases, but certainly not all.

There are for instance exotic database schemes were you'd need
at
least 2 seperate queries to get that data.

For the hash algorithm the same thing holds. We need to specify
enough moving parts so we can adapt to *many* existing
databases and
requirements, but we shouldn't even be dreaming about covering
them all.

Since parameters can contain EL as values as well, it wouldn't
be
crazy hard really to supply the encoding as a method
expression. It's
basically a byte[] -> String function.

In this case though I think encoding is a general enough moving
part
to warrant its own method in the interface. It can easily be
defaulted to
e.g. base64 and it can be overridden without exposing the
implementation by
use of a CDI decorator (or should be after we solve a bug or
problem that
currently prevents decorating).

Kind regards,
Arjan Tijms


On Thursday, July 20, 2017, Will Hopkins
<will.hopkins@oracle.com>
wrote:



On 07/20/2017 12:38 PM, Guillermo González de Agüero wrote:



Similarly, parameters like the salt and iteration count can
be
defaulted. That way you can have 3 levels:

1. Specify only the class name; you get defaults for all
parameters and it Just Works
2. Specify the parameters locally to match a specific DB or
requirement
3. (via EL) fetch the parameter values externally

The most important rule should always be that the developer
is in
full control, or can be in full control. External overrides
should be
possible, but never be required. That is -the- big mistake
that Java EE
made in the past.

I agree, but I think we're only talking about what the
mechanism
for override is, and what the trade-off between simplicity
and the override
mechanism is. In practice, I don't think parameters are going
to be very
helpful to most developers. They can be used to tweak some
simple things,
but the simple things won't typically be necessary and the
more complex
things can't be handled by parameters.
Could you please provide an example of such a setting, please?
Just
to better understand. Otherwise, we could just wait for your
new proposal
to see.


Basically, it's the encoding of the hashed password.
Parameters
like salt, iterations, hash key size, etc. are all pretty
standard. They
can be dialed up or dialed down to make the hash stronger or
weaker, and I
think "stronger vs. weaker" is the level at which most people
think about
them. I doubt many developers are concerned about specifying
precisely a
certain number of iterations for some reason, they just want
iterations
count to be high enough to be secure, without imposing
excessive overhead
or resource consumption, and security most often wins out vs.
resource
consumption these days. Most will be happy if they can turn up
the volume
with a single knob, they don't need bass and treble and
balance knobs. Even
better: if the default volume is about right, and if the
volume
automatically goes up when you're driving at higher speeds.

In contrast, the format of the encoded string is not standard
in
any way that I'm aware of. There are probably LDAP vendors
that do things
in a common way, but for any given database user store, either
the format
has already been decided, because there are existing user
accounts, or the
developer won't care as long as the default format is
reasonable (assuming
the DB schema allows enough space for the chosen encoding
format.

I can't think of any good way to standardize parameters to
describe
how the hashed password is encoded for storage, and that's the
hard part,
the part that you really have to adapt to. It's not a big deal
if you have
to use 4000 iterations instead of 2000, but it's a big deal if
you can't
parse the encoded password. And since the encoding is
completely arbitrary
and non-standard, that's the real problem, and that's what
will likely
force people to do their own implementation of HashAlgorithm.
That can't be
specified as Properties.

We could probably come up with an interface for
encoding/decoding
the hashed value, or for describing the encoded value as a
string and
passing it as a property. But that's a lot to spec out, and we
just don't
have time for it. It's complicated by the fact that decoding a
hash
potentially yields all the parameters that were used to
generate the hash,
so there'd need to be a generalized interface for returning
those in a
standard way. Plus, in some databases, the encodings used will
vary, as
legacy hashes are replaced with stronger hashes/different
encodings when
people change their password.

It's hard to argue against supplying Properties. It's not
difficult
to implement, and it might be useful. But I'm trying to keep
the interface
as simple as possible -- in part to reduce the number of
additional TCK
tests that need to be written -- and I suspect the Properties
will rarely
be useful. Some people will want to use them, but for many the
defaults
will be good enough, and many of those with specific
requirements will
already be forced to write an implementation in order to deal
with encoding
issues.

Will

--
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
<%28781%29%20442-0310>
Oracle Application Development
35 Network Drive, Burlington, MA 01803

--
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
<%28781%29%20442-0310>
Oracle Application Development
35 Network Drive, Burlington, MA 01803

--
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
<(781)%20442-0310>
Oracle Application Development
35 Network Drive, Burlington, MA 01803

--
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
<(781)%20442-0310>
Oracle Application Development
35 Network Drive, Burlington, MA 01803

--
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
<(781)%20442-0310>
Oracle Application Development
35 Network Drive, Burlington, MA 01803



--
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803


Re: LDAP Annotation and Database Hashing Proposal

Will Hopkins
 

Could we specify an injected parameters class for the constructor? I suppose that would mean a change from interface to abstract class ...


On July 24, 2017 7:24:58 AM EDT, Arjan Tijms <arjan.tijms@...> wrote:
Hi,

On Mon, Jul 24, 2017 at 10:56 AM, Will Hopkins <will.hopkins@...> wrote:
Well, let me ask this -- can we specify that the properties are passed to a constructor or init method somehow? (i.e., not to each method call?)

The reason I passed them into each method was so they could have deferred values, and the runtime can then transparently resolve them.

Also, since the CDI bean is fetched from CDI with each call (since we don't know the scope), if we use an init method, the implementation would always see two calls in succession:

init(parameters);
verify(...);

So that wouldn't make the difference we may think it makes.

I did experimented with another solution, and that's making a CDI Producer available so the parameters can be injected into the implementation. This producer could be either RequestScoped (so we can allow deferred values still, albeit resolved once per request), or ApplicationScoped (wouldn't really allow for deferred values).

This is also possible, but it does add a little more spec text than a simple extra parameter in an interface, which is why I went with the latter option initially.

Kind regards,
Arjan Tijms



 

One of my concerns is that we don't have control over the parameters when generateHash() is called, so unless the properies are passed to constructor or init, the generate() and verify() methods will have different behavior. That would provide slightly less flexibility, but the only case I can think of where it would really matter is multi-tenancy, which we aren't really claiming to support, and even then I suspect it would be rare.

On 07/24/2017 04:37 AM, Rudy De Busscher wrote:
  • Implementations can provide their own configuration mechanisms -- a standard config object that's injected, decorators, annotations of their own, abstract impls that developers extend to customize and deploy, etc. Those won't be standard, but neither would properties, because the actual property names and values would be specific to the implementation in use.
And that is what I want to avoid, that each vendor uses his own way of configuration.

The database access and hashing algorithms are then may be standardized, but when each vendor uses his own way of configuration, we are back to square one. No compatibility between the different servers8 So what is the point then of our effort?

Property names won't be standardized, I agree. But we have the same situation for JPA. There you have also properties (the hibernate.xxxx and eclipselink.xxxx values)

But at least you need to specify them in the persistence.xml file and not within other areas (like environment variables, server xml files, JNDI, ... ) for each server differently.

Rudy


On 24 July 2017 at 10:14, Will Hopkins <will.hopkins@...> wrote:
All true.

A couple of counter-points to ponder:
  • Property values will never be standard, even if we support passing them. At best they would be standard for the default PasswordHash that is supplied by the RI. A developer passing properties would have to understand the specifics of the implementation being used.
  • Passing it to the method, rather than to the constructor or an init method. And we won't ever be in a position to pass them to generateHash(), so it can never behave consistently with verifyHash.
  • Implementations can provide their own configuration mechanisms -- a standard config object that's injected, decorators, annotations of their own, abstract impls that developers extend to customize and deploy, etc. Those won't be standard, but neither would properties, because the actual property names and values would be specific to the implementation in use.

Will


On 07/24/2017 02:54 AM, Rudy De Busscher wrote:
Guillermo,

We can't add additional methods to an interface later on. This would make WAR files packages and compiled against JAVA EE 8, break when they are deployed on a Java EE 9 server for example which has the additional methods in the interface.

So if we decide now (today?) that they are out, they are out forever.

Or we have to define new interface classes.

Rudy


On 24 July 2017 at 07:46, Guillermo González de Agüero <z06.guillermo@...> wrote:
I was going to raise the same concern, although I think properties could be supported in the future by providing a new method and specifying that the old one y to be used when there are no parameters defined, and the new one when there are.

If that is acceptable and can be done in the future, I think it's not such a big deal to leave them for now if the EG don't find consensus.


Regards,

Guillermo González de Agüero


El lun., 24 de julio de 2017 6:49, Rudy De Busscher <rdebusscher@...> escribió:
Will,

regarding the properties. Indeed when we do not support the properties, they don't need to be specified at the method.

But then, we will never be able to support them in any further version since it would break the backward compatibility of the interface methods. Once this release is out, we can't change anything anymore to interface (no change in the method parameters, method count, etc ... only we can add default implemented methods)

So I guess it is in now or leave out for ever.

Rudy



On 23 July 2017 at 23:31, Will Hopkins <will.hopkins@...> wrote:
Hi Rudy,

Thanks for the feedback, responses inline.

Will

On 07/23/2017 08:05 AM, Rudy De Busscher wrote:
Will, See inline comments (hope it is clear enough, I'll put them also in a separate thread)


On 22 July 2017 at 23:17, Arjan Tijms <arjan.tijms@...> wrote:
Hi,

On Sat, Jul 22, 2017 at 10:06 PM, Will Hopkins <will.hopkins@...> wrote:
Arjan, et al.:

I've pushed some changes to the hashalgorithm branch in both security-api and security-soteria; please have a look to see what you think.
  • I considered PasswordHashAlgorithm, but that seemed too long, especially for interfaces/classes that would extend it. 


"PasswordHashAlgorithm" certainly doesn't feel too long for me.
 
  • Also renamed IdentityHashAlgorithm to PlaintextPasswordHash.

Ok 
  • Added generateHash() method.

Ok 
  • Removed hash parameters passed to verifyHash(). I did not remove the DatabaseIdentityStoreDefinition hashProperties attribute, nor did I remove the "plumbing" that passees the properties down to the hash implementation, in case we want to restore the properties. But they are not currently passed in to the PasswordHash.

Even if the standard Pbkdf2PasswordHashImpl doesn't use it, I think it doesn't hurt much to pass them in anyway.

The parameters are required. if the PasswordHash interface doesn't have methods with the parameters passed along, there is no implemtattion ever that can use them!

I assume what you mean is that the methods need the parameters passed along if we retain the hashProperties attribute ... correct?

The intent would be to either support properties, or remove the hashProperties attribute on DatabaseIdentityStoreDefinition. My change leaves them on the annotation because I didn't feel like we'd made a final decision yet, and didn't want to have to add all that back if we decided to keep them. If we don't, we would remove the annotation attribute.

 
 
  • Implemented a (more or less) constant time compare for passwords/hashes, and updated the two PasswordHash impl classes to use it.
  • Re-wrote Pbkdf2PasswordHashImpl to:
    • Generate hashes with a specific storage format (encoding), and parse that encoding when doing verifyHash(), such that the verify operation can use the algorithm/parameters/salt that were used to hash the password originally, instead of the currently configured paramters.
    • Added support for multiple flavors of PBKDF2 (i.e., "with" different Hmac algorithms). Now supports all SHA1/2 variants, but not MD5 (which doesn't appear to be available, or at least not enabled by default, in JDK8).
    • Changed the default parameters to be more in line with current best practices.

Ok

I would also consider removing SHA1. Just as MD5, it is no longer considered as really safe.
Good point, I'll do that. My thought in keeping it (and perhaps MD5, if it worked) was to support passwords hashed with legacy algorithms, but that doesn't make much sense since it's unlikely they'd be encoded the same way in the database, so the default PasswordHashAlgorithm wouldn't work anyway.

 
 
So, this is what I had in mind. It's really not so different from what you coded up, Arjan, except for some of the algorithm implementation details, the lack of properties support, and the flexible encoding format that allows default parameters to be changed while preserving the ability to verify passwords hashed with different parameters.

My reasoning behind removing the properties support is as follows:
  • The application developer already has complete control because he/she can provide a custom algorithm to meet any requirement. Properties may be convenient in some cases, but even without them the developer can do whatever they need to do.
  • We are already introducing a very large change after PFD publication. Properties mean more to specify and document, and, more crucially, they mean more test assertions and TCK test cases to write. There is already concern internally about our ability to complete the TCK on schedule.

Ok
 
  • As is hopefully made more clear by my implementation code, the algorithm itself, and related parameters, are not really the source of most variation and complexity for password hashes -- it's the content and format of the encoded hashed password, as stored in the database, that seems likely to be infinitely more variable. There are likely a number of fairly common formats, but I suspect most are ad hoc, and practices are changing rapidly over time. In any event, we simply don't have time to research that question to figure out if there are any formats common enough to standardize on. (Nor do we have time to develop and implement some kind of expression syntax to specifies what the encoding should look like.)

As I mentioned, the encoding step is very likely common enough to warrant having its own method in the interface, so developers can e.g. decorate the default algorithm and only change the encoding.

 

As a result, I think it's highly unlikely that the ability to supply properties is going to be enough to allow developers to customize everything they'll want or need to

Like the entire concept of the DatabaseIdentityStore, it's indeed unlikely to ever cover everything everyone ever wants, but the 80% rule comes in play here; it's okay if we design for the 80% and then the 20% can do whatever they want in a custom implementation.

I personally still feel parameters with reasonable defaults are still a good intermediate solution between using the standard algorithm as is, and having to write something from scratch.

That said, if there's no time, there's no time. Or in other words, it is what it is ;)


But it needs to be useable. Otherwise it is better to deliver nothing then something which isn't working properly and useable!
If the perception of Java EE security API is not good during the first months, it will never be used by anyone, even if we come up with a better version later on.

Sorry to be this hard, but that is the thruth.
I agree that it needs to be usable, but, to me, the properties/no properties question doesn't seem to be a fatal flaw -- properties would be convenient, in some cases, but lack of properties won't prevent a developer (or platform vendor) from implementing any algorithm(s) they need. Do you see it differently?

 
Kind regards,
Arjan Tijms


 
, even when they're creating a new identity store and have complete control over the format. In the case that they need to develop for an existing identity store, with existing password hash formats, the chances are close to zero that a generic password hash implementation could be adequately customized with properties.
With all of that in mind, I do think properties are a nice feature, but they don't seem absolutely necessary -- developers have full control without them -- and it's not clear they'd really be useful in very many scenarios. I suspect they would mostly be used to fiddle with parameters in cases where the default storage encoding is deemed to be acceptable, and that, in most cases, the default algorithm/parameters would probably be deemed acceptable as well, if properties weren't available. As such, I'm reluctant to take on the extra complexity and work required to support properties at this point.

Anyway, that's my proposal. Thoughts?

Will


A small detail anout the code, I would make it more Java 8 alike ( with diamond operator, foreach method, ....)

If you like, I can make a PR for that.
Sure, if you want, but let's wait until we're fully decided on the approach. Also, did you sign the OCA yet? I don't understand why that's suddenly a requirement, but apparently it is.


Rudy
 

On 07/20/2017 03:25 PM, Arjan Tijms wrote:
Hi,

The adaptability concern is basically the main concern of the entire DataBaseIdentityStore. Indeed, having a query to get the (hashed) password from the caller name and another to get the groups covers maybe 80% of use cases, but certainly not all.

There are for instance exotic database schemes were you'd need at least 2 seperate queries to get that data.

For the hash algorithm the same thing holds. We need to specify enough moving parts so we can adapt to *many* existing databases and requirements, but we shouldn't even be dreaming about covering them all.

Since parameters can contain EL as values as well, it wouldn't be crazy hard really to supply the encoding as a method expression. It's basically a byte[] -> String function.

In this case though I think encoding is a general enough moving part to warrant its own method in the interface. It can easily be defaulted to e.g. base64 and it can be overridden without exposing the implementation by use of a CDI decorator (or should be after we solve a bug or problem that currently prevents decorating).

Kind regards,
Arjan Tijms


On Thursday, July 20, 2017, Will Hopkins <will.hopkins@...> wrote:


On 07/20/2017 12:38 PM, Guillermo González de Agüero wrote:


Similarly, parameters like the salt and iteration count can be defaulted. That way you can have 3 levels:

1. Specify only the class name; you get defaults for all parameters and it Just Works
2. Specify the parameters locally to match a specific DB or requirement
3. (via EL) fetch the parameter values externally

The most important rule should always be that the developer is in full control, or can be in full control. External overrides should be possible, but never be required. That is -the- big mistake that Java EE made in the past.
I agree, but I think we're only talking about what the mechanism for override is, and what the trade-off between simplicity and the override mechanism is. In practice, I don't think parameters are going to be very helpful to most developers. They can be used to tweak some simple things, but the simple things won't typically be necessary and the more complex things can't be handled by parameters.
 
Could you please provide an example of such a setting, please? Just to better understand. Otherwise, we could just wait for your new proposal to see.

Basically, it's the encoding of the hashed password. Parameters like salt, iterations, hash key size, etc. are all pretty standard. They can be dialed up or dialed down to make the hash stronger or weaker, and I think "stronger vs. weaker" is the level at which most people think about them. I doubt many developers are concerned about specifying precisely a certain number of iterations for some reason, they just want iterations count to be high enough to be secure, without imposing excessive overhead or resource consumption, and security most often wins out vs. resource consumption these days. Most will be happy if they can turn up the volume with a single knob, they don't need bass and treble and balance knobs. Even better: if the default volume is about right, and if the volume automatically goes up when you're driving at higher speeds.

In contrast, the format of the encoded string is not standard in any way that I'm aware of. There are probably LDAP vendors that do things in a common way, but for any given database user store, either the format has already been decided, because there are existing user accounts, or the developer won't care as long as the default format is reasonable (assuming the DB schema allows enough space for the chosen encoding format.

I can't think of any good way to standardize parameters to describe how the hashed password is encoded for storage, and that's the hard part, the part that you really have to adapt to. It's not a big deal if you have to use 4000 iterations instead of 2000, but it's a big deal if you can't parse the encoded password. And since the encoding is completely arbitrary and non-standard, that's the real problem, and that's what will likely force people to do their own implementation of HashAlgorithm. That can't be specified as Properties.

We could probably come up with an interface for encoding/decoding the hashed value, or for describing the encoded value as a string and passing it as a property. But that's a lot to spec out, and we just don't have time for it. It's complicated by the fact that decoding a hash potentially yields all the parameters that were used to generate the hash, so there'd need to be a generalized interface for returning those in a standard way. Plus, in some databases, the encodings used will vary, as legacy hashes are replaced with stronger hashes/different encodings when people change their password.

It's hard to argue against supplying Properties. It's not difficult to implement, and it might be useful. But I'm trying to keep the interface as simple as possible -- in part to reduce the number of additional TCK tests that need to be written -- and I suspect the Properties will rarely be useful. Some people will want to use them, but for many the defaults will be good enough, and many of those with specific requirements will already be forced to write an implementation in order to deal with encoding issues.

Will
-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803

-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803



-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803



-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803


-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803



--
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803


Re: LDAP Annotation and Database Hashing Proposal

Arjan Tijms
 

Hi,

On Mon, Jul 24, 2017 at 10:56 AM, Will Hopkins <will.hopkins@...> wrote:
Well, let me ask this -- can we specify that the properties are passed to a constructor or init method somehow? (i.e., not to each method call?)

The reason I passed them into each method was so they could have deferred values, and the runtime can then transparently resolve them.

Also, since the CDI bean is fetched from CDI with each call (since we don't know the scope), if we use an init method, the implementation would always see two calls in succession:

init(parameters);
verify(...);

So that wouldn't make the difference we may think it makes.

I did experimented with another solution, and that's making a CDI Producer available so the parameters can be injected into the implementation. This producer could be either RequestScoped (so we can allow deferred values still, albeit resolved once per request), or ApplicationScoped (wouldn't really allow for deferred values).

This is also possible, but it does add a little more spec text than a simple extra parameter in an interface, which is why I went with the latter option initially.

Kind regards,
Arjan Tijms



 

One of my concerns is that we don't have control over the parameters when generateHash() is called, so unless the properies are passed to constructor or init, the generate() and verify() methods will have different behavior. That would provide slightly less flexibility, but the only case I can think of where it would really matter is multi-tenancy, which we aren't really claiming to support, and even then I suspect it would be rare.

On 07/24/2017 04:37 AM, Rudy De Busscher wrote:
  • Implementations can provide their own configuration mechanisms -- a standard config object that's injected, decorators, annotations of their own, abstract impls that developers extend to customize and deploy, etc. Those won't be standard, but neither would properties, because the actual property names and values would be specific to the implementation in use.
And that is what I want to avoid, that each vendor uses his own way of configuration.

The database access and hashing algorithms are then may be standardized, but when each vendor uses his own way of configuration, we are back to square one. No compatibility between the different servers8 So what is the point then of our effort?

Property names won't be standardized, I agree. But we have the same situation for JPA. There you have also properties (the hibernate.xxxx and eclipselink.xxxx values)

But at least you need to specify them in the persistence.xml file and not within other areas (like environment variables, server xml files, JNDI, ... ) for each server differently.

Rudy


On 24 July 2017 at 10:14, Will Hopkins <will.hopkins@...> wrote:
All true.

A couple of counter-points to ponder:
  • Property values will never be standard, even if we support passing them. At best they would be standard for the default PasswordHash that is supplied by the RI. A developer passing properties would have to understand the specifics of the implementation being used.
  • Passing it to the method, rather than to the constructor or an init method. And we won't ever be in a position to pass them to generateHash(), so it can never behave consistently with verifyHash.
  • Implementations can provide their own configuration mechanisms -- a standard config object that's injected, decorators, annotations of their own, abstract impls that developers extend to customize and deploy, etc. Those won't be standard, but neither would properties, because the actual property names and values would be specific to the implementation in use.

Will


On 07/24/2017 02:54 AM, Rudy De Busscher wrote:
Guillermo,

We can't add additional methods to an interface later on. This would make WAR files packages and compiled against JAVA EE 8, break when they are deployed on a Java EE 9 server for example which has the additional methods in the interface.

So if we decide now (today?) that they are out, they are out forever.

Or we have to define new interface classes.

Rudy


On 24 July 2017 at 07:46, Guillermo González de Agüero <z06.guillermo@...> wrote:
I was going to raise the same concern, although I think properties could be supported in the future by providing a new method and specifying that the old one y to be used when there are no parameters defined, and the new one when there are.

If that is acceptable and can be done in the future, I think it's not such a big deal to leave them for now if the EG don't find consensus.


Regards,

Guillermo González de Agüero


El lun., 24 de julio de 2017 6:49, Rudy De Busscher <rdebusscher@...> escribió:
Will,

regarding the properties. Indeed when we do not support the properties, they don't need to be specified at the method.

But then, we will never be able to support them in any further version since it would break the backward compatibility of the interface methods. Once this release is out, we can't change anything anymore to interface (no change in the method parameters, method count, etc ... only we can add default implemented methods)

So I guess it is in now or leave out for ever.

Rudy



On 23 July 2017 at 23:31, Will Hopkins <will.hopkins@...> wrote:
Hi Rudy,

Thanks for the feedback, responses inline.

Will

On 07/23/2017 08:05 AM, Rudy De Busscher wrote:
Will, See inline comments (hope it is clear enough, I'll put them also in a separate thread)


On 22 July 2017 at 23:17, Arjan Tijms <arjan.tijms@...> wrote:
Hi,

On Sat, Jul 22, 2017 at 10:06 PM, Will Hopkins <will.hopkins@...> wrote:
Arjan, et al.:

I've pushed some changes to the hashalgorithm branch in both security-api and security-soteria; please have a look to see what you think.
  • I considered PasswordHashAlgorithm, but that seemed too long, especially for interfaces/classes that would extend it. 


"PasswordHashAlgorithm" certainly doesn't feel too long for me.
 
  • Also renamed IdentityHashAlgorithm to PlaintextPasswordHash.

Ok 
  • Added generateHash() method.

Ok 
  • Removed hash parameters passed to verifyHash(). I did not remove the DatabaseIdentityStoreDefinition hashProperties attribute, nor did I remove the "plumbing" that passees the properties down to the hash implementation, in case we want to restore the properties. But they are not currently passed in to the PasswordHash.

Even if the standard Pbkdf2PasswordHashImpl doesn't use it, I think it doesn't hurt much to pass them in anyway.

The parameters are required. if the PasswordHash interface doesn't have methods with the parameters passed along, there is no implemtattion ever that can use them!

I assume what you mean is that the methods need the parameters passed along if we retain the hashProperties attribute ... correct?

The intent would be to either support properties, or remove the hashProperties attribute on DatabaseIdentityStoreDefinition. My change leaves them on the annotation because I didn't feel like we'd made a final decision yet, and didn't want to have to add all that back if we decided to keep them. If we don't, we would remove the annotation attribute.

 
 
  • Implemented a (more or less) constant time compare for passwords/hashes, and updated the two PasswordHash impl classes to use it.
  • Re-wrote Pbkdf2PasswordHashImpl to:
    • Generate hashes with a specific storage format (encoding), and parse that encoding when doing verifyHash(), such that the verify operation can use the algorithm/parameters/salt that were used to hash the password originally, instead of the currently configured paramters.
    • Added support for multiple flavors of PBKDF2 (i.e., "with" different Hmac algorithms). Now supports all SHA1/2 variants, but not MD5 (which doesn't appear to be available, or at least not enabled by default, in JDK8).
    • Changed the default parameters to be more in line with current best practices.

Ok

I would also consider removing SHA1. Just as MD5, it is no longer considered as really safe.
Good point, I'll do that. My thought in keeping it (and perhaps MD5, if it worked) was to support passwords hashed with legacy algorithms, but that doesn't make much sense since it's unlikely they'd be encoded the same way in the database, so the default PasswordHashAlgorithm wouldn't work anyway.

 
 
So, this is what I had in mind. It's really not so different from what you coded up, Arjan, except for some of the algorithm implementation details, the lack of properties support, and the flexible encoding format that allows default parameters to be changed while preserving the ability to verify passwords hashed with different parameters.

My reasoning behind removing the properties support is as follows:
  • The application developer already has complete control because he/she can provide a custom algorithm to meet any requirement. Properties may be convenient in some cases, but even without them the developer can do whatever they need to do.
  • We are already introducing a very large change after PFD publication. Properties mean more to specify and document, and, more crucially, they mean more test assertions and TCK test cases to write. There is already concern internally about our ability to complete the TCK on schedule.

Ok
 
  • As is hopefully made more clear by my implementation code, the algorithm itself, and related parameters, are not really the source of most variation and complexity for password hashes -- it's the content and format of the encoded hashed password, as stored in the database, that seems likely to be infinitely more variable. There are likely a number of fairly common formats, but I suspect most are ad hoc, and practices are changing rapidly over time. In any event, we simply don't have time to research that question to figure out if there are any formats common enough to standardize on. (Nor do we have time to develop and implement some kind of expression syntax to specifies what the encoding should look like.)

As I mentioned, the encoding step is very likely common enough to warrant having its own method in the interface, so developers can e.g. decorate the default algorithm and only change the encoding.

 

As a result, I think it's highly unlikely that the ability to supply properties is going to be enough to allow developers to customize everything they'll want or need to

Like the entire concept of the DatabaseIdentityStore, it's indeed unlikely to ever cover everything everyone ever wants, but the 80% rule comes in play here; it's okay if we design for the 80% and then the 20% can do whatever they want in a custom implementation.

I personally still feel parameters with reasonable defaults are still a good intermediate solution between using the standard algorithm as is, and having to write something from scratch.

That said, if there's no time, there's no time. Or in other words, it is what it is ;)


But it needs to be useable. Otherwise it is better to deliver nothing then something which isn't working properly and useable!
If the perception of Java EE security API is not good during the first months, it will never be used by anyone, even if we come up with a better version later on.

Sorry to be this hard, but that is the thruth.
I agree that it needs to be usable, but, to me, the properties/no properties question doesn't seem to be a fatal flaw -- properties would be convenient, in some cases, but lack of properties won't prevent a developer (or platform vendor) from implementing any algorithm(s) they need. Do you see it differently?

 
Kind regards,
Arjan Tijms


 
, even when they're creating a new identity store and have complete control over the format. In the case that they need to develop for an existing identity store, with existing password hash formats, the chances are close to zero that a generic password hash implementation could be adequately customized with properties.
With all of that in mind, I do think properties are a nice feature, but they don't seem absolutely necessary -- developers have full control without them -- and it's not clear they'd really be useful in very many scenarios. I suspect they would mostly be used to fiddle with parameters in cases where the default storage encoding is deemed to be acceptable, and that, in most cases, the default algorithm/parameters would probably be deemed acceptable as well, if properties weren't available. As such, I'm reluctant to take on the extra complexity and work required to support properties at this point.

Anyway, that's my proposal. Thoughts?

Will


A small detail anout the code, I would make it more Java 8 alike ( with diamond operator, foreach method, ....)

If you like, I can make a PR for that.
Sure, if you want, but let's wait until we're fully decided on the approach. Also, did you sign the OCA yet? I don't understand why that's suddenly a requirement, but apparently it is.


Rudy
 

On 07/20/2017 03:25 PM, Arjan Tijms wrote:
Hi,

The adaptability concern is basically the main concern of the entire DataBaseIdentityStore. Indeed, having a query to get the (hashed) password from the caller name and another to get the groups covers maybe 80% of use cases, but certainly not all.

There are for instance exotic database schemes were you'd need at least 2 seperate queries to get that data.

For the hash algorithm the same thing holds. We need to specify enough moving parts so we can adapt to *many* existing databases and requirements, but we shouldn't even be dreaming about covering them all.

Since parameters can contain EL as values as well, it wouldn't be crazy hard really to supply the encoding as a method expression. It's basically a byte[] -> String function.

In this case though I think encoding is a general enough moving part to warrant its own method in the interface. It can easily be defaulted to e.g. base64 and it can be overridden without exposing the implementation by use of a CDI decorator (or should be after we solve a bug or problem that currently prevents decorating).

Kind regards,
Arjan Tijms


On Thursday, July 20, 2017, Will Hopkins <will.hopkins@...> wrote:


On 07/20/2017 12:38 PM, Guillermo González de Agüero wrote:


Similarly, parameters like the salt and iteration count can be defaulted. That way you can have 3 levels:

1. Specify only the class name; you get defaults for all parameters and it Just Works
2. Specify the parameters locally to match a specific DB or requirement
3. (via EL) fetch the parameter values externally

The most important rule should always be that the developer is in full control, or can be in full control. External overrides should be possible, but never be required. That is -the- big mistake that Java EE made in the past.
I agree, but I think we're only talking about what the mechanism for override is, and what the trade-off between simplicity and the override mechanism is. In practice, I don't think parameters are going to be very helpful to most developers. They can be used to tweak some simple things, but the simple things won't typically be necessary and the more complex things can't be handled by parameters.
 
Could you please provide an example of such a setting, please? Just to better understand. Otherwise, we could just wait for your new proposal to see.

Basically, it's the encoding of the hashed password. Parameters like salt, iterations, hash key size, etc. are all pretty standard. They can be dialed up or dialed down to make the hash stronger or weaker, and I think "stronger vs. weaker" is the level at which most people think about them. I doubt many developers are concerned about specifying precisely a certain number of iterations for some reason, they just want iterations count to be high enough to be secure, without imposing excessive overhead or resource consumption, and security most often wins out vs. resource consumption these days. Most will be happy if they can turn up the volume with a single knob, they don't need bass and treble and balance knobs. Even better: if the default volume is about right, and if the volume automatically goes up when you're driving at higher speeds.

In contrast, the format of the encoded string is not standard in any way that I'm aware of. There are probably LDAP vendors that do things in a common way, but for any given database user store, either the format has already been decided, because there are existing user accounts, or the developer won't care as long as the default format is reasonable (assuming the DB schema allows enough space for the chosen encoding format.

I can't think of any good way to standardize parameters to describe how the hashed password is encoded for storage, and that's the hard part, the part that you really have to adapt to. It's not a big deal if you have to use 4000 iterations instead of 2000, but it's a big deal if you can't parse the encoded password. And since the encoding is completely arbitrary and non-standard, that's the real problem, and that's what will likely force people to do their own implementation of HashAlgorithm. That can't be specified as Properties.

We could probably come up with an interface for encoding/decoding the hashed value, or for describing the encoded value as a string and passing it as a property. But that's a lot to spec out, and we just don't have time for it. It's complicated by the fact that decoding a hash potentially yields all the parameters that were used to generate the hash, so there'd need to be a generalized interface for returning those in a standard way. Plus, in some databases, the encodings used will vary, as legacy hashes are replaced with stronger hashes/different encodings when people change their password.

It's hard to argue against supplying Properties. It's not difficult to implement, and it might be useful. But I'm trying to keep the interface as simple as possible -- in part to reduce the number of additional TCK tests that need to be written -- and I suspect the Properties will rarely be useful. Some people will want to use them, but for many the defaults will be good enough, and many of those with specific requirements will already be forced to write an implementation in order to deal with encoding issues.

Will
-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803

-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803



-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803



-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803


-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803



Re: LDAP Annotation and Database Hashing Proposal

Rudy De Busscher
 

Yes, why not. And indeed parameters are probably only dependent on the deployment and not on each request (the multi-tenancy use case you described)

I don't think that we have to define that it goes through the constructor or init method. We have only to specify that the parameters (defined at the databaseIdentityStoreDefinition) MUST be used by the methods generateHash() and verifyHash() if these parameters contain appropriate values for the hash algorithm in question.

The most important parameter is if the hashed byte array is stored as base64 or Hex encoded. This is very important.

I see that the current implementation of Pbkdf2PasswordHashImpl is always using base64.

best regards
Rudy


On 24 July 2017 at 10:56, Will Hopkins <will.hopkins@...> wrote:
Well, let me ask this -- can we specify that the properties are passed to a constructor or init method somehow? (i.e., not to each method call?)

One of my concerns is that we don't have control over the parameters when generateHash() is called, so unless the properies are passed to constructor or init, the generate() and verify() methods will have different behavior. That would provide slightly less flexibility, but the only case I can think of where it would really matter is multi-tenancy, which we aren't really claiming to support, and even then I suspect it would be rare.


On 07/24/2017 04:37 AM, Rudy De Busscher wrote:
  • Implementations can provide their own configuration mechanisms -- a standard config object that's injected, decorators, annotations of their own, abstract impls that developers extend to customize and deploy, etc. Those won't be standard, but neither would properties, because the actual property names and values would be specific to the implementation in use.
And that is what I want to avoid, that each vendor uses his own way of configuration.

The database access and hashing algorithms are then may be standardized, but when each vendor uses his own way of configuration, we are back to square one. No compatibility between the different servers8 So what is the point then of our effort?

Property names won't be standardized, I agree. But we have the same situation for JPA. There you have also properties (the hibernate.xxxx and eclipselink.xxxx values)

But at least you need to specify them in the persistence.xml file and not within other areas (like environment variables, server xml files, JNDI, ... ) for each server differently.

Rudy


On 24 July 2017 at 10:14, Will Hopkins <will.hopkins@...> wrote:
All true.

A couple of counter-points to ponder:
  • Property values will never be standard, even if we support passing them. At best they would be standard for the default PasswordHash that is supplied by the RI. A developer passing properties would have to understand the specifics of the implementation being used.
  • Passing it to the method, rather than to the constructor or an init method. And we won't ever be in a position to pass them to generateHash(), so it can never behave consistently with verifyHash.
  • Implementations can provide their own configuration mechanisms -- a standard config object that's injected, decorators, annotations of their own, abstract impls that developers extend to customize and deploy, etc. Those won't be standard, but neither would properties, because the actual property names and values would be specific to the implementation in use.

Will


On 07/24/2017 02:54 AM, Rudy De Busscher wrote:
Guillermo,

We can't add additional methods to an interface later on. This would make WAR files packages and compiled against JAVA EE 8, break when they are deployed on a Java EE 9 server for example which has the additional methods in the interface.

So if we decide now (today?) that they are out, they are out forever.

Or we have to define new interface classes.

Rudy


On 24 July 2017 at 07:46, Guillermo González de Agüero <z06.guillermo@...> wrote:
I was going to raise the same concern, although I think properties could be supported in the future by providing a new method and specifying that the old one y to be used when there are no parameters defined, and the new one when there are.

If that is acceptable and can be done in the future, I think it's not such a big deal to leave them for now if the EG don't find consensus.


Regards,

Guillermo González de Agüero


El lun., 24 de julio de 2017 6:49, Rudy De Busscher <rdebusscher@...> escribió:
Will,

regarding the properties. Indeed when we do not support the properties, they don't need to be specified at the method.

But then, we will never be able to support them in any further version since it would break the backward compatibility of the interface methods. Once this release is out, we can't change anything anymore to interface (no change in the method parameters, method count, etc ... only we can add default implemented methods)

So I guess it is in now or leave out for ever.

Rudy



On 23 July 2017 at 23:31, Will Hopkins <will.hopkins@...> wrote:
Hi Rudy,

Thanks for the feedback, responses inline.

Will

On 07/23/2017 08:05 AM, Rudy De Busscher wrote:
Will, See inline comments (hope it is clear enough, I'll put them also in a separate thread)


On 22 July 2017 at 23:17, Arjan Tijms <arjan.tijms@...> wrote:
Hi,

On Sat, Jul 22, 2017 at 10:06 PM, Will Hopkins <will.hopkins@...> wrote:
Arjan, et al.:

I've pushed some changes to the hashalgorithm branch in both security-api and security-soteria; please have a look to see what you think.
  • I considered PasswordHashAlgorithm, but that seemed too long, especially for interfaces/classes that would extend it. 


"PasswordHashAlgorithm" certainly doesn't feel too long for me.
 
  • Also renamed IdentityHashAlgorithm to PlaintextPasswordHash.

Ok 
  • Added generateHash() method.

Ok 
  • Removed hash parameters passed to verifyHash(). I did not remove the DatabaseIdentityStoreDefinition hashProperties attribute, nor did I remove the "plumbing" that passees the properties down to the hash implementation, in case we want to restore the properties. But they are not currently passed in to the PasswordHash.

Even if the standard Pbkdf2PasswordHashImpl doesn't use it, I think it doesn't hurt much to pass them in anyway.

The parameters are required. if the PasswordHash interface doesn't have methods with the parameters passed along, there is no implemtattion ever that can use them!

I assume what you mean is that the methods need the parameters passed along if we retain the hashProperties attribute ... correct?

The intent would be to either support properties, or remove the hashProperties attribute on DatabaseIdentityStoreDefinition. My change leaves them on the annotation because I didn't feel like we'd made a final decision yet, and didn't want to have to add all that back if we decided to keep them. If we don't, we would remove the annotation attribute.

 
 
  • Implemented a (more or less) constant time compare for passwords/hashes, and updated the two PasswordHash impl classes to use it.
  • Re-wrote Pbkdf2PasswordHashImpl to:
    • Generate hashes with a specific storage format (encoding), and parse that encoding when doing verifyHash(), such that the verify operation can use the algorithm/parameters/salt that were used to hash the password originally, instead of the currently configured paramters.
    • Added support for multiple flavors of PBKDF2 (i.e., "with" different Hmac algorithms). Now supports all SHA1/2 variants, but not MD5 (which doesn't appear to be available, or at least not enabled by default, in JDK8).
    • Changed the default parameters to be more in line with current best practices.

Ok

I would also consider removing SHA1. Just as MD5, it is no longer considered as really safe.
Good point, I'll do that. My thought in keeping it (and perhaps MD5, if it worked) was to support passwords hashed with legacy algorithms, but that doesn't make much sense since it's unlikely they'd be encoded the same way in the database, so the default PasswordHashAlgorithm wouldn't work anyway.

 
 
So, this is what I had in mind. It's really not so different from what you coded up, Arjan, except for some of the algorithm implementation details, the lack of properties support, and the flexible encoding format that allows default parameters to be changed while preserving the ability to verify passwords hashed with different parameters.

My reasoning behind removing the properties support is as follows:
  • The application developer already has complete control because he/she can provide a custom algorithm to meet any requirement. Properties may be convenient in some cases, but even without them the developer can do whatever they need to do.
  • We are already introducing a very large change after PFD publication. Properties mean more to specify and document, and, more crucially, they mean more test assertions and TCK test cases to write. There is already concern internally about our ability to complete the TCK on schedule.

Ok
 
  • As is hopefully made more clear by my implementation code, the algorithm itself, and related parameters, are not really the source of most variation and complexity for password hashes -- it's the content and format of the encoded hashed password, as stored in the database, that seems likely to be infinitely more variable. There are likely a number of fairly common formats, but I suspect most are ad hoc, and practices are changing rapidly over time. In any event, we simply don't have time to research that question to figure out if there are any formats common enough to standardize on. (Nor do we have time to develop and implement some kind of expression syntax to specifies what the encoding should look like.)

As I mentioned, the encoding step is very likely common enough to warrant having its own method in the interface, so developers can e.g. decorate the default algorithm and only change the encoding.

 

As a result, I think it's highly unlikely that the ability to supply properties is going to be enough to allow developers to customize everything they'll want or need to

Like the entire concept of the DatabaseIdentityStore, it's indeed unlikely to ever cover everything everyone ever wants, but the 80% rule comes in play here; it's okay if we design for the 80% and then the 20% can do whatever they want in a custom implementation.

I personally still feel parameters with reasonable defaults are still a good intermediate solution between using the standard algorithm as is, and having to write something from scratch.

That said, if there's no time, there's no time. Or in other words, it is what it is ;)


But it needs to be useable. Otherwise it is better to deliver nothing then something which isn't working properly and useable!
If the perception of Java EE security API is not good during the first months, it will never be used by anyone, even if we come up with a better version later on.

Sorry to be this hard, but that is the thruth.
I agree that it needs to be usable, but, to me, the properties/no properties question doesn't seem to be a fatal flaw -- properties would be convenient, in some cases, but lack of properties won't prevent a developer (or platform vendor) from implementing any algorithm(s) they need. Do you see it differently?

 
Kind regards,
Arjan Tijms


 
, even when they're creating a new identity store and have complete control over the format. In the case that they need to develop for an existing identity store, with existing password hash formats, the chances are close to zero that a generic password hash implementation could be adequately customized with properties.
With all of that in mind, I do think properties are a nice feature, but they don't seem absolutely necessary -- developers have full control without them -- and it's not clear they'd really be useful in very many scenarios. I suspect they would mostly be used to fiddle with parameters in cases where the default storage encoding is deemed to be acceptable, and that, in most cases, the default algorithm/parameters would probably be deemed acceptable as well, if properties weren't available. As such, I'm reluctant to take on the extra complexity and work required to support properties at this point.

Anyway, that's my proposal. Thoughts?

Will


A small detail anout the code, I would make it more Java 8 alike ( with diamond operator, foreach method, ....)

If you like, I can make a PR for that.
Sure, if you want, but let's wait until we're fully decided on the approach. Also, did you sign the OCA yet? I don't understand why that's suddenly a requirement, but apparently it is.


Rudy
 

On 07/20/2017 03:25 PM, Arjan Tijms wrote:
Hi,

The adaptability concern is basically the main concern of the entire DataBaseIdentityStore. Indeed, having a query to get the (hashed) password from the caller name and another to get the groups covers maybe 80% of use cases, but certainly not all.

There are for instance exotic database schemes were you'd need at least 2 seperate queries to get that data.

For the hash algorithm the same thing holds. We need to specify enough moving parts so we can adapt to *many* existing databases and requirements, but we shouldn't even be dreaming about covering them all.

Since parameters can contain EL as values as well, it wouldn't be crazy hard really to supply the encoding as a method expression. It's basically a byte[] -> String function.

In this case though I think encoding is a general enough moving part to warrant its own method in the interface. It can easily be defaulted to e.g. base64 and it can be overridden without exposing the implementation by use of a CDI decorator (or should be after we solve a bug or problem that currently prevents decorating).

Kind regards,
Arjan Tijms


On Thursday, July 20, 2017, Will Hopkins <will.hopkins@...> wrote:


On 07/20/2017 12:38 PM, Guillermo González de Agüero wrote:


Similarly, parameters like the salt and iteration count can be defaulted. That way you can have 3 levels:

1. Specify only the class name; you get defaults for all parameters and it Just Works
2. Specify the parameters locally to match a specific DB or requirement
3. (via EL) fetch the parameter values externally

The most important rule should always be that the developer is in full control, or can be in full control. External overrides should be possible, but never be required. That is -the- big mistake that Java EE made in the past.
I agree, but I think we're only talking about what the mechanism for override is, and what the trade-off between simplicity and the override mechanism is. In practice, I don't think parameters are going to be very helpful to most developers. They can be used to tweak some simple things, but the simple things won't typically be necessary and the more complex things can't be handled by parameters.
 
Could you please provide an example of such a setting, please? Just to better understand. Otherwise, we could just wait for your new proposal to see.

Basically, it's the encoding of the hashed password. Parameters like salt, iterations, hash key size, etc. are all pretty standard. They can be dialed up or dialed down to make the hash stronger or weaker, and I think "stronger vs. weaker" is the level at which most people think about them. I doubt many developers are concerned about specifying precisely a certain number of iterations for some reason, they just want iterations count to be high enough to be secure, without imposing excessive overhead or resource consumption, and security most often wins out vs. resource consumption these days. Most will be happy if they can turn up the volume with a single knob, they don't need bass and treble and balance knobs. Even better: if the default volume is about right, and if the volume automatically goes up when you're driving at higher speeds.

In contrast, the format of
...

[Message clipped]  


Re: LDAP Annotation and Database Hashing Proposal

Will Hopkins
 

Well, let me ask this -- can we specify that the properties are passed to a constructor or init method somehow? (i.e., not to each method call?)

One of my concerns is that we don't have control over the parameters when generateHash() is called, so unless the properies are passed to constructor or init, the generate() and verify() methods will have different behavior. That would provide slightly less flexibility, but the only case I can think of where it would really matter is multi-tenancy, which we aren't really claiming to support, and even then I suspect it would be rare.

On 07/24/2017 04:37 AM, Rudy De Busscher wrote:
  • Implementations can provide their own configuration mechanisms -- a standard config object that's injected, decorators, annotations of their own, abstract impls that developers extend to customize and deploy, etc. Those won't be standard, but neither would properties, because the actual property names and values would be specific to the implementation in use.
And that is what I want to avoid, that each vendor uses his own way of configuration.

The database access and hashing algorithms are then may be standardized, but when each vendor uses his own way of configuration, we are back to square one. No compatibility between the different servers8 So what is the point then of our effort?

Property names won't be standardized, I agree. But we have the same situation for JPA. There you have also properties (the hibernate.xxxx and eclipselink.xxxx values)

But at least you need to specify them in the persistence.xml file and not within other areas (like environment variables, server xml files, JNDI, ... ) for each server differently.

Rudy


On 24 July 2017 at 10:14, Will Hopkins <will.hopkins@...> wrote:
All true.

A couple of counter-points to ponder:
  • Property values will never be standard, even if we support passing them. At best they would be standard for the default PasswordHash that is supplied by the RI. A developer passing properties would have to understand the specifics of the implementation being used.
  • Passing it to the method, rather than to the constructor or an init method. And we won't ever be in a position to pass them to generateHash(), so it can never behave consistently with verifyHash.
  • Implementations can provide their own configuration mechanisms -- a standard config object that's injected, decorators, annotations of their own, abstract impls that developers extend to customize and deploy, etc. Those won't be standard, but neither would properties, because the actual property names and values would be specific to the implementation in use.

Will


On 07/24/2017 02:54 AM, Rudy De Busscher wrote:
Guillermo,

We can't add additional methods to an interface later on. This would make WAR files packages and compiled against JAVA EE 8, break when they are deployed on a Java EE 9 server for example which has the additional methods in the interface.

So if we decide now (today?) that they are out, they are out forever.

Or we have to define new interface classes.

Rudy


On 24 July 2017 at 07:46, Guillermo González de Agüero <z06.guillermo@...> wrote:
I was going to raise the same concern, although I think properties could be supported in the future by providing a new method and specifying that the old one y to be used when there are no parameters defined, and the new one when there are.

If that is acceptable and can be done in the future, I think it's not such a big deal to leave them for now if the EG don't find consensus.


Regards,

Guillermo González de Agüero


El lun., 24 de julio de 2017 6:49, Rudy De Busscher <rdebusscher@...> escribió:
Will,

regarding the properties. Indeed when we do not support the properties, they don't need to be specified at the method.

But then, we will never be able to support them in any further version since it would break the backward compatibility of the interface methods. Once this release is out, we can't change anything anymore to interface (no change in the method parameters, method count, etc ... only we can add default implemented methods)

So I guess it is in now or leave out for ever.

Rudy



On 23 July 2017 at 23:31, Will Hopkins <will.hopkins@...> wrote:
Hi Rudy,

Thanks for the feedback, responses inline.

Will

On 07/23/2017 08:05 AM, Rudy De Busscher wrote:
Will, See inline comments (hope it is clear enough, I'll put them also in a separate thread)


On 22 July 2017 at 23:17, Arjan Tijms <arjan.tijms@...> wrote:
Hi,

On Sat, Jul 22, 2017 at 10:06 PM, Will Hopkins <will.hopkins@...> wrote:
Arjan, et al.:

I've pushed some changes to the hashalgorithm branch in both security-api and security-soteria; please have a look to see what you think.
  • I considered PasswordHashAlgorithm, but that seemed too long, especially for interfaces/classes that would extend it. 


"PasswordHashAlgorithm" certainly doesn't feel too long for me.
 
  • Also renamed IdentityHashAlgorithm to PlaintextPasswordHash.

Ok 
  • Added generateHash() method.

Ok 
  • Removed hash parameters passed to verifyHash(). I did not remove the DatabaseIdentityStoreDefinition hashProperties attribute, nor did I remove the "plumbing" that passees the properties down to the hash implementation, in case we want to restore the properties. But they are not currently passed in to the PasswordHash.

Even if the standard Pbkdf2PasswordHashImpl doesn't use it, I think it doesn't hurt much to pass them in anyway.

The parameters are required. if the PasswordHash interface doesn't have methods with the parameters passed along, there is no implemtattion ever that can use them!

I assume what you mean is that the methods need the parameters passed along if we retain the hashProperties attribute ... correct?

The intent would be to either support properties, or remove the hashProperties attribute on DatabaseIdentityStoreDefinition. My change leaves them on the annotation because I didn't feel like we'd made a final decision yet, and didn't want to have to add all that back if we decided to keep them. If we don't, we would remove the annotation attribute.

 
 
  • Implemented a (more or less) constant time compare for passwords/hashes, and updated the two PasswordHash impl classes to use it.
  • Re-wrote Pbkdf2PasswordHashImpl to:
    • Generate hashes with a specific storage format (encoding), and parse that encoding when doing verifyHash(), such that the verify operation can use the algorithm/parameters/salt that were used to hash the password originally, instead of the currently configured paramters.
    • Added support for multiple flavors of PBKDF2 (i.e., "with" different Hmac algorithms). Now supports all SHA1/2 variants, but not MD5 (which doesn't appear to be available, or at least not enabled by default, in JDK8).
    • Changed the default parameters to be more in line with current best practices.

Ok

I would also consider removing SHA1. Just as MD5, it is no longer considered as really safe.
Good point, I'll do that. My thought in keeping it (and perhaps MD5, if it worked) was to support passwords hashed with legacy algorithms, but that doesn't make much sense since it's unlikely they'd be encoded the same way in the database, so the default PasswordHashAlgorithm wouldn't work anyway.

 
 
So, this is what I had in mind. It's really not so different from what you coded up, Arjan, except for some of the algorithm implementation details, the lack of properties support, and the flexible encoding format that allows default parameters to be changed while preserving the ability to verify passwords hashed with different parameters.

My reasoning behind removing the properties support is as follows:
  • The application developer already has complete control because he/she can provide a custom algorithm to meet any requirement. Properties may be convenient in some cases, but even without them the developer can do whatever they need to do.
  • We are already introducing a very large change after PFD publication. Properties mean more to specify and document, and, more crucially, they mean more test assertions and TCK test cases to write. There is already concern internally about our ability to complete the TCK on schedule.

Ok
 
  • As is hopefully made more clear by my implementation code, the algorithm itself, and related parameters, are not really the source of most variation and complexity for password hashes -- it's the content and format of the encoded hashed password, as stored in the database, that seems likely to be infinitely more variable. There are likely a number of fairly common formats, but I suspect most are ad hoc, and practices are changing rapidly over time. In any event, we simply don't have time to research that question to figure out if there are any formats common enough to standardize on. (Nor do we have time to develop and implement some kind of expression syntax to specifies what the encoding should look like.)

As I mentioned, the encoding step is very likely common enough to warrant having its own method in the interface, so developers can e.g. decorate the default algorithm and only change the encoding.

 

As a result, I think it's highly unlikely that the ability to supply properties is going to be enough to allow developers to customize everything they'll want or need to

Like the entire concept of the DatabaseIdentityStore, it's indeed unlikely to ever cover everything everyone ever wants, but the 80% rule comes in play here; it's okay if we design for the 80% and then the 20% can do whatever they want in a custom implementation.

I personally still feel parameters with reasonable defaults are still a good intermediate solution between using the standard algorithm as is, and having to write something from scratch.

That said, if there's no time, there's no time. Or in other words, it is what it is ;)


But it needs to be useable. Otherwise it is better to deliver nothing then something which isn't working properly and useable!
If the perception of Java EE security API is not good during the first months, it will never be used by anyone, even if we come up with a better version later on.

Sorry to be this hard, but that is the thruth.
I agree that it needs to be usable, but, to me, the properties/no properties question doesn't seem to be a fatal flaw -- properties would be convenient, in some cases, but lack of properties won't prevent a developer (or platform vendor) from implementing any algorithm(s) they need. Do you see it differently?

 
Kind regards,
Arjan Tijms


 
, even when they're creating a new identity store and have complete control over the format. In the case that they need to develop for an existing identity store, with existing password hash formats, the chances are close to zero that a generic password hash implementation could be adequately customized with properties.
With all of that in mind, I do think properties are a nice feature, but they don't seem absolutely necessary -- developers have full control without them -- and it's not clear they'd really be useful in very many scenarios. I suspect they would mostly be used to fiddle with parameters in cases where the default storage encoding is deemed to be acceptable, and that, in most cases, the default algorithm/parameters would probably be deemed acceptable as well, if properties weren't available. As such, I'm reluctant to take on the extra complexity and work required to support properties at this point.

Anyway, that's my proposal. Thoughts?

Will


A small detail anout the code, I would make it more Java 8 alike ( with diamond operator, foreach method, ....)

If you like, I can make a PR for that.
Sure, if you want, but let's wait until we're fully decided on the approach. Also, did you sign the OCA yet? I don't understand why that's suddenly a requirement, but apparently it is.


Rudy
 

On 07/20/2017 03:25 PM, Arjan Tijms wrote:
Hi,

The adaptability concern is basically the main concern of the entire DataBaseIdentityStore. Indeed, having a query to get the (hashed) password from the caller name and another to get the groups covers maybe 80% of use cases, but certainly not all.

There are for instance exotic database schemes were you'd need at least 2 seperate queries to get that data.

For the hash algorithm the same thing holds. We need to specify enough moving parts so we can adapt to *many* existing databases and requirements, but we shouldn't even be dreaming about covering them all.

Since parameters can contain EL as values as well, it wouldn't be crazy hard really to supply the encoding as a method expression. It's basically a byte[] -> String function.

In this case though I think encoding is a general enough moving part to warrant its own method in the interface. It can easily be defaulted to e.g. base64 and it can be overridden without exposing the implementation by use of a CDI decorator (or should be after we solve a bug or problem that currently prevents decorating).

Kind regards,
Arjan Tijms


On Thursday, July 20, 2017, Will Hopkins <will.hopkins@...> wrote:


On 07/20/2017 12:38 PM, Guillermo González de Agüero wrote:


Similarly, parameters like the salt and iteration count can be defaulted. That way you can have 3 levels:

1. Specify only the class name; you get defaults for all parameters and it Just Works
2. Specify the parameters locally to match a specific DB or requirement
3. (via EL) fetch the parameter values externally

The most important rule should always be that the developer is in full control, or can be in full control. External overrides should be possible, but never be required. That is -the- big mistake that Java EE made in the past.
I agree, but I think we're only talking about what the mechanism for override is, and what the trade-off between simplicity and the override mechanism is. In practice, I don't think parameters are going to be very helpful to most developers. They can be used to tweak some simple things, but the simple things won't typically be necessary and the more complex things can't be handled by parameters.
 
Could you please provide an example of such a setting, please? Just to better understand. Otherwise, we could just wait for your new proposal to see.

Basically, it's the encoding of the hashed password. Parameters like salt, iterations, hash key size, etc. are all pretty standard. They can be dialed up or dialed down to make the hash stronger or weaker, and I think "stronger vs. weaker" is the level at which most people think about them. I doubt many developers are concerned about specifying precisely a certain number of iterations for some reason, they just want iterations count to be high enough to be secure, without imposing excessive overhead or resource consumption, and security most often wins out vs. resource consumption these days. Most will be happy if they can turn up the volume with a single knob, they don't need bass and treble and balance knobs. Even better: if the default volume is about right, and if the volume automatically goes up when you're driving at higher speeds.

In contrast, the format of the encoded string is not standard in any way that I'm aware of. There are probably LDAP vendors that do things in a common way, but for any given database user store, either the format has already been decided, because there are existing user accounts, or the developer won't care as long as the default format is reasonable (assuming the DB schema allows enough space for the chosen encoding format.

I can't think of any good way to standardize parameters to describe how the hashed password is encoded for storage, and that's the hard part, the part that you really have to adapt to. It's not a big deal if you have to use 4000 iterations instead of 2000, but it's a big deal if you can't parse the encoded password. And since the encoding is completely arbitrary and non-standard, that's the real problem, and that's what will likely force people to do their own implementation of HashAlgorithm. That can't be specified as Properties.

We could probably come up with an interface for encoding/decoding the hashed value, or for describing the encoded value as a string and passing it as a property. But that's a lot to spec out, and we just don't have time for it. It's complicated by the fact that decoding a hash potentially yields all the parameters that were used to generate the hash, so there'd need to be a generalized interface for returning those in a standard way. Plus, in some databases, the encodings used will vary, as legacy hashes are replaced with stronger hashes/different encodings when people change their password.

It's hard to argue against supplying Properties. It's not difficult to implement, and it might be useful. But I'm trying to keep the interface as simple as possible -- in part to reduce the number of additional TCK tests that need to be written -- and I suspect the Properties will rarely be useful. Some people will want to use them, but for many the defaults will be good enough, and many of those with specific requirements will already be forced to write an implementation in order to deal with encoding issues.

Will
-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803

-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803



-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803



-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803


-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803


Re: LDAP Annotation and Database Hashing Proposal

Rudy De Busscher
 

  • Implementations can provide their own configuration mechanisms -- a standard config object that's injected, decorators, annotations of their own, abstract impls that developers extend to customize and deploy, etc. Those won't be standard, but neither would properties, because the actual property names and values would be specific to the implementation in use.
And that is what I want to avoid, that each vendor uses his own way of configuration.

The database access and hashing algorithms are then may be standardized, but when each vendor uses his own way of configuration, we are back to square one. No compatibility between the different servers8 So what is the point then of our effort?

Property names won't be standardized, I agree. But we have the same situation for JPA. There you have also properties (the hibernate.xxxx and eclipselink.xxxx values)

But at least you need to specify them in the persistence.xml file and not within other areas (like environment variables, server xml files, JNDI, ... ) for each server differently.

Rudy


On 24 July 2017 at 10:14, Will Hopkins <will.hopkins@...> wrote:
All true.

A couple of counter-points to ponder:
  • Property values will never be standard, even if we support passing them. At best they would be standard for the default PasswordHash that is supplied by the RI. A developer passing properties would have to understand the specifics of the implementation being used.
  • Passing it to the method, rather than to the constructor or an init method. And we won't ever be in a position to pass them to generateHash(), so it can never behave consistently with verifyHash.
  • Implementations can provide their own configuration mechanisms -- a standard config object that's injected, decorators, annotations of their own, abstract impls that developers extend to customize and deploy, etc. Those won't be standard, but neither would properties, because the actual property names and values would be specific to the implementation in use.

Will


On 07/24/2017 02:54 AM, Rudy De Busscher wrote:
Guillermo,

We can't add additional methods to an interface later on. This would make WAR files packages and compiled against JAVA EE 8, break when they are deployed on a Java EE 9 server for example which has the additional methods in the interface.

So if we decide now (today?) that they are out, they are out forever.

Or we have to define new interface classes.

Rudy


On 24 July 2017 at 07:46, Guillermo González de Agüero <z06.guillermo@...> wrote:
I was going to raise the same concern, although I think properties could be supported in the future by providing a new method and specifying that the old one y to be used when there are no parameters defined, and the new one when there are.

If that is acceptable and can be done in the future, I think it's not such a big deal to leave them for now if the EG don't find consensus.


Regards,

Guillermo González de Agüero


El lun., 24 de julio de 2017 6:49, Rudy De Busscher <rdebusscher@...> escribió:
Will,

regarding the properties. Indeed when we do not support the properties, they don't need to be specified at the method.

But then, we will never be able to support them in any further version since it would break the backward compatibility of the interface methods. Once this release is out, we can't change anything anymore to interface (no change in the method parameters, method count, etc ... only we can add default implemented methods)

So I guess it is in now or leave out for ever.

Rudy



On 23 July 2017 at 23:31, Will Hopkins <will.hopkins@...> wrote:
Hi Rudy,

Thanks for the feedback, responses inline.

Will

On 07/23/2017 08:05 AM, Rudy De Busscher wrote:
Will, See inline comments (hope it is clear enough, I'll put them also in a separate thread)


On 22 July 2017 at 23:17, Arjan Tijms <arjan.tijms@...> wrote:
Hi,

On Sat, Jul 22, 2017 at 10:06 PM, Will Hopkins <will.hopkins@...> wrote:
Arjan, et al.:

I've pushed some changes to the hashalgorithm branch in both security-api and security-soteria; please have a look to see what you think.
  • I considered PasswordHashAlgorithm, but that seemed too long, especially for interfaces/classes that would extend it. 


"PasswordHashAlgorithm" certainly doesn't feel too long for me.
 
  • Also renamed IdentityHashAlgorithm to PlaintextPasswordHash.

Ok 
  • Added generateHash() method.

Ok 
  • Removed hash parameters passed to verifyHash(). I did not remove the DatabaseIdentityStoreDefinition hashProperties attribute, nor did I remove the "plumbing" that passees the properties down to the hash implementation, in case we want to restore the properties. But they are not currently passed in to the PasswordHash.

Even if the standard Pbkdf2PasswordHashImpl doesn't use it, I think it doesn't hurt much to pass them in anyway.

The parameters are required. if the PasswordHash interface doesn't have methods with the parameters passed along, there is no implemtattion ever that can use them!

I assume what you mean is that the methods need the parameters passed along if we retain the hashProperties attribute ... correct?

The intent would be to either support properties, or remove the hashProperties attribute on DatabaseIdentityStoreDefinition. My change leaves them on the annotation because I didn't feel like we'd made a final decision yet, and didn't want to have to add all that back if we decided to keep them. If we don't, we would remove the annotation attribute.

 
 
  • Implemented a (more or less) constant time compare for passwords/hashes, and updated the two PasswordHash impl classes to use it.
  • Re-wrote Pbkdf2PasswordHashImpl to:
    • Generate hashes with a specific storage format (encoding), and parse that encoding when doing verifyHash(), such that the verify operation can use the algorithm/parameters/salt that were used to hash the password originally, instead of the currently configured paramters.
    • Added support for multiple flavors of PBKDF2 (i.e., "with" different Hmac algorithms). Now supports all SHA1/2 variants, but not MD5 (which doesn't appear to be available, or at least not enabled by default, in JDK8).
    • Changed the default parameters to be more in line with current best practices.

Ok

I would also consider removing SHA1. Just as MD5, it is no longer considered as really safe.
Good point, I'll do that. My thought in keeping it (and perhaps MD5, if it worked) was to support passwords hashed with legacy algorithms, but that doesn't make much sense since it's unlikely they'd be encoded the same way in the database, so the default PasswordHashAlgorithm wouldn't work anyway.

 
 
So, this is what I had in mind. It's really not so different from what you coded up, Arjan, except for some of the algorithm implementation details, the lack of properties support, and the flexible encoding format that allows default parameters to be changed while preserving the ability to verify passwords hashed with different parameters.

My reasoning behind removing the properties support is as follows:
  • The application developer already has complete control because he/she can provide a custom algorithm to meet any requirement. Properties may be convenient in some cases, but even without them the developer can do whatever they need to do.
  • We are already introducing a very large change after PFD publication. Properties mean more to specify and document, and, more crucially, they mean more test assertions and TCK test cases to write. There is already concern internally about our ability to complete the TCK on schedule.

Ok
 
  • As is hopefully made more clear by my implementation code, the algorithm itself, and related parameters, are not really the source of most variation and complexity for password hashes -- it's the content and format of the encoded hashed password, as stored in the database, that seems likely to be infinitely more variable. There are likely a number of fairly common formats, but I suspect most are ad hoc, and practices are changing rapidly over time. In any event, we simply don't have time to research that question to figure out if there are any formats common enough to standardize on. (Nor do we have time to develop and implement some kind of expression syntax to specifies what the encoding should look like.)

As I mentioned, the encoding step is very likely common enough to warrant having its own method in the interface, so developers can e.g. decorate the default algorithm and only change the encoding.

 

As a result, I think it's highly unlikely that the ability to supply properties is going to be enough to allow developers to customize everything they'll want or need to

Like the entire concept of the DatabaseIdentityStore, it's indeed unlikely to ever cover everything everyone ever wants, but the 80% rule comes in play here; it's okay if we design for the 80% and then the 20% can do whatever they want in a custom implementation.

I personally still feel parameters with reasonable defaults are still a good intermediate solution between using the standard algorithm as is, and having to write something from scratch.

That said, if there's no time, there's no time. Or in other words, it is what it is ;)


But it needs to be useable. Otherwise it is better to deliver nothing then something which isn't working properly and useable!
If the perception of Java EE security API is not good during the first months, it will never be used by anyone, even if we come up with a better version later on.

Sorry to be this hard, but that is the thruth.
I agree that it needs to be usable, but, to me, the properties/no properties question doesn't seem to be a fatal flaw -- properties would be convenient, in some cases, but lack of properties won't prevent a developer (or platform vendor) from implementing any algorithm(s) they need. Do you see it differently?

 
Kind regards,
Arjan Tijms


 
, even when they're creating a new identity store and have complete control over the format. In the case that they need to develop for an existing identity store, with existing password hash formats, the chances are close to zero that a generic password hash implementation could be adequately customized with properties.
With all of that in mind, I do think properties are a nice feature, but they don't seem absolutely necessary -- developers have full control without them -- and it's not clear they'd really be useful in very many scenarios. I suspect they would mostly be used to fiddle with parameters in cases where the default storage encoding is deemed to be acceptable, and that, in most cases, the default algorithm/parameters would probably be deemed acceptable as well, if properties weren't available. As such, I'm reluctant to take on the extra complexity and work required to support properties at this point.

Anyway, that's my proposal. Thoughts?

Will


A small detail anout the code, I would make it more Java 8 alike ( with diamond operator, foreach method, ....)

If you like, I can make a PR for that.
Sure, if you want, but let's wait until we're fully decided on the approach. Also, did you sign the OCA yet? I don't understand why that's suddenly a requirement, but apparently it is.


Rudy
 

On 07/20/2017 03:25 PM, Arjan Tijms wrote:
Hi,

The adaptability concern is basically the main concern of the entire DataBaseIdentityStore. Indeed, having a query to get the (hashed) password from the caller name and another to get the groups covers maybe 80% of use cases, but certainly not all.

There are for instance exotic database schemes were you'd need at least 2 seperate queries to get that data.

For the hash algorithm the same thing holds. We need to specify enough moving parts so we can adapt to *many* existing databases and requirements, but we shouldn't even be dreaming about covering them all.

Since parameters can contain EL as values as well, it wouldn't be crazy hard really to supply the encoding as a method expression. It's basically a byte[] -> String function.

In this case though I think encoding is a general enough moving part to warrant its own method in the interface. It can easily be defaulted to e.g. base64 and it can be overridden without exposing the implementation by use of a CDI decorator (or should be after we solve a bug or problem that currently prevents decorating).

Kind regards,
Arjan Tijms


On Thursday, July 20, 2017, Will Hopkins <will.hopkins@...> wrote:


On 07/20/2017 12:38 PM, Guillermo González de Agüero wrote:


Similarly, parameters like the salt and iteration count can be defaulted. That way you can have 3 levels:

1. Specify only the class name; you get defaults for all parameters and it Just Works
2. Specify the parameters locally to match a specific DB or requirement
3. (via EL) fetch the parameter values externally

The most important rule should always be that the developer is in full control, or can be in full control. External overrides should be possible, but never be required. That is -the- big mistake that Java EE made in the past.
I agree, but I think we're only talking about what the mechanism for override is, and what the trade-off between simplicity and the override mechanism is. In practice, I don't think parameters are going to be very helpful to most developers. They can be used to tweak some simple things, but the simple things won't typically be necessary and the more complex things can't be handled by parameters.
 
Could you please provide an example of such a setting, please? Just to better understand. Otherwise, we could just wait for your new proposal to see.

Basically, it's the encoding of the hashed password. Parameters like salt, iterations, hash key size, etc. are all pretty standard. They can be dialed up or dialed down to make the hash stronger or weaker, and I think "stronger vs. weaker" is the level at which most people think about them. I doubt many developers are concerned about specifying precisely a certain number of iterations for some reason, they just want iterations count to be high enough to be secure, without imposing excessive overhead or resource consumption, and security most often wins out vs. resource consumption these days. Most will be happy if they can turn up the volume with a single knob, they don't need bass and treble and balance knobs. Even better: if the default volume is about right, and if the volume automatically goes up when you're driving at higher speeds.

In contrast, the format of the encoded string is not standard in any way that I'm aware of. There are probably LDAP vendors that do things in a common way, but for any given database user store, either the format has already been decided, because there are existing user accounts, or the developer won't care as long as the default format is reasonable (assuming the DB schema allows enough space for the chosen encoding format.

I can't think of any good way to standardize parameters to describe how the hashed password is encoded for storage, and that's the hard part, the part that you really have to adapt to. It's not a big deal if you have to use 4000 iterations instead of 2000, but it's a big deal if you can't parse the encoded password. And since the encoding is completely arbitrary and non-standard, that's the real problem, and that's what will likely force people to do their own implementation of HashAlgorithm. That can't be specified as Properties.

We could probably come up with an interface for encoding/decoding the hashed value, or for describing the encoded value as a string and passing it as a property. But that's a lot to spec out, and we just don't have time for it. It's complicated by the fact that decoding a hash potentially yields all the parameters that were used to generate the hash, so there'd need to be a generalized interface for returning those in a standard way. Plus, in some databases, the encodings used will vary, as legacy hashes are replaced with stronger hashes/different encodings when people change their password.

It's hard to argue against supplying Properties. It's not difficult to implement, and it might be useful. But I'm trying to keep the interface as simple as possible -- in part to reduce the number of additional TCK tests that need to be written -- and I suspect the Properties will rarely be useful. Some people will want to use them, but for many the defaults will be good enough, and many of those with specific requirements will already be forced to write an implementation in order to deal with encoding issues.

Will
-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803

-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803



-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803



-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803


201 - 220 of 736