Add IDP Configuration settings and related tests #51682
Conversation
|
Pinging @elastic/es-security (:Security/Security) |
| private final Setting<Boolean> ENABLED_SETTING = Setting.boolSetting("xpack.idp.enabled", false, Setting.Property.NodeScope); | ||
| private static final Setting<Boolean> ENABLED_SETTING = Setting.boolSetting("xpack.idp.enabled", false, Setting.Property.NodeScope); | ||
| public static final Setting<String> IDP_ENTITY_ID = Setting.simpleString("xpack.idp.entity_id", Setting.Property.NodeScope); | ||
| public static final Setting<String> IDP_SSO_REDIRECT_ENDPOINT = Setting.simpleString("xpack.idp.sso_endpoint.redirect", value -> { |
There was a problem hiding this comment.
Should we make these Setting<URI> instead?
There was a problem hiding this comment.
I went back and forth. I settled on that we care about them being URIs mainly when we validate them while reading the setting and from there on we can treat them as Strings. Since I don't see any other use for these other than:
- Constructing metadata
- Validating Authn and Logout Requests where we will use string equality eitherway
it feels that future implementers or anyone reading the code doesn't need to care about the fact that these are URIs so I opted for the "simpler" handling.
...ugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/IdentityProviderPlugin.java
Outdated
Show resolved
Hide resolved
| this.ssoEndpoints.put("redirect", require(settings, IDP_SSO_REDIRECT_ENDPOINT)); | ||
| this.ssoEndpoints.computeIfAbsent("post", v -> settings.get(IDP_SSO_POST_ENDPOINT.getKey())); | ||
| this.sloEndpoints.computeIfAbsent("post", v -> settings.get(IDP_SLO_POST_ENDPOINT.getKey())); | ||
| this.sloEndpoints.computeIfAbsent("redirect", v -> settings.get(IDP_SLO_REDIRECT_ENDPOINT.getKey())); |
There was a problem hiding this comment.
I don't understand why we have 1 put and 3 computeIfAbsent
I think you'd get the same bevahiour with 4 put wouldn't you? None of the keys are going to be present.
There was a problem hiding this comment.
The "added benefit" of computeIfAbsent is that if the setting is not defined and settings.get returns null, then the null value won't be added to the hashmap.
I don't feel strongly about it, it seemed as a nice thing to do but I could also do with null checks here or when consuming the map values.
There was a problem hiding this comment.
It seems like it's depending on a side effect (a documented one, but still a side effect) and makes the code less clear because the reader is left trying to understand why we're using different methods.
I'm not particular fussed though.
...ck/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/idp/CloudIdp.java
Outdated
Show resolved
Hide resolved
...ck/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/idp/CloudIdp.java
Show resolved
Hide resolved
| aliases.addAll(Arrays.asList(ecAliases)); | ||
| } | ||
| if (aliases.isEmpty()) { | ||
| throw new IllegalArgumentException("The configured keystore for xpack.idp.signing.keystore does not contain any RSA or EC" + |
There was a problem hiding this comment.
My preference is to refer to the Setting object for the key
| throw new IllegalArgumentException("The configured keystore for xpack.idp.signing.keystore does not contain any RSA or EC" + | |
| throw new IllegalArgumentException("The configured keystore for " + IDP_SIGNING_KEY_ALIAS.getKey() + " does not contain any RSA or EC" + |
There was a problem hiding this comment.
Same here in general, but this is the keystore , not the alias, and we don't define the Setting object explicitly but rather
settings.addAll(X509KeyPairSettings.withPrefix("xpack.idp.signing.", false).getAllSettings());
in the plugin
...ck/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/idp/CloudIdp.java
Outdated
Show resolved
Hide resolved
...ck/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/idp/CloudIdp.java
Outdated
Show resolved
Hide resolved
...ck/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/idp/CloudIdp.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * SAML 2.0 configuration information about this IdP | ||
| */ | ||
| public interface SamlIdentityProvider { | ||
|
|
||
| String getEntityId(); | ||
|
|
||
| String getSingleSignOnEndpoint(String binding); |
There was a problem hiding this comment.
Could these return URI instead?
Also where do we expect to need these? Is it just for metadata? (which is a real need - I just want to check how we expect to consume them).
There was a problem hiding this comment.
see #51682 (comment)
Also where do we expect to need these? Is it just for metadata?
Metadata creation and incoming SAML message validation
No description provided.