I've attached a patch that addresses most of the comments below and also also adds CLI and fixes adding/removing/editing and changing the state of profiles. As we discussed on #irc, the authentication problem is still unresolved. That is, I want to be able to do the following: GET /profiles and then in the method itself determine whether or not I have already logged in (I will have a principal) and return different results accordingly. Right now, that is not working. The only way that I can guarantee that client auth takes place and the credential is provided is by putting in a security constraint that requires /profiles/* to use client authentication. But then, I cannot do GET /profiles without authentication. It seems clientAuth=want is not working. If we cannot resolve this issue, then I will need to keep using agent/* and admin/*, which are url patterns that have security contexts. And I will have to add back GET /agent/profiles and GET /agent/profiles/{id} to see all profiles that are not visible to the EE user. Thanks, Ade On Thu, 2013-06-27 at 16:40 -0500, Endi Sukma Dewata wrote:

On 6/27/2013 11:04 AM, Ade Lee wrote:

...

This adds the initial framework for viewing and managing profiles. At this point, only the viewing of profiles is tested.

Because I make some changes to some of the objects used in Cert enrollment, some of the current tests involving enrollment may fail if they use pre-generated XML.

Still, this patch is getting quite large and its time to get some eyes on it.

The next patches will add the CLI code to view profiles and then to add/edit profiles. Following that, will be patches to clean up all the TODOs - like adding the relevant exceptions and auditing.

Ade

Some comments:

1. The log messages in CATest should say 'Enabled:' and 'Visible:' because those are the attribute names being logged:

log("Enabled: " + info.isEnabled()); log("Visible: " + info.isVisible() + "\n\n");

2. The following code in CATest can be simplified:

ListIterator<ProfileAttribute> iter = entry.getValue().getAttrs().listIterator(); while (iter.hasNext()) { ProfileAttribute attr = iter.next();

into this:

for (ProfileAttribute attr : entry.getValue().getAttrs()) {

Similar code exists in CertEnrollmentRequest, BasicProfile, CertProcessor, and EnrollmentProcessor.

3. I don't think the ProfileAttribute should be changed to use IDescriptor instead of Descriptor. Are there other classes implementing IDescriptor?

The type adapter for IDescriptor is defined as Descriptor.Adapter, but this adapter will do a simple typecast from IDescriptor to Descriptor and vice versa, so it probably won't work with other classes. To properly marshal any IDescriptor into Descriptor we'd have to create a new Descriptor object and initialize it with values obtained using using IDescriptor methods. I'm also not sure how unmarshal would be able to restore the original object if it's not a Descriptor.

I think if there's no other types of descriptor (or if all descriptors will inherit from Descriptor) then we should just use Descriptor in ProfileAttribute. But if we need to support different descriptor types, we need to serialization the class name as well (see PKIException.Data class) and use a factory to recreate the proper object.

4. In ProfileData the inputs, outputs, and policySets attributes are declared between the method declarations. To improve readability we should keep the attributes in the beginning of the class before any method declarations.

Similar issues also happens in ProfileInput and ProfileOutput.

5. The ProfileData.policySets maps a String to a Vector. I think unless there's a special need for Vector (e.g. synchronized access) we should use a more generic and light-weight Collection instead.

6. The following code in BasicProfile deleteAllProfileInputs() and deleteAllProfileInputs() can be simplified:

Iterator<String> iter = mInputIds.iterator(); while (iter.hasNext()) { String id = iter.next();

into this:

for (String id : mInputIds) {

7. In ProfileService the listProfiles() is split into listEEProfiles() and listAdminProfiles() to return different results based on the visibility flag. Splitting the method doesn't prevent a user from calling a method that he's not supposed to call. To prevent that we'd have to configure ACLs, which adds maintenance. Also we probably have to write different CLI to call different methods for different roles.

Instead, we could use the original method but it should check the roles of the user and determine the visibility flag based on that.

8. In ProfileService.createProfileDataInfo() the URI is constructed using String concatenation in 2 different ways:

if (visibleOnly) { profileBuilder.path(profilePath.value() + "/profiles/" + profileId); } else { profileBuilder.path(profilePath.value() + "/agent/profiles/" + profileId); } ret.setProfileURL(profileBuilder.build().toString());

If we implement #7 it can be simplified like this:

URI uri = profileBuilder.path(ProfileResource.class).path("{id}"). build(profileId); ret.setProfileURL(uri.toString());

9. Some exceptions in ProfileService are being swallowed. I think at least for now we should throw an exception (either make the method throws generic Exception or wrap the exceptions in RuntimeException) so that we know if there's a problem. Later on we can revisit this and handle the exceptions properly.

10. The code in ProfileService.populateProfilePolicies() can be simplified as follows:

for (Map.Entry<String,Vector<ProfilePolicy>> policySet : data.getPolicySets().entrySet()) {

Great find on the Session variable. Agreed on all of the below. I will fix in separate subsequent patches. Pushing the current patch to master now. Ade On Mon, 2013-07-22 at 08:30 -0500, Endi Sukma Dewata wrote:

On 7/19/2013 8:44 AM, Ade Lee wrote:

...

GET /profiles and then in the method itself determine whether or not I have already logged in (I will have a principal) and return different results accordingly.

Right now, that is not working. The only way that I can guarantee that client auth takes place and the credential is provided is by putting in a security constraint that requires /profiles/* to use client authentication. But then, I cannot do GET /profiles without authentication. It seems clientAuth=want is not working.

Some comments:

1. It looks like the current clientAuth setting is fine, but the SSL authenticator needs to be configured to use session. See the alwaysUseSession parameter in this page: http://tomcat.apache.org/tomcat-7.0-doc/config/valve.html. I posted patch #278 to add this parameter.

Then the security constraint can be configured without <auth-constraint> to allow anonymous access:

<security-constraint> <web-resource-collection> <web-resource-name>Profiles</web-resource-name> <url-pattern>/rest/profiles/*</url-pattern> </web-resource-collection> <user-data-constraint> <transport-guarantee>CONFIDENTIAL</transport-guarantee> </user-data-constraint> </security-constraint>

I've tested this with profile-find, it will return different number of results depending on the authentication. This way we won't need to use separate agent/admin interface.

2. For the profile-find output, I think it would be useful to show some basic info such as profile name and description in addition to the ID and URL.

3. The profile-show command stores the entire profile into an output file, it doesn't show anything to the screen. I think it would be useful and more consistent to show the profile summary such as id, name, description, input and output attribute names, URL. Then people can optionally specify --output download the entire file.

4. Does the profile subsystem support renaming a profile? The profile-mod may need to take 2 parameters: the old profile name and an input file containing the new profile name and the new attributes.

5. Some methods in ProfileService would catch and swallow the exception. It might be better for now to throw a generic exception or just don't catch at all. This way the client will know if there's an error. In the future we can revisit the code to throw more specific errors.