Issue Details (XML | Word | Printable)

Key: GLASSFISH-15429
Type: Bug Bug
Status: Reopened Reopened
Priority: Major Major
Assignee: kumarjayanti
Reporter: srinik76
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
glassfish

Modifying keyfile path in a newly created config does not properly list the users

Created: 04/Jan/11 06:15 AM   Updated: 16/Nov/11 10:27 AM
Component/s: security
Affects Version/s: 3.1_ms07
Fix Version/s: future release

Time Tracking:
Not Specified

Issue Links:
Dependency
 

Tags: 3_1-exclude 3_1-next 3_1-release-note-added 3_1-release-notes 3_1_1-exclude 3_1_1-scrubbed 3_1_2-exclude
Participants: Anissa Lam, kumarjayanti, Scott Fordin and srinik76


 Description  « Hide

1. asadmin copy-config default-config new-config
2. asadmin get new-config.security-service.auth-realm.admin-realm.property.file
new-config.security-service.auth-realm.admin-realm.property.file=${com.sun.aas.instanceRoot}/config/admin-keyfile
3. asadmin set new-config.security-service.auth-realm.admin-realm.property.file=/tmp/v3/admin-keyfile
new-config.security-service.auth-realm.admin-realm.property.file=/tmp/v3/admin-keyfile
Command set executed successfully.
4. file /tmp/v3/admin-keyfile is not currently created.
5. asadmin create-file-user --authrealmname admin-realm --target new-config test
Enter the user password>
Enter the user password again>
Command create-file-user executed successfully.
6. cat /tmp/v3/admin-keyfile
test;{SSHA256}mjyhJFWxU8xUnGMY5bt+ngwj3tf6v6yOTKB7DgGKJUu6Neky/GVOsQ==;asadmin
user1;{SSHA256}rImtlHJuqi6AMSypIUyBdcs2CUEPQq3pIEHSEsndYQmhBl4ZBT+fJA==;user1
<user test is properly added to admin-keyfile under /tmp/v3 as expected.>
8. asadmin list-file-users --authrealmname admin-realm new-config
user1
Command list-file-users executed successfully.
<Expected is test,user1 but it lists user1 which it takes from ${instance_root}/domains/domain1/config/keyfile but it should list from /tmp/v3/admin-keyfile>

The list-file-users needs to be fixed to list from /tmp/v3/admin-keyfile



kumarjayanti added a comment - 04/Jan/11 09:05 PM

Srini,

Either there is no bug or you have not given me the complete set of steps. With the steps that you provided it is not clear how the user :"user1" is already present in the /tmp/v3/admin-keyfile.

Also are you using the latest builds. Here are the steps that i executed and i see no problems

1.)

$ ./asadmin start-domain
Waiting for domain1 to start ..........
Successfully started the domain : domain1
domain Location: /Users/vbkumarjayanti/Documents/workspace/glassfish/glassfish3/glassfish/domains/domain1
Log File: /Users/vbkumarjayanti/Documents/workspace/glassfish/glassfish3/glassfish/domains/domain1/logs/server.log
Admin Port: 4848
Command start-domain executed successfully.

2.)
$ ./asadmin copy-config default-config new-config

Command copy-config executed successfully.

3)
$ ./asadmin set new-config.security-service.auth-realm.admin-realm.property.file=/tmp/v3/admin-keyfile
new-config.security-service.auth-realm.admin-realm.property.file=/tmp/v3/admin-keyfile
Command set executed successfully.

4)
$ mkdir /tmp/v3
$ > /tmp/v3/admin-keyfile

5)
$ ./asadmin create-file-user --authrealmname admin-realm --target new-config test
Enter the user password>
Enter the user password again>
Command create-file-user executed successfully.

6)
$ cat /tmp/v3/admin-keyfile
test;{SSHA256}H0J8mMtMJp1BcPynsBqyDw8r0WWI30796BaFrsdmei3eBh3YDILKKg==;asadmin

7)
$ ./asadmin list-file-users --authrealmname admin-realm new-config
test
Command list-file-users executed successfully.

8)
$ ./asadmin create-file-user --authrealmname admin-realm --target new-config user1
Enter the user password>
Enter the user password again>
Command create-file-user executed successfully.

9)
$ ./asadmin list-file-users --authrealmname admin-realm new-config
test
user1
Command list-file-users executed successfully.


kumarjayanti added a comment - 04/Jan/11 09:07 PM

