-
Notifications
You must be signed in to change notification settings - Fork 12
Validate SAMLRequest signatures from Federator #91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: fume <2969850+fume@users.noreply.github.com>
…umentation, better error handling Co-authored-by: fume <2969850+fume@users.noreply.github.com>
| } | ||
|
|
||
| // Validate SAMLRequest signature from Federator | ||
| if (!_technicalChecksOptions.SkipSignaturesValidation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need a dedicated option to skip the SAMLRequest signature validation. Definetely not on the technical checks options which are dedicated to samlresponse validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created dedicated FederatorRequestValidationOptions with SkipSAMLRequestSignatureValidation setting. The ProxyController now uses this instead of TechnicalChecks options. Configuration is in the new FederatorRequestValidation section in appsettings.json. (commit 0a13bec)
| // Decode and validate that SigAlg matches expected algorithm | ||
| string decodedSigAlg = HttpUtility.UrlDecode(federatorRequest.SigAlg); | ||
| if (!decodedSigAlg.Equals("http://www.w3.org/2001/04/xmldsig-more#rsa-sha256", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| _logger.LogWarning("SAMLRequest SigAlg '{sigAlg}' is not supported. Expected rsa-sha256", decodedSigAlg); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we support even other signature algorithms? IF not, explain why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now supports RSA-SHA256, RSA-SHA1, and RSA-SHA512 per SAML specifications. The algorithm is detected from the SigAlg parameter and the corresponding HashAlgorithmName is used for signature verification. These are the standard algorithms supported by SAML HTTP-Redirect binding. (commit 0a13bec)
|
|
||
| // Try to get metadata from cache first | ||
| string metadataXml = null; | ||
| string cacheKey = "FederatorMetadata"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make this a constant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made the cache key a constant FEDERATOR_METADATA_CACHE_KEY at the class level. (commit 0a13bec)
| { | ||
| var options = new DistributedCacheEntryOptions() | ||
| { | ||
| AbsoluteExpirationRelativeToNow = TimeSpan.FromMinutes(120), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make this configurable, with a default of 240 minutes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made cache expiration configurable via FederatorRequestValidationOptions.MetadataCacheAbsoluteExpirationInMins with a default of 240 minutes. Can be configured in appsettings.json under FederatorRequestValidation section. (commit 0a13bec)
fume
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…, support multiple signature algorithms, use constant for cache key Co-authored-by: fume <2969850+fume@users.noreply.github.com>
| if (decodedSigAlg.Equals("http://www.w3.org/2001/04/xmldsig-more#rsa-sha256", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| hashAlgorithm = HashAlgorithmName.SHA256; | ||
| } | ||
| else if (decodedSigAlg.Equals("http://www.w3.org/2000/09/xmldsig#rsa-sha1", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| hashAlgorithm = HashAlgorithmName.SHA1; | ||
| } | ||
| else if (decodedSigAlg.Equals("http://www.w3.org/2001/04/xmldsig-more#rsa-sha512", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| hashAlgorithm = HashAlgorithmName.SHA512; | ||
| } | ||
| else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rewrite this block using a more readable switch case. Keep the logging for the unsupported SigAlg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored to use a switch expression for better readability. The logging for unsupported SigAlg is preserved. (commit c0fe19c)
| foreach (var cert in certificates) | ||
| { | ||
| using (RSA rsa = cert.GetRSAPublicKey()) | ||
| { | ||
| if (rsa != null && rsa.VerifyData(bytesToVerify, signatureBytes, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1)) | ||
| if (rsa != null && rsa.VerifyData(bytesToVerify, signatureBytes, hashAlgorithm, RSASignaturePadding.Pkcs1)) | ||
| { | ||
| _logger.LogDebug("SAMLRequest signature validated successfully with certificate: {subject}", cert.Subject); | ||
| return true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double check if this code is cross-platform. This solution could run both on windows and linux
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed cross-platform compatible. The GetRSAPublicKey() and VerifyData() methods are part of the standard .NET cryptography library that works on both Windows and Linux. This pattern is already used elsewhere in the codebase (FederatorResponseService, XMLSignatureExtensions). Added a comment to explicitly document cross-platform compatibility. (commit c0fe19c)
fume
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…tform compatibility comment Co-authored-by: fume <2969850+fume@users.noreply.github.com>
SAMLRequest Signature Validation Implementation - COMPLETE ✅
Summary
All feedback addressed. The implementation now:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.