[OPENAM-15758] KeyStore Secret Store fails to start due to secretId having some special characters. Created: 06/Dec/19  Updated: 15/Sep/20  Resolved: 18/Mar/20

Status: Resolved
Project: OpenAM
Component/s: secrets, XUI
Affects Version/s: 6.5.0, 6.5.1, 6.5.2.1, 6.5.2.2
Fix Version/s: 7.0.0, 6.5.3

Type: Bug Priority: Major
Reporter: C-Weng C Assignee: Lawrence Yarham
Resolution: Fixed Votes: 0
Labels: EDISON
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Target Version/s:
Sprint: AM Sustaining Sprint 70, AM Sustaining Sprint 71, AM Sustaining Sprint 72, AM Sustaining Sprint 73
Story Points: 5
Support Ticket IDs:
Functional tests:
Yes
Are the reproduction steps defined?:
Yes and I used the same an in the description, Yes but I used my own steps. (If so, please add them in a new comment)

 Description   

Bug description

Secrets for the secretId say the store password secretId or the key password secretId in the KeyStoreSecretStore happens to be in a set of alphanumeric dot separated string.

When any other label is used for Id like underscore or hyphen, the secrets will fail. The issue is that there is not UI or software validation to prevent this for be configured and may cause system startup fails (if this happens on the important realm).

How to reproduce the issue

Create a Secret keystore but uses the secret password label with say "_" or "hyphen" in as part of the string. When this is used the following exception may be seen

Caused by: com.google.common.util.concurrent.UncheckedExecutionException: org.forgerock.openam.secrets.SecretInitialisationException: Could not load some secret stores
        at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2050)
        at com.google.common.cache.LocalCache.get(LocalCache.java:3952)
        at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:3974)
....
Caused by: org.forgerock.openam.secrets.SecretInitialisationException: Could not load some secret stores
        at org.forgerock.openam.secrets.Secrets.resolveSecretStores(Secrets.java:258)
        at org.forgerock.openam.secrets.Secrets.loadSecretStores(Secrets.java:227)
        at org.forgerock.openam.secrets.Secrets.loadRealmSecrets(Secrets.java:196)
        at com.google.common.cache.CacheLoader$FunctionToCacheLoader.load(CacheLoader.java:165)
        at com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3528)
...
Caused by: java.lang.IllegalArgumentException: Label must match regex: [a-zA-Z0-9]+(\.[a-zA-Z0-9]+)*
        at org.forgerock.util.Reject.ifFalse(Reject.java:183)
        at org.forgerock.secrets.Purpose.<init>(Purpose.java:91)
        at org.forgerock.secrets.Purpose.purpose(Purpose.java:103)
        at org.forgerock.openam.secrets.config.KeyStoreSecretStore.lambda$createStore$2(KeyStoreSecretStore.java:136)
        at java.util.Optional.map(Optional.java:215)
        at org.forgerock.openam.secrets.config.KeyStoreSecretStore.createStore(KeyStoreSecretStore.java:136)
        at org.forgerock.openam.secrets.config.KeyStoreBasedSecretStoreProvider.getStore(KeyStoreBasedSecretStoreProvider.java:50)
        at org.forgerock.openam.secrets.config.KeyStoreBasedSecretStoreProvider.getStore(KeyStoreBasedSecretStoreProvider.java:38)
        at org.forgerock.openam.secrets.Secrets.resolveSecretStores(Secrets.java:245)
        ... 124 more
Expected behaviour
At least the XUI Secret UI cannot let one to create or add these illegal lables into the system and value validated first.

- Have input validation so that user will not find out. Prevent update from happening on UI or input
- Maybe also add documentaton and on the info-text the possible secretId value format
Current behaviour
Fail to load the Secret store and may have the above exception

Work around

  • Use secretId only with the format
 [a-zA-Z0-9]+(\.[a-zA-Z0-9]+)*
  • There is no documentation to indicate this is the set of valid format.

Code analysis

-



 Comments   
Comment by Peter Major [X] (Inactive) [ 14/Feb/20 ]

Lawrence Yarham Trying to address your points:
1. There is no client side validation for the secret ID fields. The secret store's ID has the isValidId validation on client side. Client side validation for the secret ID fields would require quite a lot of changes (adding a new attribute to AttributeSchema in sms.dtd; ensuring that the new attribute is returned in JSON schema as "pattern" properties, implement proper localization with meaningful error messages etc). Alternative to the previous would be to add bespoke validation to the new secret store page, which is probably not something we can aim for right now. The simplest way to validate the provided secret ID values is to validate them on the server side.
2. At one point we have implemented SecretIdValidator, but across the many iterations it became an unused class for some reason. Validating the individual attributes could be a solution, however I have some fuzzy memories that maybe attribute validators aren't executed when a new subconfig is being created. If the SecretIdValidator turns out to be a bust (e.g. because it only works on secret store edit), a ServiceConfigValidator maybe a solution. Please also keep in mind that adding attributes to annotated services may not get picked up during upgrade (upgrade is a bit more complex with these), a fresh install should pick up those kind of changes though.
3. Firstly that test is checking whether the subconfig ID can contain special characters (and doesn't actually check the secret ID values). Secondly it's your Tomcat container that rejects the request because it contains incorrectly percent encoded strings - check your Tomcat logs for clues.

  • If you want to do some more investigations, track down translation.json's alphaNumWithDotOnly and what happened with the UI side of it, probably got refactored at one point.. Regardless, I would suggest to implement server side validation for now.
  • SecretIdValidator and or a new ServiceConfigValidator should be used.

To answer Darinder Shokar's point: The dot character is not forbidden. You can have dot characters in the secret ID, as long as they are not at the beginning or at the end of the secret ID. Also two dots may not follow each other. To quote the alphaNumWithDotOnly translation:

Must not start or end with the <code>.</code> character <br>The <code>.</code> character must not be followed by another <code>.</code> character <br>Must contain <code>a-z</code>, <code>A-Z</code>, <code>0-9</code> and <code>.</code> characters only

Put it differently, keystore.pass and keystore.entry.pass are both perfectly fine secret IDs, as long as those secret IDs are actually available through other secret stores.

Generated at Tue Sep 22 12:20:35 UTC 2020 using Jira 7.13.12#713012-sha1:6e07c38070d5191bbf7353952ed38f111754533a.