SAML RelayState length

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

SAML RelayState length

Colm O hEigeartaigh
Hi all,

According to the SAML 2.0 binding spec:

RelayState data MAY be included with a SAML protocol message transmitted
with this binding. The value MUST NOT exceed 80 bytes in length

However, the relaystate we are using in Syncope, is a signed JWT, which has
length 371. Perhaps we need to reconsider making it a signed token?

Colm.


--
Colm O hEigeartaigh

Talend Community Coder
http://coders.talend.com
Reply | Threaded
Open this post in threaded view
|

Re: SAML RelayState length

ilgrosso
Administrator
On 15/08/2017 11:38, Colm O hEigeartaigh wrote:
> Hi all,
>
> According to the SAML 2.0 binding spec:
>
> RelayState data MAY be included with a SAML protocol message transmitted
> with this binding. The value MUST NOT exceed 80 bytes in length
>
> However, the relaystate we are using in Syncope, is a signed JWT, which has
> length 371. Perhaps we need to reconsider making it a signed token?

Hi Colm,
at the moment the relay state as signed JWT is used to hold [1]:

* the preference to use the (non-standard?) deflate encoding - which
might be omitted, we could just take such setting from IdP configuration
* the AuthnRequest ID, for later checking the login response [2]
* the duration, for expiration

Out of such three items, I would only keep the second but I'd rather
prefer to be relatively sure that it was not tampered with, when it
comes back for [2]: any alternative to use a signed JWT for such purpose?

Regards.

[1]
https://github.com/apache/syncope/blob/2_0_X/ext/saml2sp/logic/src/main/java/org/apache/syncope/core/logic/SAML2SPLogic.java#L327-L329
[2]
https://github.com/apache/syncope/blob/2_0_X/ext/saml2sp/logic/src/main/java/org/apache/syncope/core/logic/SAML2SPLogic.java#L408

--
Francesco Chicchiriccò

Tirasa - Open Source Excellence
http://www.tirasa.net/

Member at The Apache Software Foundation
Syncope, Cocoon, Olingo, CXF, OpenJPA, PonyMail
http://home.apache.org/~ilgrosso/

Reply | Threaded
Open this post in threaded view
|

Re: SAML RelayState length

Colm O hEigeartaigh
Hi Francesco,

On Thu, Aug 17, 2017 at 2:10 PM, Francesco Chicchiriccò <[hidden email]
> wrote:

>
> Hi Colm,
> at the moment the relay state as signed JWT is used to hold [1]:
>
> * the preference to use the (non-standard?) deflate encoding - which might
> be omitted, we could just take such setting from IdP configuration
> * the AuthnRequest ID, for later checking the login response [2]
> * the duration, for expiration
>
> Out of such three items, I would only keep the second but I'd rather
> prefer to be relatively sure that it was not tampered with, when it comes
> back for [2]: any alternative to use a signed JWT for such purpose?
>

I agree that we don't need the information about deflate encoding in there.
The alternative to sending the token is to cache the values locally (could
use EhCache, which is what we do with CXF, or store them in the session I
guess) keyed using a random String which is then the RelayState. What do
you think about switching to this approach?

Colm.


>
> Regards.
>
> [1] https://github.com/apache/syncope/blob/2_0_X/ext/saml2sp/
> logic/src/main/java/org/apache/syncope/core/logic/SAML
> 2SPLogic.java#L327-L329
> [2] https://github.com/apache/syncope/blob/2_0_X/ext/saml2sp/
> logic/src/main/java/org/apache/syncope/core/logic/SAML2SPLogic.java#L408
>
> --
> Francesco Chicchiriccò
>
> Tirasa - Open Source Excellence
> http://www.tirasa.net/
>
> Member at The Apache Software Foundation
> Syncope, Cocoon, Olingo, CXF, OpenJPA, PonyMail
> http://home.apache.org/~ilgrosso/
>
>


--
Colm O hEigeartaigh

Talend Community Coder
http://coders.talend.com
Reply | Threaded
Open this post in threaded view
|

Re: SAML RelayState length

ilgrosso
Administrator
On 30/08/2017 19:01, Colm O hEigeartaigh wrote:

