Map only specific type of OIDC Claims #58524

jkakavas · 2020-06-25T05:55:22Z

This commit changes our behavior in 2 ways:

When mapping claims to user properties ( principal, email, groups,
name), we only handle string and array of string type. Previously
we would fail to recognize an array of other types and that would
cause failures when trying to cast to String.
When adding unmapped claims to the user metadata, we only handle
string, number, boolean and arrays of these. Previously, we would
fail to recognize an array of other types and that would cause
failures when attempting to process role mappings.

For user properties that are inherently single valued, like
principal(username) we continue to support arrays of strings where
we select the first one in case this is being depended on by users
but we plan on removing this leniency in the next major release.

This commit changes our behavior in 2 ways: - When mapping claims to user properties ( principal, email, groups, name), we only handle string and array of string type. Previously we would fail to recognize an array of other types and that would cause failures when trying to cast to String. - When adding unmapped claims to the user metadata, we only handle string, number, boolean and arrays of these. Previously, we would fail to recognize an array of other types and that would cause failures when attempting to process role mappings. For user properties that are inherently single valued, like principal(username) we continue to support arrays of strings where we select the first one in case this is being depended on by users but we plan on removing this leniency in the next major release.

elasticmachine · 2020-06-25T05:55:24Z

Pinging @elastic/es-security (:Security/Authentication)

tvernum · 2020-06-25T06:33:48Z

...n/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java

+    private static boolean isAllowedTypeForClaim(Object o) {
+        return (o instanceof String || o instanceof Boolean || o instanceof Number
+            || (o instanceof Collection && ((Collection) o).stream()
+            .allMatch(c -> c instanceof String || c instanceof Boolean || c instanceof Number)));


I think we probably want to ensure that these are homogenous collections. I don't know how we would make sense of [ "1", true, 1.0 ]

think we probably want to ensure that these are heterogenous collections.

You used Greek so I get to be pedantic 🎉 s/heterogeneous/homogeneous/ :)
In all seriousness, I took a lenient stance here on purpose for metadata ( as opposed to user properties where I allowed only strings ).

I don't know how we would make sense of [ "1", true, 1.0 ]

Metadata can be used as metadata, for informational purposes so we do not necessarily need to make sense of the value of a given key.

It would work in a role mapping rule such as

"rules" : { "field" : { "groups" : "1" } },

happy to reconsider if you think this would cause an issue somewhere else or that it is too lenient for the sake of being lenient but I don't see any specific problem with it as is

My comment clearly says homogeneous 😈

Fair enough - as long as it's a considered position, I'm not worried.

ywangd

The new validateClaimType method looks a bit shaky to me. Could you please take another look? Thanks.

ywangd · 2020-06-26T00:07:18Z

...n/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java

-                            } else if (claimValueObject instanceof List == false) {
-                                throw new SettingsException("Setting [" + RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())
-                                    + " expects a claim with String or a String Array value but found a "
-                                    + claimValueObject.getClass().getName());
                            }
                            return ((List<String>) claimValueObject).stream()


The new validateClaimType method does not assert claimValueObject is a List. In theory it could be a Set or a Number.

validateClaimType right above validates that is either a Collection or a String. Then, if this is not a String, it is a Collection. How could it get to be a Set or a Number ? Maybe I didn't get what you mean

ywangd · 2020-06-26T00:12:39Z

...n/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java

+        private static void validateClaimType(Object claimValueObject, String settingKey) {
+            if (claimValueObject instanceof String == false &&


I'd prefer a more descriptive method name if possible. The current feels like it will validate the claim value is a certain specified type. But it really just asserts the value is either:

A String or

If it is a collection, it must be a collection of String

Now I am not sure if this is what we really want? Is it OK if the value is a Number?

Hey can you elaborate on what you mean be "descriptive" here? The method does what it intends to do as described in the PR description:

When mapping claims to user properties ( principal, email, groups,
name), we only handle string and array of string type. Previously
we would fail to recognize an array of other types and that would
cause failures when trying to cast to String.

Doing so, seems to me to be "Validating the type of the claim value" and as such validateClaimType seems appropriate. What would be an alternative that you are more comfortable with ?

What I mean is the name gave me the first impression that the signature would be something like:

void validateCliamType(Object value, Class<?> type)

But instead the type is not specified but should always be String or collection of it. So I'd personally prefer something like validateParsableClaim.

Fair enough, I don't necessarily agree that validateClaimType is off :) but I don't dislike validateParsableClaim enough to fortify my hill :)

Thanks Ioannis. The more important piece of my comment here is about the behaviour, i.e. the if check does not seem to do what we want to ensure, i.e. String or Collection<String>:

claimValueObject instanceof String == false && claimValueObject instanceof Collection && ((Collection) claimValueObject).stream().allMatch(c -> c instanceof String) == false

If the value is a Number, the if branch will not be executed, i.e. a Number will be considered as parsable.

Aha! Now I saw it , thanks, will address

ywangd

I commented a bit more about claimValueObject checking.

ywangd · 2020-06-30T11:37:10Z

...n/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java

+            } else if (claimValueObject instanceof Collection) {
+                if (claimValueObject instanceof Collection && ((Collection) claimValueObject).stream().allMatch(c -> c instanceof String)) {
+                    values = (List<String>) claimValueObject;
+                } else {
+                    throw new SettingsException("Setting [ " + settingKey + " expects a claim with String or a String Array value");
+                }
+            } else {
+                throw new SettingsException("Setting [ " + settingKey + " expects a claim with String or a String Array value");
+            }


