Skip to content
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

Certificate validation for RSA is optional #226

Merged
merged 2 commits into from
Oct 30, 2019

Conversation

gotjosh
Copy link
Contributor

@gotjosh gotjosh commented Oct 30, 2019

When the encryption method is RSA, the library expects that a certificate is embedded within the ACS either under ./KeyInfo/X509Data/X509Certificate or ./KeyInfo/X509Data/X509IssuerSerial to verify that the public part of our private key is valid.

After much investigation, it appears that this is not mandatory as per the SAML standard and it's just an optimisation to let the provider know ahead of time that the Key won't work.

To validate this, we did the following:

  • First, the SAML 2.0 Core Standard (section 2.3.4) states that the <EncryptedAssertion> element must be compose of: at least one <xenc:EncryptedData> and zero or more of <xenc:EncryptedKey>

  • Second, The only required elements are <xenc:EncryptedData> and <xenc:EncryptedKey>. These need to be defined according to the [XMLEnc] specification.

  • Third, Section 2.2 of the XMLenc standard describes an acceptable schema for both EncryptedData and EncryptedKey. For both of these, the <X509Data> element is not mandatory by any sorts. Also, the <EncryptedKey> definition reads:

    The EncryptedKey element is used to transport encryption keys from the originator to a known recipient(s). It may be used as a stand-alone XML document, be placed within an application document, or appear inside an EncryptedData element as a child of a ds:KeyInfo element. The key value is always encrypted to the recipient(s). When EncryptedKey is decrypted, the resulting octets are made available to the EncryptionMethod algorithm without any additional processing.

  • Finally, the algorithm https://www.w3.org/TR/2002/REC-xmlenc-core-20021210/Overview.html#rsa-oaep-mgf1p specification makes no mention of a certificate validation process.

To conclude, I think the original author's intention with this validation is referenced in this text of the XMLenc standard (section 3.5.1):

> Recipient is an optional attribute that contains a hint as to which recipient this encrypted key value is intended for. Its contents are application dependent.

Which states that applications (or libraries in this case) can add additional validations but these are subjective.

Instead of supporting the Recipient element which is optional, we think the author attempted to find a certificate embedded within the payload and use that to verify the key

We believe this validation is an optimisation and is safe to keep it optional. This PR does that, which no-ops if we have failed to find an embedded certificate in the response.

Jon Gyllensward and others added 2 commits October 30, 2019 16:23
Now that the validation is optional, the previous name did not
accurately reflected the intention.
@crewjam crewjam merged commit f521243 into crewjam:master Oct 30, 2019
atezet pushed a commit to atezet/saml that referenced this pull request Jul 9, 2022
* removed mandatory check for validating embedded certificate for rsa
* Rename validateRSAKey to validateRSAKeyIfPresent

Now that the validation is optional, the previous name did not
accurately reflected the intention.
@Jguer Jguer deleted the rsa-key-validation-optional branch January 2, 2023 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants