Hi,
On Tue, Oct 6, 2015 at 1:45 AM, Alex Kosowski <alex.kosowski_at_oracle.com> wrote:
> I think we want to standardize a simple, read-only IdentityStore API for
> 1.0.
100% agree with this. There are many advanced ideas floating around,
but there's some really low hanging fruit out there representing the
basic foundation that should be addressed first.
If we could only get the bare minimum of the identity store interface
itself as asked for by
https://java.net/jira/browse/JAVAEE_SECURITY_SPEC-18
And then a couple of standard implementations as asked for by
https://java.net/jira/browse/JAVAEE_SECURITY_SPEC-9
Then this would be a huge win already. The current proposal addresses
these two concerns quite well I think.
Reza Rahman (the reporter of both issues) said it well with "I think
in the bare minimum we need to have something as simple as this" ;)
p.s. on this topic, I did some research on how a number of existing
servers define their identity store and how they use it from their
authentication mechanism here:
http://arjan-tijms.omnifaces.org/2015/10/how-servlet-containers-all-implement.html
Kind regards,
Arjan Tijms
>
> Regards,
> Alex
>
>
> On 9/30/15 1:05 AM, Rudy De Busscher wrote:
>
> Hi All,
>
>> I do wonder if the getCallerGroups() and getCallerRoles() methods are
>> needed here. An authentication mechanism would not likely use these,
>> right?
>
>
> Is indeed a good remark. What if some of those application don't need any
> authorization, but just authentication.
>
> And I know there are a few issues around CDI and generics, but I love to see
> the interface based (or the authentication version) on a generic type.
> So that my custom identityStore could return objects instead of Strings
>
> public interface IdentityStore<T> {
> List<T> getCallerGroups(String callerName);
> List<T> getCallerRoles(String callerName);
> public CredentialValidationResult validate(Credential credential);
> }
>
>
> Didn't had the time to check all the material (project deadline by the end
> of the week and still quite some things to do :( ) but will the CRUD version
> of the IdentityStore be an Extends of this readonly version?
>
> Regards
> Rudy
>
> On 29 September 2015 at 13:57, arjan tijms <arjan.tijms_at_gmail.com> wrote:
>>
>> Hi,
>>
>> On Tue, Sep 29, 2015 at 7:42 AM, Alex Kosowski <alex.kosowski_at_oracle.com>
>> wrote:
>>
>> > Features:
>> > 1. Follows the simple model: Credentials in -> Caller, groups, roles out
>> > 2. The IdentityStore is read only, no create/update/delete for callers,
>> > groups, roles or credentials
>> > 3. Caller, Groups and Roles are just Strings, and are searchable by
>> > regular
>> > expression
>> > 4. Credential support is extendable by using CDI Qualifiers to annotate
>> > a
>> > CredentialValidator implementation
>> > 5. Standardized support for credentials, including Username/Password,
>> > Basic
>> > Authentication, Token
>> > 6. Standardized support for persistence mechanisms, including file,
>> > LDAP,
>> > database, and a JAAS adapter
>>
>> This looks very good. In fact, it's the best proposal I've seen till date.
>>
>> I do have a few comments (as usual :P):
>>
>> The IdentityStore's interface is now this:
>>
>> public interface IdentityStore {
>> List<String> getCallerGroups(String callerName);
>> List<String> getCallerRoles(String callerName);
>> public CredentialValidationResult validate(Credential credential);
>> }
>>
>> A tiny typo here; the "public" modifier is not needed of course on the
>> validate() method.
>>
>> The validate() method itself is really nice. Just passing in a
>> Credential is much better than the often seen {username, credential}
>> pair, as not every mechanism has an explicit username being passed in
>> (like e.g. certificates).
>>
>> The CredentialValidationResult is conceptually really nice as well. It
>> packs both the result status, as the result data. This is quite a bit
>> better as many existing mechanisms which depend on either an exception
>> being thrown (e.g. LoginException) or a null, and/or depend on
>> followup calls to the identity store for the groups and/or depend on
>> grabbing this data from some (TLS based) context.
>>
>> I do wonder if the getCallerGroups() and getCallerRoles() methods are
>> needed here. An authentication mechanism would not likely use these,
>> right? I do like to have the ability to ask what groups a user has
>> according to the identity store, but maybe these should be on a
>> different (optional?) interface, along with a third method
>> List<String> getCallers()? (which has to be carefully considered, as
>> the result may be too big for some stores).
>>
>> As for the implementation of CredentialValidationResult, it now
>> contains this constructor:
>>
>> public CredentialValidationResult(
>> Status status,
>> String callerName,
>> IdentityStore identityStore) {
>>
>> this(status, callerName,
>> getCallerGroups(status,callerName,identityStore),
>> getCallerRoles(status, callerName, identityStore));
>> }
>>
>> see:
>> https://github.com/javaee-security-spec/javaee-security-proposals/blob/master/identity-store-readonly/src/main/java/javax/security/identitystore/CredentialValidationResult.java#L92
>>
>> This calls out to the following static method:
>>
>> private static List<String> getCallerGroups(Status status, String
>> callerName, IdentityStore identityStore) {
>> if (null == identityStore)
>> throw new NullPointerException("identityStore");
>>
>> if (VALID == status) {
>> return identityStore.getCallerGroups(callerName);
>> } else {
>> return null;
>> }
>> }
>>
>> I wonder if this was the main reason for having
>> getCallerGroups()/getCallerRoles() on the IdentityStore interface?
>>
>> For the sake of simplicity, maybe it's better to omit this constructor
>> and the associated private static methods?
>>
>>
>> > Here is a usage case example:
>> >
>> > @Inject
>> > IdentityStore idStore;
>>
>> Perhaps it's better to have the credential type as a qualifier of the
>> IdentityStore?
>>
>> After all, the authentication mechanism already knows what the
>> credential type is (it has to interact with the user to obtain it and
>> instantiate a suitable type). With a qualified IdentityStore, a user
>> can have multiple stores available, one for each credential type.
>>
>> E.g.
>>
>> @Inject @CredentialCapable(UsernamePasswordCredential.class)
>> IdentityStore idStore;
>>
>> Then what an IdentityStore instance does inside its validate() method
>> is largely a concern of that implementation. The API can provide some
>> guidelines perhaps, but I would really hold back on putting any hard
>> requirements here (it's different of course for the default
>> implementations like mentioned in point 6 above).
>>
>> So you could have e.g. the following implementation:
>>
>> @RequestScoped
>> @CredentialCapable(UsernamePasswordCredential.class)
>> public QuickTestIdentityStore implements IdentityStore {
>>
>> public CredentialValidationResult validate(Credential credential) {
>> UsernamePasswordCredential usernamePasswordCredential =
>> (UsernamePasswordCredential) credential;
>>
>> if ("john".equals(usernamePasswordCredential.getCaller()) {
>> return new CredentialValidationResult(
>> VALID,
>> "john.harrison",
>> new ArrayList(asList("admin", "sysop", "dev")),
>> new ArrayList(asList("can_update_accounts",
>> "can_see_updates"))
>> }
>>
>> return INVALID_RESULT;
>> }
>> }
>>
>> Regarding the name "UsernamePasswordCredential", shouldn't it be
>> "CallerNamePasswordCredential"?
>>
>> The constructor already hints at this, as "callerName" is passed in
>> for the "username":
>>
>> public UsernamePasswordCredential(String callerName, Password password)
>>
>>
>> Regarding the roles that are returned now; how does this interact with
>> group to role mapping? Can the identity store leave this empty, and
>> would the authorization system (e.g. JACC or proprietary) be
>> responsible for the mapping then (as happens today), or should the
>> IdentityStore always be injected with the GroupToRole mapper and do
>> this mapping?
>>
>> All existing authentication mechanisms and specifically JASPIC only
>> need groups. They'd have to ignore the roles anyway.
>>
>>
>> We may want to specify which things should never be null. E.g. should
>> both groups and roles not better always be the empty list instead of
>> null?
>>
>> Same thing for the CredentialValidationResult returned from validate.
>> Maybe this should also never be null?
>>
>> /**
>> * Validates the given credential.
>> *
>> * @param credential The credential
>> * @return The validation result, including associated caller
>> roles and groups. Never null.
>> */
>> public CredentialValidationResult validate(Credential credential);
>>
>>
>> Hope this feedback is useful.
>>
>> Kind regards,
>> Arjan Tijms
>>
>>
>>
>>
>>
>>
>>
>> >
>> > // For example, in a JASPIC SAM...
>> > String caller = null;
>> > List<String> groups = null;
>> > List<String> roles = null;
>> > CredentialValidationResult result;
>> > Credential cred;
>> >
>> > cred = new UsernamePasswordCredential("john",new Password("secret"));
>> > result = idStore.validate(cred);
>> > if (Status.VALID == result.getStatus()) {
>> > // authentication was successful
>> > caller = result.getCallerName();
>> > groups = result.getCallerGroups();
>> > roles = result.getCallerRoles();
>> >
>> > // Callback JASPIC
>> > } else {
>> > // Invalid or not validated
>> > }
>> >
>> > I completely rewrote the Proof of Concept (POC) from the previous
>> > iteration
>> > and updated the proposal design doc. Let's discuss any issues you may
>> > have
>> > with this design.
>> >
>> > Please comment on this proposal Google doc:
>> >
>> > https://docs.google.com/document/d/1xMa32W73gPYYo53wRX60WasDDTuC7YFlI0XBm3dRym8/edit?usp=sharing
>> >
>> > The proposal Google doc should be open for comments by anyone on the
>> > jsr375-experts_at_googlegroups.com Google group. If you are having trouble
>> > commenting, please let me know. To comment, click the Comments button on
>> > the
>> > top right of the document.
>> >
>> > Here is the POC in the GitHub Proposal Repo:
>> >
>> > https://github.com/javaee-security-spec/javaee-security-proposals/tree/master/identity-store-readonly
>> >
>> > Note that the DatabaseIdentityStore in the POC is without an
>> > implementation
>> > because I ran out of time.
>> >
>> > Here is the generated JavaDoc:
>> > https://javaee-security-spec.java.net/
>> >
>> > What do you think? Once we come to a consensus for an acceptable
>> > Identity
>> > Store API design, the proposal will become the basis for the Identity
>> > Store
>> > section of the spec.
>> >
>> > With regards,
>> > Alex
>> >
>
>