[ELY-1460] External CS, PKCS11 can't be configured with externalPath

[ivassile]

wildfly-elytron: https://issues.jboss.org/browse/ELY-1460
JBEAP: https://issues.jboss.org/browse/JBEAP-13441

[mchoma]

Regarding discussion on JBEAP, this PR is, I would say, sustaining style fix introducing smallest change to fix the issue.

But as this PR target master branch, could we consider proper solution to fix the issue - removing feature "if location is not set in CLI, default credential-store name is used as location"?

[darranl]

I am not sure this is the correct fox for this problem, this change means if an invalid location is specified it would also be silently ignored.

Within this class if useExternalStorage gets set to true we should be using externalStorage - I think we need to backtrack higher up to the point where we don't want external storage but are somehow switching it on.

[ivassile]

@darranl When useExternalStorage=true we use "location" to setupExternalStorage for filebased external storage and load the KeyStore from "externalPath" [1]. The problem here is that for non-filebased type like PKCS11 we expect "location" to be null which is not the case because the default credential-store name is used as "location" when not set.
I agree with @mchoma that a proper fix is to remove "if location is not set in CLI, default credential-store name is used as location" in the subsystem, but like Brian noted in JBEAP-13441 it will be a problem when our examples don't set the location but instead rely on this default. He also suggested another solution: "tracking how the location was configured and if wasn't explicit checking if this magic default indicates a valid path and ignoring it if not". Please let me know what solution will be better to implement for this issue. Thanks!

I've also noted that in KeyStoreCredentialStore we have [2], but we don't check the keyStoreType here [3].

[1] https://github.com/ivassile/wildfly-elytron/blob/648b369d873b2b0c1c080336170ead9f16e49a41/src/main/java/org/wildfly/security/credential/store/impl/KeyStoreCredentialStore.java#L835
[2]
{@code externalPath}: specifies path to the external storage. It has to be used in conjunction with {@code external=true} and it defaults to value of {@code location} when {@code keyStoreType} is PKCS11.
[3] https://github.com/ivassile/wildfly-elytron/blob/648b369d873b2b0c1c080336170ead9f16e49a41/src/main/java/org/wildfly/security/credential/store/impl/KeyStoreCredentialStore.java#L203

[mchoma]

There exists documentation JIRA to ensure we have location explicitely set. [1]

I think rather javadoc [2] is incorrect. External CS can be in theory used for other non-filebased keystores, not only PKCS11.

Also I think we should not default externalPath to location. We should not mix these 2 different attributes, only because in non-filebased keystores location has no meaning and can be empty. And as it can be empty we reuse it for another semantic externalPath - but I think it is confusing.

[1] https://issues.jboss.org/browse/JBEAP-13848

[darranl]

I think a bug like this needs to be two steps: -

1. Verify correctness of the CredentialStore API.

From what I can tell this API already has sufficient parameters available to it to drive correct behaviour.

2. Update the model accordingly to work correctly with this API.

Configurations that already break using the management model I tend not to worry too much about as it is impossible for anyone to already be using them, but permutations of configuration that could be in use we must preserve.

On the management model we do have the option open to us to add new optional attributes to further control the behaviour.

If the current attributes are not correct we could also add new attributes which are alternatives to the current attributes and deprecate the old ones - we don't want to be jumping to deprecating everything on day 1 but if the bug is in the model design we should be looking at a compatible fix in that area.

But overall I think ignoring a value that is many cases must be set correctly within the core Elytron implementation to compensate for a model issue I think is the wrong place. I worked through some similar configuration issues this week and I think the risk here is the user accidentally ends up with an empty credential store rather than a clear error saying the credential store could not be loaded - they then end up having to debug why their successfully configured credential store contains no credentials.

[ivassile]

This issue will require elytron and elytron subsystem changes so I created core/JBEAP issues:
https://issues.jboss.org/browse/WFCORE-3458 / https://issues.jboss.org/browse/JBEAP-13978

I'll work on removing the requirement "if location is not set in CLI, default credential-store name is used as location" in elytron subsystem. In Elytron, when useExternalStorage=true and externalPath is null we should not default externalPath to location, but throw an exception.

[darranl]

@ivassile Do we have an update on where we are with this one? Do you need to us to make some time to discuss the options?

[ivassile]

@darranl I thought that with this new PR and the PR [1] submitted for WFCORE-3458, the issue is completely fixed in Elytron Subsystem and Elytron. Let me know if you think that the change is not sufficient to resolve the problem.

[1] wildfly/wildfly-core#3010

[darranl]

Ah, I missed the updated code - yes this does look ready for re-review, thank you.

[darranl]

@mchoma Have you had a chance to look at the pair of PRs? I think we are ready to get on with merging.

[mchoma]

I think this fix is OK