SAML params are hardcoded: RequestID, IssueInstant

lanorkin commented 7 years ago

I see here https://github.com/apereo/dotnet-cas-client/blob/master/DotNetCasClient/Validation/TicketValidator/Saml11TicketValidator.cs#L190 the following:

protected string RetrieveResponseFromServer(string validationUrl, string ticket)
{
    StringBuilder messageBuilder = new StringBuilder();
    messageBuilder.AppendLine(@"<SOAP-ENV:Envelope xmlns:SOAP-ENV=""http://schemas.xmlsoap.org/soap/envelope/"">");
    messageBuilder.AppendLine(@"<SOAP-ENV:Header/><SOAP-ENV:Body>");
    messageBuilder.AppendLine(@"<samlp:Request xmlns:samlp=""urn:oasis:names:tc:SAML:1.0:protocol"" ");
    messageBuilder.AppendLine(@"MajorVersion=""1"" MinorVersion=""1"" RequestID=""_192.168.16.51.1024506224022"" ");
    messageBuilder.AppendLine(@"IssueInstant=""2002-06-19T17:03:44.022Z"">");
    messageBuilder.AppendLine(@"<samlp:AssertionArtifact>" + ticket);
    messageBuilder.AppendLine(@"</samlp:AssertionArtifact></samlp:Request></SOAP-ENV:Body></SOAP-ENV:Envelope>");
    string message = messageBuilder.ToString();
    ...

As you can see everything except ticket is harcoded.

Is it intended? From my understanding, parameters like RequestID / IssueInstant should be unique / regenerated for each request.

phantomtypist commented 7 years ago

Good catch. I'm not sure why that was hardcoded. Did you do any testing with making RequestID and IssueInstant dynamic to see if intended behavior changed?

The Java client is generating those values dynamically:

@scottt732

lanorkin commented 7 years ago

What I think should be happening is

1) Client should generate random / unique RequestID field 2) Server should include InResponseTo field and fill it with RequestID value 3) Client should compare InResponseTo field with the value it sent to server and reject response if that doesn't much

I really passerby in CAS area, and I haven't look carefully at saml specs, but for me that at least seems logical

CAS server I connect to doesn't return InResponseTo value - that's separate issue, but it means that I cannot really test dynamic values in my case.

Client hardcodes these values and doesn't try to validate them from response - I think that's another issue to be fixed here

phantomtypist commented 7 years ago

I appreciate your feedback and eyeballs. I'll take a closer look at what the Java and PHP clients are doing and then take your suggestion into consideration (as a whole) and go from there.

Ideally the .NET, Java, and PHP clients (the official clients that is) should all be implemented in a similar fashion and exhibit the same behaviors.

phantomtypist commented 5 years ago

@hn3000 Code is done and there is a CI build from the develop branch available as a NuGet package on our CI MyGet feed

I'll test it when I get to work Monday. If it works you'll see a production release to the official NuGet.org site for 1.2.0.

If you want you can test the code too. In the aforementioned MyGet feed, use package version 1.2.0-ci-00074

phantomtypist commented 5 years ago

Included in release 1.2.0

apereo / dotnet-cas-client

SAML params are hardcoded: RequestID, IssueInstant #64