(feature): Add ActiveTokenEndpoint to WsFederationConfiguration #2100

victorm-hernandez · 2023-06-02T08:47:08Z

Added support to the WsFederationMetadataSerializer to read the SecurityTokenSerivceEndpoint element from WS-Federation metadata and exposed it via the WsFederationConfiguration object as ActiveTokenEndpoint.

…ses.

Instead of attempting with each of the signing keys, we will match the key id of the signature with the key id of the signing key.

brentschmaltz · 2023-06-06T14:24:20Z

src/Microsoft.IdentityModel.Protocols.WsFederation/WsFederationMetadataSerializer.cs

+            reader.MoveToContent();
+            reader.ReadEndElement();
+
+            // </PassiveRequestorEndpoint>


Should this comment be: // SecurityTokenServiceEndpoint #Resolved

Yes, updating

Looks like this was marked resolved but also not updated in the branch so far as I can tell. @victorm-hernandez do you have local changes which still will be persisted?

You are right, to avoid any confusion I will mark elements as resolved once I sent another iteration.

brentschmaltz · 2023-06-06T14:26:49Z

src/Microsoft.IdentityModel.Tokens/BaseConfiguration.cs

        /// Or the token_endpoint in the OIDC metadata.
        /// </summary>
        public virtual string TokenEndpoint { get; set; }

+        /// <summary>
+        /// Gets or sets the token endpoint specified via the metadata endpoint.
+        /// This can is the fed:SecurityTokenServiceType in WS-Federation, http://docs.oasis-open.org/wsfed/federation/v1.2/os/ws-federation-1.2-spec-os.html#:~:text=fed%3ASecurityTokenSerivceEndpoint


'This can is' => 'This is' #Resolved

Seems like it's not yet fixed?

brentschmaltz · 2023-06-06T14:50:38Z

src/Microsoft.IdentityModel.Protocols.WsFederation/WsFederationMetadataSerializer.cs

+
+            string tokenEndpoint = null;
+
+            while (reader.IsStartElement())


If the reader is not positioned on a StartElement what will happen? #Resolved

Since we know that the XML is not malformed, the only scenario where the reader is not on a start element is if the endpointreference has no inner tags and ends right there.

<wsa:EndpointReference xmlns:wsa=""http://www.w3.org/2005/08/addressing""></wsa:EndpointReference>

In that case the MoveToContent below will have no effect and finish reading the SecurityTokenServiceEndpoint tag.

The return value will be null for tokenEndpoint.

The configuration validator class has a check for null on this URI and would catch that problem.

brentschmaltz · 2023-06-06T14:57:34Z

src/Microsoft.IdentityModel.Protocols.OpenIdConnect/Configuration/OpenIdConnectConfiguration.cs

+        /// This base class property is not used in OpenIdConnect. 
+        /// </summary>
+        [JsonIgnore]
+        public override string ActiveTokenEndpoint { get; set; }


We will modify this in our 7.x release when we will use the utf8Reader and set the ActiveTokenEndpoint to the "token_endpoint" and PassiveTokenEndpoint to "authorization_endpoint".

Are we keeping track of the changes we're intending to make/are in the process of making somewhere? We'll definitely want to make sure we have really clear release notes when we go to 7.x so it's clear who's impacted.

This comment is more an FYI for the community then? There's no action for this PR correct?

src/Microsoft.IdentityModel.Protocols.WsFederation/WsFederationConstants.cs

FuPingFranco · 2023-06-08T22:28:01Z

...oft.IdentityModel.Protocols.WsFederation/Configuration/WsFederationConfigurationValidator.cs

+        /// Validates a WsFederationConfiguration.
+        /// </summary>
+        /// <param name="configuration">WsFederationConfiguration to validate</param>
+        /// <returns>A <see cref="ConfigurationValidationResult"/> that contains validation result.</returns>


nit: "that contains validation result" -> "containing the validation result" #Resolved

FuPingFranco

Looks great!

...oft.IdentityModel.Protocols.WsFederation/Configuration/WsFederationConfigurationValidator.cs

TimHannMSFT · 2023-06-09T17:02:16Z

...oft.IdentityModel.Protocols.WsFederation/Configuration/WsFederationConfigurationValidator.cs

+                    ErrorMessage = LogMessages.IDX22700,
+                    Succeeded = false
+                };
+            }


Consider doing all these checks up front and use StringBuilder to build out an error message. In the current model a developer may face one issue, update, face another issue, update...etc. Getting all the errors up front allows for more actionable errors/fewer iterations of debugging. Also less code maintenance costs IMO since if we decide to add another check it doesn't require another log message. #WontFix

The current code assume some of the checks before it. While is not a bad idea it requires a refactor of the whole method. If you dont mind I would like to postpone this improvement.

TimHannMSFT · 2023-06-09T17:08:17Z

...oft.IdentityModel.Protocols.WsFederation/Configuration/WsFederationConfigurationValidator.cs

+
+                try
+                {
+                    configuration.Signature.Verify(key, key.CryptoProviderFactory);


Looks like this can throw ArgumentNullException too. That would be a bit odd since in other cases where there's unexpected input it's returned as a failed ConfigurationValidationResult. Is this intentional? #Resolved

This is a good catch.

On the our scenario this is not a real problem. If we deserialize the configuration using the metadataSerializer class the security key always use the default crypto provider and this value is never null.

Having said that, it is possible to modify this property before calling the validator. We need to handle that scenario.

I will update the logic to check if there is a crypto provider before attempting to verify.

TimHannMSFT · 2023-06-09T17:15:46Z

src/Microsoft.IdentityModel.Protocols.WsFederation/LogMessages.cs

        internal const string IDX22804 = "IDX22804: Security token type role descriptor is expected.";
        internal const string IDX22806 = "IDX22806: Key descriptor for signing is missing in security token service type RoleDescriptor.";
        internal const string IDX22807 = "IDX22807: Token endpoint is missing in security token service type RoleDescriptor.";
        internal const string IDX22808 = "IDX22808: 'Use' attribute is missing in KeyDescriptor.";
        internal const string IDX22810 = "IDX22810: 'Issuer' value is missing in wsfederationconfiguration.";
        internal const string IDX22811 = "IDX22811: 'TokenEndpoint' value is missing in wsfederationconfiguration.";
        internal const string IDX22812 = "IDX22812: Element: '{0}' was an empty element. 'TokenEndpoint' value is missing in wsfederationconfiguration.";
+        internal const string IDX22813 = "IDX22813: 'ActiveTokenEndpoint' is missing in 'SecurityTokenServiceTypeRoleDescriptor'.";


nit: [consistency] this is in single quotes here and SecurityTokenServiceEndpoint is not in the next log. Also IDX22806, IDX22807 talk about "token service type RoleDescriptor". I think the way you have it is more clear probably but good to have consistency too for ease of understanding. #Resolved

Good point, will update.

TimHannMSFT

Did not complete looking through all the testing but the core changes LGTM, left a few comments where I believe some improvements could be considered.

dannybtsai · 2023-06-12T17:45:53Z

src/Microsoft.IdentityModel.Protocols.WsFederation/WsFederationMetadataSerializer.cs

@@ -191,6 +195,9 @@ protected virtual SecurityTokenServiceTypeRoleDescriptor ReadSecurityTokenServic
            if (string.IsNullOrEmpty(roleDescriptor.TokenEndpoint))
                LogHelper.LogWarning(LogMessages.IDX22807);

+            if (string.IsNullOrEmpty(roleDescriptor.ActiveTokenEndpoint))


This implies that roleDescriptor.ActiveTokenEndpoint can be null or empty (not an error), but ReadSecurityTokenServiceEndpoint method throws LogMessages.IDX22812 exception if the element is empty. #Resolved

This is correct. I understand how this can be confusing.

This inconsistency is there to avoid a breaking change ActiveTokenEndpoint is a new property that may not be used today by consumers of our library, if they chose to populate this class in any way but our metadata serializer this value may be null. So we cannot really enforce this new property without introducing a breaking change.

The story is different for the configuration validator. This is a new class that no one is using so it can properly validate that the field exists and has the correct value.

dannybtsai · 2023-06-12T17:50:54Z

src/Microsoft.IdentityModel.Protocols.WsFederation/WsFederationMetadataSerializer.cs

+                    reader.ReadStartElement(WsAddressing.Elements.Address, WsAddressing.Namespace);
+                    reader.MoveToContent();
+
+                    tokenEndpoint = Trim(reader.ReadContentAsString());


Do we need to catch exception from ReadContentAsString? #Resolved

There is no need, the entry point or this flow has a try/catch block that handles all exceptions

dannybtsai · 2023-06-12T18:00:51Z

...oft.IdentityModel.Protocols.WsFederation/Configuration/WsFederationConfigurationValidator.cs

+                };
+            }
+
+            if (string.IsNullOrWhiteSpace(configuration.Signature.SignedInfo.SignatureMethod))