Cannot reproduce. Make sure you are using the very latest glassfish build. And please provide the exact steps to reproduce.

I will try some other combination in the meantime to see if i can reproduce anything like what u mention.


srinik76 added a comment - 04/Jan/11 09:52 PM

I have not created the file. As per the requirement do we need to create the file.

If not created we see this problem.


kumarjayanti added a comment - 04/Jan/11 11:04 PM

The Keyfile needs to be created physically and ideally this should be done before even the set command is executed to change the keyfile property.

That said, if the keyfile is not created then when i test purely from CLI i do not get the problem you see. I see the create-file-user fails.


Anissa Lam added a comment - 04/Jan/11 11:08 PM

It is the responsibility of the user to create the keyfile ( I hope the documentation specifies that) or the security code should be smart enough to detect that the keyfile doesn't exist and create that for the user.
GUI should NOT create the keyfile.

As commented in GLASSFISH-15406, there is error given if the keyfile doesn't exist.
$ ./asadmin create-file-user --authrealmname admin-realm --target new-config test
Enter the user password>
Enter the user password again>
remote failure: Adding User test to file realm admin-realm failed.
Command create-file-user failed.

GUI should show the error to the user.
However, there is no reason given to the user WHY the creation failed. It should tell user that the keyfile doesn't exist. Thus I am re-opening this bug but downgrading it. The error message needs to be clear.


srinik76 added a comment - 04/Jan/11 11:23 PM

Tested with the latest build, create-file-user fails with following message

asadmin create-file-user --authrealmname admin-realm --target new-config test
Enter the user password>
Enter the user password again>
remote failure: Adding User test to file realm admin-realm failed. /tmp/v3/admin-keyfile (No such file or directory)
/tmp/v3/admin-keyfile (No such file or directory)

list-file-users also should report that /tmp/v3/admin-keyfile is not found, but it lists the user (admin)

asadmin list-file-users --authrealmname admin-realm new-config
admin
Command list-file-users executed successfully.


kumarjayanti added a comment - 05/Jan/11 12:23 AM

Anissa Said :
----------

GUI should show the error to the user.
However, there is no reason given to the user WHY the creation failed. It should tell user that the keyfile doesn't exist. Thus I am re-opening this bug but downgrading it. The error message needs to be clear.

--------

I checked the code and the CLI command tried to do the following :
report.setMessage(
localStrings.getLocalString("create.file.user.useraddfailed",
"Adding User {0} to the file realm {1} failed",
userName, authRealmName) + " " + localalizedErrorMsg);
report.setActionExitCode(ActionReport.ExitCode.FAILURE);
report.setFailureCause(e);

It appears when the Message is set it takes priority over setFailureCause in the admin framework. I can remove the setMessage for the next release. Or if you think it is important for you since this message has to be displayed in the GUI then please reopen the bug and i will ask for approval.

I see a similar problem with list-file-users as well where it sets all the 3 items on the report object and hence the message that was set takes priority and that message is currently not very good.

I can fix both of them as part of this. The question is would you like messages fixed for V3.1.


srinik76 added a comment - 05/Jan/11 02:52 AM

Reopening issue after discussing with kumar.

Now from the security side it will check for the keyfile existence while listing file users.


kumarjayanti added a comment - 05/Jan/11 03:15 AM

1) How bad is its impact? (Severity)
While we are unable to reproduce it by pure CLI operations it appears GUI seems to observe some incorrect file-user listing in this case. Also the message log for the Failure can be improved to indicate the real cause.

2) How often does it happen? (Frequency)
This will occur only when keyfile of a file-realm points to a non-existent File.

3) How much effort is required to fix it? (Cost)
Minor : the fix is to add a check for non-existent keyfile and throw an error.

4) What is the risk of fixing it? (Risk)

None (does not affect the Runtime). GUI wants these enhancements.


Anissa Lam added a comment - 05/Jan/11 08:09 AM

Approved.


kumarjayanti added a comment - 05/Jan/11 09:12 AM

fixed


srinik76 added a comment - 05/Jan/11 10:48 PM

Fix works fine, but when the key file is created

list-file-users should report none, but lists admin user.

Waiting for comments from kumar to reopen the issue.


srinik76 added a comment - 06/Jan/11 12:13 AM

After changing the key file and restarting the server, it works fine.


srinik76 added a comment - 06/Jan/11 12:14 AM

Comments from Kumar in email

Hi Srini, Anissa,

