This repository has been archived by the owner on Jun 6, 2023. It is now read-only.
Changes based on integrating with ADFS, Azure, and PingOne #7
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
Due to the way these elements were declared, building a request with no signature still sends the empty tags: ... To keep the request small and for better compatibility, suppress them altogether with unsigned requests.
They were defining a prefix which was already defined in the scope of their definition, and it seems encoding/xml isn't clever enough to know that it doesn't need to emit them in that case. Remove them.
This may or may not be required for our use case. Slack sets this to persistent in their implementation and I was changing things I could see.
Azure thinks dateTime values with nanosecond precision are not protocol compliant. http://www.w3.org/TR/xmlschema-2/#dateTime - Azure is wrong. It's OK, there's an ID in there anyway to resolve ambiguity.
This value is required in order to implement logout; parse it.
Setting this field to the string "true" will force the identity provider (with the power of MUST) to authenticate the presenter directly rather than rely on a previous security context.
The type changed in order to support unsigned requests, but the test did not update.
Callers may want to store the decoded XML, so allow them to do the base64 decoding and pass in the XML string. This is needed in order to set the 'originalString' method for validation.
The XML Signature standard is designed to be re-usable standard by other standards, and it's up to those other standards how they bind it into their own messages. In SAML, the binding allows signatures to appear in a number of different places. XML Signatures can't assume anything about the document structure for the standard it is used with, so instead it uses references to specify which part of the document the signature applies to. A given directory may be consistent about which node it signs, and the previous implementation was coded to assume the signature appears on the root element. However, with Azure the "Assertion" block is the signed part. Update the implementation to delegate responsibility of which node to pass to "xmlsec" for signature verification to a response method, and add a basic implementation which supports ID referencing.
Especially not with spaces. Eww. What a waste of space.
Some implementations might accept xml elements at a particular level in any order, but due to the design of XMLSchema, typically only one ordering is valid. Azure rejects requests with the signature in the wrong place. Fix this and another minor compliance issue (capitalization of an attribute not matching the spec)
Looks like I was mistaken; it's ID on the SAML specs, but Id on the XML Signature spec.
ADFS fails with 'ID6027: Enveloped Signature Transform cannot be the last transform in the chain. The last transform must compute the digest which Enveloped Signature transform is not capable of.' The signature block in requests is now essentially identical to the one returned by Azure and ADFS.
From [SAMLCore], §3.2.1:
Destination [Optional]
A URI reference indicating the address to which this request has
been sent. This is useful to prevent malicious forwarding of
requests to unintended recipients, a protection that is required
by some protocol bindings. If it is present, the actual recipient
MUST check that the URI reference identifies the location at which
the message was received. If it does not, the request MUST be
discarded. Some protocol bindings may require the use of this
attribute (see [SAMLBind]).
In the SAML Bindings spec, §3.4.5.2 we find:
Security Considerations
The presence of the user agent intermediary means that the
requester and responder cannot rely on the transport layer for
end-end authentication, integrity and confidentiality. URL-encoded
messages MAY be signed to provide origin authentication and
integrity if the encoding method specifies a means for signing.
If the message is signed, the Destination XML attribute in the
root SAML element of the protocol message MUST contain the URL to
which the sender has instructed the user agent to deliver the
message. The recipient MUST then verify that the value matches the
location at which the message has been received.
ie, this URL is here to prevent someone from taking a login request
for one IdP and using it for a different IdP.
These elements are not required by the standard, and some directory servers will refuse requests with them set. Change their types to be pointers so that they can be omitted.
It's possible to encrypt SAML response assertions; this was primarily useful in the era where it ran over non-encrypted endpoints, and is still useful if there is some information in the assertions which you don't want the user to be able to access using tools such as Firefox SAML Tracer. Being an OASIS standard, there are of course several ways this can be arranged with relation to signatures: the assertions can be signed and then encrypted, or the assertions can be encrypted and the whole message signed. This implementation allows for Assertion nodes to appear under EncryptedAssertion, which is the form that 'xmlsec1' expects and returns the plaintext versions, even though it doesn't conform to the XML schema. Callers should take utmost care to keep track of which sections have been validated and which haven't when regarding assertions as authorative.
These lines are a poor substitute for real exception logging, but carry it on for now.
The code was still depending on the original library for utils etc; use the ones in this fork instead.
Some directory servers require an explicit logout (especially if they don't honor the 'ForceAuthn' flag on AuthnRequest). Add support for this, including signing.
Switching these URLs was necessary for using our branch, but now the branch is being submitted for upstream inclusion, they should be returned to their original fork.
SAML configuration information is exchanged in this format; add support for reading and writing it.
…ure on response, assertion and encryptedassertion (#3)
ADMIN-2247 - gitleaks allowed list
INFRA-2677: GHA pipeline
INFRA-2677: fix build publish workflow
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Just to let you know, we've been successfully integrating our product against various SAML implementations using this code, and here are some of the changes which we've found are necessary. There's a caveat that not all the changes are completely API compatible, and we're not done, but I just thought I'd open this PR so we can start talking about what it will take to converge.