The check claimValueObject instanceof Collection is performed twice, which does not seem to be necessary

The if check only ensure it is a Collection<String>, not a List<String>. So there is a theoretical possibility for cast exception.

I feel this part of code can be simplified to something like the follows:

} else if (claimValueObject instanceof Collection && ((Collection) claimValueObject).stream().allMatch(c -> c instanceof String)) { values = (Collection<String>) claimValueObject; } else { throw new SettingsException("Setting [ " + settingKey + " expects a claim with String or a String Array value"); }

With above change, the return type of this method needs to be changed to Collection<String> as well.

Yeap you are right. In retrospect, I should have waited to fix this today and not last night. Thanks for calling me out !

tvernum · 2020-07-01T05:58:26Z

I don't feel I need to review this - I'm happy if @ywangd is.

jkakavas · 2020-07-03T09:21:01Z

@ywangd given that this is a bug fix, please feel free to merge this in my absense if you have bo additional comments on it

ywangd

LGTM

@ywangd given that this is a bug fix, please feel free to merge this in my absense if you have bo additional comments on it

Will merge and backport first thing on Monday.

This commit changes our behavior in 2 ways: - When mapping claims to user properties ( principal, email, groups, name), we only handle string and array of string type. Previously we would fail to recognize an array of other types and that would cause failures when trying to cast to String. - When adding unmapped claims to the user metadata, we only handle string, number, boolean and arrays of these. Previously, we would fail to recognize an array of other types and that would cause failures when attempting to process role mappings. For user properties that are inherently single valued, like principal(username) we continue to support arrays of strings where we select the first one in case this is being depended on by users but we plan on removing this leniency in the next major release.

This commit changes our behavior in 2 ways: - When mapping claims to user properties ( principal, email, groups, name), we only handle string and array of string type. Previously we would fail to recognize an array of other types and that would cause failures when trying to cast to String. - When adding unmapped claims to the user metadata, we only handle string, number, boolean and arrays of these. Previously, we would fail to recognize an array of other types and that would cause failures when attempting to process role mappings. For user properties that are inherently single valued, like principal(username) we continue to support arrays of strings where we select the first one in case this is being depended on by users but we plan on removing this leniency in the next major release. Co-authored-by: Ioannis Kakavas <[email protected]>

jkakavas added >bug :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v7.9.0 labels Jun 25, 2020

elasticmachine added the Team:Security Meta label for security team label Jun 25, 2020

jkakavas requested a review from ywangd June 25, 2020 05:55

tvernum reviewed Jun 25, 2020

View reviewed changes

ywangd reviewed Jun 26, 2020

View reviewed changes

jkakavas added 5 commits June 29, 2020 17:59

address feedback

4274675

unused import

f1b3589

Merge remote-tracking branch 'origin/master' into claims-mapping-changes

1c02a7d

Don't try to validate null claims

fd030f0

fix parsing and validation logic

cb9e3f2

jkakavas requested review from tvernum and ywangd June 30, 2020 09:02

ywangd reviewed Jun 30, 2020

View reviewed changes

jkakavas added 3 commits June 30, 2020 15:10

address feedback

ee0b57b

checkstyle

2499dc2

Merge remote-tracking branch 'origin/master' into claims-mapping-changes

41aeec6

ywangd approved these changes Jul 4, 2020

View reviewed changes

ywangd merged commit 2217705 into elastic:master Jul 6, 2020

ywangd mentioned this pull request Jul 7, 2020

Allow mixed usage of boolean and string when merging OIDC claims #59112

Merged

ywangd mentioned this pull request Jul 23, 2020

Support more types for OIDC claim mapping #60102

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Map only specific type of OIDC Claims #58524

Map only specific type of OIDC Claims #58524

jkakavas commented Jun 25, 2020

elasticmachine commented Jun 25, 2020

tvernum Jun 25, 2020 •

edited

Loading

jkakavas Jun 26, 2020

tvernum Jun 26, 2020 •

edited

Loading

ywangd left a comment

ywangd Jun 26, 2020

jkakavas Jun 26, 2020

ywangd Jun 26, 2020

jkakavas Jun 26, 2020

ywangd Jun 26, 2020

jkakavas Jun 26, 2020 •

edited

Loading

ywangd Jun 26, 2020

jkakavas Jun 26, 2020

ywangd left a comment

ywangd Jun 30, 2020

jkakavas Jun 30, 2020

tvernum commented Jul 1, 2020

jkakavas commented Jul 3, 2020

ywangd left a comment

		private static void validateClaimType(Object claimValueObject, String settingKey) {
		if (claimValueObject instanceof String == false &&

Map only specific type of OIDC Claims #58524

Map only specific type of OIDC Claims #58524

Conversation

jkakavas commented Jun 25, 2020

elasticmachine commented Jun 25, 2020

tvernum Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvernum Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

ywangd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkakavas Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvernum commented Jul 1, 2020

jkakavas commented Jul 3, 2020

ywangd left a comment

Choose a reason for hiding this comment

tvernum Jun 25, 2020 •

edited

Loading

tvernum Jun 26, 2020 •

edited

Loading

jkakavas Jun 26, 2020 •

edited

Loading