search

zitadel / zitadel

ZITADEL - Identity infrastructure, simplified for you.
https://zitadel.com
Apache License 2.0
9.12k stars 585 forks source link

Making SAML2 Relay State optional #4464

Closed SBejga closed 1 year ago

SBejga commented 2 years ago

Hello,

we are in evaluation of Zitadel as an IDP for our product. Most important feature is SSO with SAML2 and OIDC. Since SAML2 support is quite fresh in Zitadel, we had a look at it.

Description According to our tests and your docs, Zitadel requires relayState Parameter.

Unfortunately some of our applications that we want to integrate with Zitadel, does not support relayState.

<Response xmlns="urn:oasis:names:tc:SAML:2.0:protocol" ID="_8d7dc880-6998-40fb-89db-efa8a5899e28" Version="2.0" IssueInstant="2022-09-28T08:38:03.404197Z">
<Issuer xmlns="urn:oasis:names:tc:SAML:2.0:assertion" Format="urn:oasis:names:tc:SAML:2.0:nameid-format:entity">https://[REDACTED]/saml/v2/metadata</Issuer>
<Status xmlns="urn:oasis:names:tc:SAML:2.0:protocol">
<StatusCode xmlns="urn:oasis:names:tc:SAML:2.0:protocol" Value="urn:oasis:names:tc:SAML:2.0:status:RequestDenied"/>
<StatusMessage>empty relaystate</StatusMessage>
</Status>
<Assertion xmlns="urn:oasis:names:tc:SAML:2.0:assertion" Version="" ID="" IssueInstant="">
<Issuer xmlns="urn:oasis:names:tc:SAML:2.0:assertion"/>
</Assertion>
</Response>

If I compare to AzureAD as SSO solution, there is relayState optional.

AzureAD_saml_relaystate

Is there a way to disable relayState? Or make it optional?

Acceptance criteria

fforootd commented 2 years ago

Thanks @SBejga for bringing this up!

@stebenz we should look into this next week.

I think the relay state is optional in the spec. for http post binding

image

fforootd commented 2 years ago

FYI @livio-a

SBejga commented 2 years ago

Hello,

I have seen there is a commit in zitadel/saml. Can you say something about the timeline of this issue? Couple of days or weeks?

We are waiting for this fix to restart testing with our applications :-)

fforootd commented 2 years ago

Since ZITADEL v2.11.0 already uses SAML v0.0.8 I think this could be resolved.

@stebenz can you verify this please

stebenz commented 2 years ago

yes this is included in v0.0.8

ibot3 commented 1 year ago

@stebenz @fforootd Are you sure that this is resolved? I am getting a <StatusMessage>empty relaystate</StatusMessage> error message when trying to use zitadel with zammad via SAML.

With version 2.20 and 2.21.

Console says time="2023-03-13T16:14:11Z" level=error msg="empty value relayState" caller="/home/runner/go/pkg/mod/github.com/zitadel/saml@v0.0.10/pkg/provider/checker/checker.go:83"

I found the following code part in the SAML repo:

https://github.com/zitadel/saml/blob/5c05328386e296ff7a702aad173943c8ecbab5a3/pkg/provider/sso.go#L68-L76

stebenz commented 1 year ago

@ibot3 It was limited to the RedirectBinding, the standard states that even with the RedirectBinding there may be a RelayState, but most of the time it is provided, as Signature and SigAlg are also provided as parameters. In PostBinding it's handled a little differently as Signature and SigAlg are included in the Response.

So it seems that zammad provides not RelayState at all with RedirectBinding?

ibot3 commented 1 year ago

So it seems that zammad provides not RelayState at all with RedirectBinding?

I am not really a SAML pro, but it seems that Zammad uses OmniAuth-SAML. And AFAICS, OmniAuth-SAML is only setting a relaystate on logout: https://github.com/search?q=repo%3Aomniauth%2Fomniauth-saml%20relaystate&type=code

(But I may be wrong, this was only the result of a quick research without much knowledge)

phanpatrik commented 1 year ago

I can confirm this also with current version for an integration with zammad.

rhizoet commented 1 year ago

We also installed Zitadel to use it with Zammad. But we get also the error about the empty relaystate.

Is it possible to fix it? We use Zitadel for a lot of other Apps. Only Zammad makes problems.