> Hi Francesco,
>
> On Thu, Aug 17, 2017 at 2:10 PM, Francesco Chicchiriccò <[hidden email]> wrote:
>
>> Hi Colm,
>> at the moment the relay state as signed JWT is used to hold [1]:
>>
>> * the preference to use the (non-standard?) deflate encoding - which might
>> be omitted, we could just take such setting from IdP configuration
>> * the AuthnRequest ID, for later checking the login response [2]
>> * the duration, for expiration
>>
>> Out of such three items, I would only keep the second but I'd rather
>> prefer to be relatively sure that it was not tampered with, when it comes
>> back for [2]: any alternative to use a signed JWT for such purpose?
> I agree that we don't need the information about deflate encoding in there.
> The alternative to sending the token is to cache the values locally (could
> use EhCache, which is what we do with CXF, or store them in the session I
> guess) keyed using a random String which is then the RelayState. What do
> you think about switching to this approach?

What I don't really like here is the additional setup that would be needed.
The only alternative I can see is to create an Entity to store such
RelayState values in the internal storage, with a job that periodically
cleans up the expired.

WDYT?

Anyway, I see several SAML 2.0 implementations out there not enforcing
the 80 chars limit: would removing all but the AuthnRequestID from the
current JWT-based Relay State be an acceptable compromise?

Regards.

> [1] https://github.com/apache/syncope/blob/2_0_X/ext/saml2sp/
> logic/src/main/java/org/apache/syncope/core/logic/SAML
> 2SPLogic.java#L327-L329
> [2] https://github.com/apache/syncope/blob/2_0_X/ext/saml2sp/
> logic/src/main/java/org/apache/syncope/core/logic/SAML2SPLogic.java#L408

--
Francesco Chicchiriccò

Tirasa - Open Source Excellence
http://www.tirasa.net/

Member at The Apache Software Foundation
Syncope, Cocoon, Olingo, CXF, OpenJPA, PonyMail
http://home.apache.org/~ilgrosso/

Reply | Threaded
Open this post in threaded view
|

Re: SAML RelayState length

Colm O hEigeartaigh
On Thu, Aug 31, 2017 at 7:51 AM, Francesco Chicchiriccò <[hidden email]
> wrote:

>
> Anyway, I see several SAML 2.0 implementations out there not enforcing the
> 80 chars limit: would removing all but the AuthnRequestID from the current
> JWT-based Relay State be an acceptable compromise?
>

Yeah, let's just leave it for now. We can always revisit if becomes a
problem. +1 on removing the deflate encoding switch from the token. I'm not
sure about removing the expiration, it's probably a good idea to reject
stale RelayStates.

Colm.



> Regards.
>
> [1] https://github.com/apache/syncope/blob/2_0_X/ext/saml2sp/
>> logic/src/main/java/org/apache/syncope/core/logic/SAML
>> 2SPLogic.java#L327-L329
>> [2] https://github.com/apache/syncope/blob/2_0_X/ext/saml2sp/
>> logic/src/main/java/org/apache/syncope/core/logic/SAML2SPLogic.java#L408
>>
>
> --
> Francesco Chicchiriccò
>
> Tirasa - Open Source Excellence
> http://www.tirasa.net/
>
> Member at The Apache Software Foundation
> Syncope, Cocoon, Olingo, CXF, OpenJPA, PonyMail
> http://home.apache.org/~ilgrosso/
>
>


--
Colm O hEigeartaigh

Talend Community Coder
http://coders.talend.com
Reply | Threaded
Open this post in threaded view
|

Re: SAML RelayState length

ilgrosso
Administrator
On 31/08/2017 11:33, Colm O hEigeartaigh wrote:
> On Thu, Aug 31, 2017 at 7:51 AM, Francesco Chicchiriccò <[hidden email]> wrote:
>
>> Anyway, I see several SAML 2.0 implementations out there not enforcing the
>> 80 chars limit: would removing all but the AuthnRequestID from the current
>> JWT-based Relay State be an acceptable compromise?
> Yeah, let's just leave it for now. We can always revisit if becomes a
> problem. +1 on removing the deflate encoding switch from the token. I'm not
> sure about removing the expiration, it's probably a good idea to reject
> stale RelayStates.

I remember now why the deflateEncoding info is in the Relay State: the
information is needed to read the SAML response [3], at a point where it
is not already possible to identify the IdP (from which one could fetch
the same flag).

