Hi Arjan,
Thanks for your comments! Please see inline.
Regards,
Alex
On 9/29/15 7:57 AM, arjan tijms 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.
[Alex] This is a reflection of the incremental improvements driven by
the EG comments. Truly a group effort.
> 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.
[Alex] Fixed in the next revision
>
>
> 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).
[Alex] Agreed
Why are getCallerGroups and getCallerRoles in the IdentityStore API?
getCallerGroups and getCallerRoles were originally SPI contracts. Since
CredentialValidationResult may return groups and roles for the caller,
getCallerGroups and getCallerRoles were the SPI to get that data from
the persistence mechanism. If getCallerGroups or getCallerRoles are not
supported, a developer could extend the IdentityStore to implement
getCallerGroups and getCallerRoles for a persistence mechanism. Since
we would require IdentityStore implementations to implement
getCallerGroups and getCallerRoles anyway, why not make it public in the
API for convenience.
That said, the POC revealed getCallerGroups and getCallerRoles may not
be that useful as an SPI. It would work great for the
CachedIdentityStore, since all data are readily available. But for LDAP
and database, there may be optimizations during validation that would
not require separate calls to getCallerGroups and getCallerRoles. For
example, a database query may validate and return groups and roles in
one remote call, not requiring a 2nd invocation for getCallerGroups and
a 3rd invocation for getCallerRoles.
So, I agree that getCallerGroups and getCallerRoles should be removed
from the IdentityStore API. We need to better define how to customize
the standard IdentityStore LDAP and Database implementations to
efficiently get groups and roles during credential validation.
As for moving getCallerGroups and getCallerRoles....
A version of getCallerRoles is already in
javax.security.identitystore.query.CallerRoleMap. So, we could keep that
as is, for now.
https://github.com/javaee-security-spec/javaee-security-proposals/blob/master/identity-store-readonly/src/main/java/javax/security/identitystore/query/CallerRoleMap.java
As for getCallerGroups, the reverse method getCallersInGroup is in the
GroupStore interface. We could add getCallerGroups into GroupStore.
https://github.com/javaee-security-spec/javaee-security-proposals/blob/master/identity-store-readonly/src/main/java/javax/security/identitystore/query/GroupStore.java
Or perhaps represent the Caller-Group membership relationship in a new
dedicated interface like "CallerGroupMap". The CallerGroupMap interface
would be similar to CallerRoleMap, and would contain both
getCallerGroups and getCallersInGroup methods. I am leaning toward the
separate CallerGroupMap interface, since it adds incremental
caller-group membership behavior.
> 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?
[Alex] Agreed. If getCallerGroups and getCallerRoles are coming out of
the IdentityStore, no need for the constructor with IdentityStore
in CredentialValidationResult.
>> 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;
> }
> }
[Alex] When adding custom Credential support, this approach would
require one to refactor existing/tested code in existing IdentityStore
implementations.
The CredentialValidator approach separates the credential-specific logic
away from the persistence IdentityStore. Using the CredentialValidator,
custom credential support may be added to an existing IdentityStore
simply by adding a new jar to the classpath which contains the
Credential and CredentialValidator implementations.
Regarding:
> With a qualified IdentityStore, a user
> can have multiple stores available, one for each credential type.
>
> E.g.
>
> @Inject @CredentialCapable(UsernamePasswordCredential.class)
> IdentityStore idStore;
Isn't this a bit of an edge case?
So are you saying:
@Inject @CredentialCapable(UsernamePasswordCredential.class)
IdentityStore usernamePasswordStore;
@Inject @CredentialCapable(BasicAuthenticationCredential.class)
IdentityStore basicAuthStore;
@Inject @CredentialCapable(TokenCredential.class)
IdentityStore tokenStore;
The CredentialValidator approach would support this using a qualified
producer.
@Produces
@CredentialCapable(UsernamePasswordCredential.class)
public IdentityStore getIdentityStore() {
...
// Instantiate store
return new LdapIdentityStore(...);
}
@Produces
@CredentialCapable(TokenCredential.class)
public IdentityStore getIdentityStore() {
...
// Instantiate store
return new DatabaseIdentityStore(...);
}
> 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)
[Alex] Agreed
>
> Regarding the roles that are returned now; how does this interact with
> group to role mapping?
[Alex] The roles returned are optional, in case the IdentityStore does
not support role mapping.
> Can the identity store leave this empty,
[Alex] Yes, From the JavaDoc for CredentialValidationResult.getCallerRoles:
RETURNS: The list of roles that the specified caller is in, empty if
none. null if getStatus() does not return VALID or if the identity store
does not support roles.
> and
> would the authorization system (e.g. JACC or proprietary) be
> responsible for the mapping then (as happens today)
[Alex] yes
> , or should the
> IdentityStore always be injected with the GroupToRole mapper and do
> this mapping?
[Alex] If the IdentityStore supports role mapping, the IdentityStore
would determine the mapping based on the persistence store (e.g. a role
map table in a database). We could standardize configuration for the
role map queries for database, or DN entry pattern for LDAP.
> All existing authentication mechanisms and specifically JASPIC only
> need groups. They'd have to ignore the roles anyway.
[Alex] I think we discussed earlier in June, and I saw a comment in a
JIRA issue
https://java.net/jira/browse/JAVAEE_SECURITY_SPEC-8?focusedCommentId=388188&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-388188
<
https://java.net/jira/browse/JAVAEE_SECURITY_SPEC-8?focusedCommentId=388188&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-388188>
,
that role mapping could potentially be specified by apps by proposing a
role callback handler in JASPIC, and extending JASPIC to support that.
Perhaps that is how we connect the IdentityStore role mapping to the
container.
However, until JASPIC supports the role callback, I guess Role from
Identity Store would be ignored. Should we remove the role support from
Identity Store for now?
> 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?
[Alex] I was thinking null and empty lists of groups and roles have
distinct meaning in the proposed IdentityStore API. Empty means "none",
as in "no roles assigned" or "no group member". Null means
"IdentityStore does not support". I considered using
UnsupportedOperationException, but I wanted to avoid the performance hit
of regularly checking for roles, groups, etc and getting an exception.
> 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);
[Alex] Agreed
>
>
> Hope this feedback is useful.
[Alex] Very 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
>>