null check SignedInfo #Resolved

dannybtsai · 2023-06-12T18:01:08Z

...oft.IdentityModel.Protocols.WsFederation/Configuration/WsFederationConfigurationValidator.cs

+                };
+            }
+
+            if (configuration.Signature.SignedInfo.References == null || configuration.Signature.SignedInfo.References.Count == 0)


null check SignedInfo #Resolved

dannybtsai · 2023-06-12T18:05:16Z

...oft.IdentityModel.Protocols.WsFederation/Configuration/WsFederationConfigurationValidator.cs

+                        Succeeded = true
+                    };
+                }
+                catch (XmlValidationException)


Should catch other types of exceptions too?

Should break or continue? #Pending

Although the only exception expected is this, the cryptoprovider may potentially throw any other exception.

We could eat the exception and try to continue however given that we checked for the key thumbprint we do not expect any other key to be valid. We do not expect multiple entries with the same thumbprint and different crypto provider factory.

In my opinion the best thing to do is to let that exception bubble up.

The other alternative I see would be to update the ConfigurationValidationResult object to include exception information, given that that object is shared across ConfigurationValidators I would prefer to not change the pattern.

We do want to try rest of the keys in the list if one fails correct? In that case, like Danny suggested, it should be continue.

…config New scenarios handled on WsFedConfigValidaot, handle null crypto provider factory on signing keys, null signing key, null signedinfo. New error string for WsFedConfigValidator for the scenario where we cannot find the key used to sign the metadata. Updated strings for consistency. Minor XML comment updates.

sruke

LGTM!

victorm-hernandez · 2023-06-21T02:39:16Z

/azp run

RojaEnnam · 2023-06-21T19:47:03Z

src/Microsoft.IdentityModel.Protocols.WsFederation/SecurityTokenServiceTypeRoleDescriptor.cs

+        /// Passive Requestor Token endpoint
+        /// fed:PassiveRequestorEndpoint, https://docs.oasis-open.org/wsfed/federation/v1.2/os/ws-federation-1.2-spec-os.html#:~:text=fed%3ASecurityTokenServiceType/fed%3APassiveRequestorEndpoint


Can we add a line or link on what passive requestor is.

RojaEnnam · 2023-06-21T19:47:33Z

src/Microsoft.IdentityModel.Protocols.WsFederation/SecurityTokenServiceTypeRoleDescriptor.cs

        /// </summary>
        public string TokenEndpoint
        {
            get;
            set;
        }
+
+        /// <summary>
+        /// Active Requestor Token Endpoint


Same as above, can we add some description on what active requestor is.

(feature): Add ActiveTokenEndpoint to WsFederationConfiguration

7181cd3

Added support to the WsFederationMetadataSerializer to read the SecurityTokenSerivceEndpoint element from WS-Federation metadata and exposed it via the WsFederationConfiguration object as ActiveTokenEndpoint.

victorm-hernandez marked this pull request as ready for review June 2, 2023 19:09

victorm-hernandez requested review from dannybtsai, RojaEnnam and sruke June 2, 2023 19:20

(feature): Add WsFederationConfigurationValidator and related test ca…

c87f6d3

…ses.

victorm-hernandez requested a review from brentschmaltz June 3, 2023 02:29

(feature): Optimized WsFedConfigValidator signature validation.

8335438

Instead of attempting with each of the signing keys, we will match the key id of the signature with the key id of the signing key.

brentschmaltz reviewed Jun 6, 2023

View reviewed changes