I think i understand what is happening.

1. default-config is copied as new-config
2. Now the admin-realm is selected in the GUI from this new-config
3. Its KeyFile property is changed to some XYZ and the change is saved. Using an asadmin set-command
4. A physical XYZ file is created
5. Now again ManageUsers is clicked on the admin-realm of new-config
5.1. At this time ListFileUsers command still shows the original admin user that is part of the admin-realm in default-config

Is this a correct description of the issue ?. If that is true then yes that can happen.

1. Step 2 loads the admin-realm in the Backend.
2. Setp 3 modifies the property of the realm in the Config-Layer
3. Backend is not informed of this change. And since the new-config is not the running config of the Server (DAS) the ConfigListener Mechanism (which is in place) does not come into picture. So the backend realm is never updated.
4. ListFileUsers command which executes as part of step (5) above does a refresh of the keyfile contents but it never goes back to the config-layer to see if the keyfile has changed. And it does not make sense for ListFileUsers to do that (checking for changes to the realm definition in config-layer). Things would have worked if it did that but really the syncing between Config-Layer and Backend should have happened when step (3) above invoked the "asadmin set" command to modify the keyfile property.

There are 4 solutions :

1. I can provide a new hidden command which has to be executed by GUI whenever they do a asadmin set on any realm (and file-realm in particular). This command would then try to sync the Config change made by the set to the backend. For some realms this inplace update cannot be performed for-example

  • if it is an LDAPRealm of the active server config and there are applications currently using that realm and the set command tries to modify the URL of the LDAP server or the base-dn of the LDAPRealm. Such changes will require a RESTART of the Application Server.

2. Fix ListFileUsers to re-read the config-layer. Not a good idea and not the right fix.

3. GUI can indicate after the set-command that Appserver should be restarted.

4. GUI should not use an asadmin set to modify the keyfile location. Instead it should delete the existing Realm and create a New Realm with modified properties.

Let me know which approach would be best for V3.1.

regards,
kumar

Waiting for Anissa's comments to decide what approach (1 out of 4 suggested by kumar) to be taken for the solution.
Anissa/Kumar, Can I reopen the bug?


kumarjayanti added a comment - 06/Jan/11 12:24 AM

I am also not sure if a Property change in realm (using asadmin set) in an active server config will cause the Config Change Event which can be received by a registered ConfigListener.


Anissa Lam added a comment - 06/Jan/11 12:38 AM

As i understand it, only GUI user sees this problem. CLI user will not have such an issue. Correct me if i am wrong. But i just tried using CLI to do all the steps and list-file-user reports the correct list.
So, what exactly is missing in GUI code, compared to CLI, that causes this error in GUI only ? I want to understand this, and that maybe how we should fix it.

The corresponding steps in CLI works perfectly fine, and list-file-user is reporting the list of users from the new key file correctly.

~ 1) asadmin copy-config default-config TEST-config
Command copy-config executed successfully.
~ 2)
~ 2) asadmin list-file-users --authrealmname admin-realm server-config
admin
Command list-file-users executed successfully.
~ 3)
~ 3)
~ 3) asadmin get configs.config.TEST-config.security-service.auth-realm.admin-realm.property.file
configs.config.TEST-config.security-service.auth-realm.admin-realm.property.file=${com.sun.aas.instanceRoot}/config/admin-keyfile
Command get executed successfully.
~ 4)
~ 4) asadmin set configs.config.TEST-config.security-service.auth-realm.admin-realm.property.file=/tmp/keyFile
configs.config.TEST-config.security-service.auth-realm.admin-realm.property.file=/tmp/keyFile
Command set executed successfully.
~ 5)
~ 5)
~ 5) touch /tmp/keyFile
~ 6)
~ 6) asadmin list-file-users --authrealmname admin-realm TEST-config
Command list-file-users executed successfully.
~ 7)
~ 7)


Anissa Lam added a comment - 06/Jan/11 12:39 AM

Re-open issue as this is still under active discussion and GUI depends on the fix.


kumarjayanti added a comment - 06/Jan/11 12:58 AM

Your step two :
~ 2) asadmin list-file-users --authrealmname admin-realm server-config

should be changed to

2) asadmin list-file-users --authrealmname admin-realm TEST-config

and IMO that will reproduce the problem in CLI


kumarjayanti added a comment - 06/Jan/11 02:30 AM

I retested after implementing solution 1. Here is the output :