Thanks in advance.

fforootd commented 1 year ago

Hm that sound like a weird one. Let me poke around omniauth maybe I spot the problem.

rhizoet commented 1 year ago

@fforootd any news on this? Would be realy helpful if we can use SAML for auth :-)

fforootd commented 1 year ago

@fforootd any news on this? Would be realy helpful if we can use SAML for auth :-)

Nothing new so far.

@hifabienne can we include this in the backlog?

hifabienne commented 1 year ago

@fforootd any news on this? Would be realy helpful if we can use SAML for auth :-)

Nothing new so far.

@hifabienne can we include this in the backlog?

I have added it to the bug fixing list, so we can take it in the next bug fixing friday

Fotkurz commented 1 year ago

Hey any news on this? I'm trying to integrate with Greenhouse.io by using a generic IdP and the SP uses an Redirect Binding sending a GET Request with only SAMLRequest as query parameter, no relayState. Looking at the docs, relay state seems to be optional for both redirect and post bindings.

3.4.1 Required Information
Identification: urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect
3.4.3 RelayState
RelayState data MAY be included with a SAML protocol message transmitted with this binding...
3.5.1 Required Information
Identification: urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST
3.5.3 RelayState
RelayState data MAY be included with a SAML protocol message transmitted with this binding.
hifabienne commented 1 year ago

No news at the moment. We will take it in as soon as possible in a bug fixing friday. But had some more urgent issues. If someone likes to contibute, PRs are always welcome.

Fotkurz commented 1 year ago

No news at the moment. We will take it in as soon as possible in a bug fixing friday. But had some more urgent issues. If someone likes to contibute, PRs are always welcome.

I would like to but I'm really new into SAML so mistakes may happen lol. Based on the docs, maybe the relayState check could be completely removed from the sso.go ? The value is populated in the form parsing step:

https://github.com/zitadel/saml/blob/afbec8e1107e9eb330687324fd6c92bd3b2b12f7/pkg/provider/sso.go#L60

If it is as simple as it looks, I just need to open the PR in my fork...

etranger7 commented 4 months ago

@ibot3 @phanpatrik @rhizoet 
Could you give me some tips on how you integrated zitadel with zammad?
After I set it up and tried to login, the user ends up on the Zitadel console and isn't sent back to zammad.

This is the metadata xml from my instance:

<?xml version="1.0" encoding="UTF-8"?>
<EntityDescriptor entityID="http://zammad.domain.io/auth/saml/metadata"
 xmlns="urn:oasis:names:tc:SAML:2.0:metadata"
 ID="_094bafe1-0f0e-4af3-8da5-dd23e9025987"
 xmlns:alg="urn:oasis:names:tc:SAML:metadata:algsupport"
 xmlns:mdattr="urn:oasis:names:tc:SAML:metadata:attribute"
 xmlns:mdrpi="urn:oasis:names:tc:SAML:metadata:rpi"
 xmlns:mdui="urn:oasis:names:tc:SAML:metadata:ui"
 xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion">
 <SPSSODescriptor AuthnRequestsSigned="false" WantAssertionsSigned="false" protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol">
   <SingleLogoutService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect" Location="http://zammad.domain.io/auth/saml/slo" ResponseLocation="http://zammad.domain.io/auth/saml/slo"/>
   <NameIDFormat>urn:oasis:names:tc:SAML:2.0:nameid-format:transient</NameIDFormat>
   <AssertionConsumerService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST" Location="http://zammad.domain.io/auth/saml/callback" index="0" isDefault="true"/>
   <AttributeConsumingService index="1" isDefault="true">
     <ServiceName xml:lang="en">Required attributes</ServiceName>
     <RequestedAttribute FriendlyName="Email address" Name="email" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic" isRequired="false"/>
     <RequestedAttribute FriendlyName="Full name" Name="name" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic" isRequired="false"/>
     <RequestedAttribute FriendlyName="Given name" Name="first_name" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic" isRequired="false"/>
     <RequestedAttribute FriendlyName="Family name" Name="last_name" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic" isRequired="false"/>
   </AttributeConsumingService>
 </SPSSODescriptor>
</EntityDescriptor>

Any ideas on what I'm doing wrong?