About checking the Relay State expiration, the duration is currently set
to 5 seconds but I am afraid it is not curerntly verified during the
response validation.

Regards.

> [1] https://github.com/apache/syncope/blob/2_0_X/ext/saml2sp/logic/src/main/java/org/apache/syncope/core/logic/SAML
> 2SPLogic.java#L327-L329
> [2] https://github.com/apache/syncope/blob/2_0_X/ext/saml2sp/logic/src/main/java/org/apache/syncope/core/logic/SAML2SPLogic.java#L408
[3]
https://github.com/apache/syncope/blob/master/ext/saml2sp/logic/src/main/java/org/apache/syncope/core/logic/saml2/SAML2ReaderWriter.java#L150

--
Francesco Chicchiriccò

Tirasa - Open Source Excellence
http://www.tirasa.net/

Member at The Apache Software Foundation
Syncope, Cocoon, Olingo, CXF, OpenJPA, PonyMail
http://home.apache.org/~ilgrosso/

Reply | Threaded
Open this post in threaded view
|

Re: SAML RelayState length

Colm O hEigeartaigh
On Thu, Aug 31, 2017 at 11:22 AM, Francesco Chicchiriccò <
[hidden email]> wrote:

>
>
> About checking the Relay State expiration, the duration is currently set
> to 5 seconds but I am afraid it is not curerntly verified during the
> response validation.
>

5 seconds seems a bit unreasonable, the user may have to type in a username
+ password at the IdP! We could just do something similar to the code in
JWTAuthenticationProvider in terms of verifying the expiry.

Colm.


>
> Regards.
>
> [1] https://github.com/apache/syncope/blob/2_0_X/ext/saml2sp/
>> logic/src/main/java/org/apache/syncope/core/logic/SAML
>> 2SPLogic.java#L327-L329
>> [2] https://github.com/apache/syncope/blob/2_0_X/ext/saml2sp/
>> logic/src/main/java/org/apache/syncope/core/logic/SAML2SPLogic.java#L408
>>
> [3] https://github.com/apache/syncope/blob/master/ext/saml2sp/
> logic/src/main/java/org/apache/syncope/core/logic/saml
> 2/SAML2ReaderWriter.java#L150
>
> --
> Francesco Chicchiriccò
>
> Tirasa - Open Source Excellence
> http://www.tirasa.net/
>
> Member at The Apache Software Foundation
> Syncope, Cocoon, Olingo, CXF, OpenJPA, PonyMail
> http://home.apache.org/~ilgrosso/
>
>


--
Colm O hEigeartaigh

Talend Community Coder
http://coders.talend.com
Reply | Threaded
Open this post in threaded view
|

Re: SAML RelayState length

ilgrosso
Administrator
On 31/08/2017 12:29, Colm O hEigeartaigh wrote:
> On Thu, Aug 31, 2017 at 11:22 AM, Francesco Chicchiriccò <[hidden email]> wrote:
>
>> About checking the Relay State expiration, the duration is currently set
>> to 5 seconds but I am afraid it is not curerntly verified during the
>> response validation.
> 5 seconds seems a bit unreasonable, the user may have to type in a username
> + password at the IdP! We could just do something similar to the code in
> JWTAuthenticationProvider in terms of verifying the expiry.

Done:

https://git-wip-us.apache.org/repos/asf?p=syncope.git;h=55e09aa
https://git-wip-us.apache.org/repos/asf?p=syncope.git;h=b3db3b1

> [1] https://github.com/apache/syncope/blob/2_0_X/ext/saml2sp/logic/src/main/java/org/apache/syncope/core/logic/SAML2SPLogic.java#L327-L329
> [2] https://github.com/apache/syncope/blob/2_0_X/ext/saml2sp/logic/src/main/java/org/apache/syncope/core/logic/SAML2SPLogic.java#L408
>> [3] https://github.com/apache/syncope/blob/master/ext/saml2sp/logic/src/main/java/org/apache/syncope/core/logic/saml2/SAML2ReaderWriter.java#L150

--
Francesco Chicchiriccò

Tirasa - Open Source Excellence
http://www.tirasa.net/

Member at The Apache Software Foundation
Syncope, Cocoon, Olingo, CXF, OpenJPA, PonyMail
http://home.apache.org/~ilgrosso/