src/Microsoft.IdentityModel.Protocols.WsFederation/WsFederationConstants.cs Outdated Show resolved Hide resolved

FuPingFranco reviewed Jun 8, 2023

View reviewed changes

FuPingFranco approved these changes Jun 8, 2023

View reviewed changes

sruke reviewed Jun 9, 2023

View reviewed changes

...oft.IdentityModel.Protocols.WsFederation/Configuration/WsFederationConfigurationValidator.cs Outdated Show resolved Hide resolved

TimHannMSFT reviewed Jun 9, 2023

View reviewed changes

...oft.IdentityModel.Protocols.WsFederation/Configuration/WsFederationConfigurationValidator.cs Show resolved Hide resolved

TimHannMSFT reviewed Jun 9, 2023

View reviewed changes

TimHannMSFT approved these changes Jun 9, 2023

View reviewed changes

dannybtsai reviewed Jun 12, 2023

View reviewed changes

sruke approved these changes Jun 19, 2023

View reviewed changes

(style/fix): Null check added to WsFedConfigValidator, cosmetic changes

286745f

victorm-hernandez merged commit 23808d5 into dev Jun 21, 2023

RojaEnnam reviewed Jun 21, 2023

View reviewed changes

		/// Passive Requestor Token endpoint
		/// fed:PassiveRequestorEndpoint, https://docs.oasis-open.org/wsfed/federation/v1.2/os/ws-federation-1.2-spec-os.html#:~:text=fed%3ASecurityTokenServiceType/fed%3APassiveRequestorEndpoint

(feature): Add ActiveTokenEndpoint to WsFederationConfiguration #2100

(feature): Add ActiveTokenEndpoint to WsFederationConfiguration #2100

Conversation

victorm-hernandez commented Jun 2, 2023

brentschmaltz Jun 6, 2023 • edited by victorm-hernandez Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentschmaltz Jun 6, 2023 • edited by victorm-hernandez Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentschmaltz Jun 6, 2023 • edited by victorm-hernandez Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentschmaltz Jun 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FuPingFranco Jun 8, 2023 • edited by victorm-hernandez Loading

Choose a reason for hiding this comment

FuPingFranco left a comment

Choose a reason for hiding this comment

TimHannMSFT Jun 9, 2023 • edited by victorm-hernandez Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimHannMSFT Jun 9, 2023 • edited by victorm-hernandez Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimHannMSFT Jun 9, 2023 • edited by victorm-hernandez Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimHannMSFT left a comment

Choose a reason for hiding this comment

dannybtsai Jun 12, 2023 • edited by victorm-hernandez Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannybtsai Jun 12, 2023 • edited by victorm-hernandez Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannybtsai Jun 12, 2023 • edited by victorm-hernandez Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannybtsai Jun 12, 2023 • edited by victorm-hernandez Loading

Choose a reason for hiding this comment

dannybtsai Jun 12, 2023 • edited by victorm-hernandez Loading

Choose a reason for hiding this comment

victorm-hernandez Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sruke left a comment

Choose a reason for hiding this comment

victorm-hernandez commented Jun 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentschmaltz Jun 6, 2023 •

edited by victorm-hernandez

Loading

brentschmaltz Jun 6, 2023 •

edited by victorm-hernandez

Loading

brentschmaltz Jun 6, 2023 •

edited by victorm-hernandez

Loading

brentschmaltz Jun 6, 2023 •

edited

Loading

FuPingFranco Jun 8, 2023 •

edited by victorm-hernandez

Loading

TimHannMSFT Jun 9, 2023 •

edited by victorm-hernandez

Loading

TimHannMSFT Jun 9, 2023 •

edited by victorm-hernandez

Loading

TimHannMSFT Jun 9, 2023 •

edited by victorm-hernandez

Loading

dannybtsai Jun 12, 2023 •

edited by victorm-hernandez

Loading

dannybtsai Jun 12, 2023 •

edited by victorm-hernandez

Loading

dannybtsai Jun 12, 2023 •

edited by victorm-hernandez

Loading

dannybtsai Jun 12, 2023 •

edited by victorm-hernandez

Loading

dannybtsai Jun 12, 2023 •

edited by victorm-hernandez

Loading

victorm-hernandez Jun 14, 2023 •

edited

Loading