[keycloak-dev] Implementation of artifact binding (JIRA KEYCLOAK-831)

[Hynek Mlnarik]

Hi Alistair,

thank you for your willingness to contribute!

However the ARTIFACT binding would need to be implemented in full, with a
sufficient test coverage. Partial implementation cannot be accepted. It
would also need some changes in the SAML code since currently it is
basically expecting either POST or REDIRECT. One of the implications is
that boolean has been widely used to discriminate the two while enum would
be more appropriate. Such places would need to be cleaned up first.

If you would like to do that, we could start with such refactorings once
the feature freeze phase [1] finishes.

Thank you

--Hynek

[1] http://lists.jboss.org/pipermail/keycloak-dev/2018-September/011263.html

On Fri, Sep 28, 2018 at 2:35 PM Doswald Alistair <alistair.doswald at elca.ch>
wrote:

> Implementation of artifact binding (JIRA KEYCLOAK-831)
>
> Hello,
>
> Last week I did a PoC implementation of the SAML artifact binding in a
> branch off keycloak 4.3.0.Final. The implementation can be seen here at
> https://github.com/AlistairDoswald/keycloak/tree/projectathon (don't
> judge me too harshly for the quality of the code if you look at it, I had
> about 2 days to have a working implementation, which included finding out
> how that part of the protocol worked).
>
> However, I now want to write a "correct" implementation against
> keycloak/master and if possible I'd like some feedback/advice on my
> intended implementation.
>
>
> 1. General implementation
>
> >From the description in the SAML specification (see here section 3.6,
> https://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf),
> artifact binding can be used for transmitting the request message, the
> response message or both.
>
> Initially, I intend only to do the implementation for the response
> messages. If I'm not mistaken, this means only for the Response and
> LogoutResponse messages. Would this be considered a suitable implementation
> of the JIRA?
>
>
> 2. User interface
>
> When a SP requests an artifact, it can do so by specifying HTTP-Artifact
> instead of HTTP-POST or HTTP-redirect, and the process is then transparent
> with regard to the configuration of the client. However, I believe that the
> client should have a "Force artifact binding" binary slider and also a
> field to specify an artifact binding address. In this manner, the artifact
> binding can be used in conjunction with the IdP initiated login method.
>
> Importing must also set the artifact binding address if it is present in
> the SP metadata.
>
>
> 3. IdP metadata
>
> IdP metadata must contain at least one ArtifactResolutionService, I intend
> to have only one, with its index set to 0 and isDefault=true, and the
> binding set to the same address as the HTTP-POST (as for ECP)
>
>
> 4. Sending an artifact instead of the normal saml message
>
> This is the section for which I have the greatest uncertainty with respect
> to a correct implementation.
>
> Broadly this means intercepting the output response, and sending a 302
> redirect or a POSTed form with the artifact instead. Considering the length
> of the artifact, I see no reason to use a form, but should this be an
> option in the GUI?
>
> More practically, this means generating the response, saving it in the
> cache, and sending the redirect (or form) instead. I believe that the
> client's cache would be the best place to save this information (through
> the AuthenticatedClientSessionModel to be precise), but I'm not certain
> because it's the first time I'm seeking to store some new information in
> the cache. The key would be the artifact, and the value in my view should
> be the document, as that way we can create a complete signed/encrypted
> ArtifactResponse containing the Response or LogoutResponse.
>
> For the implementation details I'm not sure if it would be best to make
> the changes directly in the SamlProtocol class, or to do something similar
> to the SamlECPProfileService which overrides the methods of the
> SamlProtocol. For SamlECPProfileService the current implementation makes
> sense, but for artifact binding I fear there would be significant code
> duplication (of course, I could also do a mix with some small modifications
> in the SamlProtocol class and a SamlArtifactProfileService, or something
> similar).
>
> For triggering this artifact workflow, it would either be if the
> AuthnRequest has a ProtocolBinding set to
> urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Artifact, or if the client has
> "force artifact binding" set to true.
>
>
> 5. Receiving an ArtifactResolve message
>
> For this part, my current implementation seems correct to me: the
> soapBinding method in class SamlService is modified to check the contents
> of the soap message arriving: if it is an ArtifactResolve, the
> corresponding ArtifactResponse generated earlier is packaged in a soap
> message and sent as a response. If not, the ECP profile is tried.
>
> The key-ArtifactResponse pair is removed from the cache during this
> operation. I am, however, not sure yet how the cache should handle purging
> of expired ArtifactResponse messages that are never asked for.
>
>
> 6. Errors, logging and audit
>
> Obviously, the error handling should work as described in the protocol,
> but also be logged as such. I don't think there's any messages to log in
> INFO, but the DEBUG logs should show the messages and allow an admin to
> easily put the entire sequence together.
>
> Also, I don't think there's any need for any extra information in the
> audit logs.
>
>
> 7. Tests
>
> Obviously, I'll have to add some tests for these functions, which should
> be:
>
> - Standard unit tests for individual functions that can be separated from
> objects that would otherwise have to be mocked
> - Tests with arquillian to test the flow with artifact binding (sp
> initiated and idp initiated), the options available in the GUI (extra
> field, forced) as well as the error cases (i.e. asking twice for the same
> artifact, for an artifact that doesn't exist, etc...).
>
>
> If you have any comments (anything missing, things that should be
> implemented differently in your view, etc...) feel free to let me know.
>
> Best regards,
>
> Alistair Doswald
>
>
> _______________________________________________
> keycloak-dev mailing list
> keycloak-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>