$ ./asadmin start-domain
Waiting for domain1 to start ..........
Successfully started the domain : domain1
domain Location: /Users/vbkumarjayanti/Downloads/glassfish3/glassfish/domains/domain1
Log File: /Users/vbkumarjayanti/Downloads/glassfish3/glassfish/domains/domain1/logs/server.log
Admin Port: 4848
Command start-domain executed successfully.

$ ./asadmin copy-config default-config new-config
Command copy-config executed successfully.

$ ./asadmin list-file-users --authrealmname admin-realm new-config
admin
Command list-file-users executed successfully.

$ ./asadmin set new-config.security-service.auth-realm.admin-realm.property.file=/tmp/mykeyfile
new-config.security-service.auth-realm.admin-realm.property.file=/tmp/mykeyfile
Command set executed successfully.

$ ./asadmin __synchronize-realm-from-config --realmname admin-realm new-config
Synchronization of Realm admin-realm from Configuration Successful.
Command __synchronize-realm-from-config executed successfully.

$ ./asadmin list-file-users --authrealmname admin-realm new-config
Command list-file-users executed successfully.

$ cat /tmp/mykeyfile

$ ./asadmin create-file-user --authrealmname admin-realm --target new-config test
Enter the user password>
Enter the user password again>
Command create-file-user executed successfully.

$ ./asadmin list-file-users --authrealmname admin-realm new-config
test
Command list-file-users executed successfully.


kumarjayanti added a comment - 06/Jan/11 02:47 AM

The new hidden command will set RESTART required if the change was done to an active server config

$ ./asadmin set server-config.security-service.auth-realm.file.property.file=/tmp/mykeyfile
server-config.security-service.auth-realm.file.property.file=/tmp/mykeyfile
Command set executed successfully.

$ ./asadmin __synchronize-realm-from-config --realmname file server-config
Restart required for configuration updates to active server realm: file.
Command __synchronize-realm-from-config executed successfully.

And here is how the code in the command sets the information required for GUI for a restart. I picked up this code from :

core/kernel/src/main/java/com/sun/enterprise/v3/admin/GetRestartRequiredCommand.java

private void setRestartRequired(ActionReport report) {
report.setActionExitCode(ActionReport.ExitCode.SUCCESS);
ActionReport.MessagePart mp = report.getTopMessagePart();

Properties extraProperties = new Properties();
Map<String, Object> entity = new HashMap<String, Object>();
mp.setMessage(_localStrings.getLocalString("RESTART_REQUIRED",
"Restart required for configuration updates to active server realm: {0}.",
new Object[] {realmName}));
entity.put("restartRequired","true");
extraProperties.put("entity", entity);
((ActionReport) report).setExtraProperties(extraProperties);
}


kumarjayanti added a comment - 06/Jan/11 07:58 PM

checked in the Partial Fix which will make the GUI work.

GUI has to invoke the new hidden command whenever an asadmin set is invoked on any realm.

The CLI this bug still remains if someone does the following sequence of operations :

1. asadmin copy-config default-config new-config
2. asadmin list-file-users --authrealmname admin-realm new-config
admin
Command list-file-users executed successfully.
3. asadmin get new-config.security-service.auth-realm.admin-realm.property.file
new-config.security-service.auth-realm.admin-realm.property.file=${com.sun.aas.instanceRoot}/config/admin-keyfile
4. asadmin set new-config.security-service.auth-realm.admin-realm.property.file=/tmp/v3/admin-keyfile
new-config.security-service.auth-realm.admin-realm.property.file=/tmp/v3/admin-keyfile
Command set executed successfully.
5. create the physical keyfile at /tmp/v3/admin-keyfile

After doing these steps, the following command will give a wrong answer :

1. asadmin list-file-users --authrealmname admin-realm new-config
admin
Command list-file-users executed successfully.

This is because the asadmin set command in step-4 above updates the Configuration Layer but does not update the Backend Realm which was loaded while executing step 2. So the list command will continue to list the user admin which was present in the original realm's keyfile (one that it was referring to before the set command changed it).

Summary of the CLI Bug : If an asadmin set command is executed to change a realm-property for a realm that was loaded by the backend (due to an earlier CLI command targeted at the realm) , then the realm continues to behave as if the set command was not executed. The workaround is to restart Appserver after a set command has been used to change a property of an already loaded realm.


Scott Fordin added a comment - 15/Apr/11 10:33 AM

Issue added to 3.1 Release Notes.