Handle incoming AuthnRequests #52018

jkakavas · 2020-02-06T21:53:01Z

Expose an API that consumes (possibly signed) AuthnRequests as
defined by the HTTP-Redirect binding.
Process AuthnRequests, validate and parse them into a minimum
set of information to be used for subsequent API calls to get a
SAML Response

- Expose an API that consumes (possibly signed) AuthnRequests as defined by the HTTP-Redirect binding. - Process AuthnRequests, validate and parse them into a minimum set of information to be used for subsequent API calls to get a SAML Response

elasticmachine · 2020-02-06T21:53:04Z

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

tvernum

I didn't finish the review, but I think there's enough to go on for now.

...ovider/src/main/java/org/elasticsearch/xpack/idp/action/SamlValidateAuthnRequestRequest.java

...vider/src/main/java/org/elasticsearch/xpack/idp/action/SamlValidateAuthnRequestResponse.java

.../main/java/org/elasticsearch/xpack/idp/rest/RestSamlValidateAuthenticationRequestAction.java

...provider/src/main/java/org/elasticsearch/xpack/idp/saml/authn/SamlAuthnRequestValidator.java

jkakavas · 2020-02-13T12:55:01Z

address feedbacl and add Transport action tests

Mixed up commit messages thinking about #51830. I didn't add Transport action tests here as all it does in doExecute is to call processQueryString

tvernum · 2020-02-17T23:30:03Z

I'm going to hold off re-reviewing this until the other handler PR is merged. I think we're good, but the merge is going to be a bit messy.

…p-init-handlers

jkakavas · 2020-02-18T08:15:47Z

I'm going to hold off re-reviewing this until the other handler PR is merged. I think we're good, but the merge is going to be a bit messy.

This is ready @tvernum

...vider/src/main/java/org/elasticsearch/xpack/idp/action/SamlValidateAuthnRequestResponse.java

...provider/src/main/java/org/elasticsearch/xpack/idp/saml/authn/SamlAuthnRequestValidator.java

tvernum · 2020-02-20T12:48:45Z

...a/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilder.java

        this.clock = clock;
        this.idp = idp;
+        this.builderFactory = XMLObjectProviderRegistrySupport.getBuilderFactory();


What's the reason for dropping the SamlFactory ?

I thought that SamlUtils covers the functionality and purpose so there was no reason to have both and I moved relevant parts from SamlFactory to SamlUtils ( this is more or less what we do in the saml realm implementation ).

If you feel that we should split somehow ( Instance with initialization for loggers in Factory and all static methods in Utils for instance ) I'm happy to discuss or just do it. I have no strong preference

I found SamlUtils in security to have been a bit overloaded, and was hoping to avoid replicating it here. I'm not stuck on it, I just have an aversion to catch-all utility classes.

Thinking this through again and seeing that I've made all methods in SamlUtils static already, I'm not sure what distinction would make sense. As in what should be in Utils and what should be in the Factory class. At 300 loc, SamlUtils doesn't seem too bloated atm.

I'm open to suggestions on what should be split away and back to a Factory class, or we could merge as is and spit these somehow once/if SamlUtils gets too bloated/overloaded

This is definitely a personal preference thing, so I'm OK with merging as-is (though, in terms of preferences, I preferred the existing code).

Issues with Utility classes:

They become a catch-all dumping ground that becomes a cheap answer when something is needed, and avoids doing the harder design thinking. A good chunk of Utility class methods are working around broken class design that ideally would be fixed up front (but sometimes you have to because the broken class is in the JDK).

Related to the above, they often end up with a weird mix of responsibilities/behaviours. Because they're just chunks of code that "do something, that someone thought was useful", they typically don't go through the process of Is this the right behaviour for this method?.

Related to that, they can be hard to test well, unless they've got a clear interface & a single purpose (those are the best kind of utility method).

More importantly, it becomes hard to test code the uses utility methods. They can't be mocked (because they're static), and it's often hard to trigger some of the behaviours that you need. As an example, while writing tests for SSL error messages, it was almost impossible to force PemUtils to throw certain exceptions so that I could verify they were handled neatly.

It can be hard to find the right method, because of the dumping ground problem. Ever tried to find the right helper method in amongst XContentUtils, XContentParserUtils, XContentHelper, XContentTestUtils ?

Now, you're probably thinking Wait, SamlUtils and SamlFactory makes problem 5 worse. And that's true, but SamlInit and SamlFactory were clear. It's when we decide to take the easy option and just have a Utils class that we lose clarity about what we each class is supposed to do.

However, as I said, personal preference. This definitely isn't a hill I'd die on. But those are the sorts of reasons why I tried to start with SamlInit and SamlFactory for this project.

No need for anyone to die on this hill :) Given we've put in the time to climb it already though, I'll try and come up with something that you feel better with. I'll put the initialization logic in SamlInit to we can call initialize() on it and then move things back to a SamlFactory class which we can instantiate and call methods on.

…p-init-handlers

tvernum · 2020-02-24T01:57:47Z

@elasticmachine update branch

elasticmachine · 2020-02-24T01:57:48Z

merge conflict between base and head

tvernum · 2020-02-24T01:59:57Z

...a/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilder.java

