Hardcoded ValidUntil causing issues, SAML2 standard references it as optional

[Glowsome]

Case :

• Microfocus AccessManager IDP v4.5.3
• OneLogin-PHP Client on SP.

Issue:

• The IDP does not have a function to dynamically reload Metadata.
• As validUntil is hardcoded in \lib\Saml2\Metadata.php this is causing issues.
• Current (unwanted) Workaround is manually removing the entry from the generated Metadata.
• According https://docs.oasis-open.org/security/saml/v2.0/saml-metadata-2.0-os.pdf Line 316 the parameter is optional

Solution wanted:

• As stated above the definition of validUntill should be an Optional parameter.
• This will solve manual interaction on the Metadata before importing it into the mentioned IDP.

[pitbulk]

Is not hardcoded, you can provide it as a parameter:

• https://github.com/onelogin/php-saml/blob/master/lib/Saml2/Settings.php#L897
• https://github.com/onelogin/php-saml/blob/d074a814c45b96fd69b0401493385a17e006c533/lib/Saml2/Metadata.php#L27

[Glowsome]

Is not hardcoded, you can provide it as a parameter:

* https://github.com/onelogin/php-saml/blob/master/lib/Saml2/Settings.php#L897

* https://github.com/onelogin/php-saml/blob/d074a814c45b96fd69b0401493385a17e006c533/lib/Saml2/Metadata.php#L27

You are correct if you are referring to the option to manipulate it.
However if it is NOT defined it gets set with a default value, so whatever i do .. it is not vanishing/dropped from the generated Metadata.

Please see https://github.com/onelogin/php-saml/blob/master/lib/Saml2/Metadata.php#L30

As it is then in the Metadata by default - see https://github.com/onelogin/php-saml/blob/master/lib/Saml2/Metadata.php#L159
There is no way to get rid - other then hacking into the generate metadata manually.

So in essence it is Hardcoded.

Therefore my request still stands .. it is an optional parameter, and should not be presented in the generated Metadata if it is not defined.

[Glowsome]

Additional argument of having it not present if it is undefined in settings is the fact that if an IDP is not capable of dynamically reloading Metadata (as it is in my case) the default set expiration (as written in the code) sets it to 2 days.

So in this :

• To avoid setting/defining it to some ridiculous setting/timespan (like 2099) it should be not present in the Metadata itself.
  => This is exactly what i am trying to avoid here, as the definition of this tag then loses its purpose.
• A possible solution (if the tag cannot be ommited, and the parameter is not explicitly set) is to Query the certificate's expiry date from the IDP, and then dynamically set the ValidUntill parameter to the same as the expiry of the certificate, therefore aligning it.
  => This ofcourse will require extensive additions to the code, and IMHO it is alot more effort then to just ommit it if not defined.

[pitbulk]

If I change the behavior of the current method that generates the Metadata to not include such values if not provided, I can break other environments that expects it

[Glowsome]

If I change the behavior of the current method that generates the Metadata to not include such values if not provided, I can break other environments that expects it

I am not really sure as to interpret this reply so i will state it harshly ... ?
Either you are suggesting :

• "changing behaviour as to what is 'optional' as the standard dictates is unconveint to you"
  or
• "i wave off your arguments and just continue on this 'hardcoded-way'"

As to the ValidUntill ( and adjacent CacheDuration) parameter :
=> if a client was implemented corectly they also would have it set as an 'optional' parameter, thus not 'demanding it'

Either way i have forked the repo, and am trying to correct this (for me) unwanted way of implemenatation to get rid on this IMHO ugly hack to manually strip the metadata of the optional tags without compromising the IDP's who can coap with it.

• Glowsome

[pitbulk]

From the SAML spec: https://docs.oasis-open.org/security/saml/v2.0/saml-metadata-2.0-os.pdf Page 9

When used as the root element of a metadata instance, this element (EntitiesDescriptor or EntityDescriptor) MUST
contain either a validUntil or cacheDuration attribute. It is RECOMMENDED that only the root element of a metadata instance
contain either attribute.

So you need to set an expiration on the SAML Metadata, that why when not provided, I set default expiration times.

The IDP does not have a function to dynamically reload Metadata.

