search

OpenConext / Stepup-Gateway

Stepup Gateway
Apache License 2.0
6 stars 3 forks source link

Missing EPTI attribute results in Assertion without Subject #389

Closed phavekes closed 3 days ago

phavekes commented 3 days ago

This issue is imported from pivotal - Originaly created at Apr 25, 2017 by Pieter van der Meulen

When the remote IdP (e.g. OpenConext engineblock) does not provide the urn:mace:dir:attribute-def:eduPersonTargetedID (EPTI) attribute that the Gateway requires to set the Subject in the Assertion in the SAML Response an Assertion without Subject is created.

This is not correct behaviour. When the Gateway cannot determine the Subject element it must generate an error, and must not generate a Response.

Implementation: When any of the following is true, the Stepup-Gateway must show an error indicating that the urn:mace:dir:attribute-def:eduPersonTargetedID attribute is missing or does not have the required value. This error may be shown to the user.

Rationale: The gateway cannot continue without breaking the contract (there is a Subject) it has with the SP. Even though the Subject element is optional in the SAML standard, it is very unlikely that a SP will be able to handle missing Subject element. Instead of showing an error on the gateway we could generate a SAML error. See XXX for a discussion on this.

Backgroud: The missing or invalid EPTI is a configuration error of the IdP. In the case of OpenConext engineblock, the operator did not release this attribute by putting adding it in the ARP(s) that is/are in place between OpenConext as IdP and the Stepup Gateway. The special handling of the EPTI attribute is described in https://github.com/OpenConext/Stepup-Gateway/blob/develop/docs/SAMLProxy.md#response-from-the-remote-idp-to-the-stepup-gateway

phavekes commented 3 days ago

Is it OK if the error messages are always displayed in English or do they need to be translated? (Michiel Kodde - May 23, 2017)

phavekes commented 3 days ago 
@pmeulen, the code I mentioned yesterday can be found [here](https://github.com/OpenConext/Stepup-Gateway/blob/feature/catch-missing-epti/src/Surfnet/StepupGateway/GatewayBundle/Controller/GatewayController.php#L138-L151). Erroring out of the authentication process in the Gateway seemed unintuitive.

The question asked above still remains. Should the error message always be in English or can you provide the desired translations? (Michiel Kodde - Jun 7, 2017)

phavekes commented 3 days ago

When this situation occurs there is not much you can do. The IdP (for us: SURFconext engineblock) needs to fix its configuration so it releases the EPTI attribute. Showing an error on the gateway is OK. This is a kind of error situation where it will either work for all users of an SP, or it won\'t. The only other option is to forward the error to the SP as there is no SAML mechanism to communicate the error to the IdP. It is us, as gateway operators, that need to act and involve the IdP. Will this error be thrown such that an ART code is generated? That will help in directing the operations team to handle this situation.

Looking at the code you are actually verifying whether the Assertion from the Gateway to the SP has a Subject. This is a good check as it prevents us from releasing an assertion that the SP cannot handle, but why so late in the process? At this point an error of "Could not determine Subject" would be more correct. You know that the EPTI is missing at https://github.com/OpenConext/Stepup-Gateway/blob/develop/src/Surfnet/StepupGateway/GatewayBundle/Service/ProxyResponseService.php#L107 This is where the gateway reads the EPTI from the Assertion from the IdP and puts in the new assertion for the SP. At that point the error message you are using is correct. At some point we may want to make the way that the Subject is determined configurable to support other usage scenario\'s of the gateway.

Regarding language: all errors to the logs are in English, and I do want to keep it that way. We could translate the error message to the user, but that won\'t do the user much good. The best they can do is relay the message to someone who can help. I see no point in investing time in translating this error. (Pieter van der Meulen - Jun 7, 2017)

phavekes commented 3 days ago 
@pmeulen

Thanks for the feedback, the proposed changes have been applied. In response to your feedback: The proposed location for the assertion (ProxyResponseService) is a better suited context to perform the test on the EPTI. It however is not earlier in the process.

In addition to the previous solution, I was now able to add unit tests to validate the correct handling of the missing EPTI. (Michiel Kodde - Jun 15, 2017)