+    public <T extends XMLObject> T object(Class<T> type, QName elementName) {
+        final XMLObject obj = builderFactory.getBuilder(elementName).buildObject(elementName);
+        return cast(type, elementName, obj);
+    }


Putting aside the SamlUtils vs SamlFactory question, why move these methods here?
Surely we'll need them again when handling logout requests/response & authn failures.

tvernum · 2020-02-24T02:14:23Z

...dentity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/CloudServiceProvider.java

-        } catch (URISyntaxException e) {
-            throw new IllegalArgumentException("Invalid URI for Assertion Consumer Service", e);
-        }
+        this.assertionConsumerService = assertionConsumerService;


I still feel like this should be a URI. It is a URI and I think we should consistently treat it like one.

I don't necessarily agree that this is important.

I reverted the change as you requested. There is value in validating that it is a URL (it's a URI but we should not be validating for a URI, as it can't be a URN ) and we will be doing this in the Registration API , no harm in doing it here too. Again, I think that for all intents and purposes when it comes to SAML, handling this as a URL in code - after validation - is unnecessary and it can potentially introduce subtle bugs.
For instance we need to take care so that when comparing ACS values , we are comparing string representations of the URLs and i.e. not URL.equals which does something different.

I didn't change the IDP configuration settings to be Setting<URL> too because of the arguments here, the fact that we already validate them and finally that we have no good existing way to parse and handle Setting<URL> with optional values in a way that makes sense to me ( I can further elaborate if needed )

I'm moving these to Setting in the IDP metadata PR

tvernum · 2020-02-24T02:15:03Z

...dentity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/CloudServiceProvider.java

@@ -38,7 +48,12 @@ public String getEntityId() {
    }

    @Override
-    public URI getAssertionConsumerService() {
+    public String getNameIDPolicyFormat() {
+        return nameIdPolicyFormat;


Can we make this a Set since we've done that with the index?

…p-init-handlers

jkakavas added 2 commits February 6, 2020 23:43

Handle incoming AuthnRequests

f09d280

- Expose an API that consumes (possibly signed) AuthnRequests as defined by the HTTP-Redirect binding. - Process AuthnRequests, validate and parse them into a minimum set of information to be used for subsequent API calls to get a SAML Response

remove redundant schema files

68a13c7

jkakavas added the :Security/Security Security issues without another label label Feb 6, 2020

jkakavas requested a review from tvernum February 6, 2020 21:53

Merge branch 'feature-internal-idp' into idp-sp-init-handlers

5258399

tvernum reviewed Feb 7, 2020

View reviewed changes

jkakavas added 2 commits February 13, 2020 09:37

address feedbacl and add Transport action tests

9573ebd

Merge branch 'feature-internal-idp' into idp-sp-init-handlers

f15cc9f

jkakavas requested a review from tvernum February 13, 2020 07:56

This was referenced Feb 17, 2020

Generate SAML metadata for the Identity Provider #52415

Merged

Write SAML SP data to a dedicated index #52401

Merged

jkakavas added 2 commits February 18, 2020 10:04

Merge remote-tracking branch 'origin/feature-internal-idp' into idp-s…

eec55c1

…p-init-handlers

additions

80b0745

tvernum reviewed Feb 20, 2020

View reviewed changes

jkakavas added 2 commits February 20, 2020 17:37

address feedback

76c45ef

Merge remote-tracking branch 'origin/feature-internal-idp' into idp-s…

16826b2

…p-init-handlers

tvernum reviewed Feb 24, 2020

View reviewed changes

jkakavas added 2 commits February 24, 2020 16:05

Split Utils to Factory and Init, treat URLs as URLs

e3d379d

Merge remote-tracking branch 'origin/feature-internal-idp' into idp-s…

03eba44

…p-init-handlers

jkakavas requested a review from tvernum February 25, 2020 07:10

Support multiple allowed NameID formats per SP

eef3dd6

tvernum approved these changes Feb 26, 2020

View reviewed changes

jkakavas merged commit eaa2e4f into elastic:feature-internal-idp Feb 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle incoming AuthnRequests #52018

Handle incoming AuthnRequests #52018

jkakavas commented Feb 6, 2020

elasticmachine commented Feb 6, 2020

tvernum left a comment

jkakavas commented Feb 13, 2020

tvernum commented Feb 17, 2020

jkakavas commented Feb 18, 2020

tvernum Feb 20, 2020

jkakavas Feb 20, 2020

tvernum Feb 21, 2020

jkakavas Feb 21, 2020

tvernum Feb 24, 2020

jkakavas Feb 24, 2020

tvernum commented Feb 24, 2020

elasticmachine commented Feb 24, 2020

tvernum Feb 24, 2020

tvernum Feb 24, 2020

jkakavas Feb 24, 2020

jkakavas Feb 25, 2020

tvernum Feb 24, 2020

Handle incoming AuthnRequests #52018

Handle incoming AuthnRequests #52018

Conversation

jkakavas commented Feb 6, 2020

elasticmachine commented Feb 6, 2020

tvernum left a comment

Choose a reason for hiding this comment

jkakavas commented Feb 13, 2020

tvernum commented Feb 17, 2020

jkakavas commented Feb 18, 2020

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

tvernum commented Feb 24, 2020

elasticmachine commented Feb 24, 2020

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