This is your specific case of use. Is the IdP reading the SP Metadata directly from stored XMLs rather than process the XML when it is received and storing the parameters?

"changing behaviour as to what is 'optional' as the standard dictates is unconveint to you"

As I shared, a Metadata with no expiration does not follow the standard, that why I'm not changing the current behavior.

But in addition, if the toolkit has an specific behavior that the developers expect, is not that easy to change it, it need to be done in a major release because the change can impact current developers integrations. Currently php-saml has 9.3M of downloads at packagist, so I need to take care of all the developers trusting php-saml keeping the project stable.

You had the option to fork the project (as you did), but there are other alternatives like simplesamlphp or lightSAML

[Glowsome]

This is your specific case of use. Is the IdP reading the SP Metadata directly from stored XMLs rather than process the XML when it is received and storing the parameters?

The XML/Metadata is retrieved when creating the connection ( either retrieval via url or file upload )
There is no mechanism in place to dynamically reload/re-grab metadata other then a manual action on the IDP itself.

Also the refrenced page, and text are relevant when using EntitiesDescriptor, not the actual EntityDescriptor where its all about.

Yes i do concur that when EntitiesDescriptor is used then the 'MUST' should be honored as written in the standard.

• Glowsome

[pitbulk]

I disagree, the section is talking about |EntitiesDescriptor| or |EntityDescriptor| [One or More]

And the saml2int profile directly set validUntil as mandatory.

[SDP-MD03]
Metadata without a validUntil attribute on its root element MUST be rejected.

[Glowsome]

I disagree, the section is talking about |EntitiesDescriptor| or |EntityDescriptor| [One or More]

And the saml2int profile directly set validUntil as mandatory.

[SDP-MD03]
Metadata without a validUntil attribute on its root element MUST be rejected.

Again you are quoting me on information that refers to having as Root element. ( secttion 2.3.1 - line 310 thru 358.
In which it is stated that when is the root element then ValidUntill is a MUST.

However ...
If you were to read the next section ( 2.3.2 Element Lines 360 thru 442)
The 'MUST' is not found regarding the ValidUntill, but only mentioned as options.

[Glowsome]

Anyway - just besides the whole discussion as to yes/no being part of the standard and Must/optional my plan in regards of forking is to implement a way to omit it without impacting/affecting current implementations.

I am aiming to add functionality in a well-mannered way, without impacting a direct behavioural change in current code/ your 9.3M implementations, and still find a solution to solve the issue i am facing ( and i guess every other IDP-owner who uses the same software as i do).

So :

• if validUntill is NOT defined -> implement standard as is now present in code
• if validUntill is defined and supplied in the correct format ( as now defined in the code) then implement the setting in the Metadata as such.
• if validUntill is defined, but contains a specific keyword like (example) 'omit' then in the Metadata omit the references to validUntill (and adjacent setting cacheDuration).

IMHO this would be the easiest as it does not need the addition of a new 'setting', but an additional evaluation of a/the value if given, and it does not change current behaviour of the implementation.

• Glowsome

[Glowsome]

@pitbulk if i were to get this done, would you be willing to concider a PR if i were to submit one ?

I mean my aim is to honor both the current implementation and be able to also enable the ones who use my type/brand of IDP to be able to use the already provided code - so in essense i am trying to broaden the implementation.

I am not aiming to move away from current code just by forking, but enhance ( and theerefore gain support) thru the general code to also support my case i would very much welcome my effort.
In this i would (really) like to get a 1:1 convo going as to where we can get to a mutual agreement in implementing this without affecting current users, but does enhance it to the thing (my IDP) is in need of.

• Glowsome

[pitbulk]

Maybe the less intrusive way is to add a "$noExpiration" (by default to false) parameter to the Metadata builder and then create in the method a

$expirationTimeSection= "";
if (!$noExpiration) {
    $expirationTimeSection = """validUntil="{$validUntilTime}"
                     cacheDuration="PT{$cacheDuration}S"
                      """;
}

and then in the template

<md:EntityDescriptor xmlns:md="urn:oasis:names:tc:SAML:2.0:metadata"
              {$expirationTimeSection}entityID="{$spEntityId}">