Optional ServiceProvider.SigningCertificate #26
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The HttpRedirectBindingBuilder.SigningKey setter allows for null values.
// Check if the key is of a supported type. [SAMLBind] sect. 3.4.4.1 specifies this.
if (!(value is RSACryptoServiceProvider || value is DSA || value == null))
{
throw new ArgumentException("Signing key must be an instance of either RSACryptoServiceProvider or DSA.");
}
This expression which is used to assign to that property fails if the ServiceProvider does not have a SigningCertificate.
SigningKey = config.ServiceProvider.SigningCertificate.PrivateKey,
Added the Elvis operator to implement this.
SigningKey = config.ServiceProvider.SigningCertificate?.PrivateKey,
samunro
commented
Apr 5, 2019
| } |
There was a problem hiding this comment.
Not sure where the newline came from. I even tried to reverse the change but I think that the web interface might add it in by default.
…nseChallengeAsync The current default is currentUri which does not seem like the best choice if a value for RedirectAfterLogin is available. I was expecting the browser to be redirected to RedirectAfterLogin post authentication but I actually saw an endless loop of authentications because it was redirecting to currentUri.
…s of type SecurityTokenServiceType and ApplicationServiceType which were not expected. There was a workaround which involved removing those but that also meant that the signature had to be removed. I added types which allows the metadata to be deserialized - even if there is no special handling for them. The certificates in the SAML response were being passed in a way that the existing code did not expect. They are now parsed successfully.
…se it was a hash of the whole document that was being used instead of one based on just the assertion.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I don't have much experience with SAML so please correct me if I have anything wrong.
From what I understand, signing requests that are sent to identity providers is optional (specific identity providers might require it).
The following guard clause in the HttpRedirectBindingBuilder.SigningKey setter seems to back this up - null is allowed.
I was getting a null reference exception in this expression within SamlMessage.AuthnRequestForIdp() when I left the ServiceProvider's SigningCertificate property null.
SigningKey = config.ServiceProvider.SigningCertificate.PrivateKey,My change will result in the SigningKey being set to null if the ServiceProvider's SigningCertificate is null.