diff --git a/Sustainsys.Saml2/Configuration/SPOptions.cs b/Sustainsys.Saml2/Configuration/SPOptions.cs index 738fdf5b1..16eec9f75 100644 --- a/Sustainsys.Saml2/Configuration/SPOptions.cs +++ b/Sustainsys.Saml2/Configuration/SPOptions.cs @@ -1,5 +1,7 @@ -using Sustainsys.Saml2.Metadata; +using Microsoft.IdentityModel.Tokens; +using Sustainsys.Saml2.Metadata; using Sustainsys.Saml2.Saml2P; +using Sustainsys.Saml2.Tokens; using System; using System.Collections.Concurrent; using System.Collections.Generic; @@ -22,7 +24,7 @@ public class SPOptions /// public SPOptions() { - MetadataCacheDuration = new XsdDuration(hours: 1); + MetadataCacheDuration = new XsdDuration(hours: 1); Compatibility = new Compatibility(); OutboundSigningAlgorithm = XmlHelpers.GetDefaultSigningAlgorithmName(); MinIncomingSigningAlgorithm = XmlHelpers.GetDefaultSigningAlgorithmName(); @@ -102,7 +104,7 @@ public Saml2PSecurityTokenHandler Saml2PSecurityTokenHandler // Capture in a local variable to prevent race conditions. Reads and writes // of references are atomic so there is no need for a lock. var value = saml2PSecurityTokenHandler; - if(value == null) + if (value == null) { // Set the saved value, but don't trust it - still use a local var for the return. saml2PSecurityTokenHandler = value = new Saml2PSecurityTokenHandler(this); @@ -112,7 +114,7 @@ public Saml2PSecurityTokenHandler Saml2PSecurityTokenHandler } set { - saml2PSecurityTokenHandler = value; + saml2PSecurityTokenHandler = value; } } @@ -135,7 +137,7 @@ public EntityId EntityId } set { - if(saml2PSecurityTokenHandler != null) + if (saml2PSecurityTokenHandler != null) { throw new InvalidOperationException("Can't change entity id when a token handler has been instantiated."); } @@ -157,7 +159,7 @@ public string ModulePath } set { - if(value == null) + if (value == null) { throw new ArgumentNullException(nameof(value)); } @@ -212,7 +214,7 @@ public ICollection Contacts } readonly ICollection attributeConsumingServices - = new List(); + = new List(); /// /// Collection of attribute consuming services for the service provider. @@ -292,7 +294,7 @@ public ReadOnlyCollection MetadataCertificates var futureBothCertExists = metaDataCertificates .Any(c => c.Status == CertificateStatus.Future && c.Use == CertificateUse.Both); - foreach(var cert in metaDataCertificates) + foreach (var cert in metaDataCertificates) { // Just like we stop publishing Encryption cert immediately when a Future one is added, // in the case of a "Both" cert we should switch the current use to Signing so that Idp's stop sending @@ -358,7 +360,7 @@ private IEnumerable PublishableServiceCertificates /// overriden for each . /// public string OutboundSigningAlgorithm { get; set; } - + /// /// Metadata flag that we want assertions to be signed. /// @@ -379,7 +381,7 @@ private IEnumerable PublishableServiceCertificates public Compatibility Compatibility { get; set; } private string minIncomingSigningAlgorithm; - + /// /// Minimum accepted signature algorithm for any incoming messages. /// @@ -391,7 +393,7 @@ public string MinIncomingSigningAlgorithm } set { - if(!XmlHelpers.KnownSigningAlgorithms.Contains(value)) + if (!XmlHelpers.KnownSigningAlgorithms.Contains(value)) { throw new ArgumentException("The signing algorithm " + value + " is unknown or not supported by the current .NET Framework."); @@ -404,5 +406,22 @@ public string MinIncomingSigningAlgorithm /// Adapter to logging framework of hosting application. /// public ILoggerAdapter Logger { get; set; } - } + + private ITokenReplayCache tokenReplayCache; + public ITokenReplayCache TokenReplayCache + { + get + { + if(tokenReplayCache == null) + { + tokenReplayCache = new TokenReplayCache(); + } + return tokenReplayCache; + } + set + { + tokenReplayCache = value; + } + } +} } diff --git a/Sustainsys.Saml2/SAML2P/Saml2PSecurityTokenHandler.cs b/Sustainsys.Saml2/SAML2P/Saml2PSecurityTokenHandler.cs index 24a9b2feb..976fae239 100644 --- a/Sustainsys.Saml2/SAML2P/Saml2PSecurityTokenHandler.cs +++ b/Sustainsys.Saml2/SAML2P/Saml2PSecurityTokenHandler.cs @@ -28,19 +28,6 @@ public Saml2PSecurityTokenHandler(SPOptions spOptions) Serializer = new Saml2PSerializer(spOptions); } - // Overridden to fix the fact that the base class version uses NotBefore as the token replay expiry time - // Due to the fact that we can't override the ValidateToken function (it's overridden in the base class!) - // we have to parse the token again. - // This can be removed when: - // https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/898 - // is fixed. - protected override void ValidateTokenReplay(DateTime? expirationTime, string securityToken, TokenValidationParameters validationParameters) - { - var saml2Token = ReadSaml2Token(securityToken); - base.ValidateTokenReplay(saml2Token.Assertion.Conditions.NotOnOrAfter, - securityToken, validationParameters); - } - // TODO: needed with Microsoft.identitymodel? /// /// Process authentication statement from SAML assertion. WIF chokes if the authentication statement @@ -84,10 +71,27 @@ protected override void ProcessAuthenticationStatement(Saml2AuthenticationStatem } } - protected override Saml2SecurityToken ValidateSignature(string token, TokenValidationParameters validationParameters) + // Override and build our own logic. The problem is ValidateTokenReplay that serializes the token back. And that + // breaks because it expects some optional values to be present. + public override ClaimsPrincipal ValidateToken(string token, TokenValidationParameters validationParameters, out Microsoft.IdentityModel.Tokens.SecurityToken validatedToken) { - // Just skip signature validation -- we do this elsewhere - return ReadSaml2Token(token); + var samlToken = ReadSaml2Token(token); + + ValidateConditions(samlToken, validationParameters); + ValidateSubject(samlToken, validationParameters); + + var issuer = ValidateIssuer(samlToken.Issuer, samlToken, validationParameters); + + // Just using the assertion id for token replay. As that is part of the signed value it cannot + // be altered by someone replaying the token. + ValidateTokenReplay(samlToken.Assertion.Conditions.NotOnOrAfter, samlToken.Assertion.Id.Value, validationParameters); + + // ValidateIssuerSecurityKey not called - we have our own signature validation. + + validatedToken = samlToken; + var identity = CreateClaimsIdentity(samlToken, issuer, validationParameters); + + return new ClaimsPrincipal(identity); } - } + } } diff --git a/Sustainsys.Saml2/SAML2P/Saml2Response.cs b/Sustainsys.Saml2/SAML2P/Saml2Response.cs index 66917a519..c8574e20a 100644 --- a/Sustainsys.Saml2/SAML2P/Saml2Response.cs +++ b/Sustainsys.Saml2/SAML2P/Saml2Response.cs @@ -597,6 +597,9 @@ private IEnumerable CreateClaims(IOptions options, IdentityProvi validationParameters.RequireSignedTokens = false; validationParameters.ValidateIssuer = false; validationParameters.ValidAudience = options.SPOptions.EntityId.Id; + validationParameters.RequireAudience = false; // Audience restriction optional in SAML2 spec. + validationParameters.TokenReplayCache = options.SPOptions.TokenReplayCache; + validationParameters.ValidateTokenReplay = true; options.Notifications.Unsafe.TokenValidationParametersCreated(validationParameters, idp, XmlElement); diff --git a/Tests/Tests.Shared/Saml2P/Saml2ResponseTests.cs b/Tests/Tests.Shared/Saml2P/Saml2ResponseTests.cs index 8f9f7f95f..d41d58a98 100644 --- a/Tests/Tests.Shared/Saml2P/Saml2ResponseTests.cs +++ b/Tests/Tests.Shared/Saml2P/Saml2ResponseTests.cs @@ -1918,8 +1918,6 @@ public void Saml2Response_GetClaims_ThrowsOnWeakSigningAlgoritm() [TestMethod] public void Saml2Response_GetClaims_ThrowsOnReplayAssertionId() { - Assert.Inconclusive("Deliberately ignored test for now"); - var response = @"