Ps.
Regarding this interface:
public interface JaasSubjectPrincipalResolver {
String getCaller(Subject subject);
List<String> getCallerGroups(Subject subject, String callerName);
List<String> getCallerRoles(Subject subject, String callerName);
}
See
https://github.com/javaee-security-spec/javaee-security-proposals/blob/master/identity-store-readonly/src/main/java/javax/security/identitystore/persistence/JaasSubjectPrincipalResolver.java
May be worthwhile to remark that in a way this seems to solve a part
of the following issue:
https://java.net/jira/browse/JAVAEE_SECURITY_SPEC-11
Namely, the part where it asks for:
"Alternatively or perhaps additionally two new callback handlers could
be introduced that write resp. read the proprietary principals from a
given Subject. These handlers could work with the existing callback
types CallerPrincipalCallback and GroupPrincipalCallback."
I do wonder why the callerName is needed as second parameter in the
getCallerGroups method. As far as I know, a Subject normally would not
carry groups for multiple (different) callers. It's always one subject
per caller, although that single caller may be identified by different
IDs (principals).
Kind regards,
Arjan Tijms
On Tue, Sep 29, 2015 at 1:57 PM, 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
>>